[FFmpeg-devel] [PATCH] lavc/mpeg12dec.c: Read cc words field-wise, limit to cc_count and support extra field.
Michael Niedermayer
michael at niedermayer.cc
Thu Oct 13 21:53:31 EEST 2016
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 at 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161013/e5c1e75c/attachment.sig>
More information about the ffmpeg-devel
mailing list