James Almer:
> On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote:
>> A reference to an AV1RawFrameHeader and consequently the
>> AV1RawFrameHeader itself and everything it has a reference to leak
>> if the hardware has no AV1 decoding capabilities.
>
> It looks like it can happen if get_buffer() fails even with hardware
> support.
>
>> It happens e.g.
>> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked
>> because the return value of ffmpeg (which indicates failure when using
>> Valgrind or ASAN) is ignored when doing tests of type md5.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
>> ---
>> I switched the av_buffer_ref() and update_context_with_frame_header()
>> because the latter does not need any cleanup on failure.
>>
>> Also notice that actual decoding with this patch applied is completely
>> untested.
>>
>> libavcodec/av1dec.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>> index 1589b8f0c0..d7b2ac9d46 100644
>> --- a/libavcodec/av1dec.c
>> +++ b/libavcodec/av1dec.c
>> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext
>> *avctx, AV1Frame *f)
>> AVFrame *frame;
>> int ret;
>> - f->header_ref = av_buffer_ref(s->header_ref);
>> - if (!f->header_ref)
>> - return AVERROR(ENOMEM);
>> -
>> - f->raw_frame_header = s->raw_frame_header;
>> -
>> ret = update_context_with_frame_header(avctx, header);
>> if (ret < 0) {
>> av_log(avctx, AV_LOG_ERROR, "Failed to update context with
>> frame header\n");
>> return ret;
>> }
>> + f->header_ref = av_buffer_ref(s->header_ref);
>> + if (!f->header_ref)
>> + return AVERROR(ENOMEM);
>> +
>> + f->raw_frame_header = s->raw_frame_header;
>> +
>> if ((ret = ff_thread_get_buffer(avctx, &f->tf,
>> AV_GET_BUFFER_FLAG_REF)) < 0)
>> - return ret;
>> + goto fail;
>> frame = f->tf.f;
>> frame->key_frame = header->frame_type == AV1_FRAME_KEY;
>> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>> AV1Frame *f)
>> if (hwaccel->frame_priv_data_size) {
>> f->hwaccel_priv_buf =
>> av_buffer_allocz(hwaccel->frame_priv_data_size);
>> - if (!f->hwaccel_priv_buf)
>> + if (!f->hwaccel_priv_buf) {
>> + ret = AVERROR(ENOMEM);
>> goto fail;
>> + }
>> f->hwaccel_picture_private = f->hwaccel_priv_buf->data;
>> }
>> }
>> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx,
>> AV1Frame *f)
>> fail:
>
> Just to be safe, add a ret = 0 above this line.
>
There is a "return 0;" above this line (the non-error path does not
include this av1_frame_unref()), so this makes no sense.
>> av1_frame_unref(avctx, f);
>> - return AVERROR(ENOMEM);
>> + return ret;
>> }
>> static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,
>
> LGTM, and while unrelated to this fix, this also reveals that in some
> scenarios, decoding without hardware support reaches this point, when
> it's meant to abort when parsing the sequence header and being unable to
> set up a hardware pixel format in avctx.
>
Yeah, I get a screen full of error messages from this decoder.
> Looks like when parsing a second sequence header (Like in the second
> keyframe) where s->pix_fmt is already set to a software format,
> get_pixel_format() is not called, and so decoding proceeds to deal with
> frames.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
