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

[James Almer]

On 8/8/2021 9:34 PM, Andreas Rheinhardt wrote:
> 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?

Yeah, I'll remove the branch and just copy the field.

> (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?)

If they could differ, then ff_h264_ref_picture() would be broken. So 
yeah, like i said above I'll just copy the field.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>