[FFmpeg-devel,v3] Added Turing codec interface for ffmpeg

- This patch contains the changes to interface the Turing codec
  (http://turingcodec.org/) to ffmpeg. The patch was modified to address
  the comments in the review as follows:
  - Added a pkg-config file to list all dependencies required by
  libturing. This should address the issue pointed out by Hendrik
  Leppkes on Fri 18/11/2016
  - As per suggestions of wm4, two functions (add_option and
    finalise_options) have been created. The former appends new options
    while the latter sets up the argv array of pointers to char*
    accordingly. add_option re-allocats the buffer for options using
    realloc.
---
 LICENSE.md             |   1 +
 configure              |   5 +
 libavcodec/Makefile    |   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/libturing.c | 279 +++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 287 insertions(+)
 create mode 100644 libavcodec/libturing.c

On Thu,  1 Dec 2016 12:04:38 +0000
Matteo Naccari <matteo.naccari@bbc.co.uk> wrote:

> - This patch contains the changes to interface the Turing codec
>   (http://turingcodec.org/) to ffmpeg. The patch was modified to address
>   the comments in the review as follows:
>   - Added a pkg-config file to list all dependencies required by
>   libturing. This should address the issue pointed out by Hendrik
>   Leppkes on Fri 18/11/2016
>   - As per suggestions of wm4, two functions (add_option and
>     finalise_options) have been created. The former appends new options
>     while the latter sets up the argv array of pointers to char*
>     accordingly. add_option re-allocats the buffer for options using
>     realloc.
> ---
>  LICENSE.md             |   1 +
>  configure              |   5 +
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   1 +
>  libavcodec/libturing.c | 279 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 287 insertions(+)
>  create mode 100644 libavcodec/libturing.c
> 
> diff --git a/LICENSE.md b/LICENSE.md
> index 640633c..86e5371 100644
> --- a/LICENSE.md
> +++ b/LICENSE.md
> @@ -85,6 +85,7 @@ The following libraries are under GPL:
>  - frei0r
>  - libcdio
>  - librubberband
> +- libturing
>  - libvidstab
>  - libx264
>  - libx265
> diff --git a/configure b/configure
> index 6345fc2..022ffa9 100755
> --- a/configure
> +++ b/configure
> @@ -255,6 +255,7 @@ External library support:
>    --enable-libssh          enable SFTP protocol via libssh [no]
>    --enable-libtesseract    enable Tesseract, needed for ocr filter [no]
>    --enable-libtheora       enable Theora encoding via libtheora [no]
> +  --enable-libturing       enable H.265/HEVC encoding via libturing [no]
>    --enable-libtwolame      enable MP2 encoding via libtwolame [no]
>    --enable-libv4l2         enable libv4l2/v4l-utils [no]
>    --enable-libvidstab      enable video stabilization using vid.stab [no]
> @@ -1534,6 +1535,7 @@ EXTERNAL_LIBRARY_LIST="
>      libssh
>      libtesseract
>      libtheora
> +    libturing
>      libtwolame
>      libv4l2
>      libvidstab
> @@ -2831,6 +2833,7 @@ libspeex_decoder_deps="libspeex"
>  libspeex_encoder_deps="libspeex"
>  libspeex_encoder_select="audio_frame_queue"
>  libtheora_encoder_deps="libtheora"
> +libturing_encoder_deps="libturing"
>  libtwolame_encoder_deps="libtwolame"
>  libvo_amrwbenc_encoder_deps="libvo_amrwbenc"
>  libvorbis_decoder_deps="libvorbis"
> @@ -5096,6 +5099,7 @@ die_license_disabled gpl frei0r
>  die_license_disabled gpl libcdio
>  die_license_disabled gpl librubberband
>  die_license_disabled gpl libsmbclient
> +die_license_disabled gpl libturing
>  die_license_disabled gpl libvidstab
>  die_license_disabled gpl libx264
>  die_license_disabled gpl libx265
> @@ -5754,6 +5758,7 @@ enabled libssh            && require_pkg_config libssh libssh/sftp.h sftp_init
>  enabled libspeex          && require_pkg_config speex speex/speex.h speex_decoder_init -lspeex
>  enabled libtesseract      && require_pkg_config tesseract tesseract/capi.h TessBaseAPICreate
>  enabled libtheora         && require libtheora theora/theoraenc.h th_info_init -ltheoraenc -ltheoradec -logg
> +enabled libturing         && require_pkg_config libturing turing.h turing_version
>  enabled libtwolame        && require libtwolame twolame.h twolame_init -ltwolame &&
>                               { check_lib twolame.h twolame_encode_buffer_float32_interleaved -ltwolame ||
>                                 die "ERROR: libtwolame must be installed and version must be >= 0.3.10"; }
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 82f7fa2..cadefdc 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -880,6 +880,7 @@ OBJS-$(CONFIG_LIBSPEEX_DECODER)           += libspeexdec.o
>  OBJS-$(CONFIG_LIBSPEEX_ENCODER)           += libspeexenc.o
>  OBJS-$(CONFIG_LIBTHEORA_ENCODER)          += libtheoraenc.o
>  OBJS-$(CONFIG_LIBTWOLAME_ENCODER)         += libtwolame.o
> +OBJS-$(CONFIG_LIBTURING_ENCODER)          += libturing.o
>  OBJS-$(CONFIG_LIBVO_AMRWBENC_ENCODER)     += libvo-amrwbenc.o
>  OBJS-$(CONFIG_LIBVORBIS_DECODER)          += libvorbisdec.o
>  OBJS-$(CONFIG_LIBVORBIS_ENCODER)          += libvorbisenc.o \
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index ada9481..0e61a4a 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -610,6 +610,7 @@ void avcodec_register_all(void)
>      REGISTER_ENCDEC (LIBSPEEX,          libspeex);
>      REGISTER_ENCODER(LIBTHEORA,         libtheora);
>      REGISTER_ENCODER(LIBTWOLAME,        libtwolame);
> +    REGISTER_ENCODER(LIBTURING,         libturing);
>      REGISTER_ENCODER(LIBVO_AMRWBENC,    libvo_amrwbenc);
>      REGISTER_ENCDEC (LIBVORBIS,         libvorbis);
>      REGISTER_ENCDEC (LIBVPX_VP8,        libvpx_vp8);
> diff --git a/libavcodec/libturing.c b/libavcodec/libturing.c
> new file mode 100644
> index 0000000..fab17cd
> --- /dev/null
> +++ b/libavcodec/libturing.c
> @@ -0,0 +1,279 @@
> +/*
> + * libturing encoder
> + *
> + * Copyright (c) 2016 Turing Codec contributors
> + *
> + * 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 <turing.h>
> +#include <float.h>
> +#include "libavutil/internal.h"
> +#include "libavutil/common.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixdesc.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +#define MAX_OPTION_LENGTH 256
> +
> +typedef struct libturingEncodeContext {
> +    const AVClass *class;
> +    turing_encoder *encoder;
> +    const char *options;
> +} libturingEncodeContext;
> +
> +typedef struct optionContext {
> +    char** argv;
> +    char* options;
> +    char* s;
> +    int options_buffer_size;
> +    int buffer_filled;
> +    int options_added;
> +} optionContext;
> +
> +static av_cold int libturing_encode_close(AVCodecContext *avctx)
> +{
> +    libturingEncodeContext *ctx = avctx->priv_data;
> +
> +    if (ctx->encoder)
> +        turing_destroy_encoder(ctx->encoder);
> +
> +    return 0;
> +}
> +
> +static void add_option(const char* current_option, optionContext* option_ctx)
> +{
> +    int option_length = strlen(current_option);
> +    if (option_ctx->buffer_filled + option_length + 1 > option_ctx->options_buffer_size) {
> +        option_ctx->options_buffer_size += option_length + 1;
> +        option_ctx->options = realloc(option_ctx->options, option_ctx->options_buffer_size);

Missing error handling, and maybe overflow handling. Also, we don't use
malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why
we do this (it doesn't have much of a justification for realloc at
least), but it's probably better to be consistent.

Does the "s" field have any purpose? It seems to be redundant with the
other fields.

> +        option_ctx->s = option_ctx->options + option_ctx->buffer_filled;
> +    }
> +    strcpy(option_ctx->s, current_option);
> +    option_ctx->s += 1 + option_length;
> +    option_ctx->options_added++;
> +    option_ctx->buffer_filled += option_length + 1;
> +}
> +
> +static void finalise_options(optionContext* option_ctx)
> +{
> +    if (option_ctx->options_added) {
> +        char *p;
> +        option_ctx->argv = malloc(option_ctx->options_added * sizeof(char*));

Same here (av_malloc).

> +        p = option_ctx->options;
> +        for(int option_idx=0; option_idx<option_ctx->options_added; option_idx++) {
> +            option_ctx->argv[option_idx] = p;
> +            p += strlen(p) + 1;
> +        }
> +    }
> +}
> +
> +static av_cold int libturing_encode_init(AVCodecContext *avctx)
> +{
> +    libturingEncodeContext *ctx = avctx->priv_data;
> +
> +    optionContext encoder_options;
> +    turing_encoder_settings settings;
> +    char option_string[MAX_OPTION_LENGTH];
> +    double frame_rate;
> +
> +    encoder_options.buffer_filled = 0;
> +    encoder_options.options_added = 0;
> +    encoder_options.options_buffer_size =  strlen("turing") + 1;
> +    encoder_options.options = malloc(encoder_options.options_buffer_size);
> +    encoder_options.s = encoder_options.options;
> +    frame_rate = (double)avctx->time_base.den / (avctx->time_base.num * avctx->ticks_per_frame);
> +
> +    snprintf(option_string, MAX_OPTION_LENGTH, "turing"); add_option(option_string, &encoder_options);
> +    snprintf(option_string, MAX_OPTION_LENGTH, "--input-res=%dx%d", avctx->width, avctx->height); add_option(option_string, &encoder_options);
> +    snprintf(option_string, MAX_OPTION_LENGTH, "--frame-rate=%f", frame_rate); add_option(option_string, &encoder_options);
> +    snprintf(option_string, MAX_OPTION_LENGTH, "--frames=0"); add_option(option_string, &encoder_options);
> +
> +    {
> +        int const bit_depth = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth;
> +        if (bit_depth != 8 && bit_depth != 10) {
> +            av_log(avctx, AV_LOG_ERROR, "Encoder input must be 8- or 10-bit.\n");
> +            turing_destroy_encoder(ctx->encoder);
> +            return AVERROR_INVALIDDATA;
> +        }

I'm not sure why this us checked. Either setting the turing_csp array
takes care of enforcing this condition, or if it does not, this should
check for supported pixfmts instead. (I didn't check whether
AVCodec.pix_fmts is enforced by the generic code.)

> +        snprintf(option_string, MAX_OPTION_LENGTH, "--bit-depth=%d", bit_depth); add_option(option_string, &encoder_options);
> +        snprintf(option_string, MAX_OPTION_LENGTH, "--internal-bit-depth=%d", bit_depth); add_option(option_string, &encoder_options);
> +    }
> +
> +    if (avctx->sample_aspect_ratio.num > 0 && avctx->sample_aspect_ratio.den > 0) {
> +        int sar_num, sar_den;
> +
> +        av_reduce(&sar_num, &sar_den,
> +            avctx->sample_aspect_ratio.num,
> +            avctx->sample_aspect_ratio.den, 65535);
> +        snprintf(option_string, MAX_OPTION_LENGTH, "--sar=%d:%d", sar_num, sar_den); add_option(option_string, &encoder_options);
> +    }
> +
> +    if (ctx->options) {
> +        AVDictionary *dict = NULL;
> +        AVDictionaryEntry *en = NULL;
> +
> +        if (!av_dict_parse_string(&dict, ctx->options, "=", ":", 0)) {
> +            while ((en = av_dict_get(dict, "", en, AV_DICT_IGNORE_SUFFIX))) {
> +                int const illegal_option =
> +                    !strcmp("input-res", en->key) ||
> +                    !strcmp("frame-rate", en->key) ||
> +                    !strcmp("f", en->key) ||
> +                    !strcmp("frames", en->key) ||
> +                    !strcmp("sar", en->key) ||
> +                    !strcmp("bit-depth", en->key) ||
> +                    !strcmp("internal-bit-depth", en->key);
> +                if (illegal_option)
> +                    av_log(avctx, AV_LOG_WARNING, "%s=%s ignored.\n", en->key, en->value);
> +                else {
> +
> +                    if (turing_check_binary_option(en->key)) {
> +                        snprintf(option_string, MAX_OPTION_LENGTH, "--%s", en->key); add_option(option_string, &encoder_options);
> +                    } else {
> +                        snprintf(option_string, MAX_OPTION_LENGTH, "--%s=%s", en->key, en->value); add_option(option_string, &encoder_options);
> +                    }
> +                }
> +            }
> +            av_dict_free(&dict);
> +        }
> +    }
> +
> +    add_option("dummy-input-filename", &encoder_options);
> +
> +    finalise_options(&encoder_options);
> +
> +    settings.argv = (char const**)encoder_options.argv;
> +    settings.argc = encoder_options.options_added;
> +
> +    for (int i=0; i<settings.argc; ++i)
> +        av_log(avctx, AV_LOG_INFO, "arg %d: %s\n", i, settings.argv[i]);
> +
> +    ctx->encoder = turing_create_encoder(settings);
> +
> +    if (!ctx->encoder) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create libturing encoder.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> +        turing_bitstream const *bitstream;
> +        bitstream = turing_encode_headers(ctx->encoder);
> +        if (bitstream->size <= 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to encode headers.\n");
> +            turing_destroy_encoder(ctx->encoder);
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        avctx->extradata_size = bitstream->size;
> +
> +        avctx->extradata = av_malloc(avctx->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!avctx->extradata) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to allocate HEVC extradata %d bytes\n", avctx->extradata_size);
> +            turing_destroy_encoder(ctx->encoder);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        memcpy(avctx->extradata, bitstream->p, bitstream->size);
> +    }
> +
> +    free(encoder_options.options);
> +    free(encoder_options.argv);
> +
> +    return 0;
> +}
> +
> +static int libturing_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *pic, int *got_packet)
> +{
> +    libturingEncodeContext *ctx = avctx->priv_data;
> +    turing_encoder_output const *output;
> +    int ret = 0;
> +
> +    if (pic) {
> +        turing_picture picture;
> +
> +        picture.image[0].p = pic->data[0];
> +        picture.image[1].p = pic->data[1];
> +        picture.image[2].p = pic->data[2];
> +        picture.image[0].stride = pic->linesize[0];
> +        picture.image[1].stride = pic->linesize[1];
> +        picture.image[2].stride = pic->linesize[2];
> +        picture.pts = pic->pts;
> +        output = turing_encode_picture(ctx->encoder, &picture);
> +    } else {
> +        output = turing_encode_picture(ctx->encoder, 0);
> +    }
> +
> +    if (output->bitstream.size < 0)
> +        return AVERROR_EXTERNAL;
> +
> +    if (output->bitstream.size ==0)
> +        return 0;
> +
> +    ret = ff_alloc_packet2(avctx, pkt, output->bitstream.size, 0);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Error getting output packet.\n");
> +        return ret;
> +    }
> +
> +    memcpy(pkt->data, output->bitstream.p, output->bitstream.size);
> +
> +    pkt->pts = output->pts;
> +    pkt->dts = output->dts;
> +    if (output->keyframe)
> +        pkt->flags |= AV_PKT_FLAG_KEY;
> +
> +    *got_packet = 1;
> +    return 0;
> +}
> +
> +static const enum AVPixelFormat turing_csp[] = {
> +    AV_PIX_FMT_YUV420P10,
> +    AV_PIX_FMT_YUV420P,
> +    AV_PIX_FMT_NONE
> +};
> +
> +static av_cold void libturing_encode_init_csp(AVCodec *codec)
> +{
> +    codec->pix_fmts = turing_csp;
> +}

This seems completely unnecessary. Set pix_fmts directly in the
initializer below? See e.g. libavcodec/h264dec.c for the C99 syntax
that works.

> +
> +static const AVOption options[] = {
> +    { "turing-params", "configure additional turing encoder parameters", offsetof(libturingEncodeContext, options), AV_OPT_TYPE_STRING,{ 0 }, 0, 0, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },
> +    { NULL }
> +};
> +
> +static const AVClass class = {
> +    .class_name = "libturing",
> +    .item_name = av_default_item_name,
> +    .option = options,
> +    .version = LIBAVUTIL_VERSION_INT,
> +};
> +
> +AVCodec ff_libturing_encoder = {
> +    .name = "libturing",
> +    .long_name = NULL_IF_CONFIG_SMALL("libturing HEVC"),
> +    .type = AVMEDIA_TYPE_VIDEO,
> +    .id = AV_CODEC_ID_HEVC,
> +    .init = libturing_encode_init,
> +    .init_static_data = libturing_encode_init_csp,
> +    .encode2 = libturing_encode_frame,
> +    .close = libturing_encode_close,
> +    .priv_data_size = sizeof(libturingEncodeContext),
> +    .priv_class = &class,
> +    .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
> +};

On 12/6/2016 11:03 AM, wm4 wrote:
>> +static void add_option(const char* current_option, optionContext* option_ctx)
>> +{
>> +    int option_length = strlen(current_option);
>> +    if (option_ctx->buffer_filled + option_length + 1 > option_ctx->options_buffer_size) {
>> +        option_ctx->options_buffer_size += option_length + 1;
>> +        option_ctx->options = realloc(option_ctx->options, option_ctx->options_buffer_size);
> Missing error handling, and maybe overflow handling. Also, we don't use
> malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why
> we do this (it doesn't have much of a justification for realloc at
> least), but it's probably better to be consistent.

av_realloc() and av_realloc_array() will call _aligned_realloc() on windows
instead of realloc().
There's also the memalign_hack codepath, but that's scheduled to go anyway.

> 
> Does the "s" field have any purpose? It seems to be redundant with the
> other fields.
>

On Tue, 6 Dec 2016 11:35:49 -0300
James Almer <jamrial@gmail.com> wrote:

> On 12/6/2016 11:03 AM, wm4 wrote:
> >> +static void add_option(const char* current_option, optionContext* option_ctx)
> >> +{
> >> +    int option_length = strlen(current_option);
> >> +    if (option_ctx->buffer_filled + option_length + 1 > option_ctx->options_buffer_size) {
> >> +        option_ctx->options_buffer_size += option_length + 1;
> >> +        option_ctx->options = realloc(option_ctx->options, option_ctx->options_buffer_size);  
> > Missing error handling, and maybe overflow handling. Also, we don't use
> > malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why
> > we do this (it doesn't have much of a justification for realloc at
> > least), but it's probably better to be consistent.  
> 
> av_realloc() and av_realloc_array() will call _aligned_realloc() on windows
> instead of realloc().

Oh I see, av_free calls _aligned_free, and normal malloc/realloc/free
doesn't work with the Windows _aligned_* functions.

> There's also the memalign_hack codepath, but that's scheduled to go anyway.

Let's hope so.

> Missing error handling, and maybe overflow handling. Also, we don't use

> malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why we

> do this (it doesn't have much of a justification for realloc at least), but it's

> probably better to be consistent.


Ok, point taken on the av_* functions. On the handling bit, I was thinking to flag the error via av_log and then call exit_program(1), after, of course, having released the memory allocated up to that point. Would that be ok?

>

> Does the "s" field have any purpose? It seems to be redundant with the

> other fields.


It has. It points to the position in the options buffer which is going to be filled with the command line being parsed. So it's not redundant.



-----------------------------
http://www.bbc.co.uk
This e-mail (and any attachments) is confidential and
may contain personal views which are not the views of the BBC unless specifically stated.
If you have received it in
error, please delete it from your system.
Do not use, copy or disclose the
information in any way nor act in reliance on it and notify the sender
immediately.
Please note that the BBC monitors e-mails
sent or received.
Further communication will signify your consent to
this.
-----------------------------

On Wed, Dec 14, 2016 at 3:23 PM, Matteo Naccari
<Matteo.Naccari@bbc.co.uk> wrote:
>> Missing error handling, and maybe overflow handling. Also, we don't use
>> malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why we
>> do this (it doesn't have much of a justification for realloc at least), but it's
>> probably better to be consistent.
>
> Ok, point taken on the av_* functions. On the handling bit, I was thinking to flag the error via av_log and then call exit_program(1), after, of course, having released the memory allocated up to that point. Would that be ok?
>

Calling exit or abort from a library is never ok. Just return error
codes back to the avcodec framework and let it handle the error
instead of exiting right here.

- Hendrik

On Wed, 14 Dec 2016 14:23:05 +0000
Matteo Naccari <Matteo.Naccari@bbc.co.uk> wrote:

> > Missing error handling, and maybe overflow handling. Also, we don't use
> > malloc/realloc/free, but av_malloc/av_realloc/av_free. I'm not sure why we
> > do this (it doesn't have much of a justification for realloc at least), but it's
> > probably better to be consistent.  
> 
> Ok, point taken on the av_* functions. On the handling bit, I was thinking to flag the error via av_log and then call exit_program(1), after, of course, having released the memory allocated up to that point. Would that be ok?

Of course this would be not ok. We never flag errors via av_log (how
would this even work). We also don't call exit() in the libraries, if
you mean this. Or maybe you're talking about ffmpeg.c - no, ffmpeg.c is
not the only API user, and of course not all API users should be
updated to do some weird h264_turing codec error handling.

You do what all other codecs in libavcodec also do: return an error
code to the caller, and maybe log an error message (not in obscure OOM
cases, probably).

> >
> > Does the "s" field have any purpose? It seems to be redundant with the
> > other fields.  
> 
> It has. It points to the position in the options buffer which is going to be filled with the command line being parsed. So it's not redundant.

Then what does buffer_filled do?

On Wed, Dec 14, 2016 at 14:23:05 +0000, Matteo Naccari wrote:

> Ok, point taken on the av_* functions. On the handling bit, I was
> thinking to flag the error via av_log and then call exit_program(1),
> after, of course, having released the memory allocated up to that
> point. Would that be ok?

Certainly not, as exit_program() is for the programs, and neither
available for nor acceptable in a library. In the library, you need to
provide "exit" paths, and pass proper return/error codes through the
functions and to the API. (Use of av_log(): Yes, where appropriate.)

Moritz

> > It has. It points to the position in the options buffer which is going to be

> filled with the command line being parsed. So it's not redundant.

> 

> Then what does buffer_filled do?


It keeps track on the number of characters written in the options buffer so far. I appreciate this could have been computed on the fly by doing: option_ctx->s - option_ctx->options but I though the code would have been more readable this way.

Hello everyone,

Any news on the integration of the patch identified in the subject line?

Best regards,
Matteo

On Tue, 31 Jan 2017 12:42:54 +0000
Matteo Naccari <Matteo.Naccari@bbc.co.uk> wrote:

> Hello everyone,
> 
> Any news on the integration of the patch identified in the subject line?
> 
> Best regards,
> Matteo

That's pretty old by now. Can you send a v4 patch set?

2017-01-31 13:42 GMT+01:00 Matteo Naccari <Matteo.Naccari@bbc.co.uk>:
> Hello everyone,
>
> Any news on the integration of the patch identified in the subject line?

So far, you haven't removed the calls to malloc().
Please use av_freep() (instead of av_free) to free the memory.

Carl Eugen

2017-01-31 13:52 GMT+01:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 2017-01-31 13:42 GMT+01:00 Matteo Naccari <Matteo.Naccari@bbc.co.uk>:

>> Any news on the integration of the patch identified in the subject line?
>
> So far, you haven't removed the calls to malloc().
> Please use av_freep() (instead of av_free) to free the memory.

And please replace all:
if (condition)
do;
else {
...

with:
if (condition) {
do;
} else {
...

Carl Eugen

> That's pretty old by now. Can you send a v4 patch set?

Sorry my bad, I've already sent a v4 on the 14/12. Do you want me to rebase the patch to the latest commit on the branch master?

Re: On removal of malloc, free, etc. I think this v4 takes care of this as the commit messages says.

2017-01-31 14:06 GMT+01:00 Matteo Naccari <Matteo.Naccari@bbc.co.uk>:

> Re: On removal of malloc, free, etc. I think this v4 takes care of this as the commit messages says.

I don't think so.

Carl Eugen