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

[Derek Buitenhuis]

On 6/29/2017 3:06 PM, Saverio Blasi wrote:
> ---
>  LICENSE.md             |   1 +
>  configure              |   6 +
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   1 +
>  libavcodec/libturing.c | 313 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 322 insertions(+)
>  create mode 100755 libavcodec/libturing.c

1. Missing version bump.

2. patcheck says:
    Possible security issue, make sure this is safe or use snprintf/av_strl*
    patcheck.stdout:186:+    strcpy(option_ctx->s, current_option);

3. libturing git HEAD won't even build with this patch, because it's broken:

    END /tmp/ffconf.VbgfCWVe/test.c
    gcc -D_ISOC99_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 -std=c11 -fomit-frame-pointer -pthread -I/usr/local/include -L/usr/local/lib -L/usr/local/lib/boost -c -o /tmp/ffconf.VbgfCWVe/test.o /tmp/ffconf.VbgfCWVe/test.c
    In file included from /tmp/ffconf.VbgfCWVe/test.c:1:0:
    /usr/local/include/turing.h:87:1: error: unknown type name 'bool'
     bool turing_check_binary_option(const char *option);
     ^
    ERROR: libturing not found using pkg-config

The API apparently uses C++ bool or C99 stdbool (but doesn't include stdbool.h),
neither of which is OK in FFmpeg, AFAIK. Keep in mind that C99 bool and C++
bool are not compatible.                                                               

> +    if (option_ctx->buffer_filled + option_length + 1 > option_ctx->options_buffer_size) {
> +        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);
> +            }
> +            option_ctx->options = temp_ptr;
> +        }

You are not allowed to pass memory allocated with av_malloc to av_realloc. This is explicitly
stated in the documentation.

> +                    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)) {
> +                        goto fail;

dict gets leaked here.
> +    } else {
> +        output = turing_encode_picture(ctx->encoder, 0);

NULL instead of 0.

- Derek