diff mbox series

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

Message ID 20220516165912.3731545-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
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Vignesh Venkat May 16, 2022, 4:59 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 Venkat May 19, 2022, 4:13 p.m. UTC | #1 
On Mon, May 16, 2022 at 9:59 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 d7be593a86..9310a393fe 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7445,6 +7445,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;
> @@ -7501,34 +7508,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;
> @@ -7536,7 +7534,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.
> @@ -7552,8 +7550,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;
> +            }
>          }
>      }
>
> @@ -7670,6 +7670,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.550.gb090851708-goog
>

Any comments on this one? If not, can this be merged please?
Vignesh Venkat May 31, 2022, 7:11 p.m. UTC | #2 
On Thu, May 19, 2022 at 9:13 AM Vignesh Venkatasubramanian
<vigneshv@google.com> wrote:
>
> On Mon, May 16, 2022 at 9:59 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 d7be593a86..9310a393fe 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -7445,6 +7445,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;
> > @@ -7501,34 +7508,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;
> > @@ -7536,7 +7534,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.
> > @@ -7552,8 +7550,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;
> > +            }
> >          }
> >      }
> >
> > @@ -7670,6 +7670,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.550.gb090851708-goog
> >
>
> Any comments on this one? If not, can this be merged please?
>

Another ping on this please?

> --
> Vignesh
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 d7be593a86..9310a393fe 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7445,6 +7445,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;
@@ -7501,34 +7508,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;
@@ -7536,7 +7534,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.
@@ -7552,8 +7550,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;
+            }
         }
     }
 
@@ -7670,6 +7670,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 }
 };