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.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
