[PATCH] Fix decoding of DNxHD video in MXF container
I found two problems with decoding of DNxHD encoded videos in MXF container. 1. The DNxHD codec signature in libavformat/mxf.c is wrong. 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. The patches fixing the decoding problems are attached. Note that I didn't look at MXF encoding, which might or might not be affected by problem 2. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd
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?
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.
@@ -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 patches fixing the decoding problems are attached. Note that I didn't look at MXF encoding, which might or might not be affected by problem 2.
This would need lavc to flag interlacing somehow, which only happens per frame ATM (I think). /Tomas
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. This change can't break anything, because DNxHD in MXF never worked in the first place, because of the other bug I am addressing.
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. 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.
@@ -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.
The patches fixing the decoding problems are attached. Note that I didn't look at MXF encoding, which might or might not be affected by problem 2. This would need lavc to flag interlacing somehow, which only happens per frame ATM (I think). Maybe. We aren't using FFMpeg for encoding MXFs though, so that's not up to me to implement.
-- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd
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
Updated versions of my patches are attached. The old DNxHD signature is retained and MXFFrameLayout enum was introduced. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd
On 07/02/2012 14:47, Joseph Artsimovich wrote:
Updated versions of my patches are attached. The old DNxHD signature is retained and MXFFrameLayout enum was introduced. Sent the wrong version that wouldn't build. Correct version attached.
-- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd
On 02/07/2012 06:55 AM, Joseph Artsimovich wrote:
On 07/02/2012 14:47, Joseph Artsimovich wrote:
Updated versions of my patches are attached. The old DNxHD signature is retained and MXFFrameLayout enum was introduced. Sent the wrong version that wouldn't build. Correct version attached.
0001-Fix-frame-height-vs-field-height-confusion-in-MXF-de.patch
LGTM
From b3f3ef7ecfc03fb6c501173b50115334f422f81e Mon Sep 17 00:00:00 2001 From: Joseph Artsimovich <joseph@mirriad.com> Date: Tue, 7 Feb 2012 10:48:43 +0000 Subject: [PATCH 1/2] Fix frame height vs field height confusion in MXF decoding.
0002-Add-correct-essence-signature-for-VC-3-DNxHD.patch
From 4c8c3ec9f0c2c8a627b05e9d76c9031933c41140 Mon Sep 17 00:00:00 2001 From: Joseph Artsimovich <joseph@mirriad.com> Date: Tue, 7 Feb 2012 10:49:49 +0000 Subject: [PATCH 2/2] Add correct essence signature for VC-3/DNxHD.
The old (probably incorrect) one is still there just in case. --- libavformat/mxf.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/libavformat/mxf.c b/libavformat/mxf.c index b39a9b6..191d93b 100644 --- a/libavformat/mxf.c +++ b/libavformat/mxf.c @@ -41,7 +41,8 @@ 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,0x03,0x02,0x00,0x00 }, 14, CODEC_ID_DNXHD }, /* ??? possibly wrong */ + { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x71,0x00,0x00,0x00 }, 13, CODEC_ID_DNXHD }, /* SMPTE VC-3/DNxHD */ { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x02,0x01,0x32,0x00,0x00 }, 14, CODEC_ID_H264 }, /* H.264/MPEG-4 AVC Intra */ { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02,0x02,0x00 }, 15, CODEC_ID_V210 }, /* V210 */ /* SoundEssenceCompression */
I think it's fine to remove the old one -- Baptiste COUDURIER Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA FFmpeg maintainer http://www.ffmpeg.org
On Tue, 2012-02-07 at 14:55 +0000, Joseph Artsimovich wrote:
On 07/02/2012 14:47, Joseph Artsimovich wrote:
Updated versions of my patches are attached. The old DNxHD signature is retained and MXFFrameLayout enum was introduced. Sent the wrong version that wouldn't build. Correct version attached.
+ 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. */
Strange indentation makes this a little hard to read. Both LGTM otherwise. /Tomas
On Wed, Feb 08, 2012 at 09:41:04AM +0100, Tomas Härdin wrote:
On Tue, 2012-02-07 at 14:55 +0000, Joseph Artsimovich wrote:
On 07/02/2012 14:47, Joseph Artsimovich wrote:
Updated versions of my patches are attached. The old DNxHD signature is retained and MXFFrameLayout enum was introduced. Sent the wrong version that wouldn't build. Correct version attached.
+ 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. */
Strange indentation makes this a little hard to read. Both LGTM otherwise.
First patch applied second patch seems to have been applied already Thanks to Joseph for the patch and baptiste and tomas for the review and sorry for the delay, i was hoping someone else would be fixing the indention so i didnt had to do it ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"
participants (4)
-
Baptiste Coudurier -
Joseph Artsimovich -
Michael Niedermayer -
Tomas Härdin