| Message ID | 20231206082220.5532-7-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/7] avutil/tests/imgutils: factorize basic tests to new function | expand |
| 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 |
On date Wednesday 2023-12-06 09:22:20 +0100, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/APIchanges | 3 +++ > doc/codecs.texi | 14 ++++++++++++++ > libavcodec/avcodec.h | 4 ++++ > libavcodec/decode.c | 6 ++++++ > libavcodec/options_table.h | 1 + > libavcodec/version.h | 2 +- > 6 files changed, 29 insertions(+), 1 deletion(-) LGTM but I'll let someone with more familiarity of the lavc internals comment on this.
Hi, On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/APIchanges | 3 +++ > doc/codecs.texi | 14 ++++++++++++++ > libavcodec/avcodec.h | 4 ++++ > libavcodec/decode.c | 6 ++++++ > libavcodec/options_table.h | 1 + > libavcodec/version.h | 2 +- > 6 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 416e2bec5e..f839504a64 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -2,6 +2,9 @@ The last version increases of all libraries were on > 2023-02-09 > > API changes, most recent first: > > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h > + Add AV_CODEC_FLAG_CLEAR. > + > 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h > Add av_image_fill_color() > > diff --git a/doc/codecs.texi b/doc/codecs.texi > index 5b950b4560..0504a535f2 100644 > --- a/doc/codecs.texi > +++ b/doc/codecs.texi > @@ -76,6 +76,20 @@ Apply interlaced motion estimation. > Use closed gop. > @item output_corrupt > Output even potentially corrupted frames. > +@item clear > +Clear the contents of the video buffer before decoding the next picture > to it. > + > +Usually if only a part of a picture is affected by a decode error then the > +decoder (if it implements error concealment) tries to hide it by > interpolating > +pixels from neighbouring areas or in some cases from the previous frame. > Even > +without error concealment it is quite likely that the affected area will > +contain pixels from an earlier frame, due to frame pooling. > No comment on the patch itself, but wouldn't our users (and the C standard itself) consider it a security issue to return stale (or worse: uninitialized) data while pretending that it's safe to access? I thought touching uninitialized data was UB. Ronald
On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote: > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 2cfb3fcf97..f9b18a2c35 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > > validate_avframe_allocation(avctx, frame); > > + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == > AVMEDIA_TYPE_VIDEO) { > + uint32_t color[4] = {0}; > + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], > frame->linesize[2], frame->linesize[3]}; > + av_image_fill_color(frame->data, linesize, frame->format, color, > frame->width, frame->height); > I think it's be safer to add a return check here
Quoting Ronald S. Bultje (2023-12-07 02:44:36) > Hi, > > On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote: > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > --- > > doc/APIchanges | 3 +++ > > doc/codecs.texi | 14 ++++++++++++++ > > libavcodec/avcodec.h | 4 ++++ > > libavcodec/decode.c | 6 ++++++ > > libavcodec/options_table.h | 1 + > > libavcodec/version.h | 2 +- > > 6 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/doc/APIchanges b/doc/APIchanges > > index 416e2bec5e..f839504a64 100644 > > --- a/doc/APIchanges > > +++ b/doc/APIchanges > > @@ -2,6 +2,9 @@ The last version increases of all libraries were on > > 2023-02-09 > > > > API changes, most recent first: > > > > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h > > + Add AV_CODEC_FLAG_CLEAR. > > + > > 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h > > Add av_image_fill_color() > > > > diff --git a/doc/codecs.texi b/doc/codecs.texi > > index 5b950b4560..0504a535f2 100644 > > --- a/doc/codecs.texi > > +++ b/doc/codecs.texi > > @@ -76,6 +76,20 @@ Apply interlaced motion estimation. > > Use closed gop. > > @item output_corrupt > > Output even potentially corrupted frames. > > +@item clear > > +Clear the contents of the video buffer before decoding the next picture > > to it. > > + > > +Usually if only a part of a picture is affected by a decode error then the > > +decoder (if it implements error concealment) tries to hide it by > > interpolating > > +pixels from neighbouring areas or in some cases from the previous frame. > > Even > > +without error concealment it is quite likely that the affected area will > > +contain pixels from an earlier frame, due to frame pooling. > > > > No comment on the patch itself, but wouldn't our users (and the C standard > itself) consider it a security issue to return stale I don't see the security issue in returning previously-returned frame data.
Quoting Marton Balint (2023-12-06 09:22:20) > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/APIchanges | 3 +++ > doc/codecs.texi | 14 ++++++++++++++ > libavcodec/avcodec.h | 4 ++++ > libavcodec/decode.c | 6 ++++++ > libavcodec/options_table.h | 1 + > libavcodec/version.h | 2 +- > 6 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 416e2bec5e..f839504a64 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 > > API changes, most recent first: > > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h > + Add AV_CODEC_FLAG_CLEAR. I'm not a huge fan of calling it just 'clear'. How about something more descriptive like 'wipe_frames'. > + > 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h > Add av_image_fill_color() > > diff --git a/doc/codecs.texi b/doc/codecs.texi > index 5b950b4560..0504a535f2 100644 > --- a/doc/codecs.texi > +++ b/doc/codecs.texi > @@ -76,6 +76,20 @@ Apply interlaced motion estimation. > Use closed gop. > @item output_corrupt > Output even potentially corrupted frames. > +@item clear > +Clear the contents of the video buffer before decoding the next picture to it. > + > +Usually if only a part of a picture is affected by a decode error then the > +decoder (if it implements error concealment) tries to hide it by interpolating > +pixels from neighbouring areas or in some cases from the previous frame. Even > +without error concealment it is quite likely that the affected area will > +contain pixels from an earlier frame, due to frame pooling. > + > +For quality checking this might not be desirable, because it makes the errors > +less noticable. By using this flag, and combining it with disabled error > +concealment (@code{-ec 0}) it is possible to ensure that no leftover data from > +an earlier frame is presented in areas affected by decode errors. > + > @end table > > @item time_base @var{rational number} > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 7fb44e28f4..97848e942f 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -312,6 +312,10 @@ typedef struct RcOverride{ > * loop filter. > */ > #define AV_CODEC_FLAG_LOOP_FILTER (1 << 11) > +/** > + * Clear frame buffer contents before decoding. Clear the contents of each frame buffer after it is allocated with AVCodecContext.get_buffer2() and before anything is decoded into it. Note that this may have a significant performance cost. > + */ > +#define AV_CODEC_FLAG_CLEAR (1 << 12) > /** > * Only decode/encode grayscale. > */ > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 2cfb3fcf97..f9b18a2c35 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > > validate_avframe_allocation(avctx, frame); > > + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { > + uint32_t color[4] = {0}; > + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; > + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); Should this check for errors?
On Thu, 7 Dec 2023, Anton Khirnov wrote: > Quoting Ronald S. Bultje (2023-12-07 02:44:36) >> Hi, >> >> On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote: >> >> > Signed-off-by: Marton Balint <cus@passwd.hu> >> > --- >> > doc/APIchanges | 3 +++ >> > doc/codecs.texi | 14 ++++++++++++++ >> > libavcodec/avcodec.h | 4 ++++ >> > libavcodec/decode.c | 6 ++++++ >> > libavcodec/options_table.h | 1 + >> > libavcodec/version.h | 2 +- >> > 6 files changed, 29 insertions(+), 1 deletion(-) >> > >> > diff --git a/doc/APIchanges b/doc/APIchanges >> > index 416e2bec5e..f839504a64 100644 >> > --- a/doc/APIchanges >> > +++ b/doc/APIchanges >> > @@ -2,6 +2,9 @@ The last version increases of all libraries were on >> > 2023-02-09 >> > >> > API changes, most recent first: >> > >> > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h >> > + Add AV_CODEC_FLAG_CLEAR. >> > + >> > 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h >> > Add av_image_fill_color() >> > >> > diff --git a/doc/codecs.texi b/doc/codecs.texi >> > index 5b950b4560..0504a535f2 100644 >> > --- a/doc/codecs.texi >> > +++ b/doc/codecs.texi >> > @@ -76,6 +76,20 @@ Apply interlaced motion estimation. >> > Use closed gop. >> > @item output_corrupt >> > Output even potentially corrupted frames. >> > +@item clear >> > +Clear the contents of the video buffer before decoding the next picture >> > to it. >> > + >> > +Usually if only a part of a picture is affected by a decode error then the >> > +decoder (if it implements error concealment) tries to hide it by >> > interpolating >> > +pixels from neighbouring areas or in some cases from the previous frame. >> > Even >> > +without error concealment it is quite likely that the affected area will >> > +contain pixels from an earlier frame, due to frame pooling. >> > >> >> No comment on the patch itself, but wouldn't our users (and the C standard >> itself) consider it a security issue to return stale > > I don't see the security issue in returning previously-returned frame > data. I guess what Ronald means that it is possible that the decoder frame pool allocates data in heap previously containing sensitive data, and that might never get overwritten in case of faulty input before passing it to the user. The simple fix for that is to clear frame pool buffers on creation? I am not sure if it is actually UB to read uninitialzied data from the heap though. Regards, Marton
On Thu, 7 Dec 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-12-06 09:22:20) >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> doc/APIchanges | 3 +++ >> doc/codecs.texi | 14 ++++++++++++++ >> libavcodec/avcodec.h | 4 ++++ >> libavcodec/decode.c | 6 ++++++ >> libavcodec/options_table.h | 1 + >> libavcodec/version.h | 2 +- >> 6 files changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 416e2bec5e..f839504a64 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 >> >> API changes, most recent first: >> >> +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h >> + Add AV_CODEC_FLAG_CLEAR. > > I'm not a huge fan of calling it just 'clear'. How about something more > descriptive like 'wipe_frames'. Wipe reminds me of the wipe effect. How about 'predecode_clear'? >> + >> 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h >> Add av_image_fill_color() >> >> diff --git a/doc/codecs.texi b/doc/codecs.texi >> index 5b950b4560..0504a535f2 100644 >> --- a/doc/codecs.texi >> +++ b/doc/codecs.texi >> @@ -76,6 +76,20 @@ Apply interlaced motion estimation. >> Use closed gop. >> @item output_corrupt >> Output even potentially corrupted frames. >> +@item clear >> +Clear the contents of the video buffer before decoding the next picture to it. >> + >> +Usually if only a part of a picture is affected by a decode error then the >> +decoder (if it implements error concealment) tries to hide it by interpolating >> +pixels from neighbouring areas or in some cases from the previous frame. Even >> +without error concealment it is quite likely that the affected area will >> +contain pixels from an earlier frame, due to frame pooling. >> + >> +For quality checking this might not be desirable, because it makes the errors >> +less noticable. By using this flag, and combining it with disabled error >> +concealment (@code{-ec 0}) it is possible to ensure that no leftover data from >> +an earlier frame is presented in areas affected by decode errors. >> + >> @end table >> >> @item time_base @var{rational number} >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index 7fb44e28f4..97848e942f 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -312,6 +312,10 @@ typedef struct RcOverride{ >> * loop filter. >> */ >> #define AV_CODEC_FLAG_LOOP_FILTER (1 << 11) >> +/** >> + * Clear frame buffer contents before decoding. > > Clear the contents of each frame buffer after it is allocated with > AVCodecContext.get_buffer2() and before anything is decoded into it. > > Note that this may have a significant performance cost. ok. > >> + */ >> +#define AV_CODEC_FLAG_CLEAR (1 << 12) >> /** >> * Only decode/encode grayscale. >> */ >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 2cfb3fcf97..f9b18a2c35 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS >> >> validate_avframe_allocation(avctx, frame); >> >> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { >> + uint32_t color[4] = {0}; >> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; >> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); > > Should this check for errors? Lack of error checking is intentional. av_image_fill_color might not support all pixel formats, definitely not support hwaccel formats. It might make sense to warn the user once, but I don't think propagating the error back is needed here. I primarily thought of this as a QC feature (even thought about making the color fill green by default to make it more noticeable (YUV green happens to be 0,0,0), but for that I'd need similar checks for colorspaces to what I have for fill_black())... Regards, Marton
Hi, Le 8 décembre 2023 00:47:13 GMT+02:00, Marton Balint <cus@passwd.hu> a écrit : > > >On Thu, 7 Dec 2023, Anton Khirnov wrote: > >> Quoting Ronald S. Bultje (2023-12-07 02:44:36) >>> Hi, >>> >>> On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote: >>> >>> > Signed-off-by: Marton Balint <cus@passwd.hu> >>> > --- >>> > doc/APIchanges | 3 +++ >>> > doc/codecs.texi | 14 ++++++++++++++ >>> > libavcodec/avcodec.h | 4 ++++ >>> > libavcodec/decode.c | 6 ++++++ >>> > libavcodec/options_table.h | 1 + >>> > libavcodec/version.h | 2 +- >>> > 6 files changed, 29 insertions(+), 1 deletion(-) >>> > >>> > diff --git a/doc/APIchanges b/doc/APIchanges >>> > index 416e2bec5e..f839504a64 100644 >>> > --- a/doc/APIchanges >>> > +++ b/doc/APIchanges >>> > @@ -2,6 +2,9 @@ The last version increases of all libraries were on >>> > 2023-02-09 >>> > >>> > API changes, most recent first: >>> > >>> > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h >>> > + Add AV_CODEC_FLAG_CLEAR. >>> > + >>> > 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h >>> > Add av_image_fill_color() >>> > >>> > diff --git a/doc/codecs.texi b/doc/codecs.texi >>> > index 5b950b4560..0504a535f2 100644 >>> > --- a/doc/codecs.texi >>> > +++ b/doc/codecs.texi >>> > @@ -76,6 +76,20 @@ Apply interlaced motion estimation. >>> > Use closed gop. >>> > @item output_corrupt >>> > Output even potentially corrupted frames. >>> > +@item clear >>> > +Clear the contents of the video buffer before decoding the next picture >>> > to it. >>> > + >>> > +Usually if only a part of a picture is affected by a decode error then the >>> > +decoder (if it implements error concealment) tries to hide it by >>> > interpolating >>> > +pixels from neighbouring areas or in some cases from the previous frame. >>> > Even >>> > +without error concealment it is quite likely that the affected area will >>> > +contain pixels from an earlier frame, due to frame pooling. >>> > >>> >>> No comment on the patch itself, but wouldn't our users (and the C standard >>> itself) consider it a security issue to return stale >> >> I don't see the security issue in returning previously-returned frame >> data. > >I guess what Ronald means that it is possible that the decoder frame pool allocates data in heap previously containing sensitive data, and that might never get overwritten in case of faulty input before passing it to the user. > >The simple fix for that is to clear frame pool buffers on creation? > >I am not sure if it is actually UB to read uninitialzied data from the heap though. Reading uninitialised data is UB if the type representation is not surjective (e.g. bool, and potentially compound types with padding). Of course there are all sorts of other problems that could indirectly cause UB such as implicitly assuming that an integer fits a certain range and triggering an undefined overflow otherwise. > >Regards, >Marton >_______________________________________________ >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".
On 07/12/2023 23:11, Marton Balint wrote: > On Thu, 7 Dec 2023, Anton Khirnov wrote: >> Quoting Marton Balint (2023-12-06 09:22:20) >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> doc/APIchanges | 3 +++ >>> doc/codecs.texi | 14 ++++++++++++++ >>> libavcodec/avcodec.h | 4 ++++ >>> libavcodec/decode.c | 6 ++++++ >>> libavcodec/options_table.h | 1 + >>> libavcodec/version.h | 2 +- >>> 6 files changed, 29 insertions(+), 1 deletion(-) >>> >>> diff --git a/doc/APIchanges b/doc/APIchanges >>> index 416e2bec5e..f839504a64 100644 >>> --- a/doc/APIchanges >>> +++ b/doc/APIchanges >>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 >>> >>> API changes, most recent first: >>> >>> +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h >>> + Add AV_CODEC_FLAG_CLEAR. >> >> I'm not a huge fan of calling it just 'clear'. How about something more >> descriptive like 'wipe_frames'. > > Wipe reminds me of the wipe effect. How about 'predecode_clear'? > >>> + >>> 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h >>> Add av_image_fill_color() >>> >>> diff --git a/doc/codecs.texi b/doc/codecs.texi >>> index 5b950b4560..0504a535f2 100644 >>> --- a/doc/codecs.texi >>> +++ b/doc/codecs.texi >>> @@ -76,6 +76,20 @@ Apply interlaced motion estimation. >>> Use closed gop. >>> @item output_corrupt >>> Output even potentially corrupted frames. >>> +@item clear >>> +Clear the contents of the video buffer before decoding the next picture to it. >>> + >>> +Usually if only a part of a picture is affected by a decode error then the >>> +decoder (if it implements error concealment) tries to hide it by interpolating >>> +pixels from neighbouring areas or in some cases from the previous frame. Even >>> +without error concealment it is quite likely that the affected area will >>> +contain pixels from an earlier frame, due to frame pooling. >>> + >>> +For quality checking this might not be desirable, because it makes the errors >>> +less noticable. By using this flag, and combining it with disabled error >>> +concealment (@code{-ec 0}) it is possible to ensure that no leftover data from >>> +an earlier frame is presented in areas affected by decode errors. >>> + >>> @end table >>> >>> @item time_base @var{rational number} >>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >>> index 7fb44e28f4..97848e942f 100644 >>> --- a/libavcodec/avcodec.h >>> +++ b/libavcodec/avcodec.h >>> @@ -312,6 +312,10 @@ typedef struct RcOverride{ >>> * loop filter. >>> */ >>> #define AV_CODEC_FLAG_LOOP_FILTER (1 << 11) >>> +/** >>> + * Clear frame buffer contents before decoding. >> >> Clear the contents of each frame buffer after it is allocated with >> AVCodecContext.get_buffer2() and before anything is decoded into it. >> >> Note that this may have a significant performance cost. > > ok. > >> >>> + */ >>> +#define AV_CODEC_FLAG_CLEAR (1 << 12) >>> /** >>> * Only decode/encode grayscale. >>> */ >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>> index 2cfb3fcf97..f9b18a2c35 100644 >>> --- a/libavcodec/decode.c >>> +++ b/libavcodec/decode.c >>> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS >>> >>> validate_avframe_allocation(avctx, frame); >>> >>> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { >>> + uint32_t color[4] = {0}; >>> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; >>> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); >> >> Should this check for errors? > > Lack of error checking is intentional. av_image_fill_color might not support all pixel formats, definitely not support hwaccel formats. It might make sense to warn the user once, but I don't think propagating the error back is needed here. I don't think I agree with this? Even if you say it isn't, it still looks like a privacy and security feature and so people may treat it as such. Consider a transcoding application with user-supplied ingest - without something like this, a malformed input from the user could leak random previously-deallocated memory, which could contain anything (frames from other users, private keys, etc.). So, if the user has explicitly requested that frames are cleared then IMO it should either clear them or fail. (I do think the feature is a good idea. Supporting hardware formats there is probably straightforward in at least some cases if useful.) Thanks, - Mark
Quoting Marton Balint (2023-12-08 00:11:21) > Wipe reminds me of the wipe effect. How about 'predecode_clear'? Fine with me I guess. > > > >> + */ > >> +#define AV_CODEC_FLAG_CLEAR (1 << 12) > >> /** > >> * Only decode/encode grayscale. > >> */ > >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >> index 2cfb3fcf97..f9b18a2c35 100644 > >> --- a/libavcodec/decode.c > >> +++ b/libavcodec/decode.c > >> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > >> > >> validate_avframe_allocation(avctx, frame); > >> > >> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { > >> + uint32_t color[4] = {0}; > >> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; > >> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); > > > > Should this check for errors? > > Lack of error checking is intentional. av_image_fill_color might not > support all pixel formats, definitely not support hwaccel formats. It > might make sense to warn the user once, but I don't think propagating the > error back is needed here. > > I primarily thought of this as a QC feature (even thought about making the > color fill green by default to make it more noticeable (YUV green happens > to be 0,0,0), but for that I'd need similar checks for colorspaces to > what I have for fill_black())... As Mark said, I expect people to want to use it as a security feature. So either it should work reliably, or it should be made very clear that it's for debugging only. For non-hwaccel pixel formats, you can fall back on memsetting the buffer to 0.
On Tue, 12 Dec 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-12-08 00:11:21) >> Wipe reminds me of the wipe effect. How about 'predecode_clear'? > > Fine with me I guess. > >>> >>>> + */ >>>> +#define AV_CODEC_FLAG_CLEAR (1 << 12) >>>> /** >>>> * Only decode/encode grayscale. >>>> */ >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>> index 2cfb3fcf97..f9b18a2c35 100644 >>>> --- a/libavcodec/decode.c >>>> +++ b/libavcodec/decode.c >>>> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>> >>>> validate_avframe_allocation(avctx, frame); >>>> >>>> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { >>>> + uint32_t color[4] = {0}; >>>> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; >>>> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); >>> >>> Should this check for errors? >> >> Lack of error checking is intentional. av_image_fill_color might not >> support all pixel formats, definitely not support hwaccel formats. It >> might make sense to warn the user once, but I don't think propagating the >> error back is needed here. >> >> I primarily thought of this as a QC feature (even thought about making the >> color fill green by default to make it more noticeable (YUV green happens >> to be 0,0,0), but for that I'd need similar checks for colorspaces to >> what I have for fill_black())... > > As Mark said, I expect people to want to use it as a security feature. > So either it should work reliably, or it should be made very clear that > it's for debugging only. > > For non-hwaccel pixel formats, you can fall back on memsetting the > buffer to 0. IMHO for security you don't need to clear every frame, only new ones in the buffer pool. Considering that it only affects the first few frames, we might want to do that unconditionally, and not based on a flag. And it is better to do this on the buffer level (because you might not overwrite some bytes because of alignment with a color fill). So for this flag, I'd rather make it clear it is not security-related, and also that it has performance impact. Regards, Marton
On Tue, Dec 12, 2023 at 12:23:38PM +0100, Anton Khirnov wrote: > Quoting Marton Balint (2023-12-08 00:11:21) > > Wipe reminds me of the wipe effect. How about 'predecode_clear'? > > Fine with me I guess. > > > > > > >> + */ > > >> +#define AV_CODEC_FLAG_CLEAR (1 << 12) > > >> /** > > >> * Only decode/encode grayscale. > > >> */ > > >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c > > >> index 2cfb3fcf97..f9b18a2c35 100644 > > >> --- a/libavcodec/decode.c > > >> +++ b/libavcodec/decode.c > > >> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > > >> > > >> validate_avframe_allocation(avctx, frame); > > >> > > >> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { > > >> + uint32_t color[4] = {0}; > > >> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; > > >> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); > > > > > > Should this check for errors? > > > > Lack of error checking is intentional. av_image_fill_color might not > > support all pixel formats, definitely not support hwaccel formats. It > > might make sense to warn the user once, but I don't think propagating the > > error back is needed here. > > > > I primarily thought of this as a QC feature (even thought about making the > > color fill green by default to make it more noticeable (YUV green happens > > to be 0,0,0), but for that I'd need similar checks for colorspaces to > > what I have for fill_black())... > > As Mark said, I expect people to want to use it as a security feature. > So either it should work reliably, or it should be made very clear that > it's for debugging only. > > For non-hwaccel pixel formats, you can fall back on memsetting the > buffer to 0. For security, there may be other less vissible things that should be cleared too for example B frames in some codecs can use motion vectors from surrounding frames. Also the correct thing is to apply error concealment to replace all parts which have not been filled in. Not to leave them uninitialized. Not only does that preserve privacy it also produces much better looking frames We have error concealment code, it should be used. Not arguing against this feature here. Just saying for security/privacy it has a price as it needs to be done before we know if a frame is damaged, while error concealment is done only on actually damaged frames Of course you may want to do both to be really "sure" ... thx [...]
Quoting Marton Balint (2023-12-12 19:37:57) > > > On Tue, 12 Dec 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-12-08 00:11:21) > >> Wipe reminds me of the wipe effect. How about 'predecode_clear'? > > > > Fine with me I guess. > > > >>> > >>>> + */ > >>>> +#define AV_CODEC_FLAG_CLEAR (1 << 12) > >>>> /** > >>>> * Only decode/encode grayscale. > >>>> */ > >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >>>> index 2cfb3fcf97..f9b18a2c35 100644 > >>>> --- a/libavcodec/decode.c > >>>> +++ b/libavcodec/decode.c > >>>> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS > >>>> > >>>> validate_avframe_allocation(avctx, frame); > >>>> > >>>> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { > >>>> + uint32_t color[4] = {0}; > >>>> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; > >>>> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); > >>> > >>> Should this check for errors? > >> > >> Lack of error checking is intentional. av_image_fill_color might not > >> support all pixel formats, definitely not support hwaccel formats. It > >> might make sense to warn the user once, but I don't think propagating the > >> error back is needed here. > >> > >> I primarily thought of this as a QC feature (even thought about making the > >> color fill green by default to make it more noticeable (YUV green happens > >> to be 0,0,0), but for that I'd need similar checks for colorspaces to > >> what I have for fill_black())... > > > > As Mark said, I expect people to want to use it as a security feature. > > So either it should work reliably, or it should be made very clear that > > it's for debugging only. > > > > For non-hwaccel pixel formats, you can fall back on memsetting the > > buffer to 0. > > IMHO for security you don't need to clear every frame, only new ones in > the buffer pool. Considering that it only affects the first few > frames, we might want to do that unconditionally, and not based on a > flag. And it is better to do this on the buffer level (because you might > not overwrite some bytes because of alignment with a color fill). > > So for this flag, I'd rather make it clear it is not security-related, and > also that it has performance impact. So then maybe make a FF_EC flag?
On Wed, 13 Dec 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-12-12 19:37:57) >> >> >> On Tue, 12 Dec 2023, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2023-12-08 00:11:21) >>>> Wipe reminds me of the wipe effect. How about 'predecode_clear'? >>> >>> Fine with me I guess. >>> >>>>> >>>>>> + */ >>>>>> +#define AV_CODEC_FLAG_CLEAR (1 << 12) >>>>>> /** >>>>>> * Only decode/encode grayscale. >>>>>> */ >>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>>>> index 2cfb3fcf97..f9b18a2c35 100644 >>>>>> --- a/libavcodec/decode.c >>>>>> +++ b/libavcodec/decode.c >>>>>> @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>>>> >>>>>> validate_avframe_allocation(avctx, frame); >>>>>> >>>>>> + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { >>>>>> + uint32_t color[4] = {0}; >>>>>> + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; >>>>>> + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); >>>>> >>>>> Should this check for errors? >>>> >>>> Lack of error checking is intentional. av_image_fill_color might not >>>> support all pixel formats, definitely not support hwaccel formats. It >>>> might make sense to warn the user once, but I don't think propagating the >>>> error back is needed here. >>>> >>>> I primarily thought of this as a QC feature (even thought about making the >>>> color fill green by default to make it more noticeable (YUV green happens >>>> to be 0,0,0), but for that I'd need similar checks for colorspaces to >>>> what I have for fill_black())... >>> >>> As Mark said, I expect people to want to use it as a security feature. >>> So either it should work reliably, or it should be made very clear that >>> it's for debugging only. >>> >>> For non-hwaccel pixel formats, you can fall back on memsetting the >>> buffer to 0. >> >> IMHO for security you don't need to clear every frame, only new ones in >> the buffer pool. Considering that it only affects the first few >> frames, we might want to do that unconditionally, and not based on a >> flag. And it is better to do this on the buffer level (because you might >> not overwrite some bytes because of alignment with a color fill). >> >> So for this flag, I'd rather make it clear it is not security-related, and >> also that it has performance impact. > > So then maybe make a FF_EC flag? I thought about using that, but there are plenty of error concealment code which only checks if avctx->error_concealment is nonzero or zero, and not specific EC flags. So unless that is fixed (which might break existing behaviour) one cannot introduce a new EC flag and disable error concealment at the same time... Regards, Marton
Quoting Marton Balint (2023-12-13 18:09:45) > On Wed, 13 Dec 2023, Anton Khirnov wrote: > > Quoting Marton Balint (2023-12-12 19:37:57) > >> > >> So for this flag, I'd rather make it clear it is not security-related, and > >> also that it has performance impact. > > > > So then maybe make a FF_EC flag? > > I thought about using that, but there are plenty of error concealment > code which only checks if avctx->error_concealment is nonzero or zero, and > not specific EC flags. So unless that is fixed (which might break existing > behaviour) one cannot introduce a new EC flag and disable error > concealment at the same time... If you don't feel like fixing all the places that do such checks, you could instead * add a flag in DecodeContext * in ff_decode_preinit(), map your new FF_EC_PREDECODE_CLEAR to the internal flag * clear FF_EC_PREDECODE_CLEAR in AVCodecContext That should avoid breaking any existing behavior.
diff --git a/doc/APIchanges b/doc/APIchanges index 416e2bec5e..f839504a64 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 API changes, most recent first: +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h + Add AV_CODEC_FLAG_CLEAR. + 2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h Add av_image_fill_color() diff --git a/doc/codecs.texi b/doc/codecs.texi index 5b950b4560..0504a535f2 100644 --- a/doc/codecs.texi +++ b/doc/codecs.texi @@ -76,6 +76,20 @@ Apply interlaced motion estimation. Use closed gop. @item output_corrupt Output even potentially corrupted frames. +@item clear +Clear the contents of the video buffer before decoding the next picture to it. + +Usually if only a part of a picture is affected by a decode error then the +decoder (if it implements error concealment) tries to hide it by interpolating +pixels from neighbouring areas or in some cases from the previous frame. Even +without error concealment it is quite likely that the affected area will +contain pixels from an earlier frame, due to frame pooling. + +For quality checking this might not be desirable, because it makes the errors +less noticable. By using this flag, and combining it with disabled error +concealment (@code{-ec 0}) it is possible to ensure that no leftover data from +an earlier frame is presented in areas affected by decode errors. + @end table @item time_base @var{rational number} diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 7fb44e28f4..97848e942f 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -312,6 +312,10 @@ typedef struct RcOverride{ * loop filter. */ #define AV_CODEC_FLAG_LOOP_FILTER (1 << 11) +/** + * Clear frame buffer contents before decoding. + */ +#define AV_CODEC_FLAG_CLEAR (1 << 12) /** * Only decode/encode grayscale. */ diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 2cfb3fcf97..f9b18a2c35 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1675,6 +1675,12 @@ FF_ENABLE_DEPRECATION_WARNINGS validate_avframe_allocation(avctx, frame); + if (avctx->flags & AV_CODEC_FLAG_CLEAR && avctx->codec_type == AVMEDIA_TYPE_VIDEO) { + uint32_t color[4] = {0}; + ptrdiff_t linesize[4] = {frame->linesize[0], frame->linesize[1], frame->linesize[2], frame->linesize[3]}; + av_image_fill_color(frame->data, linesize, frame->format, color, frame->width, frame->height); + } + ret = ff_attach_decode_data(frame); if (ret < 0) goto fail; diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index ee243d9894..23ff4cddf8 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -62,6 +62,7 @@ static const AVOption avcodec_options[] = { {"frame_duration", "use frame durations", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_FRAME_DURATION}, .unit = "flags"}, {"pass1", "use internal 2-pass ratecontrol in first pass mode", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_PASS1 }, INT_MIN, INT_MAX, 0, "flags"}, {"pass2", "use internal 2-pass ratecontrol in second pass mode", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_PASS2 }, INT_MIN, INT_MAX, 0, "flags"}, +{"clear", "clear frames before decoding", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_CLEAR }, INT_MIN, INT_MAX, V|D, "flags"}, {"gray", "only decode/encode grayscale", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_GRAY }, INT_MIN, INT_MAX, V|E|D, "flags"}, {"psnr", "error[?] variables will be set during encoding", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_PSNR }, INT_MIN, INT_MAX, V|E, "flags"}, {"ildct", "use interlaced DCT", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_INTERLACED_DCT }, INT_MIN, INT_MAX, V|E, "flags"}, diff --git a/libavcodec/version.h b/libavcodec/version.h index 1008fead27..34b059a8a9 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,7 +29,7 @@ #include "version_major.h" -#define LIBAVCODEC_VERSION_MINOR 35 +#define LIBAVCODEC_VERSION_MINOR 36 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
Signed-off-by: Marton Balint <cus@passwd.hu> --- doc/APIchanges | 3 +++ doc/codecs.texi | 14 ++++++++++++++ libavcodec/avcodec.h | 4 ++++ libavcodec/decode.c | 6 ++++++ libavcodec/options_table.h | 1 + libavcodec/version.h | 2 +- 6 files changed, 29 insertions(+), 1 deletion(-)