[FFmpeg-devel] Regression in mxf decoding

[Michael Niedermayer]

On Tue, May 22, 2012 at 10:51:03AM +0200, Matthieu Bouron wrote:
> 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

applied

thanks!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120522/ce1ed920/attachment.asc>