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

---
 libavformat/mxf.h    |  1 +
 libavformat/mxfdec.c | 32 +++++++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 5 deletions(-)

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

>
> 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".

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

>
> 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".