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

[Marton Balint]

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