[FFmpeg-devel] [PATCH] Fix decoding of DNxHD video in MXF container

[Tomas Härdin]

On Thu, 2012-02-02 at 09:36 +0000, Joseph Artsimovich wrote:
> On 01/02/2012 20:13, Tomas Härdin wrote:
> > On Tue, 2012-01-31 at 13:22 +0000, Joseph Artsimovich wrote:
> >> I found two problems with decoding of DNxHD encoded videos in MXF
> >> container.
> >>
> >> 1. The DNxHD codec signature in libavformat/mxf.c is wrong.
> >> @@ -41,7 +41,7 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
> >>       { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x02,0x01,0x02,0x00 }, 13,    CODEC_ID_DVVIDEO }, /* DV25 IEC PAL */
> >>       { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x07,0x04,0x01,0x02,0x02,0x03,0x01,0x01,0x00 }, 14,   CODEC_ID_JPEG2000 }, /* JPEG2000 Codestream */
> >>       { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x01,0x7F,0x00,0x00,0x00 }, 13,   CODEC_ID_RAWVIDEO }, /* Uncompressed */
> >> -    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x03,0x02,0x00,0x00 }, 14,      CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD */
> >> +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x71,0x00,0x00,0x00 }, 13,      CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD */
> > Why not simply add the new one instead of risking old files breaking?
> > Do we know what sample prompted the old value?
> My data comes from SMPTE 2019-4-2009 and is consistent with samples I've 
> got. In fact, the byte that differs, that is 0x71 *is* what identifies 
> VC-3 encoding.

Indeed, I just double-checked in RP224. It also doesn't list the old UL,
the closest match being:

 06.0E.2B.34.04.01.01.07 04.01.02.02.03.00.00.00 = Individual Picture Coding Schemes

On a related note, I'm working on a command line tool for identifying
ULs.

> This change can't break anything, because DNxHD in MXF 
> never worked in the first place, because of the other bug I am addressing.

I wouldn't be so sure about that - progressive DNxHD with that UL would
work, no? I wouldn't want anything to break..
Baptiste, do you happen to remember what prompted the old UL?

> >> 2. Frame height is calculated incorrectly for interlaced videos.  What's
> >> stored in PictureEssenceDescriptor in MXF is not a frame height but
> >> field height.
> >> This is a general problem that should affect other codecs.  I checked
> >> how D-10 (IMX) in MXF container ends up being handled correctly, and it
> >> turned out it's because of stream parsing.
> > Does the DNxHD essence format not store resolution or does the decoder
> > simply not parse it?
> > This is obviously an issue for raw interlaced video though.
> DNxHD resolution can be derived from compression id, which is stored in 
> the compressed stream. DNxHD stream parsing is not implemented in 
> FFMpeg.

Then the DNxHD decoder should be fixed..

> Anyway, the bug is in MXF parsing and is not limited to DNxHD. I 
> don't have access to the MXF spec, but I've got "The MXF book" that 
> confirms the value FFMpeg is treating as frame height is actually a 
> field height.

Indeed. Anyway, this fix is at least an improvement. We can fix the
decoder later.

> >> @@ -1391,7 +1395,9 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> >>               if (st->codec->codec_id == CODEC_ID_NONE)
> >>                   st->codec->codec_id = container_ul->id;
> >>               st->codec->width = descriptor->width;
> >> -            st->codec->height = descriptor->height;
> >> +            st->codec->height = descriptor->height; /* field height, not frame height */
> >> +            if (descriptor->frame_layout == 1) /* interlaced */
> >> +                st->codec->height *= 2;
> > What about frame_layout == 2 (I forget its name)?
> > The number 1 is magic - consider adding an enum for all FrameLayout
> > values.
> "The MXF book" doesn't list the possible values and I can't find where I 
> got it from. If you point me to the right direction, I'll be happy to 
> make it an enum.

I have the specs, but I admit that finding a list of those values is..
hard. I know this though:

 FullFrame = 0
 MixedFields = 3

I suppose we can prettify this later though. I'm sick atm and thus in no
mood for digging through specs..

/Tomas