[FFmpeg-devel] [PATCH v7] - Added Turing codec interface for ffmpeg

[Clément Bœsch]

On Tue, Feb 21, 2017 at 05:15:59PM +0000, Saverio Blasi wrote:
[...]
>  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

You probably want to specify a minimal version

[...]
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
> +02110-1301 USA  */
> +

This looks mangled.

> +#include <turing.h>

Please add a line break after this

> +#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;

Style here and in other places: * stick to the var

> +    int options_buffer_size;
> +    int buffer_filled;
> +    int options_added;
> +} optionContext;
> +

> +static av_cold int libturing_encode_close(AVCodecContext *avctx) {

Style: here and in other places missing line break before "{" for
functions.

> +    libturingEncodeContext *ctx = avctx->priv_data;
> +

> +    if (ctx->encoder) {
> +        turing_destroy_encoder(ctx->encoder);
> +    }

Note: the NULL check should probably be part of the libturing API to
simplify code paths for the users (just like the stdlib free(3)
convention).

> +    return 0;
> +}
> +
> +static av_cold int add_option(const char* current_option, 
> +optionContext* option_ctx) {

This function should be replaced with AVBPrint API. It will be much
simpler (that function will basically disappear) and will allow the caller
to check for errors only once.

[...]
> +static av_cold int libturing_encode_init(AVCodecContext *avctx) {
> +    libturingEncodeContext *ctx = avctx->priv_data;
> +    const int bit_depth = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth;
> +    int error_code = 0;
> +

> +    optionContext encoder_options;

use "encoder_options = {0}" so you don't need to fill each and every field
later on, and risk to forget one in the future.

[...]
> +                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);

you could use av_match_name(en->key, "input-res,frame-rate,f,...") here.

> +                if (illegal_option) {
> +                    av_log(avctx, AV_LOG_WARNING, "%s=%s ignored - this parameter is inferred from ffmpeg.\n", en->key, en->value);
> +                } else {
> +                    if (turing_check_binary_option(en->key)) {
> +                        snprintf(option_string, MAX_OPTION_LENGTH, "--%s", en->key);
> +                    } else {
> +                        snprintf(option_string, MAX_OPTION_LENGTH, "--%s=%s", en->key, en->value);
> +                    }
> +                    if ((error_code = add_option(option_string, &encoder_options)) > 0) {
> +                        goto fail;

leaking dict here.

> +                    }
> +                }
> +            }
> +            av_dict_free(&dict);
> +        }
> +    }
> +
> +    if ((error_code = add_option("dummy-input-filename", &encoder_options)) > 0) {
> +        goto fail;
> +    }
> +
> +    if ((error_code = finalise_options(&encoder_options)) > 0) {
> +        goto fail;
> +    }
> +
> +    settings.argv = (char const**)encoder_options.argv;
> +    settings.argc = encoder_options.options_added;
> +

> +    for (int i=0; i<settings.argc; ++i) {

- int is not allowed here
- the style is broken (missing spaces)
- ++i is a c++ idiom and doesn't belong here

> +        av_log(avctx, AV_LOG_VERBOSE, "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");
> +        error_code = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
> +
> +    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);
> +            error_code = AVERROR_INVALIDDATA;
> +            goto fail;
> +        }
> +
> +        avctx->extradata_size = bitstream->size;
> +
> +        avctx->extradata = av_mallocz(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);
> +            error_code =  AVERROR(ENOMEM);
> +            goto fail;
> +        }
> +
> +        memcpy(avctx->extradata, bitstream->p, bitstream->size);
> +    }
> +
> +    av_freep(&encoder_options.argv);
> +    av_freep(&encoder_options.options);
> +    return 0;
> +
> +fail:

> +    av_log(avctx, AV_LOG_ERROR, "Error while initialising the Turing codec.\n");

This message is useless as it won't provide any additional information.

Dropping that log allows you to replace the "fail" label with an "out"
label and drop the 3 lines above your current "fail" label.

> +    av_freep(&encoder_options.argv);
> +    av_freep(&encoder_options.options);
> +    return error_code;
> +}
> +
[...]
> +    ret = ff_alloc_packet2(avctx, pkt, output->bitstream.size, 0);
> +    if (ret < 0) {

> +        av_log(avctx, AV_LOG_ERROR, "Error getting output packet.\n");

Useless log, please drop it

> +        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 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 },

"{ 0 }, 0, 0" is ugly; drop them and use a named ".flags = AV_OPT..."

[...]

Regards,

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170319/b4e54d37/attachment.sig>