Re: [FFmpeg-devel] [PATCH 1/2] AVFrame: add an opaque_ref field

[wm4]

On Sat, 11 Feb 2017 17:30:24 +0100
Michael Niedermayer  wrote:

> On Sat, Feb 11, 2017 at 02:17:36PM +0100, wm4 wrote:
> > This is an extended version of the AVFrame.opaque field, which can be
> > used to attach arbitrary user information to an AVFrame.
> > 
> > The usefulness of the opaque field is rather limited, because it can
> > store only up to 32 bits of information (or 64 bit on 64 bit systems).
> > It's not possible to set this field to a memory allocation, because
> > there is no way to deallocate it correctly.
> > 
> > The opaque_ref field circumvents this by letting the user set an
> > AVBuffer, which makes the user data refcounted.
> > 
> > Signed-off-by: Anton Khirnov 
> > 
> > Merges Libav commit 04f3bd349651.
> > ---
> >  doc/APIchanges  |  3 +++
> >  libavutil/frame.c   |  9 +
> >  libavutil/frame.h   | 11 +++
> >  libavutil/version.h |  2 +-
> >  4 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 8bca71ef36..81e49c22b9 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,9 @@ libavutil: 2015-08-28
> >  
> >  API changes, most recent first:
> >  
> > +2017-02-11 - xxx - lavu 55.47.100 - frame.h
> > +  Add AVFrame.opaque_ref.
> > +
> >  2017-01-31 - xxx - lavu 55.46.100 / 55.20.0 - cpu.h
> >Add AV_CPU_FLAG_SSSE3SLOW.
> >  
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index a08e0c539d..2913982e91 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -379,6 +379,13 @@ FF_DISABLE_DEPRECATION_WARNINGS
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >  #endif
> >  
> > +av_buffer_unref(>opaque_ref);
> > +if (src->opaque_ref) {
> > +dst->opaque_ref = av_buffer_ref(src->opaque_ref);
> > +if (!dst->opaque_ref)
> > +return AVERROR(ENOMEM);
> > +}  
> 
> not just here but
> the error handling in the whole function is not optimal,
> ideally the destination frame should be left intact on error while
> this could leave a half copied, unref and left as is mix

Well, there's not much you can do. Side data in the function has the
same problem. I think if a caller expects consistency, he will have to
unref the destination frame if this function fails. And that should
still work even with this new field.

The alternative would be either unreffing the entire frame as part of
the failure path of this function (a bit too radical?), or carefully
backing up the frame and partially copying back the result on failure
(would be too much?). I thought about working on a byte-copied
temporary AVFrame, but that doesn't help with all the references.

So I left this unchanged.

> 
> > +
> >  return 0;
> >  }
> >  
> > @@ -513,6 +520,8 @@ void av_frame_unref(AVFrame *frame)
> >  
> >  av_buffer_unref(>hw_frames_ctx);
> >  
> > +av_buffer_unref(>opaque_ref);
> > +
> >  get_frame_defaults(frame);
> >  }
> >  
> > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > index b4500923af..3023559beb 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -538,6 +538,17 @@ typedef struct AVFrame {
> >   * AVHWFramesContext describing the frame.
> >   */
> >  AVBufferRef *hw_frames_ctx;
> > +
> > +/**
> > + * AVBufferRef for free use by the API user. Libav will never check the
> > + * contents of the buffer ref. Libav calls av_buffer_unref() on it 
> > when  
> 
> wrong project name
> 
> [...]

Changed and pushed both patches.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] AVFrame: add an opaque_ref field

[Michael Niedermayer]

On Sat, Feb 11, 2017 at 02:17:36PM +0100, wm4 wrote:
> This is an extended version of the AVFrame.opaque field, which can be
> used to attach arbitrary user information to an AVFrame.
> 
> The usefulness of the opaque field is rather limited, because it can
> store only up to 32 bits of information (or 64 bit on 64 bit systems).
> It's not possible to set this field to a memory allocation, because
> there is no way to deallocate it correctly.
> 
> The opaque_ref field circumvents this by letting the user set an
> AVBuffer, which makes the user data refcounted.
> 
> Signed-off-by: Anton Khirnov 
> 
> Merges Libav commit 04f3bd349651.
> ---
>  doc/APIchanges  |  3 +++
>  libavutil/frame.c   |  9 +
>  libavutil/frame.h   | 11 +++
>  libavutil/version.h |  2 +-
>  4 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 8bca71ef36..81e49c22b9 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2015-08-28
>  
>  API changes, most recent first:
>  
> +2017-02-11 - xxx - lavu 55.47.100 - frame.h
> +  Add AVFrame.opaque_ref.
> +
>  2017-01-31 - xxx - lavu 55.46.100 / 55.20.0 - cpu.h
>Add AV_CPU_FLAG_SSSE3SLOW.
>  
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index a08e0c539d..2913982e91 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -379,6 +379,13 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
> +av_buffer_unref(>opaque_ref);
> +if (src->opaque_ref) {
> +dst->opaque_ref = av_buffer_ref(src->opaque_ref);
> +if (!dst->opaque_ref)
> +return AVERROR(ENOMEM);
> +}

not just here but
the error handling in the whole function is not optimal,
ideally the destination frame should be left intact on error while
this could leave a half copied, unref and left as is mix


> +
>  return 0;
>  }
>  
> @@ -513,6 +520,8 @@ void av_frame_unref(AVFrame *frame)
>  
>  av_buffer_unref(>hw_frames_ctx);
>  
> +av_buffer_unref(>opaque_ref);
> +
>  get_frame_defaults(frame);
>  }
>  
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index b4500923af..3023559beb 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -538,6 +538,17 @@ typedef struct AVFrame {
>   * AVHWFramesContext describing the frame.
>   */
>  AVBufferRef *hw_frames_ctx;
> +
> +/**
> + * AVBufferRef for free use by the API user. Libav will never check the
> + * contents of the buffer ref. Libav calls av_buffer_unref() on it when

wrong project name

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel