diff mbox series

[FFmpeg-devel,v1] avcodec/av1_parser: ensure only one show frame packed data parsered

Message ID 20211203080920.1948453-1-fei.w.wang@intel.com
State New
Headers show
Series [FFmpeg-devel,v1] avcodec/av1_parser: ensure only one show frame packed data parsered
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Fei Wang Dec. 3, 2021, 8:09 a.m. UTC
Split packed data when it contains multiple show frames in some
non-standard bitstream. This can benefit downstream decoder which can
decode continuously instead of interrupt with unexpected error.

Signed-off-by: Fei Wang <fei.w.wang@intel.com>
---
 libavcodec/av1_parser.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Michael Niedermayer Dec. 3, 2021, 10:51 a.m. UTC | #1
On Fri, Dec 03, 2021 at 04:09:20PM +0800, Fei Wang wrote:
> Split packed data when it contains multiple show frames in some
> non-standard bitstream. This can benefit downstream decoder which can
> decode continuously instead of interrupt with unexpected error.
> 
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>  libavcodec/av1_parser.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

This results in a infinite loop

endlessly displaying
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
[av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment.
[av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.

i will send you the testsample privatly

thx

[...]
James Almer Dec. 3, 2021, 12:36 p.m. UTC | #2
On Fri, Dec 3, 2021 at 5:12 AM Fei Wang <fei.w.wang@intel.com> wrote:

> Split packed data when it contains multiple show frames in some
> non-standard bitstream. This can benefit downstream decoder which can
> decode continuously instead of interrupt with unexpected error.
>
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>  libavcodec/av1_parser.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/av1_parser.c b/libavcodec/av1_parser.c
> index d2dfdb3580..d6f5cace4b 100644
> --- a/libavcodec/av1_parser.c
> +++ b/libavcodec/av1_parser.c
> @@ -59,11 +59,10 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>      const CodedBitstreamAV1Context *av1 = s->cbc->priv_data;
>      const AV1RawSequenceHeader *seq;
>      const AV1RawColorConfig *color;
> +    int pic_found = 0;
> +    int next = 0;
>      int ret;
>
> -    *out_data = data;
> -    *out_size = size;
> -
>      ctx->key_frame         = -1;
>      ctx->pict_type         = AV_PICTURE_TYPE_NONE;
>      ctx->picture_structure = AV_PICTURE_STRUCTURE_UNKNOWN;
> @@ -100,6 +99,8 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>          const AV1RawOBU *obu = unit->content;
>          const AV1RawFrameHeader *frame;
>
> +        next += unit->data_size;
> +
>          if (unit->type == AV1_OBU_FRAME)
>              frame = &obu->obu.frame.header;
>          else if (unit->type == AV1_OBU_FRAME_HEADER)
> @@ -113,6 +114,12 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>          if (!frame->show_frame && !frame->show_existing_frame)
>              continue;
>
> +        /* split data if it contains multi show frames */
> +        if (pic_found) {
> +            next -= unit->data_size;
> +            break;
> +        }
> +
>          ctx->width  = frame->frame_width_minus_1 + 1;
>          ctx->height = frame->frame_height_minus_1 + 1;
>
> @@ -131,8 +138,12 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>              break;
>          }
>          ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
> +        pic_found = 1;
>      }
>
> +    *out_size = next;
> +    *out_data = data;
> +
>      switch (av1->bit_depth) {
>      case 8:
>          ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY8
> @@ -171,7 +182,7 @@ end:
>
>      s->cbc->log_ctx = NULL;
>
> -    return size;
> +    return next;
>  }
>
>  static const CodedBitstreamUnitType decompose_unit_types[] = {
> --
> 2.25.1
>
>
No, the parser must not split the packets. We used to do this with the VP9
parser and the result was the creation of broken output files.

What you could do instead is autoinsert the frame_split BSF in the decoder,
same as we're doing for VP9. It should have the same effect.
Fei Wang Dec. 6, 2021, 5:20 a.m. UTC | #3
On Fri, 2021-12-03 at 11:51 +0100, Michael Niedermayer wrote:
> On Fri, Dec 03, 2021 at 04:09:20PM +0800, Fei Wang wrote:
> > Split packed data when it contains multiple show frames in some
> > non-standard bitstream. This can benefit downstream decoder which
> > can
> > decode continuously instead of interrupt with unexpected error.
> > 
> > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > ---
> >  libavcodec/av1_parser.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> This results in a infinite loop
> 
> endlessly displaying
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.
> [av1 @ 0x55f62c2a6a00] Invalid OBU length: 1071323, but only 768
> bytes remaining in fragment.
> [av1 @ 0x55f62c2a6a00] Failed to parse temporal unit.

Thanks Michael to catch this. Will check it soon.

Fei
Thanks

> 
> i will send you the testsample privatly
> 
> thx
> 
> [...]
> _______________________________________________
> 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".
Fei Wang Dec. 6, 2021, 6:21 a.m. UTC | #4
On Fri, 2021-12-03 at 09:36 -0300, James Almer wrote:
> On Fri, Dec 3, 2021 at 5:12 AM Fei Wang <fei.w.wang@intel.com> wrote:
> 
> > Split packed data when it contains multiple show frames in some
> > non-standard bitstream. This can benefit downstream decoder which
> > can
> > decode continuously instead of interrupt with unexpected error.
> > 
> > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > ---
> >  libavcodec/av1_parser.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/av1_parser.c b/libavcodec/av1_parser.c
> > index d2dfdb3580..d6f5cace4b 100644
> > --- a/libavcodec/av1_parser.c
> > +++ b/libavcodec/av1_parser.c
> > @@ -59,11 +59,10 @@ static int
> > av1_parser_parse(AVCodecParserContext *ctx,
> >      const CodedBitstreamAV1Context *av1 = s->cbc->priv_data;
> >      const AV1RawSequenceHeader *seq;
> >      const AV1RawColorConfig *color;
> > +    int pic_found = 0;
> > +    int next = 0;
> >      int ret;
> > 
> > -    *out_data = data;
> > -    *out_size = size;
> > -
> >      ctx->key_frame         = -1;
> >      ctx->pict_type         = AV_PICTURE_TYPE_NONE;
> >      ctx->picture_structure = AV_PICTURE_STRUCTURE_UNKNOWN;
> > @@ -100,6 +99,8 @@ static int av1_parser_parse(AVCodecParserContext
> > *ctx,
> >          const AV1RawOBU *obu = unit->content;
> >          const AV1RawFrameHeader *frame;
> > 
> > +        next += unit->data_size;
> > +
> >          if (unit->type == AV1_OBU_FRAME)
> >              frame = &obu->obu.frame.header;
> >          else if (unit->type == AV1_OBU_FRAME_HEADER)
> > @@ -113,6 +114,12 @@ static int
> > av1_parser_parse(AVCodecParserContext *ctx,
> >          if (!frame->show_frame && !frame->show_existing_frame)
> >              continue;
> > 
> > +        /* split data if it contains multi show frames */
> > +        if (pic_found) {
> > +            next -= unit->data_size;
> > +            break;
> > +        }
> > +
> >          ctx->width  = frame->frame_width_minus_1 + 1;
> >          ctx->height = frame->frame_height_minus_1 + 1;
> > 
> > @@ -131,8 +138,12 @@ static int
> > av1_parser_parse(AVCodecParserContext *ctx,
> >              break;
> >          }
> >          ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
> > +        pic_found = 1;
> >      }
> > 
> > +    *out_size = next;
> > +    *out_data = data;
> > +
> >      switch (av1->bit_depth) {
> >      case 8:
> >          ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY8
> > @@ -171,7 +182,7 @@ end:
> > 
> >      s->cbc->log_ctx = NULL;
> > 
> > -    return size;
> > +    return next;
> >  }
> > 
> >  static const CodedBitstreamUnitType decompose_unit_types[] = {
> > --
> > 2.25.1
> > 
> > 
> No, the parser must not split the packets. We used to do this with
> the VP9
> parser and the result was the creation of broken output files.
Any more information on failed case for vp9? I tested this patch by
decoding several av1 bitstream, all looks good.

And I checked wm4's change on vp9/vp9_parser, the commit only shows it
benefits on multi-thread decoding. So I guess if vp9 failed case is
related to multi-thread decodeing? If yes, then insert frame_split bsf
in av1 decoder will be a better choice. Even av1 decoder doesn't
support multi-threading decode now, it can avoid issue if it is
supported someday.

> 
> What you could do instead is autoinsert the frame_split BSF in the
> decoder,
> same as we're doing for VP9. It should have the same effect.
> _______________________________________________
> 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/libavcodec/av1_parser.c b/libavcodec/av1_parser.c
index d2dfdb3580..d6f5cace4b 100644
--- a/libavcodec/av1_parser.c
+++ b/libavcodec/av1_parser.c
@@ -59,11 +59,10 @@  static int av1_parser_parse(AVCodecParserContext *ctx,
     const CodedBitstreamAV1Context *av1 = s->cbc->priv_data;
     const AV1RawSequenceHeader *seq;
     const AV1RawColorConfig *color;
+    int pic_found = 0;
+    int next = 0;
     int ret;
 
-    *out_data = data;
-    *out_size = size;
-
     ctx->key_frame         = -1;
     ctx->pict_type         = AV_PICTURE_TYPE_NONE;
     ctx->picture_structure = AV_PICTURE_STRUCTURE_UNKNOWN;
@@ -100,6 +99,8 @@  static int av1_parser_parse(AVCodecParserContext *ctx,
         const AV1RawOBU *obu = unit->content;
         const AV1RawFrameHeader *frame;
 
+        next += unit->data_size;
+
         if (unit->type == AV1_OBU_FRAME)
             frame = &obu->obu.frame.header;
         else if (unit->type == AV1_OBU_FRAME_HEADER)
@@ -113,6 +114,12 @@  static int av1_parser_parse(AVCodecParserContext *ctx,
         if (!frame->show_frame && !frame->show_existing_frame)
             continue;
 
+        /* split data if it contains multi show frames */
+        if (pic_found) {
+            next -= unit->data_size;
+            break;
+        }
+
         ctx->width  = frame->frame_width_minus_1 + 1;
         ctx->height = frame->frame_height_minus_1 + 1;
 
@@ -131,8 +138,12 @@  static int av1_parser_parse(AVCodecParserContext *ctx,
             break;
         }
         ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
+        pic_found = 1;
     }
 
+    *out_size = next;
+    *out_data = data;
+
     switch (av1->bit_depth) {
     case 8:
         ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY8
@@ -171,7 +182,7 @@  end:
 
     s->cbc->log_ctx = NULL;
 
-    return size;
+    return next;
 }
 
 static const CodedBitstreamUnitType decompose_unit_types[] = {