[FFmpeg-devel] [PATCH] avformat/srt: add Haivision Open SRT protocol

[Derek Buitenhuis]

On 10/11/2017 12:08 PM, Nablet Developer wrote:
> sure, what do you suggest? use "libopensrt" or just "libsrt"? the second 
> one wasn't recommended, because it introduces confusion with subtitle 
> format library.

Probably the former, but I have no real strong opinion on changing it.

> it's documented very briefly (few words for each parameter) in 
> srt/srt.h. I'll remove reference to private header.

That's a shame, but I suppose this isn't an FFmpeg problem.

>>> +static int srt_neterrno(void)
>> I know technically (void) is needed in C, but we don't do this in the codebase, AFAIK.
> this throws compiler warning (GCC 5.4.0):
> warning: function declaration isn’t a prototype [-Wstrict-prototypes]

Hmm, wasn't aware we used that warning by default. Fine to leave it as-is, if so.

> it will fail anyway, because it will return res (which is NULL) below. 
> make it more explicit?

No, that's fine. Thanks for the explanation.

> sorry, I didn't get this one - there is only one such ternary check in 
> code (for localaddr).

I'm saying all user-provided options should be checked in a single place, during init,
rather than all over the place in the code, where they are used.

> this one is intentional - getaddrinfo may return multiple addresses 
> (e.g. IPv4 and IPv6), and if we can't open the first, we still can have 
> a chance with second and so one. if none is opened, then it will fail below.

OK. Maybe a av_log debug or comment. Thanks for the explanation.
> this one is actually used and set into url_get_file_handle of 
> URLProtocol. or shall I omit it?

Sorry, I missed that; apologies. It's fine as-is.

>> [...]
> sorry, what do you mean by "[...]"?

Literally just means 'co comment, continue reading below.'

>>> +static int srt_set_options_pre(URLContext *h, int srt_fd)
>>> +{
>>> +    SRTContext *s = h->priv_data;
>> You should not be defining functions using the srt_ namespaces, considering
>> this is the namespace of libsrt, which you are using!

^ Making sure this isn't forgot ^

>> If I'm reading this right, you're setting what's supposed to be a user option?
>>
>> That seems... bad. Same for otehr instances.
> my understanding is that protocols allows to set options as part of URL, 
> e.g. "udp://127.0.0.1:7777?reuse_socket=1" shall set reuse_socket option 
> as well. is it acceptable

So user options are overridden by URL params...?

>>> +            } else {
>>> +                /* FIXME: using the monotonic clock would be better,
>>> +                   but it does not exist on all supported platforms. */
>> Can we provide an internal or external API to enable its use when available?
> if it's question to me - I don't know. what would you recommend to do in 
> this patch?

It's kind of outside the scope of this patch, but I think an internal API
would be useful. I don't think implementing such an API is a requirement
for this patch, just a nice-to-have.

- Derek