[FFmpeg-devel] [PATCH] added support for hardware assist H264 video encoding for the Raspberry Pi

Mark Thompson sw at jkqxz.net
Wed Apr 6 01:03:28 CEST 2016


On 01/04/16 03:27, Amancio Hasty wrote:
>  LICENSE.md             |  36 +++++
>  MAINTAINERS            |   1 +
>  configure              |  11 ++
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   2 +
>  libavcodec/vc264.c     | 389 +++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 440 insertions(+)
>  create mode 100644 libavcodec/vc264.c
>
> diff --git a/LICENSE.md b/LICENSE.md
> index a70f486..363e4e4 100644
> --- a/LICENSE.md
> +++ b/LICENSE.md
> @@ -124,3 +124,39 @@ license, requires a proprietary binary blob at run time, and is deemed to be
>  incompatible with the GPL. We are not certain if it is compatible with the
>  LGPL, but we require `--enable-nonfree` even with LGPL configurations in case
>  it is not.
> +
> +### libilclient and vc264.c hardware assist h264 encoder
> +If libilclient is enabled and vc264.c compiled , then this copyright
> +notice must accompany any binary which is compiled against
> +the library libavcodec. Additionally, this copyright notice can be viewed
> +against such  binary by using the unix shell commands :
> +strings ffmpeg_binary | grep -i copyright
> +
> +--
> +Copyright (c) 2012, Broadcom Europe Ltd
> +Copyright (c) 2012, Kalle Vahlman <zuh at iki>
> +                    Tuomas Kulve <tuomas at kulve.fi>
> +All rights reserved.
> +
> +Redistribution and use in source and binary forms, with or without
> +modification, are permitted provided that the following conditions are met:
> +* Redistributions of source code must retain the above copyright
> +      notice, this list of conditions and the following disclaimer.
> +      * Redistributions in binary form must reproduce the above copyright
> +      notice, this list of conditions and the following disclaimer in the
> +      documentation and/or other materials provided with the distribution.
> +      * Neither the name of the copyright holder nor the
> +      names of its contributors may be used to endorse or promote products
> +      derived from this software without specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY
> +DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +--
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a993a67..71a321c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -278,6 +278,7 @@ Codecs:
>    vble.c                                Derek Buitenhuis
>    vc1*                                  Kostya Shishkov, Christophe Gisquet
>    vc2*                                  Rostislav Pehlivanov
> +  vc264.c                               Amancio Hasty
>    vcr1.c                                Michael Niedermayer
>    vda_h264_dec.c                        Xidorn Quan
>    videotoolboxenc.c                     Rick Kern
> diff --git a/configure b/configure
> index e8c4a7b..cd6563a 100755
> --- a/configure
> +++ b/configure
> @@ -277,6 +277,7 @@ External library support:
>    --disable-lzma           disable lzma [autodetect]
>    --enable-decklink        enable Blackmagic DeckLink I/O support [no]
>    --enable-mediacodec      enable Android MediaCodec support [no]
> +  --enable-libilclient     enable h264 encoding via ilclient [no]
>    --enable-mmal            enable decoding via MMAL [no]
>    --enable-netcdf          enable NetCDF, needed for sofalizer filter [no]
>    --enable-nvenc           enable NVIDIA NVENC support [no]
> @@ -1477,6 +1478,7 @@ EXTERNAL_LIBRARY_LIST="
>      libgsm
>      libiec61883
>      libilbc
> +    libilclient
>      libkvazaar
>      libmfx
>      libmodplug
> @@ -2533,6 +2535,9 @@ h264_d3d11va_hwaccel_deps="d3d11va"
>  h264_d3d11va_hwaccel_select="h264_decoder"
>  h264_dxva2_hwaccel_deps="dxva2"
>  h264_dxva2_hwaccel_select="h264_decoder"
> +h264_libilclient_encoder_deps="libilclient"
> +h264_libilclient_encoder_select="libilclient"
> +h264_libilclient_hwaccel_deps="libilclient"

I think you're a little confused about what a hwaccel is here - a hwaccel is a decode acceleration tool, not anything to do with encode.  Remove all the references to hwaccels, they aren't relevant at all to the patch.

>  h264_mediacodec_decoder_deps="mediacodec"
>  h264_mediacodec_decoder_select="h264_mp4toannexb_bsf h264_parser"
>  h264_mmal_decoder_deps="mmal"
> @@ -5548,6 +5553,12 @@ enabled libgsm            && { for gsm_hdr in "gsm.h" "gsm/gsm.h"; do
>                                     check_lib "${gsm_hdr}" gsm_create -lgsm && break;
>                                 done || die "ERROR: libgsm not found"; }
>  enabled libilbc           && require libilbc ilbc.h WebRtcIlbcfix_InitDecode -lilbc
> +enabled libilclient       && { check_lib ilclient.h ilclient_init -L/opt/vc/lib -lilclient  -lGLESv2 -lEGL -lopenmaxil -lbcm_host -lvcos -lvchiq_arm -lrt ||
> +                                { ! enabled cross_compile && {
> +                                    add_cflags -isystem/opt/vc/include/ -isystem/opt/vc/include/interface/vmcs_host/linux -isystem/opt/vc/include/interface/vcos/pthreads -isystem/opt/vc/include/interface/vmcs_host/linux -isystem/opt/vc/src/hello_pi/libs/ilclient ;
> +                                    add_extralibs -L/opt/vc/lib/ -lilclient  -lGLESv2 -lEGL -lopenmaxil -lbcm_host -lvcos -lvchiq_arm -lrt -ldl ; }
> +                                check_lib ilclient.h ilclient_init ; } ||
> +                               die "ERROR: libilclient not found"; }
>  enabled libkvazaar        && require_pkg_config "kvazaar >= 0.8.1" kvazaar.h kvz_api_get
>  enabled libmfx            && require_pkg_config libmfx "mfx/mfxvideo.h" MFXInit
>  enabled libmodplug        && require_pkg_config libmodplug libmodplug/modplug.h ModPlug_Load

As I understand it, an OpenMAX IL client is any program which uses OpenMAX IL. Here, this is a library which calls itself "ilclient" which is specific to the Broadcom hardware, containing interfaces which we would not expect to find on another OpenMAX IL client instance, right? The patch then uses a combination of standard OpenMAX IL calls and calls into this nonstandard client library.

So, is this library actually required, or could it be eliminated in favour of only using standard OpenMAX IL functions, which might then allow it to be transferrable to another OpenMAX IL implementation?

Alternatively, if the additional nonstandard layer is required, then can these options perhaps be given a more descriptive name to reduce the possibility of confusion?  (Something like "bcm_vc_omxil_h264" would contain all the information, though I admit is rather clumsy.)


> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 8c14268..6ebe37a 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -835,6 +835,7 @@ OBJS-$(CONFIG_LIBMP3LAME_ENCODER)         += libmp3lame.o mpegaudiodecheader.o
>  OBJS-$(CONFIG_LIBOPENCORE_AMRNB_DECODER)  += libopencore-amr.o
>  OBJS-$(CONFIG_LIBOPENCORE_AMRNB_ENCODER)  += libopencore-amr.o
>  OBJS-$(CONFIG_LIBOPENCORE_AMRWB_DECODER)  += libopencore-amr.o
> +OBJS-$(CONFIG_H264_LIBILCLIENT_ENCODER)   += vc264.o
>  OBJS-$(CONFIG_LIBOPENH264_ENCODER)        += libopenh264enc.o
>  OBJS-$(CONFIG_LIBOPENJPEG_DECODER)        += libopenjpegdec.o
>  OBJS-$(CONFIG_LIBOPENJPEG_ENCODER)        += libopenjpegenc.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index f498041..377ba86 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -74,6 +74,7 @@ void avcodec_register_all(void)
>      initialized = 1;
>
>      /* hardware accelerators */
> +    REGISTER_HWACCEL(H264_LIBILCLIENT,  h264_libilclient);

Remove this.

>      REGISTER_HWACCEL(H263_VAAPI,        h263_vaapi);
>      REGISTER_HWACCEL(H263_VIDEOTOOLBOX, h263_videotoolbox);
>      REGISTER_HWACCEL(H264_D3D11VA,      h264_d3d11va);
> @@ -196,6 +197,7 @@ void avcodec_register_all(void)
>      REGISTER_ENCDEC (H263P,             h263p);
>      REGISTER_DECODER(H264,              h264);
>      REGISTER_DECODER(H264_CRYSTALHD,    h264_crystalhd);
> +    REGISTER_ENCODER(H264_LIBILCLIENT,  h264_libilclient);
>      REGISTER_DECODER(H264_MEDIACODEC,   h264_mediacodec);
>      REGISTER_DECODER(H264_MMAL,         h264_mmal);
>      REGISTER_DECODER(H264_QSV,          h264_qsv);
> diff --git a/libavcodec/vc264.c b/libavcodec/vc264.c
> new file mode 100644
> index 0000000..f013247
> --- /dev/null
> +++ b/libavcodec/vc264.c
> @@ -0,0 +1,389 @@
> +/*  H.264 hardware assist video encoding code taken from
> + * raspberry's os :
> + *   /opt/vc/src/hello_pi/hello_encode/encode.c
> + */
> +
> +volatile char vc264_bsd_license[]={
> +"/*"
> +"Copyright (c) 2012, Broadcom Europe Ltd"
> +"Copyright (c) 2012, Kalle Vahlman <zuh at iki>"
> +"                    Tuomas Kulve <tuomas at kulve.fi>"
> +"All rights reserved."
> +""
> +"Redistribution and use in source and binary forms, with or without"
> +"modification, are permitted provided that the following conditions are met:"
> +"* Redistributions of source code must retain the above copyright"
> +"      notice, this list of conditions and the following disclaimer."
> +"      * Redistributions in binary form must reproduce the above copyright"
> +"      notice, this list of conditions and the following disclaimer in the"
> +"      documentation and/or other materials provided with the distribution."
> +"      * Neither the name of the copyright holder nor the"
> +"      names of its contributors may be used to endorse or promote products"
> +"      derived from this software without specific prior written permission."
> +""
> +"THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS \"AS IS\" AND"
> +"ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED"
> +"WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE"
> +"DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY"
> +"DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES"
> +"(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;"
> +"LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND"
> +"ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT"
> +"(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS"
> +"SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE."
> +"*/"};
> +
> +/*
> + * ffmpeg driver for hardware assist video H.264 encoding using Broadcom's GPU
> + * Copyright (C) 2016 Amancio Hasty ahasty at gmail.com
> + *
> + *
> + * 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
> + *
> + */
> +
> +
> +/**
> + * @ file vc264.c
> + * Broadcom bm2865's Visual Core hardware assist h264 using
> +   openMax interface to the GPU.
> +
> +*/
> +
> +#include <stdio.h>
> +#include <stdlib.h>

I don't think these headers are used.

> +#include <string.h>
> +#define OMX_SKIP64BIT
> +#include "bcm_host.h"
> +#include "ilclient.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +typedef struct VC264Context {
> +    OMX_VIDEO_PARAM_PORTFORMATTYPE format;
> +    OMX_PARAM_PORTDEFINITIONTYPE def;
> +    COMPONENT_T *video_encode;
> +    COMPONENT_T *list[5];

Only list[0] is used?

> +    OMX_BUFFERHEADERTYPE *buf;
> +    OMX_BUFFERHEADERTYPE *out;
> +    ILCLIENT_T *client;
> +    OMX_VIDEO_PARAM_BITRATETYPE bitrateType;
> +    int width;
> +    int height;
> +    int bit_rate;
> +} VC264Context;
> +
> +
> +static int vc264_init(AVCodecContext * avctx)
> +{
> +
> +
> +
> +    OMX_ERRORTYPE r;
> +    int error;
> +
> +
> +
> +    VC264Context *vc = avctx->priv_data;
> +
> +    memset(&vc->list, 0, sizeof(vc->list));
> +    vc->width = avctx->width;
> +    vc->height = avctx->height;
> +    vc->bit_rate = avctx->bit_rate;
> +
> +#if FF_API_CODED_FRAME
> +    FF_DISABLE_DEPRECATION_WARNINGS avctx->coded_frame = av_frame_alloc();
> +    avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
> +    FF_ENABLE_DEPRECATION_WARNINGS
> +#endif

Where is this used?

> +
> +    bcm_host_init();

Can this fail?

> +    if ((vc->client = ilclient_init()) == NULL) {
> +        return AVERROR(ENOSYS);
> +    }
> +    error = OMX_Init();
> +
> +    if (error != OMX_ErrorNone) {
> +        ilclient_destroy(vc->client);
> +        av_log(avctx, AV_LOG_ERROR, "in vc264_init OMX_Init failed ");
> +        return AVERROR(ENOSYS);
> +    }
> +    // create video_encode
> +    r = ilclient_create_component(vc->client, &vc->video_encode,
> +                                  (char *) "video_encode",
> +                                  ILCLIENT_DISABLE_ALL_PORTS |
> +                                  ILCLIENT_ENABLE_INPUT_BUFFERS |
> +                                  ILCLIENT_ENABLE_OUTPUT_BUFFERS);
> +
> +    if (r != 0) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "ilclient_create_component() for video_encode failed with %x!",
> +               r);
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    vc->list[0] = vc->video_encode;
> +
> +    // get current settings of video_encode component from port 200
> +    memset(&vc->def, 0, sizeof(OMX_PARAM_PORTDEFINITIONTYPE));
> +    vc->def.nSize = sizeof(OMX_PARAM_PORTDEFINITIONTYPE);
> +    vc->def.nVersion.nVersion = OMX_VERSION;
> +    vc->def.nPortIndex = 200;

Where did 200 come from?  Is that a constant we could find in a header file rather than pulling it out of nowhere?

> +
> +
> +    if (OMX_GetParameter
> +        (ILC_GET_HANDLE(vc->video_encode), OMX_IndexParamPortDefinition,
> +         &vc->def) != OMX_ErrorNone) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: OMX_GetParameter() for video_encode port 200 failed!",
> +               __FUNCTION__, __LINE__);

This __FUNCTION__, __LINE__ thing seems somewhat redundant.  It would be more useful to log the actual error code rather than just saying it failed.

(Also, __FUNCTION__ is not standard.  Use __func__ if you really want that in the output.)

> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    // Port 200: in 1/1 115200 16 enabled,not pop.,not cont. 320x240 320x240 @1966080 20
> +    vc->def.format.video.nFrameWidth = vc->width;
> +    vc->def.format.video.nFrameHeight = vc->height;
> +    vc->def.format.video.xFramerate = avctx->time_base.den << 16;
> +    vc->def.format.video.nSliceHeight = vc->def.format.video.nFrameHeight;
> +    vc->def.format.video.nStride = vc->def.format.video.nFrameWidth;

These two values are what, the maximum allowed values, which we then must be constrained to fit inside?  If so, you should probably make sure here that width/height do actually fit, because it will do something nasty later if they don't.

> +    vc->def.format.video.eColorFormat = OMX_COLOR_FormatYUV420PackedPlanar;
> +
> +
> +    r = OMX_SetParameter(ILC_GET_HANDLE(vc->video_encode),
> +                         OMX_IndexParamPortDefinition, &vc->def);
> +    if (r != OMX_ErrorNone) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: OMX_SetParameter() for video_encode port 200 failed with %x!",
> +               __FUNCTION__, __LINE__, r);
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    memset(&vc->format, 0, sizeof(OMX_VIDEO_PARAM_PORTFORMATTYPE));
> +    vc->format.nSize = sizeof(OMX_VIDEO_PARAM_PORTFORMATTYPE);
> +    vc->format.nVersion.nVersion = OMX_VERSION;
> +    vc->format.nPortIndex = 201;
> +    vc->format.eCompressionFormat = OMX_VIDEO_CodingAVC;
> +
> +    r = OMX_SetParameter(ILC_GET_HANDLE(vc->video_encode),
> +                         OMX_IndexParamVideoPortFormat, &vc->format);
> +    if (r != OMX_ErrorNone) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: OMX_SetParameter() for video_encode port 201 failed with %x!",
> +               __FUNCTION__, __LINE__, r);
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +
> +    memset(&vc->bitrateType, 0, sizeof(OMX_VIDEO_PARAM_BITRATETYPE));
> +    vc->bitrateType.nSize = sizeof(OMX_VIDEO_PARAM_BITRATETYPE);
> +    vc->bitrateType.nVersion.nVersion = OMX_VERSION;
> +    vc->bitrateType.eControlRate = OMX_Video_ControlRateVariable;
> +
> +
> +    vc->bitrateType.nTargetBitrate = avctx->bit_rate;
> +    vc->bitrateType.nPortIndex = 201;
> +    r = OMX_SetParameter(ILC_GET_HANDLE(vc->video_encode),
> +                         OMX_IndexParamVideoBitrate, &vc->bitrateType);
> +    if (r != OMX_ErrorNone) {
> +
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: OMX_SetParameter() for bitrate for video_encode port 201 failed with %x!",
> +               __FUNCTION__, __LINE__, r);
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    // get current bitrate
> +    memset(&vc->bitrateType, 0, sizeof(OMX_VIDEO_PARAM_BITRATETYPE));
> +    vc->bitrateType.nSize = sizeof(OMX_VIDEO_PARAM_BITRATETYPE);
> +    vc->bitrateType.nVersion.nVersion = OMX_VERSION;
> +    vc->bitrateType.nPortIndex = 201;
> +
> +    if (OMX_GetParameter
> +        (ILC_GET_HANDLE(vc->video_encode), OMX_IndexParamVideoBitrate,
> +         &vc->bitrateType) != OMX_ErrorNone) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: OMX_GetParameter() for video_encode for bitrate port 201 failed!",
> +               __FUNCTION__, __LINE__);
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    if (ilclient_change_component_state(vc->video_encode, OMX_StateIdle) ==
> +        -1) {
> +
> +        av_log(avctx, AV_LOG_ERROR,
> +               "%s:%d: ilclient_change_component_state(vc->video_encode, OMX_StateIdle) failed",
> +               __FUNCTION__, __LINE__);
> +    }
> +
> +
> +    if (ilclient_enable_port_buffers
> +        (vc->video_encode, 200, NULL, NULL, NULL) != 0) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "enabling port buffers for 200 failed!\n");
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +    if (ilclient_enable_port_buffers
> +        (vc->video_encode, 201, NULL, NULL, NULL) != 0) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "enabling port buffers for 201 failed!\n");
> +        return AVERROR_UNKNOWN;
> +    }
> +
> +
> +    ilclient_change_component_state(vc->video_encode, OMX_StateExecuting);

Always succeeds?

> +
> +    return 0;
> +}

All of the error returns feel dubious - see question below under vc264_close().


> +static int vc264_encode(AVCodecContext * avctx, AVPacket * avpkt,
> +                        const AVFrame * frame, int *got_packet_ptr)
> +{
> +
> +    VC264Context *vc = avctx->priv_data;
> +    OMX_ERRORTYPE r;
> +    OMX_BUFFERHEADERTYPE *buf;
> +    OMX_BUFFERHEADERTYPE *out;
> +    char *dst;
> +    char *in_buf;
> +    int ret;
> +    int height16;
> +    int b_size;
> +
> +    height16 = vc->height + 15 & ~15;
> +    b_size = (vc->width * height16 * 3) / 2;
> +
> +
> +    buf = ilclient_get_input_buffer(vc->video_encode, 200, 1);
> +    if (buf == NULL) {
> +        av_log(avctx, AV_LOG_ERROR, " vc264_encode: ran out gpu buffers");

And then return success anyway?

> +    } else {
> +    /*********** fill buffer ************/
> +
> +        if (frame->opaque == NULL) {
> +            in_buf = frame->data[0];
> +        } else {
> +            in_buf = frame->opaque;
> +        }
> +
> +        memcpy(buf->pBuffer, in_buf, b_size);
> +        buf->nFilledLen = b_size;

This makes a lot of assumptions about the layout of the input frame, such as that the height is a multiple of 16, the pitches match exactly what the hardware is expecting, and the chroma planes immediately follow the luma plane (and are not line-interleaved).

You can avoid these by copying a bit more carefully, say with suitable calls to av_image_copy_plane().

> +
> +
> +        if (OMX_EmptyThisBuffer(ILC_GET_HANDLE(vc->video_encode), buf) !=
> +            OMX_ErrorNone) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                   "vc264_encode: Error emptying buffer!\n");

And continue anyway when an error happens?  Will that do anything sensible?

> +        }
> +
> +        out = ilclient_get_output_buffer(vc->video_encode, 201, 1);

Can't fail?

> +
> +        r = OMX_FillThisBuffer(ILC_GET_HANDLE(vc->video_encode), out);
> +        if (r != OMX_ErrorNone) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                   "vc264_encode: Error filling buffer: %x\n", r);
> +        }
> +
> +
> +        if ((ret = ff_alloc_packet2(avctx, avpkt, out->nFilledLen, 0)))
> +            return ret;
> +        dst = avpkt->data;
> +        *got_packet_ptr = 1;

You want to set the timestamps on this output packet.  Since this is all in-line, I assume you have no B-frames or other funny ordering?  If that is true you should be able to just copy the pts and dts from the input frame pts.

It would also be nice to set whether it is a key frame or not (flags |= AV_PKT_FLAG_KEY), so that the output file is seekable.  If you have no way to get this information from the codec itself, you could try to look at the NAL unit types in the output.

> +
> +        /* out->pBuffer has buffer of size out->nFilledLen */
> +
> +        memcpy(dst, out->pBuffer, out->nFilledLen);
> +        out->nFilledLen = 0;

How does the output buffer get freed here - does it disappear automatically somehow, or is it actually reused repeatedly?  A comment would be nice, because the code makes it look like it leaks.

> +        return 0;
> +    }
> +    return 0;
> +}
> +
> +static int vc264_close(AVCodecContext * avctx)
> +{
> +    VC264Context *vc = avctx->priv_data;
> +    ilclient_disable_port_buffers(vc->video_encode, 200, NULL, NULL, NULL);
> +    ilclient_disable_port_buffers(vc->video_encode, 201, NULL, NULL, NULL);
> +
> +    ilclient_state_transition(vc->list, OMX_StateIdle);
> +    ilclient_state_transition(vc->list, OMX_StateLoaded);
> +
> +    ilclient_cleanup_components(vc->list);
> +
> +    OMX_Deinit();
> +
> +    ilclient_destroy(vc->client);

Do all of these still work if called after vc264_open() failed?

> +
> +#if FF_API_CODED_FRAME
> +    FF_DISABLE_DEPRECATION_WARNINGS av_frame_free(&avctx->coded_frame);
> +    FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +        return 0;
> +}
> +
> +AVHWAccel ff_h264_libilclient_hwaccel = {
> +    .name = "h264_libilclient",
> +    .type = AVMEDIA_TYPE_VIDEO,
> +    .id = AV_CODEC_ID_H264,
> +    .pix_fmt = AV_PIX_FMT_YUV420P
> +};

Remove this.

> +
> +static const AVCodecDefault x264_defaults[] = {
> +    {"b", "0"},
> +    {"bf", "-1"},
> +    {"flags2", "0"},
> +    {"g", "-1"},
> +    {"i_qfactor", "-1"},
> +    {"b_qfactor", "-1"},
> +    {"qmin", "-1"},
> +    {"qmax", "-1"},
> +    {"qdiff", "-1"},
> +    {"qblur", "-1"},
> +    {"qcomp", "-1"},
> +//     { "rc_lookahead",     "-1" },
> +    {"refs", "-1"},
> +    {"sc_threshold", "-1"},
> +    {"trellis", "-1"},
> +    {"nr", "-1"},
> +    {"me_range", "-1"},
> +    {"me_method", "-1"},
> +    {"subq", "-1"},
> +    {"b_strategy", "-1"},
> +    {"keyint_min", "-1"},
> +    {"coder", "-1"},
> +    {"cmp", "-1"},
> +    {NULL},
> +};

As far as I can tell, "b"/bit_rate is the only parameter which is actually used here?  What are the others doing?  Also, probably shouldn't be called "x264"...

Perhaps some documentation would help.

> +
> +AVCodec ff_h264_libilclient_encoder = {
> +    .name = "h264_libilclient",
> +    .long_name =
> +        NULL_IF_CONFIG_SMALL
> +        ("Video Core H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"),
> +    .type = AVMEDIA_TYPE_VIDEO,
> +    .id = AV_CODEC_ID_H264,
> +    .priv_data_size = sizeof(VC264Context),
> +    .init = vc264_init,
> +    .encode2 = vc264_encode,
> +    .close = vc264_close,
> +    .capabilities = 0,
> +    .defaults = x264_defaults,
> +    .pix_fmts = (const enum AVPixelFormat[]) {AV_PIX_FMT_YUV420P,
> +                                              AV_PIX_FMT_NONE},
> +
> +};
> --
> 2.1.4
>



More information about the ffmpeg-devel mailing list