On 20/02/2017 12:02, Martin Storsjö wrote:
> On Mon, 20 Feb 2017, Luca Barbato wrote:
>
>> ---
>> libavformat/rtsp.c | 80
>> ++++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 48 insertions(+), 32 deletions(-)
>
> I'm not a fan of this.
>
> Previously, while a bit entangled, we have one single poll loop which
> can react to messages both on the control channel and on the udp sockets
> as soon as they happen. After this, we need to wait for the next udp
> packet, or a timeout in that poll loop in the worst case!, before
> actually reacting to messages on the control channel.
If I read the code correctly it gives priority to rtp and it might just
return data from the first socket if it happens that there is always
data available...
> And even then, you have an even worse issue in the actual implementation
> of it:
>
>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index e3b8a7a..f2d620b 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>> @@ -1906,29 +1906,60 @@ redirect:
>> }
>> #endif /* CONFIG_RTSP_DEMUXER || CONFIG_RTSP_MUXER */
>>
>> +#if CONFIG_RTSP_DEMUXER
>> +static int rtsp_read_streaming_commands(AVFormatContext *s)
>> +{
>> + RTSPState *rt = s->priv_data;
>> + struct pollfd p[1] = { { .fd =
>> ffurl_get_file_handle(rt->rtsp_hd),
>> + .events = POLLIN } };
>> + int ret = poll(p, 1, POLL_TIMEOUT_MS);
>
> Absolutely not. Did you think this through?
>
> If you are going to do this this way, this needs to have a zero timeout.
> Absolutely not a nonzero one. Did you even try this?
> This would block you to receiving at most 10 udp packets per second,
> since every single one of them proceed to blocking for currently 100 ms
> while waiting for a potential command on the control channel.
>
> Despite that, I'm not ok with this patch. Perhaps you have a better
> solution for it all further ahead which makes it a non-issue, then it
> might be discussable. But currently this just looks like a bad
> regression for no real benefit.
The next batch would move the udp to threaded reads so the whole thing
would be a non-issue, but if we want to keep a fallback either we have
to keep the original polling loop.
I probably should think about how to directly feed the queue that is
already present to spare a memcpy and not have to redo the stream
selection logic that is already present.
> // Martin
> _______________________________________________
> libav-devel mailing list
> [email protected]
> https://lists.libav.org/mailman/listinfo/libav-devel
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
