[RFC] vda: Add proper name prefixes to public symbols and deprecate the others

Message ID 1378203665-8915-1-git-send-email-diego@biurrun.de
State New
Headers show

Commit Message

Diego Biurrun Sept. 3, 2013, 10:21 a.m.
---

This is an RFC and still needs testing, even compile testing; I have no Mac.

 libavcodec/vda.h      |   97 ++++++++++++++++++++++++++++++++++++++++++++++++-
 libavcodec/vda_h264.c |   30 +++++++++++----
 libavcodec/version.h  |    3 ++
 3 files changed, 121 insertions(+), 9 deletions(-)

Comments

Luca Barbato Sept. 3, 2013, 10:29 a.m. | #1
On 03/09/13 12:21, Diego Biurrun wrote:
> ---
> 
> This is an RFC and still needs testing, even compile testing; I have no Mac.
> 

On my mac I have something similar using the avhwaccel namespace.

lu
Luca Barbato Sept. 3, 2013, 10:37 a.m. | #2
On 03/09/13 12:29, Luca Barbato wrote:
> On 03/09/13 12:21, Diego Biurrun wrote:
>> ---
>>
>> This is an RFC and still needs testing, even compile testing; I have no Mac.
>>
> 
> On my mac I have something similar using the avhwaccel namespace.
> 
> lu
> 

Btw those functions assume they get an hwaccel pointer allocated while
doesn't seem to me anything allocates it.

I guess we'll have to discuss with the vlc-mac people or have me look at
their code once again.

lu
Rémi Denis-Courmont Sept. 5, 2013, 10:33 a.m. | #3
On Tue,  3 Sep 2013 12:21:05 +0200, Diego Biurrun <diego@biurrun.de>
wrote:
> ---
> 
> This is an RFC and still needs testing, even compile testing; I have no
> Mac.

There should be no needs to export functions for hwaccel as currently
designed. Thus exporting functions looks like a step backward to me.

(Renaming the structure is fine though.)

> 
>  libavcodec/vda.h      |   97
>  ++++++++++++++++++++++++++++++++++++++++++++++++-
>  libavcodec/vda_h264.c |   30 +++++++++++----
>  libavcodec/version.h  |    3 ++
>  3 files changed, 121 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/vda.h b/libavcodec/vda.h
> index 987b94f..9e67d41 100644
> --- a/libavcodec/vda.h
> +++ b/libavcodec/vda.h
> @@ -29,10 +29,10 @@
>   * Public libavcodec VDA header.
>   */
>  
> -#include "libavcodec/version.h"
> -
>  #include <stdint.h>
>  
> +#include "version.h"
> +
>  // emmintrin.h is unable to compile with -std=c99
>  -Werror=missing-prototypes
>  // http://openradar.appspot.com/8026390
>  #undef __GNUC_STDC_INLINE__
> @@ -54,6 +54,86 @@
>   *
>   * The application must make it available as
>   AVCodecContext.hwaccel_context.
>   */
> +typedef struct AVVDAContext {
> +    /**
> +     * VDA decoder object.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set/Unset by libavcodec.
> +     */
> +    VDADecoder decoder;

That should be set by the user.

> +
> +    /**
> +     * The Core Video pixel buffer that contains the current image
data.
> +     *
> +     * encoding: unused
> +     * decoding: Set by libavcodec. Unset by user.

That scheme cannot work and I guess it is responsible for the massive
memory leaks experienced in VLC's VDA.

> +     */
> +    CVPixelBufferRef cv_buffer;

This should probably be in the AVframe rather than the the decoder
context.

> +
> +    /**
> +     * Use the hardware decoder in synchronous mode.
> +     *
> +     * encoding: unused
> +     * decoding: Set by user.
> +     */
> +    int use_sync_decoding;
> +
> +    /**
> +     * The frame width.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set/Unset by user.
> +     */
> +    int width;

Not sure why this is needed at all.

> +
> +    /**
> +     * The frame height.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set/Unset by user.
> +     */
> +    int height;
> +
> +    /**
> +     * The frame format.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set/Unset by user.
> +     */
> +    int format;
> +
> +    /**
> +     * The pixel format for output image buffers.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set/Unset by user.
> +     */
> +    OSType cv_pix_fmt_type;
> +
> +    /**
> +     * The current bitstream buffer.
> +     */
> +    uint8_t *priv_bitstream;

Lets not reproduce the VDPAU mistake again. This belongs in the hwaccel
priv_data.

> +
> +    /**
> +     * The current size of the bitstream.
> +     */
> +    int priv_bitstream_size;

Ditto.

> +
> +    /**
> +     * The reference size used for fast reallocation.
> +     */
> +    int priv_allocated_size;

Ditto.

> +} AVVDAContext;
> +
> +/**
> + * This structure is used to provide the necessary configurations and
data
> + * to the VDA Libav HWAccel implementation.
> + *
> + * The application must make it available as
> AVCodecContext.hwaccel_context.
> + */
> +attribute_deprecated
>  struct vda_context {
>      /**
>       * VDA decoder object.
> @@ -128,12 +208,25 @@ struct vda_context {
>  };
>  
>  /** Create the video decoder. */
> +int av_vda_create_decoder(struct AVVDAContext *vda_ctx,
> +                          uint8_t *extradata,
> +                          int extradata_size);
> +
> +/** Destroy the video decoder. */
> +int av_vda_destroy_decoder(struct AVVDAContext *vda_ctx);
> +
> +
> +#if FF_API_VDAPRIVPREFIX
> +/** Create the video decoder. */
> +attribute_deprecated
>  int ff_vda_create_decoder(struct vda_context *vda_ctx,
>                            uint8_t *extradata,
>                            int extradata_size);
>  
>  /** Destroy the video decoder. */
> +attribute_deprecated
>  int ff_vda_destroy_decoder(struct vda_context *vda_ctx);
> +#endif /* FF_API_VDAPRIVPREFIX */
>  
>  /**
>   * @}
> diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
> index 6c1845a..b552282 100644
> --- a/libavcodec/vda_h264.c
> +++ b/libavcodec/vda_h264.c
> @@ -26,6 +26,7 @@
>  
>  #include "libavutil/avutil.h"
>  #include "h264.h"
> +#include "version.h"
>  #include "vda.h"
>  
>  /* Decoder callback that adds the VDA frame to the queue in display
>  order. */
> @@ -35,7 +36,7 @@ static void vda_decoder_callback(void *vda_hw_ctx,
>                                   uint32_t infoFlags,
>                                   CVImageBufferRef image_buffer)
>  {
> -    struct vda_context *vda_ctx = vda_hw_ctx;
> +    struct AVVDAContext *vda_ctx = vda_hw_ctx;
>  
>      if (!image_buffer)
>          return;
> @@ -46,7 +47,7 @@ static void vda_decoder_callback(void *vda_hw_ctx,
>      vda_ctx->cv_buffer = CVPixelBufferRetain(image_buffer);
>  }
>  
> -static int vda_sync_decode(struct vda_context *vda_ctx)
> +static int vda_sync_decode(struct AVVDAContext *vda_ctx)
>  {
>      OSStatus status;
>      CFDataRef coded_frame;
> @@ -71,7 +72,7 @@ static int vda_h264_start_frame(AVCodecContext *avctx,
>                                  av_unused const uint8_t *buffer,
>                                  av_unused uint32_t size)
>  {
> -    struct vda_context *vda_ctx         = avctx->hwaccel_context;
> +    struct AVVDAContext *vda_ctx = avctx->hwaccel_context;
>  
>      if (!vda_ctx->decoder)
>          return -1;
> @@ -85,7 +86,7 @@ static int vda_h264_decode_slice(AVCodecContext
*avctx,
>                                   const uint8_t *buffer,
>                                   uint32_t size)
>  {
> -    struct vda_context *vda_ctx         = avctx->hwaccel_context;
> +    struct AVVDAContext *vda_ctx = avctx->hwaccel_context;
>      void *tmp;
>  
>      if (!vda_ctx->decoder)
> @@ -110,7 +111,7 @@ static int vda_h264_decode_slice(AVCodecContext
*avctx,
>  static int vda_h264_end_frame(AVCodecContext *avctx)
>  {
>      H264Context *h                      = avctx->priv_data;
> -    struct vda_context *vda_ctx         = avctx->hwaccel_context;
> +    struct AVVDAContext *vda_ctx        = avctx->hwaccel_context;
>      AVFrame *frame                      = &h->cur_pic_ptr->f;
>      int status;
>  
> @@ -126,7 +127,7 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
>      return status;
>  }
>  
> -int ff_vda_create_decoder(struct vda_context *vda_ctx,
> +int av_vda_create_decoder(struct AVVDAContext *vda_ctx,
>                            uint8_t *extradata,
>                            int extradata_size)
>  {
> @@ -210,7 +211,7 @@ int ff_vda_create_decoder(struct vda_context
*vda_ctx,
>      return status;
>  }
>  
> -int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
> +int av_vda_destroy_decoder(struct AVVDAContext *vda_ctx)
>  {
>      OSStatus status = kVDADecoderNoErr;
>  
> @@ -222,6 +223,21 @@ int ff_vda_destroy_decoder(struct vda_context
> *vda_ctx)
>      return status;
>  }
>  
> +#if FF_API_VDAPRIVPREFIX
> +int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
> +{
> +    return av_vda_destroy_decoder((struct AVVDAContext *) vda_ctx);
> +}
> +
> +int ff_vda_create_decoder(struct vda_context *vda_ctx,
> +                          uint8_t *extradata,
> +                          int extradata_size)
> +{
> +    return av_vda_create_decoder((struct AVVDAContext *) vda_ctx,
> extradata,
> +                                 extradata_size);
> +}
> +#endif /* FF_API_VDAPRIVPREFIX */
> +
>  AVHWAccel ff_h264_vda_hwaccel = {
>      .name           = "h264_vda",
>      .type           = AVMEDIA_TYPE_VIDEO,
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 980c5c5..5465693 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -73,5 +73,8 @@
>  #ifndef FF_API_VOXWARE
>  #define FF_API_VOXWARE           (LIBAVCODEC_VERSION_MAJOR < 56)
>  #endif
> +#ifndef FF_API_VDAPRIVPREFIX
> +#define FF_API_VDAPRIVPREFIX     (LIBAVCODEC_VERSION_MAJOR < 56)
> +#endif
>  
>  #endif /* AVCODEC_VERSION_H */
Luca Barbato Sept. 5, 2013, 11:38 a.m. | #4
On 05/09/13 12:33, Rémi Denis-Courmont wrote:
> On Tue,  3 Sep 2013 12:21:05 +0200, Diego Biurrun <diego@biurrun.de>
> wrote:
>> ---
>>
>> This is an RFC and still needs testing, even compile testing; I have no
>> Mac.
> 
> There should be no needs to export functions for hwaccel as currently
> designed. Thus exporting functions looks like a step backward to me.

We could try to draft the correct behaviour and implement it. The
current codebase is quite sketchy.

lu
Stefano Pigozzi Sept. 5, 2013, 4:22 p.m. | #5
On Thu, Sep 5, 2013 at 12:33 PM, Rémi Denis-Courmont <remi@remlab.net> wrote:
>> +    /**
>> +     * The Core Video pixel buffer that contains the current image
> data.
>> +     *
>> +     * encoding: unused
>> +     * decoding: Set by libavcodec. Unset by user.
>
> That scheme cannot work and I guess it is responsible for the massive
> memory leaks experienced in VLC's VDA.

IIRC the VDA framework itself creates these CVPixelBuffers (it has a
pool of them). CVPixelBuffers are refcounted and if their refcount
reaches 0 the underlying IOSurface can be reused by the OS. Maybe the
AVFrame could be made to call CVPixelBufferRelease when it is
deallocated. Client media players can then store these CVPixelBuffers
and call CVPixelBufferRetain to hold onto them.

Btw, I have no idea why one has to provide a get_buffer implementation
when using VDA's hwaccel API since the "buffer" is created by libav
itself. In mpv-player I have been providing dummy buffers...

Stefano
Rémi Denis-Courmont Sept. 5, 2013, 4:42 p.m. | #6
Le jeudi 5 septembre 2013 18:22:23 Stefano Pigozzi a écrit :
> On Thu, Sep 5, 2013 at 12:33 PM, Rémi Denis-Courmont <remi@remlab.net> 
wrote:
> >> +    /**
> >> +     * The Core Video pixel buffer that contains the current image
> > 
> > data.
> > 
> >> +     *
> >> +     * encoding: unused
> >> +     * decoding: Set by libavcodec. Unset by user.
> > 
> > That scheme cannot work and I guess it is responsible for the massive
> > memory leaks experienced in VLC's VDA.
> 
> IIRC the VDA framework itself creates these CVPixelBuffers (it has a
> pool of them). CVPixelBuffers are refcounted and if their refcount
> reaches 0 the underlying IOSurface can be reused by the OS. Maybe the
> AVFrame could be made to call CVPixelBufferRelease when it is
> deallocated.

All other hwaccel allocate the pixel buffers from get_buffer().

> Client media players can then store these CVPixelBuffers
> and call CVPixelBufferRetain to hold onto them.

You could maybe make a special case for VDA then, and have libavcodec allocate 
and deallocate the video buffers. But regardless, libavcodec is not supposed to 
override the AVFrame.data[] pointers while decoding.

Furthermore, the pixel buffer handle should be in AVFrame, not in the decoder 
context.

> Btw, I have no idea why one has to provide a get_buffer implementation
> when using VDA's hwaccel API since the "buffer" is created by libav
> itself. In mpv-player I have been providing dummy buffers...

Yes. This is a design bug of the current VDA hwaccel implementation.

Patch

diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..9e67d41 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -29,10 +29,10 @@ 
  * Public libavcodec VDA header.
  */
 
-#include "libavcodec/version.h"
-
 #include <stdint.h>
 
+#include "version.h"
+
 // emmintrin.h is unable to compile with -std=c99 -Werror=missing-prototypes
 // http://openradar.appspot.com/8026390
 #undef __GNUC_STDC_INLINE__
@@ -54,6 +54,86 @@ 
  *
  * The application must make it available as AVCodecContext.hwaccel_context.
  */
+typedef struct AVVDAContext {
+    /**
+     * VDA decoder object.
+     *
+     * - encoding: unused
+     * - decoding: Set/Unset by libavcodec.
+     */
+    VDADecoder decoder;
+
+    /**
+     * The Core Video pixel buffer that contains the current image data.
+     *
+     * encoding: unused
+     * decoding: Set by libavcodec. Unset by user.
+     */
+    CVPixelBufferRef cv_buffer;
+
+    /**
+     * Use the hardware decoder in synchronous mode.
+     *
+     * encoding: unused
+     * decoding: Set by user.
+     */
+    int use_sync_decoding;
+
+    /**
+     * The frame width.
+     *
+     * - encoding: unused
+     * - decoding: Set/Unset by user.
+     */
+    int width;
+
+    /**
+     * The frame height.
+     *
+     * - encoding: unused
+     * - decoding: Set/Unset by user.
+     */
+    int height;
+
+    /**
+     * The frame format.
+     *
+     * - encoding: unused
+     * - decoding: Set/Unset by user.
+     */
+    int format;
+
+    /**
+     * The pixel format for output image buffers.
+     *
+     * - encoding: unused
+     * - decoding: Set/Unset by user.
+     */
+    OSType cv_pix_fmt_type;
+
+    /**
+     * The current bitstream buffer.
+     */
+    uint8_t *priv_bitstream;
+
+    /**
+     * The current size of the bitstream.
+     */
+    int priv_bitstream_size;
+
+    /**
+     * The reference size used for fast reallocation.
+     */
+    int priv_allocated_size;
+} AVVDAContext;
+
+/**
+ * This structure is used to provide the necessary configurations and data
+ * to the VDA Libav HWAccel implementation.
+ *
+ * The application must make it available as AVCodecContext.hwaccel_context.
+ */
+attribute_deprecated
 struct vda_context {
     /**
      * VDA decoder object.
@@ -128,12 +208,25 @@  struct vda_context {
 };
 
 /** Create the video decoder. */
+int av_vda_create_decoder(struct AVVDAContext *vda_ctx,
+                          uint8_t *extradata,
+                          int extradata_size);
+
+/** Destroy the video decoder. */
+int av_vda_destroy_decoder(struct AVVDAContext *vda_ctx);
+
+
+#if FF_API_VDAPRIVPREFIX
+/** Create the video decoder. */
+attribute_deprecated
 int ff_vda_create_decoder(struct vda_context *vda_ctx,
                           uint8_t *extradata,
                           int extradata_size);
 
 /** Destroy the video decoder. */
+attribute_deprecated
 int ff_vda_destroy_decoder(struct vda_context *vda_ctx);
+#endif /* FF_API_VDAPRIVPREFIX */
 
 /**
  * @}
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..b552282 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -26,6 +26,7 @@ 
 
 #include "libavutil/avutil.h"
 #include "h264.h"
+#include "version.h"
 #include "vda.h"
 
 /* Decoder callback that adds the VDA frame to the queue in display order. */
@@ -35,7 +36,7 @@  static void vda_decoder_callback(void *vda_hw_ctx,
                                  uint32_t infoFlags,
                                  CVImageBufferRef image_buffer)
 {
-    struct vda_context *vda_ctx = vda_hw_ctx;
+    struct AVVDAContext *vda_ctx = vda_hw_ctx;
 
     if (!image_buffer)
         return;
@@ -46,7 +47,7 @@  static void vda_decoder_callback(void *vda_hw_ctx,
     vda_ctx->cv_buffer = CVPixelBufferRetain(image_buffer);
 }
 
-static int vda_sync_decode(struct vda_context *vda_ctx)
+static int vda_sync_decode(struct AVVDAContext *vda_ctx)
 {
     OSStatus status;
     CFDataRef coded_frame;
@@ -71,7 +72,7 @@  static int vda_h264_start_frame(AVCodecContext *avctx,
                                 av_unused const uint8_t *buffer,
                                 av_unused uint32_t size)
 {
-    struct vda_context *vda_ctx         = avctx->hwaccel_context;
+    struct AVVDAContext *vda_ctx = avctx->hwaccel_context;
 
     if (!vda_ctx->decoder)
         return -1;
@@ -85,7 +86,7 @@  static int vda_h264_decode_slice(AVCodecContext *avctx,
                                  const uint8_t *buffer,
                                  uint32_t size)
 {
-    struct vda_context *vda_ctx         = avctx->hwaccel_context;
+    struct AVVDAContext *vda_ctx = avctx->hwaccel_context;
     void *tmp;
 
     if (!vda_ctx->decoder)
@@ -110,7 +111,7 @@  static int vda_h264_decode_slice(AVCodecContext *avctx,
 static int vda_h264_end_frame(AVCodecContext *avctx)
 {
     H264Context *h                      = avctx->priv_data;
-    struct vda_context *vda_ctx         = avctx->hwaccel_context;
+    struct AVVDAContext *vda_ctx        = avctx->hwaccel_context;
     AVFrame *frame                      = &h->cur_pic_ptr->f;
     int status;
 
@@ -126,7 +127,7 @@  static int vda_h264_end_frame(AVCodecContext *avctx)
     return status;
 }
 
-int ff_vda_create_decoder(struct vda_context *vda_ctx,
+int av_vda_create_decoder(struct AVVDAContext *vda_ctx,
                           uint8_t *extradata,
                           int extradata_size)
 {
@@ -210,7 +211,7 @@  int ff_vda_create_decoder(struct vda_context *vda_ctx,
     return status;
 }
 
-int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
+int av_vda_destroy_decoder(struct AVVDAContext *vda_ctx)
 {
     OSStatus status = kVDADecoderNoErr;
 
@@ -222,6 +223,21 @@  int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
     return status;
 }
 
+#if FF_API_VDAPRIVPREFIX
+int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
+{
+    return av_vda_destroy_decoder((struct AVVDAContext *) vda_ctx);
+}
+
+int ff_vda_create_decoder(struct vda_context *vda_ctx,
+                          uint8_t *extradata,
+                          int extradata_size)
+{
+    return av_vda_create_decoder((struct AVVDAContext *) vda_ctx, extradata,
+                                 extradata_size);
+}
+#endif /* FF_API_VDAPRIVPREFIX */
+
 AVHWAccel ff_h264_vda_hwaccel = {
     .name           = "h264_vda",
     .type           = AVMEDIA_TYPE_VIDEO,
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 980c5c5..5465693 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -73,5 +73,8 @@ 
 #ifndef FF_API_VOXWARE
 #define FF_API_VOXWARE           (LIBAVCODEC_VERSION_MAJOR < 56)
 #endif
+#ifndef FF_API_VDAPRIVPREFIX
+#define FF_API_VDAPRIVPREFIX     (LIBAVCODEC_VERSION_MAJOR < 56)
+#endif
 
 #endif /* AVCODEC_VERSION_H */