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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
