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

[Hendrik Leppkes]

Am 29.06.2017 21:45 schrieb "Derek Buitenhuis" <derek.buitenhuis at gmail.com>:

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.


This requirement is no longer documented for ffmpeg as no systems are known
that actually require this.


> +                    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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel