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

[Marton Balint]

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