[FFmpeg-devel] [PATCH] libavformat: add librist protocol

[James Almer]

On 12/23/2020 8:53 PM, James Almer wrote:
> On 12/23/2020 8:32 PM, Paul B Mahol wrote:
>> +static int librist_close(URLContext *h)
>> +{
>> +    RISTContext *s = h->priv_data;
>> +    int ret = 0;
>> +
>> +    s->peer = NULL;
>> +
>> +    if (s->rist_ctx)
>> +        ret = rist_destroy(s->rist_ctx);
>> +    if (ret < 0)
>> +        return risterr2ret(ret);
>> +    s->rist_ctx = NULL;
>> +
>> +    if (s->peer_config)
>> +        free((void *)s->peer_config);
>> +    s->peer_config = NULL;
>> +
>> +    if (s->logging_settings)
>> +        free((void *)s->logging_settings);
> 
> This is not a good idea. Afaik there could be more than one c library at 
> play, the one linked by librist, and the one linked by libavformat.
> This is why most libraries always end up adding custom free functions.
> 
> I see that rist_logging_set(), or anything in logging.h from this 
> library for that matter, is undocumented, but reading the source code it 
> seems it accepts your own allocated rist_logging_settings struct.
> So IMO first consider asking for some doxy addition to the functions in 
> order to make that officially supported, and then allocate it yourself 
> in librist_open() with av_malloc(sizeof(*s->logging_settings)). 
> Alternatively, they could provide a new rist_logging_free() function of 
> sorts.

I guess you could also just keep it in heap instead of allocating it. No 
need to free it that way since it'll be part of RISTContext.

> 
> And same for peer_config above, except that rist_parse_address() does 
> not accept your own allocated rist_peer_config struct. It only accepts 
> an already initialized one from a previous call, so either that's 
> changed, or a custom free() or a reset_to_defaults() function is added.

Not so much this one, for the same reason you can't pass a freshly 
allocated one.

> 
>> +    s->logging_settings = NULL;
>> +
>> +    return 0;
>> +}
>> +
>> +static int librist_open(URLContext *h, const char *uri, int flags)
>> +{
>> +    RISTContext *s = h->priv_data;
>> +    int ret;
>> +
>> +    ret = rist_logging_set(&s->logging_settings, s->log_level, 
>> log_cb, h, NULL, stderr);
>> +    if (ret < 0)
>> +        return risterr2ret(ret);
>