| Message ID | 57DAED56.3010909@impactstudiopro.com |
|---|---|
| State | Changes Requested |
| Headers | show |
2016-09-15 20:49 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: > This updates the mpeg12dec.c DVD caption decoding to decode > field-wise and handle caption packets with an extra field. It obeys > the cc_count field, though still validates the count like the prior code. Is there a sample that shows the difference? > I will start on a patch to mpeg12enc.c to encode A53 captions > from the side data. That's great! Carl Eugen
On 09/15/2016 12:31 PM, Carl Eugen Hoyos wrote: > 2016-09-15 20:49 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: > >> This updates the mpeg12dec.c DVD caption decoding to decode >> field-wise and handle caption packets with an extra field. It obeys >> the cc_count field, though still validates the count like the prior code. > > Is there a sample that shows the difference? > The DVD release of "Big Hero 6" has CC data packets in the VOB that frequently encode one extra field then start on the opposite field in the next packet. Assuming that the caption data comes in pairs per frame means that you miss out on the last caption word and captions get slightly garbled. That problem is also visible when playing the same VOB in VLC player with the captions (under "subtitles") enabled. My patch decodes the caption data field-wise so the extra field is not missed which then fixes the slightly garbled captions. Do you need me to upload a small 20-second clip of the VOB where the garbled captions are visible? >> I will start on a patch to mpeg12enc.c to encode A53 captions >> from the side data. > > That's great! Yes! > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2016-09-15 21:52 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: > Do you need me to upload a small 20-second clip of the VOB where > the garbled captions are visible? I believe it would be useful. Thank you, Carl Eugen
I just realized this has been sitting in my inbox for awhile. Here's the clip in question. If you play it in VLC player with "Closed Captions 1" selected, you'll notice that some of the text in the captions is missing. https://www.dropbox.com/s/orrzn9vfcjmk7vb/bighero6.evenoddfieldcc.vob?dl=0 On 09/15/2016 02:49 PM, Carl Eugen Hoyos wrote: > 2016-09-15 21:52 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: > >> Do you need me to upload a small 20-second clip of the VOB where >> the garbled captions are visible? > > I believe it would be useful. > > Thank you, Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2016-10-13 8:29 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: > I just realized this has been sitting in my inbox for awhile. > > Here's the clip in question. > > If you play it in VLC player with "Closed Captions 1" selected, you'll notice that some of the text in the captions is missing. > > https://www.dropbox.com/s/orrzn9vfcjmk7vb/bighero6.evenoddfieldcc.vob?dl=0 I tested the following command line with and without your patch: $ ffmpeg -f lavfi -i movie=bighero6.evenoddfieldcc.vob[out0+subcc] out.ass Output is identical, what do I miss? Thank you, Carl Eugen
True. It just occurred to me that VLC player probably is using it's own CC user packet reader. However it still helps to read the user packet properly instead of counting by full frames. Also this code shows how to read cc_count properly. It's not incorrect by any means, it was just mis-interpreted by the original code. The original code would return an extra 0x0000 in the side data because of the way it counts the words, the extra 0x0000 likely coming from the next start code following the CC packet. I'd rather the side data contain only the CC data, then downstream code has less filtering to worry about. As a bonus the code will tell you at the debug log level if the number of caption words is wrong given the cc_count. If you're writing code to generate this packet it helps to have the library tell you your cc_count is wrong. On 10/13/2016 02:50 AM, Carl Eugen Hoyos wrote: > 2016-10-13 8:29 GMT+02:00 Jonathan Campbell <jonathan@impactstudiopro.com>: >> I just realized this has been sitting in my inbox for awhile. >> >> Here's the clip in question. >> >> If you play it in VLC player with "Closed Captions 1" selected, you'll notice that some of the text in the captions is missing. >> >> https://www.dropbox.com/s/orrzn9vfcjmk7vb/bighero6.evenoddfieldcc.vob?dl=0 > > I tested the following command line with and without your patch: > $ ffmpeg -f lavfi -i movie=bighero6.evenoddfieldcc.vob[out0+subcc] out.ass > > Output is identical, what do I miss? > > Thank you, Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Thu, Sep 15, 2016 at 11:49:58AM -0700, Jonathan Campbell wrote: > I'm sorry if the previous posting was not recognized as a patch. > > This updates the mpeg12dec.c DVD caption decoding to decode field-wise and handle caption packets with an extra field. It obeys the cc_count field, though still validates the count like the prior code. > > I will start on a patch to mpeg12enc.c to encode A53 captions from the side data. > > Jonathan Campbell > mpeg12dec.c | 45 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 13 deletions(-) > 77d60e69e6a08145f4f165c97456f92958d1d5e3 0001-Read-cc-words-field-wise-limit-to-cc_count-and-suppo.patch > From 8d64027573588a62728faebba55d67c00a3d4e3f Mon Sep 17 00:00:00 2001 > From: Jonathan Campbell <jonathan@castus.tv> > Date: Wed, 14 Sep 2016 10:57:04 -0700 > Subject: [PATCH] Read cc words field-wise, limit to cc_count and support extra > field. > > This code validates the cc words the same as the prior code this > replaced in case cc_count is too large. Field counting is used in case > caption source does not use the LSB to signal even/odd field. > --- > libavcodec/mpeg12dec.c | 45 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 13 deletions(-) This patch basically rewrites the code and without any reference I can compare it against its difficult to make heads or tails of this. or said differently how do you know this is correct ? or how can i verify that all this is correct ? some testcases or specification references would be useful > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > index ca51c97..7c65f77 100644 > --- a/libavcodec/mpeg12dec.c > +++ b/libavcodec/mpeg12dec.c > @@ -2265,6 +2265,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > /* extract DVD CC data > * > * uint32_t user_data_start_code 0x000001B2 (big endian) > + * -------------------- p[0] starts here --------------------- > * uint16_t user_identifier 0x4343 "CC" > * uint8_t user_data_type_code 0x01 > * uint8_t caption_block_size 0xF8 > @@ -2273,7 +2274,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > * bit 6 caption_filler 0 > * bit 5:1 caption_block_count number of caption blocks (pairs of caption words = frames). Most DVDs use 15 per start of GOP. > * bit 0 caption_extra_field_added 1=one additional caption word > - * > + * -------------------- p[5] starts here --------------------- > * struct caption_field_block { > * uint8_t > * bit 7:1 caption_filler 0x7F (all 1s) > @@ -2287,30 +2288,48 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > * Don't assume that the first caption word is the odd field. There do exist MPEG files in the wild that start > * on the even field. There also exist DVDs in the wild that encode an odd field count and the > * caption_extra_field_added/caption_odd_field_first bits change per packet to allow that. */ > - int cc_count = 0; > + int cc_count = 0; /* number of caption fields */ These are cosmetic changes, they belong in a seperate patch > + int caption_block_count = p[4] & 0x3F; /* you can treat bits 5:0 as number of fields */ > int i; > - // There is a caption count field in the data, but it is often > - // incorrect. So count the number of captions present. > - for (i = 5; i + 6 <= buf_size && ((p[i] & 0xfe) == 0xfe); i += 6) > + > + for (i = 5; cc_count < caption_block_count && (i + 3) <= buf_size; i += 3) { you change the code from "scan till 0xFF/FE" to scan caption_block_count why ? Is there a reference or testcase ? > + if ((p[i] & 0xfe) != 0xfe) { > + av_log(avctx, AV_LOG_DEBUG, "cc_count is too large (%d > %d) or junk data in DVD caption packet\n",caption_block_count,cc_count); > + break; > + } ive the suspicioun this should not stop here but scan all, then again thats off topic as it did this before > + > cc_count++; > + } > + > // Transform the DVD format into A53 Part 4 format > if (cc_count > 0) { > av_freep(&s1->a53_caption); > - s1->a53_caption_size = cc_count * 6; > + s1->a53_caption_size = cc_count * 3; > s1->a53_caption = av_malloc(s1->a53_caption_size); > if (s1->a53_caption) { > - uint8_t field1 = !!(p[4] & 0x80); > + uint8_t field1 = (p[4] >> 7) & 1; /* caption_odd_field_first */ belongs into cosmetic patch, also the &1 is uneeded > + uint8_t pfield = 0xFF; /* DVDs that don't use the caption_field_odd bit always seem to leave it on */ > uint8_t *cap = s1->a53_caption; > + > p += 5; > for (i = 0; i < cc_count; i++) { > - cap[0] = (p[0] == 0xff && field1) ? 0xfc : 0xfd; > + /* If the source actually uses the caption_odd_field bit, then use that to determine the field. > + * Else, toggle between fields to keep track for DVDs where p[0] == 0xFF at all times. */ > + if (p[0] != pfield) > + field1 = p[0] & 1; /* caption_field_odd */ > + > + /* in A53 part 4, 0xFC = odd field, 0xFD = even field */ > + cap[0] = field1 ? 0xFC : 0xFD; > cap[1] = p[1]; > cap[2] = p[2]; > - cap[3] = (p[3] == 0xff && !field1) ? 0xfc : 0xfd; > - cap[4] = p[4]; > - cap[5] = p[5]; > - cap += 6; > - p += 6; > + > + av_log(avctx, AV_LOG_DEBUG, "DVD CC field1=%u(%s) 0x%02x%02x prev=0x%02x cur=0x%02x\n", > + field1,field1?"odd":"even",cap[1],cap[2],pfield,p[0]); that produces a printout for each packet, that should probably be trace instead of debug level [...]
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index ca51c97..7c65f77 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -2265,6 +2265,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, /* extract DVD CC data * * uint32_t user_data_start_code 0x000001B2 (big endian) + * -------------------- p[0] starts here --------------------- * uint16_t user_identifier 0x4343 "CC" * uint8_t user_data_type_code 0x01 * uint8_t caption_block_size 0xF8 @@ -2273,7 +2274,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, * bit 6 caption_filler 0 * bit 5:1 caption_block_count number of caption blocks (pairs of caption words = frames). Most DVDs use 15 per start of GOP. * bit 0 caption_extra_field_added 1=one additional caption word - * + * -------------------- p[5] starts here --------------------- * struct caption_field_block { * uint8_t * bit 7:1 caption_filler 0x7F (all 1s) @@ -2287,30 +2288,48 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, * Don't assume that the first caption word is the odd field. There do exist MPEG files in the wild that start * on the even field. There also exist DVDs in the wild that encode an odd field count and the * caption_extra_field_added/caption_odd_field_first bits change per packet to allow that. */ - int cc_count = 0; + int caption_block_count = p[4] & 0x3F; /* you can treat bits 5:0 as number of fields */ + int cc_count = 0; /* number of caption fields */ int i; - // There is a caption count field in the data, but it is often - // incorrect. So count the number of captions present. - for (i = 5; i + 6 <= buf_size && ((p[i] & 0xfe) == 0xfe); i += 6) + + for (i = 5; cc_count < caption_block_count && (i + 3) <= buf_size; i += 3) { + if ((p[i] & 0xfe) != 0xfe) { + av_log(avctx, AV_LOG_DEBUG, "cc_count is too large (%d > %d) or junk data in DVD caption packet\n",caption_block_count,cc_count); + break; + } + cc_count++; + } + // Transform the DVD format into A53 Part 4 format if (cc_count > 0) { av_freep(&s1->a53_caption); - s1->a53_caption_size = cc_count * 6; + s1->a53_caption_size = cc_count * 3; s1->a53_caption = av_malloc(s1->a53_caption_size); if (s1->a53_caption) { - uint8_t field1 = !!(p[4] & 0x80); + uint8_t field1 = (p[4] >> 7) & 1; /* caption_odd_field_first */ + uint8_t pfield = 0xFF; /* DVDs that don't use the caption_field_odd bit always seem to leave it on */ uint8_t *cap = s1->a53_caption; + p += 5; for (i = 0; i < cc_count; i++) { - cap[0] = (p[0] == 0xff && field1) ? 0xfc : 0xfd; + /* If the source actually uses the caption_odd_field bit, then use that to determine the field. + * Else, toggle between fields to keep track for DVDs where p[0] == 0xFF at all times. */ + if (p[0] != pfield) + field1 = p[0] & 1; /* caption_field_odd */ + + /* in A53 part 4, 0xFC = odd field, 0xFD = even field */ + cap[0] = field1 ? 0xFC : 0xFD; cap[1] = p[1]; cap[2] = p[2]; - cap[3] = (p[3] == 0xff && !field1) ? 0xfc : 0xfd; - cap[4] = p[4]; - cap[5] = p[5]; - cap += 6; - p += 6; + + av_log(avctx, AV_LOG_DEBUG, "DVD CC field1=%u(%s) 0x%02x%02x prev=0x%02x cur=0x%02x\n", + field1,field1?"odd":"even",cap[1],cap[2],pfield,p[0]); + + pfield = p[0]; + field1 ^= 1; + cap += 3; + p += 3; } } }
I'm sorry if the previous posting was not recognized as a patch. This updates the mpeg12dec.c DVD caption decoding to decode field-wise and handle caption packets with an extra field. It obeys the cc_count field, though still validates the count like the prior code. I will start on a patch to mpeg12enc.c to encode A53 captions from the side data. Jonathan Campbell From 8d64027573588a62728faebba55d67c00a3d4e3f Mon Sep 17 00:00:00 2001 From: Jonathan Campbell <jonathan@castus.tv> Date: Wed, 14 Sep 2016 10:57:04 -0700 Subject: [PATCH] Read cc words field-wise, limit to cc_count and support extra field. This code validates the cc words the same as the prior code this replaced in case cc_count is too large. Field counting is used in case caption source does not use the LSB to signal even/odd field. --- libavcodec/mpeg12dec.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)