[FFmpeg-devel] [PATCH] avutil/frame: Add avcodec_private_ref to AVFrame

[James Almer]

On 11/5/2017 12:25 PM, Michael Niedermayer wrote:
> On Sun, Nov 05, 2017 at 02:52:50PM +0100, Hendrik Leppkes wrote:
>> On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>>> This gives libavcodec a field that it can freely and safely use.
>>> Avoiding the need of wraping of a users opaque_ref field and its issues.
>>>
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>  libavutil/frame.c |  8 +++++++-
>>>  libavutil/frame.h | 10 ++++++++++
>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>> index 982fbb5c81..6ddaef1e74 100644
>>> --- a/libavutil/frame.c
>>> +++ b/libavutil/frame.c
>>> @@ -383,12 +383,17 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>  #endif
>>>
>>>      av_buffer_unref(&dst->opaque_ref);
>>> +    av_buffer_unref(&dst->avcodec_private_ref);
>>>      if (src->opaque_ref) {
>>>          dst->opaque_ref = av_buffer_ref(src->opaque_ref);
>>>          if (!dst->opaque_ref)
>>>              return AVERROR(ENOMEM);
>>>      }
>>> -
>>> +    if (src->avcodec_private_ref) {
>>> +        dst->avcodec_private_ref = av_buffer_ref(src->avcodec_private_ref);
>>> +        if (!dst->avcodec_private_ref)
>>> +            return AVERROR(ENOMEM);
>>> +    }
>>>      return 0;
>>>  }
>>>
>>> @@ -524,6 +529,7 @@ void av_frame_unref(AVFrame *frame)
>>>      av_buffer_unref(&frame->hw_frames_ctx);
>>>
>>>      av_buffer_unref(&frame->opaque_ref);
>>> +    av_buffer_unref(&frame->avcodec_private_ref);
>>>
>>>      get_frame_defaults(frame);
>>>  }
>>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>>> index 0c6aab1c02..73b7d949a9 100644
>>> --- a/libavutil/frame.h
>>> +++ b/libavutil/frame.h
>>> @@ -563,6 +563,16 @@ typedef struct AVFrame {
>>>      /**
>>>       * @}
>>>       */
>>> +    /**
>>> +     * AVBufferRef for free use by libavcodec. Code outside avcodec will never
>>> +     * check or change the contents of the buffer ref. FFmpeg calls
>>> +     * av_buffer_unref() on it when the frame is unreferenced.
>>> +     * av_frame_copy_props() calls create a new reference with av_buffer_ref()
>>> +     * for the target frame's avcodec_private_ref field.
>>> +     *
>>> +     * avcodec should never assign mutually incompatible types to this field.
>>> +     */
>>> +    AVBufferRef *avcodec_private_ref;
>>>  } AVFrame;
>>>
>>>  #if FF_API_FRAME_GET_SET
>>
>> I would prefer if this field would not be library-specific, but
>> perhaps just "private_ref" which is not allowed to be touched by
>> users, and documented to only be valid while within one library - ie.
>> if you pass a frame from avcodec to avfilter, avfilter could take over
>> the field (and just free any info, if its still in there).
>> This would avoid any chances of adding a multitude of fields later,
>> and a single AVFrame instance is not going to be used in multiple
>> libraries at the same time anyway (the contents might, but not the
>> actual AVFrame struct)
> 
> that should be easy to implement ...
> 
> a disadvantage is the slightly higher chance of mixing up types if
> some codepath doesnt cleanup the field
> 
> question is what do most prefer ?
> avcodec_private_ref ?           (that is one for each of the 2 libs)
> private_ref ?
> avframe_internal_ref ?          (that is a private struct defined in avutil similar to AVCodecInternal)

Discard my suggestion. Being inside an internal opaque struct would
require avpriv_ functions to access from within avcodec, and as BtbN
mentioned on IRC earlier today we should definitely avoid that.

So take Hendrik's suggestion, unless somebody starts a vote to force
wm4's original implementation instead.