[FFmpeg-devel] Regression in mxf decoding

[Tim Nicholson]

On 21/05/12 10:30, Matthieu Bouron wrote:
> On Mon, May 21, 2012 at 09:30:33AM +0100, Tim Nicholson wrote:
>> On 18/05/12 18:08, Tim Nicholson wrote:
>>> Just spotted that ffmpeg is failing to correctly determine the size of
>>> V210 material in an mxf wrapper. Have been running a git bisect for a
>>> while now and narrowed it down as follows, but have run out of time today.
>>>
>>> [..]
>>
>> OK its 70ca163bc558b2194fd9e73502bd13073c214ef5 ....
>
> According to SMPTE 377M the values introduced by this patch are the
> correct ones. I suspect the code which handle the frame layout values to be
> wrong (maybe i am missing something).
>
> In case of mixed fields layout the code should not double the fields heigh
> to get the frame heigh since (quoting SMPTE):
>
> MIXED_FIELDS (3): an interlaced lattice as for SEPARATE_FIELDS above, stored
> as a *single* matrix of interleaved lines 1,2,3,4,5,6,...
>
> So i beleive that doubling the field height to get the frame heigh is only
> valid for separate fields layout.
>
> Can anyone comment on this ?
That would seem right from my reading of SMPTE, and soundings from other
colleagues concur. However I have just noticed in your subsequent patch the
following adjacent code:-

@@ -1520,6 +1520,7 @@ static int
mxf_parse_structural_metadata(MXFContext *mxf)
                 case SeparateFields:
                 case MixedFields:
                     st->codec->height *= 2; /* Turn field height into
frame height. */
+                    break;
                 default:
                     av_log(mxf->fc, AV_LOG_INFO, "Unknown frame layout
type: %d\n", descriptor->frame_layout);
             }

It looks like mixed and separate are being handled the same, should it
not be:-

                 case SeparateFields:
                     st->codec->height *= 2; /* Turn field height into
frame height. */
                     break;
                 case MixedFields:
                     break;
                 default:
                     av_log(mxf->fc, AV_LOG_INFO, "Unknown frame layout
type: %d\n", descriptor->frame_layout);
             }

I have not yet looked at it in context though...
>
> [...]
>
> I can confirm the regression with the sample you gave me when i was working on
> timecode extraction (BRD35721202.mxf).
>
-- 
Tim