[libav-devel] [PATCH] hwaccel: Video Decoder Acceleration (VDA) support

Sebastien Zwickert dilaroga at gmail.com
Thu Nov 3 10:22:37 CET 2011


On Nov 2, 2011, at 11:45 PM, Diego Biurrun wrote:

> Another fresh patch with some of these suggestions already applied
> will arrive on the ml soon.
> 
> On Wed, Nov 02, 2011 at 10:44:41PM +0100, Diego Biurrun wrote:
>> 
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -45,6 +45,7 @@ RDFT-OBJS-$(CONFIG_HARDCODED_TABLES)   += sin_tables.o
>> OBJS-$(CONFIG_SINEWIN)                 += sinewin.o
>> OBJS-$(CONFIG_VAAPI)                   += vaapi.o
>> +OBJS-$(CONFIG_VDA)                     += vda.o
>> OBJS-$(CONFIG_VDPAU)                   += vdpau.o
>> 
>> @@ -178,6 +179,7 @@ OBJS-$(CONFIG_H264_DECODER)            += h264.o                               \
>>                                           mpegvideo.o error_resilience.o
>> OBJS-$(CONFIG_H264_VAAPI_HWACCEL)      += vaapi_h264.o
>> +OBJS-$(CONFIG_H264_VDA_HWACCEL)        += vda_h264.o
>> OBJS-$(CONFIG_HUFFYUV_DECODER)         += huffyuv.o
>> OBJS-$(CONFIG_HUFFYUV_ENCODER)         += huffyuv.o
> 
> How much sense does VDA make without H.264 VDA?  It seems to me both
> should just be enabled together.

I don't know the future of VDA framework. Maybe more codecs will be supported.


> 
>> --- /dev/null
>> +++ b/libavcodec/vda.c
>> @@ -0,0 +1,264 @@
>> +
>> +static int64_t vda_pts_from_dictionary(CFDictionaryRef user_info)
>> +{
>> +    CFNumberRef pts;
>> +    int64_t outValue = 0;
>> +
>> +    if (NULL == user_info)
> 
> Just
> 
>  if (!user_info)
> 
> is preferred.

Ok. I forgot to remove this one. I'll do.

> 
>> +static void vda_decoder_callback(void *vda_hw_ctx,
>> +                                 CFDictionaryRef user_info,
>> +                                 OSStatus status,
>> +                                 uint32_t infoFlags,
>> +                                 CVImageBufferRef image_buffer)
>> +{
>> +    struct vda_context *vda_ctx = (struct vda_context*)vda_hw_ctx;
> 
> This void* cast should be unnecessary.
> 
>> +    new_frame = (vda_frame*)av_mallocz(sizeof(vda_frame));
> 
> This void* cast is unnecessary.
> 
>> +int ff_vda_create_decoder(struct vda_context *vda_ctx,
>> +                          uint8_t *extradata,
>> +                          int extradata_size)
>> +{
>> +
>> +    status = VDADecoderCreate(config_info,
>> +                              NULL,
>> +                              (VDADecoderOutputCallback *)vda_decoder_callback,
>> +                              (void *)vda_ctx,
>> +                              &vda_ctx->decoder );
> 
> Is the cast necessary?

Probably not  as the two previously mentioned. I'll move these useless casts.

> 
>> +int ff_vda_destroy_decoder(struct vda_context *vda_ctx)
>> +{
>> +    OSStatus status = kVDADecoderNoErr;
>> +
>> +    if (vda_ctx->decoder)
>> +        status = VDADecoderDestroy(vda_ctx->decoder);
>> +
>> +    vda_clear_queue(vda_ctx);
>> +
>> +    if (vda_ctx->queue_mutex != NULL)
> 
> Same here, dropping the "!= NULL" is preferred.
> 
>> --- /dev/null
>> +++ b/libavcodec/vda.h
>> @@ -0,0 +1,138 @@
>> +
>> +#ifndef AVCODEC_VDA_H
>> +#define AVCODEC_VDA_H
>> +
>> +#include <stdint.h>
>> +
>> +// emmintrin.h is unable to compile with -std=c99 -Werror=missing-prototypes
>> +// http://openradar.appspot.com/8026390
>> +#undef __GNUC_STDC_INLINE__
>> +
>> +#define Picture QuickdrawPicture
>> +
>> +#include <pthread.h>
>> +#include <VideoDecodeAcceleration/VDADecoder.h>
>> +
>> +#include "avcodec.h"
> 
> The #define looks dangerous to me, we have a struct Picture, so I fear
> for unintended side effects.

Undefining Picture after including <VideoDecoderAcceleration/VDADecoder.h> would
be safe as suggested Janne.

> 
> Also, can the emmintrin.h workaround be done somewhere else?  Where does
> it get #included from?

I have to look more precisely at this workaround. I wrote it a long time ago, I forgot the details.

> 
>> --- /dev/null
>> +++ b/libavcodec/vda_internal.h
>> @@ -0,0 +1,49 @@
>> +
>> +#ifndef AVCODEC_VDA_INTERNAL_H
>> +#define AVCODEC_VDA_INTERNAL_H
>> +
>> +#include <CoreFoundation/CFDictionary.h>
>> +#include <CoreFoundation/CFNumber.h>
>> +#include <CoreFoundation/CFData.h>
>> +#include <CoreFoundation/CFString.h>
>> +
>> +#include "h264.h"
>> +#include "h264data.h"
>> +#include "vda.h"
>> +
>> +/** Send a frame data to the hardware decoder. */
>> +int ff_vda_decoder_decode(struct vda_context *vda_ctx,
>> +                          uint8_t *bitstream,
>> +                          int bitstream_size,
>> +                          int64_t frame_pts);
>> +
>> +#endif /* AVCODEC_VDA_INTERNAL_H */
> 
> These #includes should be placed in the files that actually use them.

Ok.


Thanks for reviewing!

Best regards,

Sebastien.


More information about the libav-devel mailing list