[FFmpeg-devel] [PATCH 2/3] avcodec/h264_picture: add ff_h264_replace_picture()

[Andreas Rheinhardt]

James Almer:
> On 8/8/2021 8:16 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Will remove unnecessary allocations when both src and dst picture
>>> contain
>>> references to the same buffers.
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>   libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++
>>>   libavcodec/h264dec.h      |  1 +
>>>   2 files changed, 46 insertions(+)
>>>
>>> diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c
>>> index 89aef37edd..1073d9e7e0 100644
>>> --- a/libavcodec/h264_picture.c
>>> +++ b/libavcodec/h264_picture.c
>>> @@ -142,6 +142,51 @@ fail:
>>>       return ret;
>>>   }
>>>   +int ff_h264_replace_picture(H264Context *h, H264Picture *dst,
>>> H264Picture *src)
>>
>> Is there something that hinders you from making src const? If so, I
>> don't see it.
> 
> Amended locally (Also h264_copy_picture_params() in patch 1/3).
> 
>>
>>
>>> +{
>>> +    int ret, i;
>>> +
>>> +    if (!src->f || !src->f->buf[0]) {
>>> +        ff_h264_unref_picture(h, dst);
>>> +        return 0;
>>> +    }
>>> +
>>> +    av_assert0(src->tf.f == src->f);
>>> +
>>> +    dst->tf.f = dst->f;
>>> +    ff_thread_release_buffer(h->avctx, &dst->tf);
>>> +    ret = ff_thread_ref_frame(&dst->tf, &src->tf);
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +
>>> +    ret  = av_buffer_replace(&dst->qscale_table_buf,
>>> src->qscale_table_buf);
>>> +    ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf);
>>> +    ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf);
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        ret  = av_buffer_replace(&dst->motion_val_buf[i],
>>> src->motion_val_buf[i]);
>>> +        ret |= av_buffer_replace(&dst->ref_index_buf[i],
>>> src->ref_index_buf[i]);
>>> +        if (ret < 0)
>>> +            goto fail;
>>> +    }
>>> +
>>> +    if (src->hwaccel_picture_private) {
>>
>> dst in this function is allowed to be used/dirty; so I don't see
>> anything that precludes dst->hwaccel_picture_private/hwaccel_priv_buf to
>> be set while the same is not true for src. But then this check means
>> that dst is not equivalent to src.
>>
>>> +        ret = av_buffer_replace(&dst->hwaccel_priv_buf,
>>> src->hwaccel_priv_buf);
>>> +        if (ret < 0)
>>> +            goto fail;
>>> +        dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data;
>>
>> This is the only thing that needs to be under if.
> 
> Amended locally into:
> 
>>     ret = av_buffer_replace(&dst->hwaccel_priv_buf,
>> src->hwaccel_priv_buf);
>>     if (ret < 0)
>>         goto fail;
>>
>>     dst->hwaccel_picture_private = NULL;
>>     if (src->hwaccel_picture_private)
>>         dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data;

On second look wouldn't dst->hwaccel_picture_private =
src->hwaccel_picture_private be the same thing, but without a branch?
(And even if it could happen (I doubt it) that
src->hwaccel_picture_private and src->hwaccel_priv_buf->data differ then
shouldn't we set the hwaccel_picture_private from src's
hwaccel_picture_private to make src and dst equivalent?)

- Andreas