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

Nablet Developer sdk at nablet.com
Wed Oct 11 14:08:22 EEST 2017


On 10/10/2017 22:47, Derek Buitenhuis wrote:
> On 10/10/2017 7:29 AM, Nablet Developer wrote:
>> @@ -293,6 +293,7 @@ External library support:
>>     --enable-opengl          enable OpenGL rendering [no]
>>     --enable-openssl         enable openssl, needed for https support
>>                              if gnutls is not used [no]
>> +  --enable-opensrt         enable Haivision Open SRT protoco [no]
> Usually we have this in the form of --enable-libXXX for external libs. OpenSSL
> and stuff doesn't follow this for reasons I do not know... maybe someone knows
> if we have a real rule set or (more likely) it's just random.
sure, what do you suggest? use "libopensrt" or just "libsrt"? the second 
one wasn't recommended, because it introduces confusion with subtitle 
format library.

>> +    /* SRT socket options (srt/srt.h), see srt/common/socketoptions.hpp */
> Is this even a public header? Does libsrt not have real documentation?

it's documented very briefly (few words for each parameter) in 
srt/srt.h. I'll remove reference to private header.
>> +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]

>> +    if ((error = getaddrinfo(node, service, &hints, &res))) {
>> +        res = NULL;
>> +        av_log(h, AV_LOG_ERROR, "getaddrinfo(%s, %s): %s\n",
>> +               node ? node : "unknown",
>> +               service,
>> +               gai_strerror(error));
>> +    }
>> +
>> +    return res;
> We're OK with not hard failing here?
it will fail anyway, because it will return res (which is NULL) below. 
make it more explicit?
>> +    if (((struct sockaddr *) &s->dest_addr)->sa_family)
>> +        family = ((struct sockaddr *) &s->dest_addr)->sa_family;
>> +    res0 = srt_resolve_host(h, (localaddr && localaddr[0]) ? localaddr : NULL,
>> +                            s->local_port,
>> +                            SOCK_DGRAM, family, AI_PASSIVE);
> Shouldn't all these ternary checks for user options be in a single place,
> instead of all over the file? It's pretty messy.
sorry, I didn't get this one - there is only one such ternary check in 
code (for localaddr).

>
>> +    for (res = res0; res; res=res->ai_next) {
>> +        srt_fd = srt_socket(res->ai_family, SOCK_DGRAM, 0);
>> +        if (srt_fd != SRT_INVALID_SOCK) break;
>> +        log_net_error(NULL, AV_LOG_ERROR, "socket");
>> +    }
> Again no hard failing?
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.

>> +
>> +/**
>> + * Return the srt file handle for select() usage to wait for several RTP
>> + * streams at the same time.
>> + * @param h media file context
>> + */
>> +static int srt_get_file_handle(URLContext *h)
>> +{
>> +    SRTContext *s = h->priv_data;
>> +    return s->srt_fd;
>> +}
> Ditto.
this one is actually used and set into url_get_file_handle of 
URLProtocol. or shall I omit it?
>> +        char *next = strchr(source_start, ',');
>> +        if (next)
>> +            *next = '\0';
>> +        sources[*num_sources] = av_strdup(source_start);
>> +        if (!sources[*num_sources])
>> +            return AVERROR(ENOMEM);
>> +        source_start = next + 1;
>> +        (*num_sources)++;
>> +        if (*num_sources >= max_sources || !next)
>> +            break;
>> +    }
>> +    return 0;
>> +}
> [...]
sorry, what do you mean by "[...]"?
>
>> +
>> +/* - The "PRE" options must be set prior to connecting and can't be altered
>> +/    on a connected socket, however if set on a listening socket, they are
>> +/    derived by accept-ed socket. */
>> +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!
>
>> +    p = strchr(uri, '?');
>> +    if (p) {
>> +        if (av_find_info_tag(buf, sizeof(buf), "reuse", p)) {
>> +            char *endptr = NULL;
>> +            s->reuse_socket = strtol(buf, &endptr, 10);
>> +            /* assume if no digits were found it is a request to enable it */
>> +            if (buf == endptr)
>> +                s->reuse_socket = 1;
> 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
>> +    return 0;
>> +#if HAVE_PTHREAD_CANCEL
>> + thread_fail:
>> +    pthread_cond_destroy(&s->cond);
>> + cond_fail:
>> +    pthread_mutex_destroy(&s->mutex);
>> +#endif
> I assume the fallthrough is intended here.
correct
>
>> +            } 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?

everything else is clear and all errors are valid and I am fixing them. 
thanks again for the review.


More information about the ffmpeg-devel mailing list