[x264-devel] [Git][videolan/x264][master] Add new API function: x264_param_cleanup

[Alexander Strasser]

Hi Anton,

that's a tricky change for parameter management!

I couldn't spot serious errors while reading the diff 1.5 times.

So only some trivial comments follow.


On 2020-07-02 22:24 +0200, Anton Mitrofanov wrote:
>
>
> Anton Mitrofanov pushed to branch master at VideoLAN / x264
>
>
> Commits:
> 4c2aafd8 by Anton Mitrofanov at 2020-07-02T22:23:50+02:00
> Add new API function: x264_param_cleanup
>
> Should be called to free struct members allocated internally by libx264,
> e.g. by x264_param_parse.
> Partially based on videolan/x264!18 by Derek Buitenhuis.
>
> - - - - -
>
>
> 7 changed files:
>
> - common/base.c
> - common/base.h
> - common/frame.c
> - encoder/encoder.c
> - encoder/ratecontrol.c
> - x264.c
> - x264.h
>
>
> Changes:
>
> =====================================
> common/base.c
> =====================================
> @@ -198,6 +198,66 @@ error:
>      return NULL;
>  }

[...]

> +char *x264_strdup( x264_param_t *param, const char *src )

IMHO the naming is a bit unfortunate.

Wouldn't x264_param_strdup or similar have been a little bit better?

If I read x264_strdup I think it's an implementation of the well known
strdup function in x264's code base. Therefore I would expect the same
function signature as strdup usually has.

If I understood correctly, this function is not considered public API
and therefore could still be renamed if you agree another name would
be better.


> +{
> +    strdup_buffer *buf = param->opaque;
> +    if( !buf )
> +    {
> +        buf = malloc( BUFFER_OFFSET + BUFFER_DEFAULT_SIZE * sizeof(void *) );
> +        if( !buf )
> +            goto fail;
> +        buf->size = BUFFER_DEFAULT_SIZE;
> +        buf->count = 0;
> +        param->opaque = buf;
> +    }
> +    else if( buf->count == buf->size )
> +    {
> +        if( buf->size > (INT_MAX - BUFFER_OFFSET) / 2 / (int)sizeof(void *) )
> +            goto fail;
> +        int new_size = buf->size * 2;
> +        buf = realloc( buf, BUFFER_OFFSET + new_size * sizeof(void *) );

This will leak memory if realloc fails.

I guess it wouldn't be a big problem in typical usage scenarios, but
probably better to just handle it correctly.


> +        if( !buf )
> +            goto fail;
> +        buf->size = new_size;
> +        param->opaque = buf;
> +    }
> +    char *res = strdup( src );
> +    if( !res )
> +        goto fail;
> +    buf->ptr[buf->count++] = res;
> +    return res;
> +fail:
> +    x264_log_internal( X264_LOG_ERROR, "strdup failed\n" );

Maybe changing the log to "x264_strdup failed\n" is more precise.


[...]


Best regards,
  Alexander