[FEATURE] Support mp4 tx3g & multitrack subtitles #898

harrynull · 2018-01-13T11:34:59Z

Please prefix your pull request with one of the following: [FEATURE] [FIX] [IMPROVEMENT].

In raising this pull request, I confirm the following (please check boxes):

I have read and understood the contributors guide.
I have checked that another pull request for this purpose does not exist.
I have considered, and confirmed that this submission will be valuable to others.
I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

I have never used CCExtractor.
I have used CCExtractor just a couple of times.
I absolutely love CCExtractor, but have not contributed previously.
I am an active contributor to CCExtractor.

This will fix #807 .
FEATURE:

sbtl:tx3g and text:tx3g handler.
Support multitrack subtitles.

KNOWN ISSUES:

Styles & extensions of tx3g subtitles are ignored.
The empty default file will be created, even though sometimes(when there're multiple tracks) we don't use it.
-o outputfilename option is not honered. I don't think I caused it though. It seems to be broken long time ago.

NOTE:

I didn't test the situation when a sample has multiple atoms in it (Is it possible? I don't have a video sample to verify).
Although I think the timing is handled correctly, I don't have enough samples to verify.
All my local mp4 samples are extracted correctly, but I'd give the sample platform a go anyway.

harrynull · 2018-01-13T11:38:31Z

The output of the sample in the issue is attached here: output.zip.

cfsmp3 · 2018-01-13T15:40:24Z

src/gpacmp4/mp4.c

 		mprint("Found %d AVC track(s). ", avc_track_count);
-	}
+	else
+		mprint("Found no AVC track(s). ", file);


This is not correct - that "file" is not being consumed in the format string.

cfsmp3 · 2018-01-13T15:41:34Z

src/gpacmp4/mp4.c

+		if (sub_type == GF_ISOM_SUBTYPE_C708) {
+			if (!is_ccdp) {
+				mprint("Your video file seems to be an interesting sample for us (%4.4s)\n", data);
+				mprint("We haven't met c708 subtitle not in a \'ccdp\' atom before\n");


We haven't seen a c708 subtitle outside a ccdp atom before

Sounds better

cfsmp3 · 2018-01-13T15:42:21Z

src/gpacmp4/mp4.c

+		{
+			if (is_ccdp) {
+				mprint("Your video file seems to be an interesting sample for us\n");
+				mprint("We haven't met c608 subtitle in a \'ccdp\' atom before\n");


Similar to above

cfsmp3 · 2018-01-13T15:44:35Z

src/gpacmp4/mp4.c

+	return -1;
+}
+
+#define MEDIA_TYPE(type, subtype) (((u64)(type)<<32)+(subtype))


Maybe move close to top of file? defines in the middle of the file are likely to cause confusion (since they only start applying from the point they appear, so suddenly function position in the file becomes more important than it should be)

cfsmp3 · 2018-01-13T15:45:10Z

src/gpacmp4/mp4.c

 			media = open()
 			for each track in media
-				if track is AVC track
+				swtich track


cfsmp3 · 2018-01-13T15:47:07Z

src/gpacmp4/mp4.c

+
+#define MEDIA_TYPE(type, subtype) (((u64)(type)<<32)+(subtype))
+
+void switch_output_file(struct lib_ccx_ctx *ctx, struct encoder_ctx *enc_ctx, int track_id) {


Let's call the function mp4_switch_output_file()
OR move to the file that contains all the file related stuff

cfsmp3 · 2018-01-13T15:47:55Z

src/gpacmp4/mp4.c

 	if((f = gf_isom_open(file, GF_ISOM_OPEN_READ, NULL)) == NULL)
 	{
-		mprint("failed to open\n");
+		mprint("Failed to open\n");


"Failed to open input file (gf_isom_open() returned error)"

cfsmp3 · 2018-01-13T15:48:23Z

src/gpacmp4/mp4.c

+				switch_output_file(ctx, enc_ctx, i);
+			}
+			if (process_xdvb_track(ctx, file, f, i + 1, &dec_sub) != 0) {
+				mprint("Error\n");


"Error on process_xdvb_track()"

cfsmp3 · 2018-01-13T15:49:00Z

src/gpacmp4/mp4.c

-			{
-				mprint("error\n");
+			if (process_avc_track(ctx, file, f, i + 1, &dec_sub) != 0) {
+				mprint("Error\n");


"Error on process_avc_track()"

cfsmp3 · 2018-01-13T15:50:18Z

src/gpacmp4/mp4.c

+							(unsigned char)(subtype >> 24 % 0x100), (unsigned char)((subtype >> 16) % 0x100),
+							(unsigned char)((subtype >> 8) % 0x100), (unsigned char)(subtype % 0x100));
+
+						mprint("", track_type);


track_type is not being consumed by mprint()
It it's a char *, then mprint("%s", track_type)

saurabhshri · 2018-01-21T14:45:59Z

src/gpacmp4/mp4.c

 	return data;
 }

+// Rrocess clcp type atom


Rrocess--> Process.

saurabhshri · 2018-01-21T14:49:49Z

src/gpacmp4/mp4.c

+		encode_sub(enc_ctx, dec_sub); // encode the previous subtitle
+		has_previous_sub = 0;
+	}
+	if (encode_last_only) return;


I see in #L422 last_sub is declared but not used it?

Is this if block work in progress? Also, please fix this empty return, the function is expected to return an int and may cause build to fail.

@saurabhshri
Sorry that I missed your comment till now.
The purpose of this if block is merely to exit the function early so that it won't attempt to process more data. (since it's the last one)
And it seems that it has already been fixed.

Null added 2 commits January 13, 2018 19:23

Support mp4 tx3g & multitrack subtitles

904868e

Fix indentation

73ab7db

cfsmp3 requested changes Jan 13, 2018

View reviewed changes

Null added 2 commits January 14, 2018 08:30

Minor changes

3cd0b71

Add a comment

cb9ff19

cfsmp3 merged commit 71ac0ad into CCExtractor:master Jan 14, 2018

saurabhshri reviewed Jan 21, 2018

View reviewed changes


		#define MEDIA_TYPE(type, subtype) (((u64)(type)<<32)+(subtype))

		void switch_output_file(struct lib_ccx_ctx ctx, struct encoder_ctx enc_ctx, int track_id) {

[FEATURE] Support mp4 tx3g & multitrack subtitles #898

[FEATURE] Support mp4 tx3g & multitrack subtitles #898

Uh oh!

Conversation

harrynull commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harrynull commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harrynull Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

harrynull commented Jan 13, 2018 •

edited

Loading

harrynull commented Jan 13, 2018 •

edited

Loading

harrynull Feb 5, 2018 •

edited

Loading