diff mbox series

[FFmpeg-devel,v2] libavcodec/cbs_av1: Add size check before parse obu

Message ID 20220329082921.756174-1-wenbin.chen@intel.com
State New
Headers show
Series [FFmpeg-devel,v2] libavcodec/cbs_av1: Add size check before parse obu | expand

Checks

Context Check Description
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Wenbin Chen March 29, 2022, 8:29 a.m. UTC
cbs_av1_write_unit() check pbc size after parsing obu frame, and return
AVERROR(ENOSPC) if pbc is small. pbc will be reallocated and this obu
frame will be parsed again, but this may cause error because
CodedBitstreamAV1Context has already been updated, for example
ref_order_hint is updated and will not match the same obu frame. Now size
check is added before parsing obu frame to avoid this error.

Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
---
 libavcodec/cbs_av1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Mark Thompson April 30, 2022, 5:45 p.m. UTC | #1
On 29/03/2022 09:29, Wenbin Chen wrote:
> cbs_av1_write_unit() check pbc size after parsing obu frame, and return
> AVERROR(ENOSPC) if pbc is small. pbc will be reallocated and this obu
> frame will be parsed again, but this may cause error because
> CodedBitstreamAV1Context has already been updated, for example
> ref_order_hint is updated and will not match the same obu frame. Now size
> check is added before parsing obu frame to avoid this error.
> 
> Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
> ---
>   libavcodec/cbs_av1.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index 1229480567..29e7bc16df 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -1075,6 +1075,9 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx,
>           put_bits32(pbc, 0);
>       }
>   
> +    if (8 * (unit->data_size + obu->obu_size) > put_bits_left(pbc))
> +        return AVERROR(ENOSPC);

unit->data_size is not usefully set when we are writing here (it might be the size of the old bitstream in editing cases, or it might just be zero).

> +
>       td = NULL;
>       start_pos = put_bits_count(pbc);
>   
> @@ -1196,9 +1199,6 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx,
>       flush_put_bits(pbc);
>       av_assert0(data_pos <= start_pos);
>   
> -    if (8 * obu->obu_size > put_bits_left(pbc))
> -        return AVERROR(ENOSPC);
> -
>       if (obu->obu_size > 0) {
>           memmove(pbc->buf + data_pos,
>                   pbc->buf + start_pos, header_size);

So, this doesn't work?  The header hasn't been written that point, so you don't know if there is enough space for both the OBU header and the OBU data.

Having the check in both places would be fine (the newly-added one being a way to bail early when there definitely isn't enough space), but that wouldn't do what you want.

I'm not sure what the right answer is here.  Do we need some way to unwind the written header?  The initial buffer size is 1MB and gets doubled each time, so this is not going to be hit very often.

- Mark
Wenbin Chen May 5, 2022, 3:55 a.m. UTC | #2
> On 29/03/2022 09:29, Wenbin Chen wrote:
> > cbs_av1_write_unit() check pbc size after parsing obu frame, and return
> > AVERROR(ENOSPC) if pbc is small. pbc will be reallocated and this obu
> > frame will be parsed again, but this may cause error because
> > CodedBitstreamAV1Context has already been updated, for example
> > ref_order_hint is updated and will not match the same obu frame. Now
> size
> > check is added before parsing obu frame to avoid this error.
> >
> > Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
> > ---
> >   libavcodec/cbs_av1.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> > index 1229480567..29e7bc16df 100644
> > --- a/libavcodec/cbs_av1.c
> > +++ b/libavcodec/cbs_av1.c
> > @@ -1075,6 +1075,9 @@ static int
> cbs_av1_write_obu(CodedBitstreamContext *ctx,
> >           put_bits32(pbc, 0);
> >       }
> >
> > +    if (8 * (unit->data_size + obu->obu_size) > put_bits_left(pbc))
> > +        return AVERROR(ENOSPC);
> 
> unit->data_size is not usefully set when we are writing here (it might be the
> size of the old bitstream in editing cases, or it might just be zero).

Thank you for pointing this out. If data_size is unset this check wouldn't work and
the problem still occurs. I will try to find a better way to fix this.

> 
> > +
> >       td = NULL;
> >       start_pos = put_bits_count(pbc);
> >
> > @@ -1196,9 +1199,6 @@ static int
> cbs_av1_write_obu(CodedBitstreamContext *ctx,
> >       flush_put_bits(pbc);
> >       av_assert0(data_pos <= start_pos);
> >
> > -    if (8 * obu->obu_size > put_bits_left(pbc))
> > -        return AVERROR(ENOSPC);
> > -
> >       if (obu->obu_size > 0) {
> >           memmove(pbc->buf + data_pos,
> >                   pbc->buf + start_pos, header_size);
> 
> So, this doesn't work?  The header hasn't been written that point, so you
> don't know if there is enough space for both the OBU header and the OBU
> data.
> 
> Having the check in both places would be fine (the newly-added one being a
> way to bail early when there definitely isn't enough space), but that wouldn't
> do what you want.

Ok, I will keep the both places in my next patch if I still fix issue in this way. 

> 
> I'm not sure what the right answer is here.  Do we need some way to unwind
> the written header?  The initial buffer size is 1MB and gets doubled each time,
> so this is not going to be hit very often.

Unwinding header is an alternative way. I will check If it is possible.

This problem is rare. The problem occurs when I frame below buffer size but one P/B
frame in the gop is greater than buffer size.

Thanks
Wenbin

> 
> - Mark
> _______________________________________________
> 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/cbs_av1.c b/libavcodec/cbs_av1.c
index 1229480567..29e7bc16df 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -1075,6 +1075,9 @@  static int cbs_av1_write_obu(CodedBitstreamContext *ctx,
         put_bits32(pbc, 0);
     }
 
+    if (8 * (unit->data_size + obu->obu_size) > put_bits_left(pbc))
+        return AVERROR(ENOSPC);
+
     td = NULL;
     start_pos = put_bits_count(pbc);
 
@@ -1196,9 +1199,6 @@  static int cbs_av1_write_obu(CodedBitstreamContext *ctx,
     flush_put_bits(pbc);
     av_assert0(data_pos <= start_pos);
 
-    if (8 * obu->obu_size > put_bits_left(pbc))
-        return AVERROR(ENOSPC);
-
     if (obu->obu_size > 0) {
         memmove(pbc->buf + data_pos,
                 pbc->buf + start_pos, header_size);