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

[Marton Balint]

On Wed, 23 Dec 2020, Paul B Mahol wrote:

> On Wed, Dec 23, 2020 at 10:30 PM Marton Balint <cus at passwd.hu> wrote:
>
>>
>>
>>
>> INT_MAX for pkt_size is probably not a good default. pkt_size is used (at
>> least in other protocols) to determine the maximum allowed packet size for
>> output, and the maximum supported packet size for input. I am not sure
>> what you can pump into RIST (mpegts most likely?) but probably it should
>> be determined for that usage. e.g. 1316?
>>
>> Also the URLContext->max_packet_size should be set based on the
>> packet_size.
>>
>> > +    { "log_level",   "set loglevel",    OFFSET(log_level),
>>  AV_OPT_TYPE_INT,   {.i64=-1},                   -1, INT_MAX, .flags = D|E
>> },
>> > +    { "secret", "set encryption secret",OFFSET(secret),
>> AV_OPT_TYPE_STRING,{.str=""},                    0, 0,       .flags = D|E },
>>
>> default should rather be NULL, no?
>>
>
> Same thing.

Surely there is no significant difference, I just feel that it cleaner to 
use NULL as unspecified default.

[...]

>> > +static int librist_open(URLContext *h, const char *uri, int flags)
>> > +{
>> > +    RISTContext *s = h->priv_data;
>> > +    int ret;
>> > +
>> > +    s->fd = rist_flow_id_create();
>> > +
>> > +    ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb,
>> h, NULL, stderr);
>>
>> stderr should not be given, because we are using our own log callback.
>>
>
> Not used, thus not really relevant.

OK, but it still confused me that stderr may be used for anything. So 
yet again I believe it is cleaner to use NULL.

[...]

>> Some error handling should be done, e.g. reject encryption sizes not
>> supported, reject encryption without secret given, etc.
>>
>
> That is already handled.

As far as I see invalid encryption sizes are not rejected but silently 
ignored.

>> > +static int librist_get_file_handle(URLContext *h)
>> > +{
>> > +    RISTContext *s = h->priv_data;
>> > +
>> > +    return s->fd;
>> > +}
>>
>> I don't think this is right, s->fd is a flow id, not an ordinary file
>> descriptor. You probably don't need this callback anyway.
>>
>
> Are you really sure it is not needed?

Nicolas already explained the use case for this callback, so yes, I think 
it is not needed.

Regards,
Marton