[FFmpeg-devel] Regression in mxf decoding

[Matthieu Bouron]

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:-
> 
> @@ -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...

Corresponding patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mxfdec-fix-frame-height-computation-for-mixed-fields.patch
Type: text/x-diff
Size: 1148 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120521/93664902/attachment.bin>