[FFmpeg-devel] Regression in mxf decoding

[Matthieu Bouron]

On Mon, May 21, 2012 at 01:24:33PM +0200, 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:-
> > 
> > @@ -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.

> From c591cc86e301c142f2c842e5e5f0dbaacd3698f0 Mon Sep 17 00:00:00 2001
> From: Matthieu Bouron <matthieu.bouron at smartjog.com>
> Date: Mon, 21 May 2012 13:05:01 +0200
> Subject: [PATCH] mxfdec: fix frame height computation for mixed fields layout
> 
> ---
>  libavformat/mxfdec.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 8da3367..4cfbf95 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1517,8 +1517,9 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>                      break; /* The correct thing to do here is fall through, but by breaking we might be
>                                able to decode some streams at half the vertical resolution, rather than not al all.
>                                It's also for compatibility with the old behavior. */
> -                case SeparateFields:
>                  case MixedFields:
> +                    break;
> +                case SeparateFields:
>                      st->codec->height *= 2; /* Turn field height into frame height. */
>                      break;
>                  default:
> -- 
> 1.7.10
> 

ping