[FFmpeg-devel] mpeg12dec fix up DVD caption handling

[Jonathan Campbell]

On 09/13/2016 12:42 PM, Moritz Barsnick wrote:
> On Mon, Sep 12, 2016 at 18:19:43 -0700, Jonathan Campbell wrote:
>> Subject: [PATCH 2/7] read caption words field-wise, count properly and limit
>>  to cc_count. transfer each CC word taking into consideration immediate CC
>>  field bit or for DVDs that don't use it, keep track according to first field
>>  specified at start of DVD caption packet.
> 
> We're apparently missing patches 3 to 6 of 7?

In the branch I wrote these patches, I wrote a quick proof-of-concept program to read, dump, and decode the A53 caption side data from the AVFrame to help confirm that the data was read correctly from various MPEG and VOB files on my hard drive. That code was patches 3 to 6 and not relevant to what I posted therefore not included.

You can look at that program on Github if you like: https://github.com/joncampbell123/FFmpeg/commit/98ce62db24d0d537d9ae99610c67d7caa09a0a0c.patch

> 
> Anyways, commit messages should consist of a first line as summary (not
> too long) with a prefix such as "lavc/mpeg12dec: ", then an empty line
> separating the following optional, but possibly verbose commit
> explanation or continuation. Don't try to squeeze mulitple sentences
> into the commit line. (Check other commits for reference.)

Good idea. Will do.

> 
>> +            if ((p[i] & 0xfe) != 0xfe) {
>> +                av_log(avctx, AV_LOG_DEBUG, "cc_count is too large (%u > %u) or junk data in DVD caption packet",(unsigned int)caption_block_count,(unsigned int)cc_count);
> 
> I don't see the need to cast ints to unsigned ints for printing. Either
> just print with %d, or use unsigned ints in the first place if you
> never (ever) need negative values.

That's true. I'm used to using printf() against library structures that may or may not use unsigned int or unsigned long.

> 
>> +                    /* 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. */
> 
> Very nitpicky: If you use full sentences ending in a period, then also
> begin them with capitals, like writing proper text. Else my eyes hurt.
> (Or: Else it's hard finding the beginning.)

Will fix.

> ;-)
> 
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

Should I make these changes and resubmit them? (except for the documentation patch which I saw)

Jonathan Campbell