[FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Derek Buitenhuis]

On 11/20/2017 10:35 AM, Matteo Naccari wrote:
> +enabled libturing         && require_pkg_config libturing libturing turing.h turing_version &&
> +                             { check_cpp_condition turing.h "TURING_API_VERSION > 1" ||
> +                               die "ERROR: libturing requires turing api version 2 or greater."; }

Why exactly is libturing's API version not correlated with its soversion?

> +#define MAX_OPTION_LENGTH 256

I probably asked this in a previous iteration, but why is there an
arbitrary limit?

> +        if (!(option_ctx->options)) {
> +            option_ctx->options = av_malloc(option_length + 1);
> +            if (!(option_ctx->options)) {
> +                return AVERROR(ENOMEM);
> +            }
> +        } else {
> +            temp_ptr = av_realloc(option_ctx->options, option_ctx->options_buffer_size + option_length + 1);
> +            if (!(temp_ptr)) {
> +                return AVERROR(ENOMEM);
> +            }
The if/else is redundant, since av_realloc can take a NULL pointer.

> +
> +    snprintf(option_string, MAX_OPTION_LENGTH, "--input-res=%dx%d", avctx->width, avctx->height);
> +    if (error_code = add_option(option_string, &encoder_options)) {
> +        goto fail;
> +    }

Since the return value for snprintf is not checked, won't this lead to mangled arguments if
its truncated? Same for below
> +
> +    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);

Shouldn't calls to turing_destroy_encoder (or preferably libturing_encode_close) be
handled at the fail label rather than all over the place?

> +    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");

Redundant generic log.

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

Is there no way to support AV_CODEC_FLAG_GLOBAL_HEADER directly (see libx265.c)?

- Derek