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

Submitted by Michael Niedermayer on Nov. 8, 2017, 10:55 p.m.

Details

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

Commit Message

Michael Niedermayer Nov. 8, 2017, 10:55 p.m.
This gives FFmpeg libs a field that they 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 | 12 ++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Rostislav Pehlivanov Nov. 8, 2017, 11:05 p.m.
On 8 November 2017 at 22:55, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> This gives FFmpeg libs a field that they 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 | 12 ++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 982fbb5c81..662a7e5ab5 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->private_ref);
>      if (src->opaque_ref) {
>          dst->opaque_ref = av_buffer_ref(src->opaque_ref);
>          if (!dst->opaque_ref)
>              return AVERROR(ENOMEM);
>      }
> -
> +    if (src->private_ref) {
> +        dst->private_ref = av_buffer_ref(src->private_ref);
> +        if (!dst->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->private_ref);
>
>      get_frame_defaults(frame);
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 0c6aab1c02..7b9bec054a 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -563,6 +563,18 @@ typedef struct AVFrame {
>      /**
>       * @}
>       */
> +    /**
> +     * AVBufferRef for free use by libavcodec / libavfilte / .... Code
> outside
> +     * the FFmpeg libs 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 private_ref field.
> +     *
> +     * The field should be set to NULL by the FFmpeg libs before passing
> a frame
> +     * to the user.
> +     */
> +    AVBufferRef *private_ref;
>  } AVFrame;
>
>  #if FF_API_FRAME_GET_SET
> --
> 2.15.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


This makes it sound as if it would get used and can't be overwritten (e.g.
feeding frames from a decoder to an encoder and zeroing this field would
make things go bad).

Just say its temporary storage only and that it doesn't get used after
decoding or outputting.
Michael Niedermayer Nov. 9, 2017, 2:37 a.m.
On Wed, Nov 08, 2017 at 11:05:48PM +0000, Rostislav Pehlivanov wrote:
> On 8 November 2017 at 22:55, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > This gives FFmpeg libs a field that they 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 | 12 ++++++++++++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index 982fbb5c81..662a7e5ab5 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->private_ref);
> >      if (src->opaque_ref) {
> >          dst->opaque_ref = av_buffer_ref(src->opaque_ref);
> >          if (!dst->opaque_ref)
> >              return AVERROR(ENOMEM);
> >      }
> > -
> > +    if (src->private_ref) {
> > +        dst->private_ref = av_buffer_ref(src->private_ref);
> > +        if (!dst->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->private_ref);
> >
> >      get_frame_defaults(frame);
> >  }
> > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > index 0c6aab1c02..7b9bec054a 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -563,6 +563,18 @@ typedef struct AVFrame {
> >      /**
> >       * @}
> >       */
> > +    /**
> > +     * AVBufferRef for free use by libavcodec / libavfilte / .... Code
> > outside
> > +     * the FFmpeg libs 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 private_ref field.
> > +     *
> > +     * The field should be set to NULL by the FFmpeg libs before passing
> > a frame
> > +     * to the user.
> > +     */
> > +    AVBufferRef *private_ref;
> >  } AVFrame;
> >
> >  #if FF_API_FRAME_GET_SET
> > --
> > 2.15.0
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> 
> This makes it sound as if it would get used and can't be overwritten (e.g.
> feeding frames from a decoder to an encoder and zeroing this field would
> make things go bad).

I think i do not understand how you arrive at this from the documentation


> 
> Just say its temporary storage only and that it doesn't get used after
> decoding or outputting.

Thats would not be correct.
this field can be used by libavfilter for example and that also makes it
important to clarify who is respnsible for clearing it

Because if libavcodec doesnt clear it and its passed into libavfilter
which also doesnt clear it that would be bad

with one field per lib thats less an issue as there would be no type
incompatibility

[...]

Patch hide | download patch | download mbox

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 982fbb5c81..662a7e5ab5 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->private_ref);
     if (src->opaque_ref) {
         dst->opaque_ref = av_buffer_ref(src->opaque_ref);
         if (!dst->opaque_ref)
             return AVERROR(ENOMEM);
     }
-
+    if (src->private_ref) {
+        dst->private_ref = av_buffer_ref(src->private_ref);
+        if (!dst->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->private_ref);
 
     get_frame_defaults(frame);
 }
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0c6aab1c02..7b9bec054a 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -563,6 +563,18 @@  typedef struct AVFrame {
     /**
      * @}
      */
+    /**
+     * AVBufferRef for free use by libavcodec / libavfilte / .... Code outside
+     * the FFmpeg libs 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 private_ref field.
+     *
+     * The field should be set to NULL by the FFmpeg libs before passing a frame
+     * to the user.
+     */
+    AVBufferRef *private_ref;
 } AVFrame;
 
 #if FF_API_FRAME_GET_SET