Re: Regression in mxf decoding

On 21/05/12 12:24, Matthieu Bouron wrote:
> On Mon, May 21, 2012 at 11:18:12AM +0100, Tim Nicholson wrote:
>> 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:-
>>
>>  <at>  <at>  -1520,6 +1520,7  <at>  <at>  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...
> 
> Corresponding patch attached.

There is a superfluous ">" in the first line that makes 'git am' barf.

I did the same, and it seems to have done the trick. Happy to go with
yours as you've published it...

--

-- 
Tim