diff mbox series

[FFmpeg-devel] avformat/mxfdec: fix pixel format extraction for cinema j2k

Message ID 20200913224218.60972-1-remiachard@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mxfdec: fix pixel format extraction for cinema j2k
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Rémi Achard Sept. 13, 2020, 10:42 p.m. UTC
---
 libavformat/mxf.h    |  1 +
 libavformat/mxfdec.c | 32 +++++++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 5 deletions(-)

Comments

Tomas Härdin Sept. 14, 2020, 9:40 a.m. UTC | #1
sön 2020-09-13 klockan 23:42 +0100 skrev Rémi Achard:
> 
> @@ -855,15 +857,19 @@ static int mxf_read_cryptographic_context(void *arg, AVIOContext *pb, int tag, i
>  
>  static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs, int *count)
>  {
> -    *count = avio_rb32(pb);
> -    av_free(*refs);
> -    *refs = av_calloc(*count, sizeof(UID));
> +    int local_count;
> +
> +    local_count = avio_rb32(pb);
> +    *refs = av_realloc_array(*refs, *count + local_count, sizeof(UID));
> +
>      if (!*refs) {
>          *count = 0;
>          return AVERROR(ENOMEM);
>      }
>      avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> -    avio_read(pb, (uint8_t *)*refs, *count * sizeof(UID));
> +    avio_read(pb, (uint8_t *)(*refs)[*count], local_count * sizeof(UID));
> +    *count += local_count;
> +
>      return 0;
>  }

Why this added hunk? Appending strong refs like this is likely to have
all sorts of unintended effects. The same MaterialPackages will appear
multiple times for example.

/Tomas
Rémi Achard Sept. 14, 2020, 9:48 a.m. UTC | #2
>
> Why this added hunk? Appending strong refs like this is likely to have
> all sorts of unintended effects. The same MaterialPackages will appear
> multiple times for example.


This is my attempt to fix an issue in the fate tests.

If you look at mxf_read_generic_descriptor, there is two places where we
call mxf_read_strong_ref_array, if this happens on the same descriptor the
second call will override the first one. This happen in the test suite for
an XAVC file CDCI descriptor, see the previous post I made for the breaking
test example.

I'll look into your concerns.

Le lun. 14 sept. 2020 à 10:40, Tomas Härdin <tjoppen@acc.umu.se> a écrit :

> sön 2020-09-13 klockan 23:42 +0100 skrev Rémi Achard:
> >
> > @@ -855,15 +857,19 @@ static int mxf_read_cryptographic_context(void
> *arg, AVIOContext *pb, int tag, i
> >
> >  static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs, int
> *count)
> >  {
> > -    *count = avio_rb32(pb);
> > -    av_free(*refs);
> > -    *refs = av_calloc(*count, sizeof(UID));
> > +    int local_count;
> > +
> > +    local_count = avio_rb32(pb);
> > +    *refs = av_realloc_array(*refs, *count + local_count, sizeof(UID));
> > +
> >      if (!*refs) {
> >          *count = 0;
> >          return AVERROR(ENOMEM);
> >      }
> >      avio_skip(pb, 4); /* useless size of objects, always 16 according
> to specs */
> > -    avio_read(pb, (uint8_t *)*refs, *count * sizeof(UID));
> > +    avio_read(pb, (uint8_t *)(*refs)[*count], local_count *
> sizeof(UID));
> > +    *count += local_count;
> > +
> >      return 0;
> >  }
>
> Why this added hunk? Appending strong refs like this is likely to have
> all sorts of unintended effects. The same MaterialPackages will appear
> multiple times for example.
>
> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Tomas Härdin Sept. 14, 2020, 10:36 a.m. UTC | #3
mån 2020-09-14 klockan 10:48 +0100 skrev Rémi Achard:
> > Why this added hunk? Appending strong refs like this is likely to have
> > all sorts of unintended effects. The same MaterialPackages will appear
> > multiple times for example.
> 
> This is my attempt to fix an issue in the fate tests.

FATE works fine for me, at least with a default configuration. Can you
provide what options you're using?

> If you look at mxf_read_generic_descriptor, there is two places where we
> call mxf_read_strong_ref_array, if this happens on the same descriptor the
> second call will override the first one. This happen in the test suite for
> an XAVC file CDCI descriptor, see the previous post I made for the breaking
> test example.

You mean there's both a subdescriptor using local tag 0x3F01 and one
using mxf_subdescriptor_array_smpte or mxf_subdescriptor_array_interop?
In the same CDCIDescriptor?

/Tomas
Rémi Achard Sept. 14, 2020, 7:59 p.m. UTC | #4
>
> FATE works fine for me, at least with a default configuration. Can you
> provide what options you're using?


So the original patch I submitted errored in the test suite, you can see
the error here
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200913184510.85070-1-remiachard@gmail.com/

I managed to reproduce the error, just didn't rsync the fate-suite the
first time, what options do you want me to provide ?

You mean there's both a subdescriptor using local tag 0x3F01 and one
> using mxf_subdescriptor_array_smpte or mxf_subdescriptor_array_interop?
> In the same CDCIDescriptor?


 Just tried to dig a bit deeper and it appears to be a Multi Descriptor Set
that contains at least 6 (1 more for each additional audio tracks) Sub
Descriptors, including the mentioned CICD, and a Container Constraints Sub
Descriptor Set. For this Multi Descriptor, mxf_read_strong_ref_array is
called multiple times and the original patches mess things up. I'm sourcing
this info from both the XAVC spec and the mxfdec debug output that I ran on
the fate-suite MXF XAVC as well as some XAVC I've got lying around.

Do you think this array reallocation must move
outside mxf_read_strong_ref_array back to mxf_read_generic_descriptor or
this involve more structural changes ?

Le lun. 14 sept. 2020 à 11:37, Tomas Härdin <tjoppen@acc.umu.se> a écrit :

> mån 2020-09-14 klockan 10:48 +0100 skrev Rémi Achard:
> > > Why this added hunk? Appending strong refs like this is likely to have
> > > all sorts of unintended effects. The same MaterialPackages will appear
> > > multiple times for example.
> >
> > This is my attempt to fix an issue in the fate tests.
>
> FATE works fine for me, at least with a default configuration. Can you
> provide what options you're using?
>
> > If you look at mxf_read_generic_descriptor, there is two places where we
> > call mxf_read_strong_ref_array, if this happens on the same descriptor
> the
> > second call will override the first one. This happen in the test suite
> for
> > an XAVC file CDCI descriptor, see the previous post I made for the
> breaking
> > test example.
>
> You mean there's both a subdescriptor using local tag 0x3F01 and one
> using mxf_subdescriptor_array_smpte or mxf_subdescriptor_array_interop?
> In the same CDCIDescriptor?
>
> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index fc587f19f0..3fb3c6d74d 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -49,6 +49,7 @@  enum MXFMetadataSetType {
     TaggedValue,
     TapeDescriptor,
     AVCSubDescriptor,
+    JPEG2000PictureSubDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 6f6e8d586c..c94876804c 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -325,9 +325,11 @@  static const uint8_t mxf_encrypted_essence_container[]     = { 0x06,0x0e,0x2b,0x
 static const uint8_t mxf_random_index_pack_key[]           = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x11,0x01,0x00 };
 static const uint8_t mxf_sony_mpeg4_extradata[]            = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0e,0x06,0x06,0x02,0x02,0x01,0x00,0x00 };
 static const uint8_t mxf_avid_project_name[]               = { 0xa5,0xfb,0x7b,0x25,0xf6,0x15,0x94,0xb9,0x62,0xfc,0x37,0x17,0x49,0x2d,0x42,0xbf };
-static const uint8_t mxf_jp2k_rsiz[]                       = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02,0x01,0x00 };
+static const uint8_t mxf_jp2k_rsiz[]                       = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x01,0x00,0x00,0x00 };
 static const uint8_t mxf_indirect_value_utf16le[]          = { 0x4c,0x00,0x02,0x10,0x01,0x00,0x00,0x00,0x00,0x06,0x0e,0x2b,0x34,0x01,0x04,0x01,0x01 };
 static const uint8_t mxf_indirect_value_utf16be[]          = { 0x42,0x01,0x10,0x02,0x00,0x00,0x00,0x00,0x00,0x06,0x0e,0x2b,0x34,0x01,0x04,0x01,0x01 };
+static const uint8_t mxf_subdescriptor_array_smpte[]       = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x04,0x06,0x10,0x00,0x00 };
+static const uint8_t mxf_subdescriptor_array_interop[]     = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x02,0x06,0x01,0x01,0x04,0x06,0x10,0x00,0x00 };
 
 #define IS_KLV_KEY(x, y) (!memcmp(x, y, sizeof(y)))
 
@@ -855,15 +857,19 @@  static int mxf_read_cryptographic_context(void *arg, AVIOContext *pb, int tag, i
 
 static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs, int *count)
 {
-    *count = avio_rb32(pb);
-    av_free(*refs);
-    *refs = av_calloc(*count, sizeof(UID));
+    int local_count;
+
+    local_count = avio_rb32(pb);
+    *refs = av_realloc_array(*refs, *count + local_count, sizeof(UID));
+
     if (!*refs) {
         *count = 0;
         return AVERROR(ENOMEM);
     }
     avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
-    avio_read(pb, (uint8_t *)*refs, *count * sizeof(UID));
+    avio_read(pb, (uint8_t *)(*refs)[*count], local_count * sizeof(UID));
+    *count += local_count;
+
     return 0;
 }
 
@@ -1272,6 +1278,11 @@  static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
                 rsiz == FF_PROFILE_JPEG2000_DCINEMA_4K)
                 descriptor->pix_fmt = AV_PIX_FMT_XYZ12;
         }
+        if (IS_KLV_KEY(uid, mxf_subdescriptor_array_smpte)
+         || IS_KLV_KEY(uid, mxf_subdescriptor_array_interop)) {
+            return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
+                                                 &descriptor->sub_descriptors_count);
+        }
         break;
     }
     return 0;
@@ -2498,6 +2509,16 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
                 }
             }
 
+            if (st->codecpar->codec_id == AV_CODEC_ID_JPEG2000) {
+                MXFDescriptor *desc = NULL;
+                for (k = 0; k < descriptor->sub_descriptors_count; k++) {
+                    if ((desc = mxf_resolve_strong_ref(mxf, &descriptor->sub_descriptors_refs[k], JPEG2000PictureSubDescriptor))) {
+                        st->codecpar->format = desc->pix_fmt;
+                        break;
+                    }
+                }
+            }
+
             if (st->codecpar->codec_id == AV_CODEC_ID_RAWVIDEO) {
                 st->codecpar->format = descriptor->pix_fmt;
                 if (st->codecpar->format == AV_PIX_FMT_NONE) {
@@ -2753,6 +2774,7 @@  static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x5c,0x00 }, mxf_read_generic_descriptor, sizeof(MXFDescriptor), Descriptor }, /* VANC/VBI - SMPTE 436M */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x5e,0x00 }, mxf_read_generic_descriptor, sizeof(MXFDescriptor), Descriptor }, /* MPEG2AudioDescriptor */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x64,0x00 }, mxf_read_generic_descriptor, sizeof(MXFDescriptor), Descriptor }, /* DC Timed Text Descriptor */
+    { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x5a,0x00 }, mxf_read_generic_descriptor, sizeof(MXFDescriptor), JPEG2000PictureSubDescriptor }, /* JPEG2000PictureSubDescriptor */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3A,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Static Track */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3B,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Generic Track */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x14,0x00 }, mxf_read_timecode_component, sizeof(MXFTimecodeComponent), TimecodeComponent },