[FFmpeg-devel] [PATCH]: Video Decoder Acceleration (VDA) HWAccel module for Mac OS X

[Michael Niedermayer]

On Tue, Nov 01, 2011 at 05:39:30PM +0100, Sebastien Zwickert wrote:
> 
> On Nov 1, 2011, at 4:43 PM, Carl Eugen Hoyos wrote:
> 
> > Sebastien Zwickert <dilaroga <at> gmail.com> writes:
> > 
> >> Change done in the upated patch in attachment.
> > 
> > Please bump libavutil and libavcodec version number and please add a line to the
> > Changelog.
> 
> Done. patch updated.
> 
> Best regards,
> 
> Sebastien.
> 

[...]

> +/* Mutex manager callback. */
> +static int ff_vda_lock_operation(void **mtx, enum AVLockOp op)

the ff prefix is normally used for non static private functions
within one library in ffmpeg.


> +{
> +    switch(op) {
> +        case AV_LOCK_CREATE:
> +            *mtx = av_malloc(sizeof(pthread_mutex_t));
> +            if(!*mtx)
> +                return 1;
> +            return !!pthread_mutex_init(*mtx, NULL);
> +        case AV_LOCK_OBTAIN:
> +            return !!pthread_mutex_lock(*mtx);
> +        case AV_LOCK_RELEASE:
> +            return !!pthread_mutex_unlock(*mtx);
> +        case AV_LOCK_DESTROY:
> +            pthread_mutex_destroy(*mtx);

> +            av_free(*mtx);

av_freep() is a bit safer as it zeros mtx


[...]
> +/* Removes and releases all frames from the queue. */
> +static void ff_vda_clear_queue(struct vda_context *vda_ctx)
> +{
> +    vda_frame *top_frame;
> +    top_frame = NULL;
> +
> +    ff_vda_lock_operation(&vda_ctx->queue_mutex, AV_LOCK_OBTAIN);
> +
> +    while (vda_ctx->queue != NULL)
> +    {
> +        top_frame = vda_ctx->queue;

> +        vda_ctx->queue = (vda_frame *)top_frame->next_frame;

the cast should be unneeded, the type seems to be matching


> +        vda_ctx->queue_depth--;
> +        ff_vda_release_vda_frame(top_frame);
> +    }
> +
> +    ff_vda_lock_operation(&vda_ctx->queue_mutex, AV_LOCK_RELEASE);
> +}
> +
> +
> +/* Decoder callback that adds the vda frame to the queue in display order. */
> +static void ff_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;
> +    vda_frame *new_frame = NULL;
> +    vda_frame *queue_walker = NULL;
> +
> +    if (NULL == image_buffer)
> +        return;
> +
> +    if (kCVPixelFormatType_422YpCbCr8 != CVPixelBufferGetPixelFormatType(image_buffer))
> +        return;
> +

> +    new_frame = (vda_frame *)calloc(sizeof(vda_frame), 1);

this probably should be av_mallocz()
av_free() cannot be used on calloced() data



> +    new_frame->next_frame = NULL;
> +    new_frame->cv_buffer = CVPixelBufferRetain(image_buffer);
> +    new_frame->pts = ff_vda_pts_from_dictionary(user_info);
> +
> +    ff_vda_lock_operation(&vda_ctx->queue_mutex, AV_LOCK_OBTAIN);
> +
> +    queue_walker = vda_ctx->queue;
> +
> +    if (!queue_walker || (new_frame->pts < queue_walker->pts))
> +    {
> +        /* we have an empty queue, or this frame earlier than the current queue head */
> +        new_frame->next_frame = queue_walker;
> +        vda_ctx->queue = new_frame;
> +    }
> +    else
> +    {
> +        /* walk the queue and insert this frame where it belongs in display order */
> +        bool inserted = false;

> +        vda_frame *next_frame = NULL;

the variable is always written before read so the init is unneeded


> +
> +        while (!inserted)
> +        {
> +            next_frame = queue_walker->next_frame;
> +
> +            if (!next_frame || (new_frame->pts < next_frame->pts))
> +            {
> +                new_frame->next_frame = next_frame;
> +                queue_walker->next_frame = new_frame;

> +                inserted = true;

using break; here would avoid the need for the "inserted" variable


[...]
> +vda_frame *ff_vda_queue_pop(struct vda_context *vda_ctx)
> +{
> +    if (!vda_ctx->queue_depth)
> +        return NULL;

this is the only case where queue_depth is read, can it be replaced
by a   vda_ctx->queue == NULL check ?
if so you could remove the queue_depth variable and all code
maintaining its value


[...]
> +static int decode_slice(AVCodecContext *avctx,
> +                        const uint8_t *buffer,
> +                        uint32_t size)
> +{
> +    H264Context *h = avctx->priv_data;
> +    struct vda_context *vda_ctx = avctx->hwaccel_context;
> +    struct vda_picture_context *pic_ctx = h->s.current_picture_ptr->f.hwaccel_picture_private;
> +    void *tmp;
> +
> +    if (!vda_ctx->decoder)
> +        return -1;
> +

> +    tmp = NULL;
> +    tmp = av_realloc(pic_ctx->bitstream, pic_ctx->bitstream_size+size+4);

the tmp = NULL is unneeded
also please consider using *av_fast_realloc() if its possible in this
case here as its faster (*alloc is actually slow, so never downsizing
and instead reusing buffers is faster)


> +    if (!tmp)
> +        return AVERROR(ENOMEM);
> +
> +    pic_ctx->bitstream = tmp;
> +
> +    AV_WB32(pic_ctx->bitstream+pic_ctx->bitstream_size, size);
> +    memcpy(pic_ctx->bitstream+pic_ctx->bitstream_size+4, buffer, size);
> +
> +    pic_ctx->bitstream_size += size + 4;
> +
> +    return 0;
> +}
> +
> +static int end_frame(AVCodecContext *avctx)
> +{
> +    H264Context *h = avctx->priv_data;
> +    struct vda_context *vda_ctx = avctx->hwaccel_context;
> +    struct vda_picture_context *pic_ctx = h->s.current_picture_ptr->f.hwaccel_picture_private;
> +    AVFrame *frame = (AVFrame*)h->s.current_picture_ptr;
> +    int status;
> +
> +    if (!vda_ctx->decoder || !pic_ctx->bitstream)
> +        return -1;
> +
> +    status = ff_vda_decoder_decode(vda_ctx, pic_ctx->bitstream,
> +                                   pic_ctx->bitstream_size,
> +                                   frame->reordered_opaque);
> +
> +    if (status)
> +        av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
> +
> +    av_free(pic_ctx->bitstream);

av_freep() is safer (against double free bugs for example)


[...]

also, if you want to maintain this code in ffmpeg,
please add yourself to the MAINTAINERS file


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111101/c24c15e8/attachment.asc>