diff mbox series

[FFmpeg-devel] avformat/mov: Only read the primary item for AVIF

Message ID 20220424183514.986374-1-vigneshv@google.com
State New
Headers show
Series [FFmpeg-devel] avformat/mov: Only read the primary item for AVIF | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Vignesh Venkatasubramanian April 24, 2022, 6:35 p.m. UTC
Update the still AVIF parser to only read the primary item. With this
patch, AVIF still images with exif/icc/alpha channel will no longer
fail to parse.

For example, this patch enables parsing of files in:
https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft

Partially fixes trac ticket #7621

Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com>
---
 libavformat/isom.h |  1 +
 libavformat/mov.c  | 41 +++++++++++++++++++++--------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

Comments

Vignesh Venkatasubramanian May 2, 2022, 9:39 p.m. UTC | #1
On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> Update the still AVIF parser to only read the primary item. With this
> patch, AVIF still images with exif/icc/alpha channel will no longer
> fail to parse.
>
> For example, this patch enables parsing of files in:
> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
>
> Partially fixes trac ticket #7621
>
> Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com>
> ---
>  libavformat/isom.h |  1 +
>  libavformat/mov.c  | 41 +++++++++++++++++++++--------------------
>  2 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index cf36f04d5b..f05c2d9c28 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -317,6 +317,7 @@ typedef struct MOVContext {
>      uint32_t mfra_size;
>      uint32_t max_stts_delta;
>      int is_still_picture_avif;
> +    int primary_item_id;
>  } MOVContext;
>
>  int ff_mp4_read_descr_len(AVIOContext *pb);
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 3e83e54a77..6be0f317ca 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7449,6 +7449,13 @@ static int rb_size(AVIOContext *pb, uint64_t* value, int size)
>      return size;
>  }
>
> +static int mov_read_pitm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +    avio_rb32(pb);  // version & flags.
> +    c->primary_item_id = avio_rb16(pb);
> +    return atom.size;
> +}
> +
>  static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      int version, offset_size, length_size, base_offset_size, index_size;
> @@ -7505,34 +7512,25 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          return AVERROR_PATCHWELCOME;
>      }
>      item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
> -    if (item_count > 1) {
> -        // For still AVIF images, we only support one item. Second item will
> -        // generally be found for AVIF images with alpha channel. We don't
> -        // support them as of now.
> -        av_log(c->fc, AV_LOG_ERROR, "iloc: item_count > 1 not supported.\n");
> -        return AVERROR_PATCHWELCOME;
> -    }
>
>      // Populate the necessary fields used by mov_build_index.
> -    sc->stsc_count = item_count;
> -    sc->stsc_data = av_malloc_array(item_count, sizeof(*sc->stsc_data));
> +    sc->stsc_count = 1;
> +    sc->stsc_data = av_malloc_array(1, sizeof(*sc->stsc_data));
>      if (!sc->stsc_data)
>          return AVERROR(ENOMEM);
>      sc->stsc_data[0].first = 1;
>      sc->stsc_data[0].count = 1;
>      sc->stsc_data[0].id = 1;
> -    sc->chunk_count = item_count;
> -    sc->chunk_offsets =
> -        av_malloc_array(item_count, sizeof(*sc->chunk_offsets));
> +    sc->chunk_count = 1;
> +    sc->chunk_offsets = av_malloc_array(1, sizeof(*sc->chunk_offsets));
>      if (!sc->chunk_offsets)
>          return AVERROR(ENOMEM);
> -    sc->sample_count = item_count;
> -    sc->sample_sizes =
> -        av_malloc_array(item_count, sizeof(*sc->sample_sizes));
> +    sc->sample_count = 1;
> +    sc->sample_sizes = av_malloc_array(1, sizeof(*sc->sample_sizes));
>      if (!sc->sample_sizes)
>          return AVERROR(ENOMEM);
> -    sc->stts_count = item_count;
> -    sc->stts_data = av_malloc_array(item_count, sizeof(*sc->stts_data));
> +    sc->stts_count = 1;
> +    sc->stts_data = av_malloc_array(1, sizeof(*sc->stts_data));
>      if (!sc->stts_data)
>          return AVERROR(ENOMEM);
>      sc->stts_data[0].count = 1;
> @@ -7540,7 +7538,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      sc->stts_data[0].duration = 0;
>
>      for (int i = 0; i < item_count; i++) {
> -        (version < 2) ? avio_rb16(pb) : avio_rb32(pb);  // item_id;
> +        int item_id = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
>          if (version > 0)
>              avio_rb16(pb);  // construction_method.
>          avio_rb16(pb);  // data_reference_index.
> @@ -7556,8 +7554,10 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>              if (rb_size(pb, &extent_offset, offset_size) < 0 ||
>                  rb_size(pb, &extent_length, length_size) < 0)
>                  return AVERROR_INVALIDDATA;
> -            sc->sample_sizes[0] = extent_length;
> -            sc->chunk_offsets[0] = base_offset + extent_offset;
> +            if (item_id == c->primary_item_id) {
> +                sc->sample_sizes[0] = extent_length;
> +                sc->chunk_offsets[0] = base_offset + extent_offset;
> +            }
>          }
>      }
>
> @@ -7674,6 +7674,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = {
>  { MKTAG('S','A','3','D'), mov_read_SA3D }, /* ambisonic audio box */
>  { MKTAG('S','A','N','D'), mov_read_SAND }, /* non diegetic audio box */
>  { MKTAG('i','l','o','c'), mov_read_iloc },
> +{ MKTAG('p','i','t','m'), mov_read_pitm },
>  { 0, NULL }
>  };
>
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>

Any comments on this one? If not, can this be merged please? :)
Vignesh Venkatasubramanian May 9, 2022, 4:32 p.m. UTC | #2
On Mon, May 2, 2022 at 2:39 PM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> <vigneshv@google.com> wrote:
> >
> > Update the still AVIF parser to only read the primary item. With this
> > patch, AVIF still images with exif/icc/alpha channel will no longer
> > fail to parse.
> >
> > For example, this patch enables parsing of files in:
> > https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> >
> > Partially fixes trac ticket #7621
> >
> > Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com>
> > ---
> >  libavformat/isom.h |  1 +
> >  libavformat/mov.c  | 41 +++++++++++++++++++++--------------------
> >  2 files changed, 22 insertions(+), 20 deletions(-)
> >
> > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > index cf36f04d5b..f05c2d9c28 100644
> > --- a/libavformat/isom.h
> > +++ b/libavformat/isom.h
> > @@ -317,6 +317,7 @@ typedef struct MOVContext {
> >      uint32_t mfra_size;
> >      uint32_t max_stts_delta;
> >      int is_still_picture_avif;
> > +    int primary_item_id;
> >  } MOVContext;
> >
> >  int ff_mp4_read_descr_len(AVIOContext *pb);
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 3e83e54a77..6be0f317ca 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -7449,6 +7449,13 @@ static int rb_size(AVIOContext *pb, uint64_t* value, int size)
> >      return size;
> >  }
> >
> > +static int mov_read_pitm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > +{
> > +    avio_rb32(pb);  // version & flags.
> > +    c->primary_item_id = avio_rb16(pb);
> > +    return atom.size;
> > +}
> > +
> >  static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> >      int version, offset_size, length_size, base_offset_size, index_size;
> > @@ -7505,34 +7512,25 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >          return AVERROR_PATCHWELCOME;
> >      }
> >      item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
> > -    if (item_count > 1) {
> > -        // For still AVIF images, we only support one item. Second item will
> > -        // generally be found for AVIF images with alpha channel. We don't
> > -        // support them as of now.
> > -        av_log(c->fc, AV_LOG_ERROR, "iloc: item_count > 1 not supported.\n");
> > -        return AVERROR_PATCHWELCOME;
> > -    }
> >
> >      // Populate the necessary fields used by mov_build_index.
> > -    sc->stsc_count = item_count;
> > -    sc->stsc_data = av_malloc_array(item_count, sizeof(*sc->stsc_data));
> > +    sc->stsc_count = 1;
> > +    sc->stsc_data = av_malloc_array(1, sizeof(*sc->stsc_data));
> >      if (!sc->stsc_data)
> >          return AVERROR(ENOMEM);
> >      sc->stsc_data[0].first = 1;
> >      sc->stsc_data[0].count = 1;
> >      sc->stsc_data[0].id = 1;
> > -    sc->chunk_count = item_count;
> > -    sc->chunk_offsets =
> > -        av_malloc_array(item_count, sizeof(*sc->chunk_offsets));
> > +    sc->chunk_count = 1;
> > +    sc->chunk_offsets = av_malloc_array(1, sizeof(*sc->chunk_offsets));
> >      if (!sc->chunk_offsets)
> >          return AVERROR(ENOMEM);
> > -    sc->sample_count = item_count;
> > -    sc->sample_sizes =
> > -        av_malloc_array(item_count, sizeof(*sc->sample_sizes));
> > +    sc->sample_count = 1;
> > +    sc->sample_sizes = av_malloc_array(1, sizeof(*sc->sample_sizes));
> >      if (!sc->sample_sizes)
> >          return AVERROR(ENOMEM);
> > -    sc->stts_count = item_count;
> > -    sc->stts_data = av_malloc_array(item_count, sizeof(*sc->stts_data));
> > +    sc->stts_count = 1;
> > +    sc->stts_data = av_malloc_array(1, sizeof(*sc->stts_data));
> >      if (!sc->stts_data)
> >          return AVERROR(ENOMEM);
> >      sc->stts_data[0].count = 1;
> > @@ -7540,7 +7538,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >      sc->stts_data[0].duration = 0;
> >
> >      for (int i = 0; i < item_count; i++) {
> > -        (version < 2) ? avio_rb16(pb) : avio_rb32(pb);  // item_id;
> > +        int item_id = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
> >          if (version > 0)
> >              avio_rb16(pb);  // construction_method.
> >          avio_rb16(pb);  // data_reference_index.
> > @@ -7556,8 +7554,10 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >              if (rb_size(pb, &extent_offset, offset_size) < 0 ||
> >                  rb_size(pb, &extent_length, length_size) < 0)
> >                  return AVERROR_INVALIDDATA;
> > -            sc->sample_sizes[0] = extent_length;
> > -            sc->chunk_offsets[0] = base_offset + extent_offset;
> > +            if (item_id == c->primary_item_id) {
> > +                sc->sample_sizes[0] = extent_length;
> > +                sc->chunk_offsets[0] = base_offset + extent_offset;
> > +            }
> >          }
> >      }
> >
> > @@ -7674,6 +7674,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = {
> >  { MKTAG('S','A','3','D'), mov_read_SA3D }, /* ambisonic audio box */
> >  { MKTAG('S','A','N','D'), mov_read_SAND }, /* non diegetic audio box */
> >  { MKTAG('i','l','o','c'), mov_read_iloc },
> > +{ MKTAG('p','i','t','m'), mov_read_pitm },
> >  { 0, NULL }
> >  };
> >
> > --
> > 2.36.0.rc2.479.g8af0fa9b8e-goog
> >
>
> Any comments on this one? If not, can this be merged please? :)
>

Another ping on this.

> --
> Vignesh
James Zern June 1, 2022, 5:30 p.m. UTC | #3
On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
<vigneshv-at-google.com@ffmpeg.org> wrote:
>
> Update the still AVIF parser to only read the primary item. With this
> patch, AVIF still images with exif/icc/alpha channel will no longer
> fail to parse.
>
> For example, this patch enables parsing of files in:
> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
>

Can some of the failing files or similar ones be added to fate?
Vignesh Venkatasubramanian June 1, 2022, 8:38 p.m. UTC | #4
On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> <vigneshv-at-google.com@ffmpeg.org> wrote:
> >
> > Update the still AVIF parser to only read the primary item. With this
> > patch, AVIF still images with exif/icc/alpha channel will no longer
> > fail to parse.
> >
> > For example, this patch enables parsing of files in:
> > https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> >
>
> Can some of the failing files or similar ones be added to fate?

There are no fate tests for AVIF parsing as of now. I was thinking
about adding some for the muxing. But i am not sure what can be done
here for the parsing.
James Zern June 2, 2022, 8:34 p.m. UTC | #5
On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
> >
> > On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> > <vigneshv-at-google.com@ffmpeg.org> wrote:
> > >
> > > Update the still AVIF parser to only read the primary item. With this
> > > patch, AVIF still images with exif/icc/alpha channel will no longer
> > > fail to parse.
> > >
> > > For example, this patch enables parsing of files in:
> > > https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> > >
> >
> > Can some of the failing files or similar ones be added to fate?
>
> There are no fate tests for AVIF parsing as of now. I was thinking
> about adding some for the muxing. But i am not sure what can be done
> here for the parsing.
>

Thanks. Are there any for general mov/mp4 parsing that could be
extended? This looks good otherwise.
Vignesh Venkatasubramanian June 8, 2022, 5:21 p.m. UTC | #6
On Thu, Jun 2, 2022 at 1:35 PM James Zern <jzern@google.com> wrote:
>
> On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
> <vigneshv@google.com> wrote:
> >
> > On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
> > >
> > > On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> > > <vigneshv-at-google.com@ffmpeg.org> wrote:
> > > >
> > > > Update the still AVIF parser to only read the primary item. With this
> > > > patch, AVIF still images with exif/icc/alpha channel will no longer
> > > > fail to parse.
> > > >
> > > > For example, this patch enables parsing of files in:
> > > > https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> > > >
> > >
> > > Can some of the failing files or similar ones be added to fate?
> >
> > There are no fate tests for AVIF parsing as of now. I was thinking
> > about adding some for the muxing. But i am not sure what can be done
> > here for the parsing.
> >
>
> Thanks. Are there any for general mov/mp4 parsing that could be
> extended? This looks good otherwise.

From what i see, most of the mov tests only seem to be for muxing. I'm
not entirely certain about how to add a test for AVIF parsing. If
anybody has an idea, i'd be open to adding it.


--
Vignesh
Gyan Doshi June 9, 2022, 7:50 a.m. UTC | #7
On 2022-06-08 10:51 pm, Vignesh Venkatasubramanian wrote:
> On Thu, Jun 2, 2022 at 1:35 PM James Zern <jzern@google.com> wrote:
>> On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
>> <vigneshv@google.com> wrote:
>>> On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
>>>> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
>>>> <vigneshv-at-google.com@ffmpeg.org> wrote:
>>>>> Update the still AVIF parser to only read the primary item. With this
>>>>> patch, AVIF still images with exif/icc/alpha channel will no longer
>>>>> fail to parse.
>>>>>
>>>>> For example, this patch enables parsing of files in:
>>>>> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
>>>>>
>>>> Can some of the failing files or similar ones be added to fate?
>>> There are no fate tests for AVIF parsing as of now. I was thinking
>>> about adding some for the muxing. But i am not sure what can be done
>>> here for the parsing.
>>>
>> Thanks. Are there any for general mov/mp4 parsing that could be
>> extended? This looks good otherwise.
>  From what i see, most of the mov tests only seem to be for muxing. I'm
> not entirely certain about how to add a test for AVIF parsing. If
> anybody has an idea, i'd be open to adding it.

Basic test would use the framemd5 muxer to compare demuxed packets with 
a reference.
See fate-ffmpeg-streamloop

Regards,
Gyan
Vignesh Venkatasubramanian June 10, 2022, 5:34 p.m. UTC | #8
On Thu, Jun 9, 2022 at 12:50 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2022-06-08 10:51 pm, Vignesh Venkatasubramanian wrote:
> > On Thu, Jun 2, 2022 at 1:35 PM James Zern <jzern@google.com> wrote:
> >> On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
> >> <vigneshv@google.com> wrote:
> >>> On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
> >>>> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> >>>> <vigneshv-at-google.com@ffmpeg.org> wrote:
> >>>>> Update the still AVIF parser to only read the primary item. With this
> >>>>> patch, AVIF still images with exif/icc/alpha channel will no longer
> >>>>> fail to parse.
> >>>>>
> >>>>> For example, this patch enables parsing of files in:
> >>>>> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> >>>>>
> >>>> Can some of the failing files or similar ones be added to fate?
> >>> There are no fate tests for AVIF parsing as of now. I was thinking
> >>> about adding some for the muxing. But i am not sure what can be done
> >>> here for the parsing.
> >>>
> >> Thanks. Are there any for general mov/mp4 parsing that could be
> >> extended? This looks good otherwise.
> >  From what i see, most of the mov tests only seem to be for muxing. I'm
> > not entirely certain about how to add a test for AVIF parsing. If
> > anybody has an idea, i'd be open to adding it.
>
> Basic test would use the framemd5 muxer to compare demuxed packets with
> a reference.
> See fate-ffmpeg-streamloop
>

Thank you i have added a couple of fate tests.

I am not sure how to add the AVIF files to the fate test server. I
have them on Google Drive here:

https://drive.google.com/file/d/1diZoM0Ew7Co3Yh5w5y1J-3IiBYVmUv9J/view?usp=sharing
https://drive.google.com/file/d/1DdrD1mW36evt40a4RkeLYpx-oojmbc3z/view?usp=sharing

These links should be publicly available without any sign in required.
Please let me know if there is another preferred way of sharing test
files and i can share it that way.

Also for the record, these files were created by remuxing an existing
file in the fate suite.

still_image.avif - contains the first frame from
av1-test-vectors/av1-1-b8-02-allintra.ivf
still_image_exif.avif - contains the first frame from
av1-test-vectors/av1-1-b8-02-allintra.ivf with dummy exif data.

> Regards,
> Gyan
> _______________________________________________
> 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".
Vignesh Venkatasubramanian June 10, 2022, 5:37 p.m. UTC | #9
On Fri, Jun 10, 2022 at 10:33 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Vignesh Venkatasubramanian:
> > Update the still AVIF parser to only read the primary item. With this
> > patch, AVIF still images with exif/icc/alpha channel will no longer
> > fail to parse.
> >
> > For example, this patch enables parsing of files in:
> > https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> >
> > Adding two fate tests:
> > 1) demuxing of still image with 1 item - this test will pass regardlesss
> >    of this patch.
> > 2) demuxing of still image with 2 items - this test will fail without
> >    this patch and will pass with patch applied.
> >
> > Partially fixes trac ticket #7621
> >
> > Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com>
> > ---
> >  libavformat/isom.h                            |  1 +
> >  libavformat/mov.c                             | 41 ++++++++++---------
> >  tests/fate/mov.mak                            |  9 ++++
> >  .../fate/mov-avif-demux-still-image-1-item    | 11 +++++
> >  .../mov-avif-demux-still-image-multiple-items | 11 +++++
> >  5 files changed, 53 insertions(+), 20 deletions(-)
> >  create mode 100644 tests/ref/fate/mov-avif-demux-still-image-1-item
> >  create mode 100644 tests/ref/fate/mov-avif-demux-still-image-multiple-items
> >
> > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> > index 2fae054423..842c7f9aa1 100644
> > --- a/tests/fate/mov.mak
> > +++ b/tests/fate/mov.mak
> > @@ -17,6 +17,8 @@ FATE_MOV = fate-mov-3elist \
> >             fate-mov-bbi-elst-starts-b \
> >             fate-mov-neg-firstpts-discard-frames \
> >             fate-mov-stream-shorter-than-movie \
> > +           fate-mov-avif-demux-still-image-1-item \
> > +           fate-mov-avif-demux-still-image-multiple-items \
> >
> >  FATE_MOV_FFPROBE = fate-mov-neg-firstpts-discard \
> >                     fate-mov-neg-firstpts-discard-vorbis \
> > @@ -138,6 +140,13 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV, SRT_DEMUXER TTML
> >  fate-mov-mp4-ttml-stpp: CMD = transcode srt $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> >  fate-mov-mp4-ttml-dfxp: CMD = transcode srt $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" "-of json -show_entries packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> >
> > +# avif demuxing - still image with 1 item.
> > +fate-mov-avif-demux-still-image-1-item: CMD = framemd5 -i $(TARGET_SAMPLES)/avif/still_image.avif -c:v copy
> > +
> > +# avif demuxing - still image with multiple items. only the primary item will be
> > +# parsed.
> > +fate-mov-avif-demux-still-image-multiple-items: CMD = framemd5 -i $(TARGET_SAMPLES)/avif/still_image_exif.avif -c:v copy
>
> Can we create such files with our muxer? In this case one could use a
> remux test, i.e. a test that creates the file and the demuxes the just
> created file.
>

Unfortunately, the file with exif cannot be created using the ffmpeg's
muxer as of now. So we will have to get that externally (i made that
with MP4Box).

The other still image file however can be creating using our muxer. I
will add another test that does remuxing for the 1-item case in a
separate patch if that's alright (since it's not directly related to
what this patch is doing). I was also going to add some fate tests for
animated AVIF muxing/parsing, so i will include that in the other
patch as well.

> > +
> >  # Resulting remux should have:
> >  # 1. first audio stream with AV_DISPOSITION_HEARING_IMPAIRED
> >  # 2. second audio stream with AV_DISPOSITION_VISUAL_IMPAIRED | DESCRIPTIONS
> > diff --git a/tests/ref/fate/mov-avif-demux-still-image-1-item b/tests/ref/fate/mov-avif-demux-still-image-1-item
> > new file mode 100644
> > index 0000000000..93773afd4e
> > --- /dev/null
> > +++ b/tests/ref/fate/mov-avif-demux-still-image-1-item
> > @@ -0,0 +1,11 @@
> > +#format: frame checksums
> > +#version: 2
> > +#hash: MD5
> > +#extradata 0,                              13, b52ae298d37128862ef1918cf916239c
> > +#tb 0: 1/1
> > +#media_type 0: video
> > +#codec_id 0: av1
> > +#dimensions 0: 352x288
> > +#sar 0: 1/1
> > +#stream#, dts,        pts, duration,     size, hash
> > +0,          0,          0,        1,    36265, 235b0c6e389c4084845981e08d60db04
> > diff --git a/tests/ref/fate/mov-avif-demux-still-image-multiple-items b/tests/ref/fate/mov-avif-demux-still-image-multiple-items
> > new file mode 100644
> > index 0000000000..93773afd4e
> > --- /dev/null
> > +++ b/tests/ref/fate/mov-avif-demux-still-image-multiple-items
> > @@ -0,0 +1,11 @@
> > +#format: frame checksums
> > +#version: 2
> > +#hash: MD5
> > +#extradata 0,                              13, b52ae298d37128862ef1918cf916239c
> > +#tb 0: 1/1
> > +#media_type 0: video
> > +#codec_id 0: av1
> > +#dimensions 0: 352x288
> > +#sar 0: 1/1
> > +#stream#, dts,        pts, duration,     size, hash
> > +0,          0,          0,        1,    36265, 235b0c6e389c4084845981e08d60db04
>
> _______________________________________________
> 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".
Vignesh Venkatasubramanian June 13, 2022, 4:32 p.m. UTC | #10
On Fri, Jun 10, 2022 at 10:34 AM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> On Thu, Jun 9, 2022 at 12:50 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >
> >
> >
> > On 2022-06-08 10:51 pm, Vignesh Venkatasubramanian wrote:
> > > On Thu, Jun 2, 2022 at 1:35 PM James Zern <jzern@google.com> wrote:
> > >> On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
> > >> <vigneshv@google.com> wrote:
> > >>> On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
> > >>>> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> > >>>> <vigneshv-at-google.com@ffmpeg.org> wrote:
> > >>>>> Update the still AVIF parser to only read the primary item. With this
> > >>>>> patch, AVIF still images with exif/icc/alpha channel will no longer
> > >>>>> fail to parse.
> > >>>>>
> > >>>>> For example, this patch enables parsing of files in:
> > >>>>> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> > >>>>>
> > >>>> Can some of the failing files or similar ones be added to fate?
> > >>> There are no fate tests for AVIF parsing as of now. I was thinking
> > >>> about adding some for the muxing. But i am not sure what can be done
> > >>> here for the parsing.
> > >>>
> > >> Thanks. Are there any for general mov/mp4 parsing that could be
> > >> extended? This looks good otherwise.
> > >  From what i see, most of the mov tests only seem to be for muxing. I'm
> > > not entirely certain about how to add a test for AVIF parsing. If
> > > anybody has an idea, i'd be open to adding it.
> >
> > Basic test would use the framemd5 muxer to compare demuxed packets with
> > a reference.
> > See fate-ffmpeg-streamloop
> >
>
> Thank you i have added a couple of fate tests.
>
> I am not sure how to add the AVIF files to the fate test server. I
> have them on Google Drive here:
>
> https://drive.google.com/file/d/1diZoM0Ew7Co3Yh5w5y1J-3IiBYVmUv9J/view?usp=sharing
> https://drive.google.com/file/d/1DdrD1mW36evt40a4RkeLYpx-oojmbc3z/view?usp=sharing
>
> These links should be publicly available without any sign in required.
> Please let me know if there is another preferred way of sharing test
> files and i can share it that way.
>

Also, i forgot to mention that these files have to be uploaded under a
new directory called "avif" in the fate server.

> Also for the record, these files were created by remuxing an existing
> file in the fate suite.
>
> still_image.avif - contains the first frame from
> av1-test-vectors/av1-1-b8-02-allintra.ivf
> still_image_exif.avif - contains the first frame from
> av1-test-vectors/av1-1-b8-02-allintra.ivf with dummy exif data.
>
> > Regards,
> > Gyan
> > _______________________________________________
> > 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".
>
>
>
> --
> Vignesh
Vignesh Venkatasubramanian June 21, 2022, 5:12 p.m. UTC | #11
On Mon, Jun 13, 2022 at 9:32 AM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> On Fri, Jun 10, 2022 at 10:34 AM Vignesh Venkatasubramanian
> <vigneshv@google.com> wrote:
> >
> > On Thu, Jun 9, 2022 at 12:50 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> > >
> > >
> > >
> > > On 2022-06-08 10:51 pm, Vignesh Venkatasubramanian wrote:
> > > > On Thu, Jun 2, 2022 at 1:35 PM James Zern <jzern@google.com> wrote:
> > > >> On Wed, Jun 1, 2022 at 1:38 PM Vignesh Venkatasubramanian
> > > >> <vigneshv@google.com> wrote:
> > > >>> On Wed, Jun 1, 2022 at 10:30 AM James Zern <jzern@google.com> wrote:
> > > >>>> On Sun, Apr 24, 2022 at 11:35 AM Vignesh Venkatasubramanian
> > > >>>> <vigneshv-at-google.com@ffmpeg.org> wrote:
> > > >>>>> Update the still AVIF parser to only read the primary item. With this
> > > >>>>> patch, AVIF still images with exif/icc/alpha channel will no longer
> > > >>>>> fail to parse.
> > > >>>>>
> > > >>>>> For example, this patch enables parsing of files in:
> > > >>>>> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Microsoft
> > > >>>>>
> > > >>>> Can some of the failing files or similar ones be added to fate?
> > > >>> There are no fate tests for AVIF parsing as of now. I was thinking
> > > >>> about adding some for the muxing. But i am not sure what can be done
> > > >>> here for the parsing.
> > > >>>
> > > >> Thanks. Are there any for general mov/mp4 parsing that could be
> > > >> extended? This looks good otherwise.
> > > >  From what i see, most of the mov tests only seem to be for muxing. I'm
> > > > not entirely certain about how to add a test for AVIF parsing. If
> > > > anybody has an idea, i'd be open to adding it.
> > >
> > > Basic test would use the framemd5 muxer to compare demuxed packets with
> > > a reference.
> > > See fate-ffmpeg-streamloop
> > >
> >
> > Thank you i have added a couple of fate tests.
> >
> > I am not sure how to add the AVIF files to the fate test server. I
> > have them on Google Drive here:
> >
> > https://drive.google.com/file/d/1diZoM0Ew7Co3Yh5w5y1J-3IiBYVmUv9J/view?usp=sharing
> > https://drive.google.com/file/d/1DdrD1mW36evt40a4RkeLYpx-oojmbc3z/view?usp=sharing
> >
> > These links should be publicly available without any sign in required.
> > Please let me know if there is another preferred way of sharing test
> > files and i can share it that way.
> >
>
> Also, i forgot to mention that these files have to be uploaded under a
> new directory called "avif" in the fate server.
>
> > Also for the record, these files were created by remuxing an existing
> > file in the fate suite.
> >
> > still_image.avif - contains the first frame from
> > av1-test-vectors/av1-1-b8-02-allintra.ivf
> > still_image_exif.avif - contains the first frame from
> > av1-test-vectors/av1-1-b8-02-allintra.ivf with dummy exif data.
> >
> > > Regards,
> > > Gyan
> > > _______________________________________________
> > > 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".
> >
> >
> >
> > --
> > Vignesh
>
>
>
> --
> Vignesh

Ping on this please.
diff mbox series

Patch

diff --git a/libavformat/isom.h b/libavformat/isom.h
index cf36f04d5b..f05c2d9c28 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -317,6 +317,7 @@  typedef struct MOVContext {
     uint32_t mfra_size;
     uint32_t max_stts_delta;
     int is_still_picture_avif;
+    int primary_item_id;
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3e83e54a77..6be0f317ca 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7449,6 +7449,13 @@  static int rb_size(AVIOContext *pb, uint64_t* value, int size)
     return size;
 }
 
+static int mov_read_pitm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+    avio_rb32(pb);  // version & flags.
+    c->primary_item_id = avio_rb16(pb);
+    return atom.size;
+}
+
 static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     int version, offset_size, length_size, base_offset_size, index_size;
@@ -7505,34 +7512,25 @@  static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         return AVERROR_PATCHWELCOME;
     }
     item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
-    if (item_count > 1) {
-        // For still AVIF images, we only support one item. Second item will
-        // generally be found for AVIF images with alpha channel. We don't
-        // support them as of now.
-        av_log(c->fc, AV_LOG_ERROR, "iloc: item_count > 1 not supported.\n");
-        return AVERROR_PATCHWELCOME;
-    }
 
     // Populate the necessary fields used by mov_build_index.
-    sc->stsc_count = item_count;
-    sc->stsc_data = av_malloc_array(item_count, sizeof(*sc->stsc_data));
+    sc->stsc_count = 1;
+    sc->stsc_data = av_malloc_array(1, sizeof(*sc->stsc_data));
     if (!sc->stsc_data)
         return AVERROR(ENOMEM);
     sc->stsc_data[0].first = 1;
     sc->stsc_data[0].count = 1;
     sc->stsc_data[0].id = 1;
-    sc->chunk_count = item_count;
-    sc->chunk_offsets =
-        av_malloc_array(item_count, sizeof(*sc->chunk_offsets));
+    sc->chunk_count = 1;
+    sc->chunk_offsets = av_malloc_array(1, sizeof(*sc->chunk_offsets));
     if (!sc->chunk_offsets)
         return AVERROR(ENOMEM);
-    sc->sample_count = item_count;
-    sc->sample_sizes =
-        av_malloc_array(item_count, sizeof(*sc->sample_sizes));
+    sc->sample_count = 1;
+    sc->sample_sizes = av_malloc_array(1, sizeof(*sc->sample_sizes));
     if (!sc->sample_sizes)
         return AVERROR(ENOMEM);
-    sc->stts_count = item_count;
-    sc->stts_data = av_malloc_array(item_count, sizeof(*sc->stts_data));
+    sc->stts_count = 1;
+    sc->stts_data = av_malloc_array(1, sizeof(*sc->stts_data));
     if (!sc->stts_data)
         return AVERROR(ENOMEM);
     sc->stts_data[0].count = 1;
@@ -7540,7 +7538,7 @@  static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     sc->stts_data[0].duration = 0;
 
     for (int i = 0; i < item_count; i++) {
-        (version < 2) ? avio_rb16(pb) : avio_rb32(pb);  // item_id;
+        int item_id = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
         if (version > 0)
             avio_rb16(pb);  // construction_method.
         avio_rb16(pb);  // data_reference_index.
@@ -7556,8 +7554,10 @@  static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             if (rb_size(pb, &extent_offset, offset_size) < 0 ||
                 rb_size(pb, &extent_length, length_size) < 0)
                 return AVERROR_INVALIDDATA;
-            sc->sample_sizes[0] = extent_length;
-            sc->chunk_offsets[0] = base_offset + extent_offset;
+            if (item_id == c->primary_item_id) {
+                sc->sample_sizes[0] = extent_length;
+                sc->chunk_offsets[0] = base_offset + extent_offset;
+            }
         }
     }
 
@@ -7674,6 +7674,7 @@  static const MOVParseTableEntry mov_default_parse_table[] = {
 { MKTAG('S','A','3','D'), mov_read_SA3D }, /* ambisonic audio box */
 { MKTAG('S','A','N','D'), mov_read_SAND }, /* non diegetic audio box */
 { MKTAG('i','l','o','c'), mov_read_iloc },
+{ MKTAG('p','i','t','m'), mov_read_pitm },
 { 0, NULL }
 };