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

[Alexander Strasser]

On 2020-07-06 23:36 +0300, BugMaster wrote:
> On Mon, 6 Jul 2020 09:23:59 +0200, Alexander Strasser wrote:
> > [...]
>
> >> +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.
>
> I am not sure about this (I don't have such expections when I see it).

It's a minor thing and probably a bit subjective. Also one can see
it has a different signature at the call site (takes two arguments).

So if you don't feel like renaming, that's totally fine for me.


> > 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.
>
> Yes, renaming is possible if we decide that another name would be more
> appropriate for it.
>
> >> +{
> >> +    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.
>
> I don't see any memory leaks here. And result of realloc correctly
> checked for NULL later.

Sorry, my bad.

The original pointer is still stored in param->opaque and would
be correctly freed on a call to x264_param_cleanup .


> >> +        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.
>
> Probably yes. Especially if we decide to rename this function.


  Alexander