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

Submitted by Michael Niedermayer on Nov. 5, 2017, 12:34 p.m.

Details

Message ID 20171105123451.22556-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 5, 2017, 12:34 p.m.
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@niedermayer.cc>
---
 libavutil/frame.c |  8 +++++++-
 libavutil/frame.h | 10 ++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

James Almer Nov. 5, 2017, 1:35 p.m.
On 11/5/2017 9:34 AM, Michael Niedermayer 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.

Could this perhaps be in an opaque internal struct instead, much like
AVCodecInternal and whatnot? As wm4 said in the relevant discussion,
this approach is nonoptimal and *will* snowball into a mess of fields if
other libav* libraries start requiring their own buffers in a frame.
An internal field of an opaque struct being in a public header is much
cleaner and easier to maintain than adding such specific fields that may
at some point in the future need to be removed.

No actual comments about the approach in question to solve the issue.
Will leave that to someone more knowledgeable. But at least I'm glad
something is being done about it.

> 
> Signed-off-by: Michael Niedermayer <michael@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
>
Hendrik Leppkes Nov. 5, 2017, 1:52 p.m.
On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer
<michael@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@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)

- Hendrik
Michael Niedermayer Nov. 5, 2017, 3:20 p.m.
On Sun, Nov 05, 2017 at 10:35:06AM -0300, James Almer wrote:
> On 11/5/2017 9:34 AM, Michael Niedermayer 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.
> 
> Could this perhaps be in an opaque internal struct instead, much like
> AVCodecInternal and whatnot?

We could do a AVFrameInternal but that would require some differenecs
to AVCodecInternal.

The AVBufferRef from the patch has its own reference counting and
destruction callback. (which avcodec can use for cleaning it up)

a straight AVFrameInternal (in AVCodecInternal style) would not have
that, we could place the AVFrameInternal inside a AVBufferRef or
provide seperate API / fields to make cleanup from libavcodec possible.


> As wm4 said in the relevant discussion,
> this approach is nonoptimal and *will* snowball into a mess of fields if
> other libav* libraries start requiring their own buffers in a frame.

I remember and i think i didnt reply there but this is not really
a plausible scenario.

If we added a field per lib we would have realistically 2 fields
one for libavcodec and one for libavfilter. Beyond that even with
constructed use cases it seems to become hard (at least to me ATM) to
see what else would be added. libswscale ? libavformat ? but what would
they do with their fields ?

But really i primarly want this to move forward, i have no real
preferrance as long as we dont cram muliple opaques in a single opaque
field with wraping or other.


> An internal field of an opaque struct being in a public header is much
> cleaner and easier to maintain than adding such specific fields that may
> at some point in the future need to be removed.
> 
> No actual comments about the approach in question to solve the issue.
> Will leave that to someone more knowledgeable. But at least I'm glad
> something is being done about it.
> 

[...]
Michael Niedermayer Nov. 5, 2017, 3:25 p.m.
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@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@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)


[...]
Rostislav Pehlivanov Nov. 5, 2017, 6:08 p.m.
On 5 November 2017 at 13:52, Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer
> <michael@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@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)
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


No, this would be horrible, lavfi should be able to use what's in the
avbufferref without reinitializing anything that it should contain. lavu
should handle anything that has to be initialized in the struct the
avbufferref backs so that all internal libraries can use it.
Hendrik Leppkes Nov. 5, 2017, 6:18 p.m.
On Sun, Nov 5, 2017 at 7:08 PM, Rostislav Pehlivanov
<atomnuker@gmail.com> wrote:
> On 5 November 2017 at 13:52, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
>> On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer
>> <michael@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@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)
>>
>> - Hendrik
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
> No, this would be horrible, lavfi should be able to use what's in the
> avbufferref without reinitializing anything that it should contain. lavu
> should handle anything that has to be initialized in the struct the
> avbufferref backs so that all internal libraries can use it.

The entire point of this field is to contain opaque private data that
no other library ever has any business of even knowing exists.
If additional data needs to be shared between libraries, it should be
a properly defined public shared structure.

- Hendrik
James Almer Nov. 5, 2017, 11:48 p.m.
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@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@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.
Timo Rothenpieler Nov. 6, 2017, 12:19 p.m.
Am 05.11.2017 um 14:35 schrieb James Almer:
> On 11/5/2017 9:34 AM, Michael Niedermayer 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.
> 
> Could this perhaps be in an opaque internal struct instead, much like
> AVCodecInternal and whatnot? As wm4 said in the relevant discussion,
> this approach is nonoptimal and *will* snowball into a mess of fields if
> other libav* libraries start requiring their own buffers in a frame.
> An internal field of an opaque struct being in a public header is much
> cleaner and easier to maintain than adding such specific fields that may
> at some point in the future need to be removed.

The problem here is that avcodec, due to nested decoders and the like, 
might potentially wrap this field multiple times internally, so it 
basically has to be something avcodec specific.
Timo Rothenpieler Nov. 6, 2017, 12:23 p.m.
>> 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)

I like private_ref.

Following this approach also keeps the diff to libav small.
James Almer Nov. 6, 2017, 1:23 p.m.
On 11/6/2017 9:19 AM, Timo Rothenpieler wrote:
> Am 05.11.2017 um 14:35 schrieb James Almer:
>> On 11/5/2017 9:34 AM, Michael Niedermayer 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.
>>
>> Could this perhaps be in an opaque internal struct instead, much like
>> AVCodecInternal and whatnot? As wm4 said in the relevant discussion,
>> this approach is nonoptimal and *will* snowball into a mess of fields if
>> other libav* libraries start requiring their own buffers in a frame.
>> An internal field of an opaque struct being in a public header is much
>> cleaner and easier to maintain than adding such specific fields that may
>> at some point in the future need to be removed.
> 
> The problem here is that avcodec, due to nested decoders and the like,
> might potentially wrap this field multiple times internally, so it
> basically has to be something avcodec specific.

And an opaque internal struct would require avpriv_ symbols to be
accessed from outside libavutil, so it's an ugly solution nonetheless. I
already told Michael to discard the suggestion.

Patch hide | download patch | download mbox

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