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

[Moritz Barsnick]

On Mon, Nov 20, 2017 at 10:35:39 +0000, Matteo Naccari wrote:

I don't recall all of the previous review remarks, but these may be
new:

>  LICENSE.md             |   1 +
>  configure              |   6 +

Please add a changelog entry.

>      librubberband
> +	libturing
>      libvidstab

You used a tab instead spaces.

> + #if defined(_MSC_VER)
> +#define TURING_API_IMPORTS 1
> +#endif

Stray whitespace in front of "#if".

> +    int error_code = 0;
> +    int i = 0;

I believe these initializations are never used.

> +    if (error_code = add_option("turing", &encoder_options)) {
> +        goto fail;
> +    }

This assignment in an if() clause will give a compiler warning if you
don't add an extra set of brackets around the assingment.

Preferred method is actually:
   error_code = add_option("turing", &encoder_options);
   if (error_code) {
       goto fail;
   }

> +    if (error_code = add_option("--frames=0", &encoder_options)) {
> +        goto fail;
> +    }

Same here, and further occurences below.

> +            error_code =  AVERROR(ENOMEM);

Stray whitespace.

> +    int ret = 0;

Unused assignment.

> +    av_strlcpy(option_ctx->s, current_option, (option_length + 1));
> +    option_ctx->s += 1 + option_length;
> +    option_ctx->options_added++;
> +    option_ctx->buffer_filled += option_length + 1;

It's a bit confusing to read, if you use "option_length + 1" twice, and
"1 + option_length" the third time.

> +static const AVOption options[] = {
> +    { "turing-params", "configure additional turing encoder parameters", offsetof(libturingEncodeContext, options), AV_OPT_TYPE_STRING,{ .str = NULL }, 0, 0, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },

IMHO you can drop the word "configure ".

Cheers,
Moritz