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
>> <mich...@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 <mich...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
