[FFmpeg-devel] [PATCH] hwaccel: add VideoToolbox support.

[Sebastien Zwickert]

Hello,

On Sep 10, 2012, at 2:49 AM, Michael Niedermayer <michaelni at gmx.at> wrote:

> Hi Sebastien
> 
> On Sun, Sep 09, 2012 at 11:28:19AM +0200, Sebastien Zwickert wrote:
> [...]
> 
>> diff --git a/configure b/configure
>> index 7fac4e9..ff49eec 100755
>> --- a/configure
>> +++ b/configure
>> @@ -133,6 +133,7 @@ Component options:
>>   --enable-dxva2           enable DXVA2 code
>>   --enable-vaapi           enable VAAPI code [autodetect]
>>   --enable-vda             enable VDA code   [autodetect]
>> +  --enable-vt              enable VideoToolbox code [autodetect]
>>   --enable-vdpau           enable VDPAU code [autodetect]
>> 
>> Individual component options:
>> @@ -1171,6 +1172,7 @@ CONFIG_LIST="
>>     thumb
>>     vaapi
>>     vda
>> +    vt
>>     vdpau
>>     version3
>>     xmm_clobber_test
> 
> nit++ alphabetical order (also at various other places)

Fixed.

> [...]
> 
>> @@ -3532,6 +3539,11 @@ if ! disabled vda; then
>>     fi
>> fi
>> 
>> +# check for VideoToolbox header
>> +if ! disabled vda && check_header VideoToolbox/VideoToolbox.h; then
> 
> vda is wrong here i suspect

Ooops. Fixed!

> [...]
>> new file mode 100644
>> index 0000000..d8b9e0f
>> --- /dev/null
>> +++ b/libavcodec/vt.c
>> @@ -0,0 +1,388 @@
>> +/*
>> + * VideoToolbox hardware acceleration
>> + *
>> + * copyright (c) 2012 Sebastien Zwickert
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include "libavutil/avutil.h"
>> +#include "vt_internal.h"
>> +
> 
>> +static int vt_write_mp4_descr_length(PutBitContext *pb, int length, int compact)
> 
> compact is 0 on all calls, is it intended to have both variants
> implemented but just one used ?
> 
> 
>> +{
>> +    int i;
>> +    uint8_t b;
>> +    int nb;
>> +
>> +    if (compact) {
>> +        if (length <= 0x7F)
>> +            nb = 1;
>> +        else if (length <= 0x3FFF)
>> +             nb = 2;
>> +        else if (length <= 0x1FFFFF)
>> +            nb = 3;
>> +        else
>> +            nb = 4;
>> +    }
>> +    else
>> +        nb = 4;
>> +
>> +    for (i = nb-1; i >= 0; i--) {
>> +        b = (length >> (i * 7)) & 0x7F;
>> +        if (i != 0) {
>> +            b |= 0x80;
>> +        }
>> +        put_bits(pb, 8, b);
>> +    }
>> +
>> +    return nb;
>> +}
>> +
>> +static CFDataRef vt_esds_extradata_create(uint8_t *extradata, int size)
>> +{
>> +    CFDataRef data;
>> +    uint8_t *rw_extradata;
>> +    PutBitContext pb;
>> +    int full_size = 3 + (5 + (13 + (5 + size))) + 3;
>> +    int config_size = 13 + 5 + size;
>> +    int padding = 12;
>> +    int s, i = 0;
>> +
>> +    if (!(rw_extradata = av_mallocz(sizeof(uint8_t)*(full_size+padding))))
>> +        return NULL;
>> +
>> +    init_put_bits(&pb, rw_extradata, full_size+padding);
>> +    put_bits(&pb, 8, 0);        ///< version
>> +    put_bits(&pb, 24, 0);       ///< flags
>> +
>> +    // elementary stream descriptor
>> +    put_bits(&pb, 8, 0x03);     ///< ES_DescrTag
>> +    vt_write_mp4_descr_length(&pb, full_size, 0);
>> +    put_bits(&pb, 16, 0);       ///< esid
>> +    put_bits(&pb, 8, 0);        ///< stream priority (0-32)
>> +
>> +    // decoder configuration descriptor
>> +    put_bits(&pb, 8, 0x04);     ///< DecoderConfigDescrTag
>> +    vt_write_mp4_descr_length(&pb, config_size, 0);
>> +    put_bits(&pb, 8, 32);       ///< object type indication. 32 = CODEC_ID_MPEG4
>> +    put_bits(&pb, 8, 0x11);     ///< stream type
>> +    put_bits(&pb, 24, 0);       ///< buffer size
>> +    put_bits32(&pb, 0);         ///< max bitrate
>> +    put_bits32(&pb, 0);         ///< avg bitrate
>> +
>> +    // decoder specific descriptor
>> +    put_bits(&pb, 8, 0x05);     ///< DecSpecificInfoTag
>> +    vt_write_mp4_descr_length(&pb, size, 0);
>> +    for (i = 0; i < size; i++)
>> +        put_bits(&pb, 8, extradata[i]);
>> +
>> +    // SLConfigDescriptor
>> +    put_bits(&pb, 8, 0x06);     ///< SLConfigDescrTag
>> +    put_bits(&pb, 8, 0x01);     ///< length
>> +    put_bits(&pb, 8, 0x02);     ///<
>> +
>> +    flush_put_bits(&pb);
>> +    s = put_bits_count(&pb) / 8;
> 
> all the written values are in multiples of 8 bit, the code should be
> slightly simpler if it uses the bytestream API for this
> 
> also some of this smells like duplicate relative to the mp4 code
> but i suspect factorizing this may be quite hard so this is more a
> note than a suggestion to change it unless you see an easy way to
> factorize it.

Thanks to you for the note. I did success in factorizing this. I only have to export publicly
the esds writing function for that. Maybe be should I split this part in a dedicated patch ?

> [...]
>> +int ff_vt_buffer_copy(struct vt_context *vt_ctx, const uint8_t *buffer, uint32_t size)
>> +{
>> +    void *tmp;
>> +    tmp = av_fast_realloc(vt_ctx->priv_bitstream,
>> +                          &vt_ctx->priv_allocated_size,
>> +                          size);
>> +    if (!tmp)
>> +        return AVERROR(ENOMEM);
>> +
>> +    vt_ctx->priv_bitstream = tmp;
>> +
>> +    memcpy(vt_ctx->priv_bitstream, buffer, size);
> 
> if the buffers contents are completely overwritten then you dont
> need realloc. av_fast_malloc() should be faster
> there also can be alignment sub optimalities with realloc

Done.
Thanks to you for explanations.

>> +
>> +    vt_ctx->priv_bitstream_size = size;
>> +
>> +    return 0;
>> +}
>> +
>> +void ff_vt_end_frame(MpegEncContext *s)
>> +{
>> +    struct vt_context * const vt_ctx = s->avctx->hwaccel_context;
>> +    AVFrame *frame = &s->current_picture_ptr->f;
>> +
>> +    frame->data[3] = (void*)vt_ctx->cv_buffer;
>> +}
>> +
>> +int ff_vt_session_create(struct vt_context *vt_ctx,
>> +                         CMVideoFormatDescriptionRef fmt_desc,
>> +                         CFDictionaryRef decoder_spec,
>> +						 CFDictionaryRef buf_attr)
> 
> tabs arent allowed in ffmpeg git 

Tabs removed.

> [...]
>> +void ff_vt_session_invalidate(struct vt_context *vt_ctx)
>> +{
>> +    if (vt_ctx->cm_fmt_desc)
>> +        CFRelease(vt_ctx->cm_fmt_desc);
>> +
>> +    if (vt_ctx->session)
>> +        VTDecompressionSessionInvalidate(vt_ctx->session);
>> +
> 
>> +    av_freep(&vt_ctx->priv_bitstream);
> 
> i would suggest to pair all freeing of priv_bitstream, with setting
> its length to 0. It might be redundant but its more robust from a
> security standpoint

Thx. Done.

> [...]
>> […]
>> +
>> +static int end_frame(AVCodecContext *avctx)
>> +{
>> +    struct vt_context *vt_ctx = avctx->hwaccel_context;
>> +    int status;
>> +
>> +    if (!vt_ctx->session || !vt_ctx->priv_bitstream)
>> +        return -1;
>> +
>> +    status = ff_vt_session_decode_frame(vt_ctx);
>> +
>> +    if (status) {
>> +        av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
>> +        return status;
>> +    }
>> +
>> +    ff_vt_end_frame(avctx->priv_data);
>> +
>> +    return status
> 
> duplicates
> These can be moved to a common file and used from there

Done.

> [...]
> 
> thanks!

Thanks to you for reviewing this patch. I'll send an updated patch where I fixed some comments syntax,
code style, typo, cosmetics and I give a name more readable in the configure (videotoolbox instead of vt).

Best regards,

--
Sebastien Zwickert