[FFmpeg-devel] [PATCH v2 7/7] avcodec: add AV_CODEC_FLAG_CLEAR

[Mark Thompson]

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 at 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.
>>> + at 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