[FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.

[Matt Oliver]

On 3 December 2016 at 22:33, Nicolas George <george at nsup.org> wrote:

> Le tridi 13 frimaire, an CCXXV, Matt Oliver a écrit :
> > I was just writing an email to rephrase but you beet me to it (sorry i
> > wasnt faster)
>
> No problem.
>
> > I should rephrase this to "Why is polling with an acceptable
> timeout/sleep
> > not acceptable". Obviously a event based callback would be more efficient
> > but wouldnt setting an appropriate recv timeout then checking for thread
> > exit before looping the recv call be an acceptable compromise. The thread
> > exit check is minimal and if not set it would just go back into the
> > blocking recv until data is received or the timeout triggers again. Long
> > term callbacks would be better but would this be acceptable short term.
>
> What would you consider an appropriate timeout, more precisely?
>

Thats the trick, not to large that it results noticeable shutdown delays to
the user but not to quick that it polls to often, on a modern machine id
think something like 0.01s would be ok.


> First, I would like to remind you that you cannot block with a timeout,
> you would have to rework the code to use the ill-named poll(), which
> would bring it back to its state before the use of pthread_cancel().
>

I was thinking of just using setsockopt to set a value for SO_RCVTIMEO only
in circular_buffer_task_rx. That way recv will timeout at the specified
interval which will then use the existing code that checks the recv return.
In case of timeout that check will continue back to top of loop that will
have the s->close_req check to terminate the thread if needed.
something like:

+  setsockopt(s->udp_fd, SOL_SOCKET, SO_RCVTIMEO, &timeout,
sizeof(timeout));
    while(1) {
        int len;
+      if (s->close_req)
+              goto end;
        pthread_mutex_unlock(&s->mutex);
        len = recv(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0);
        pthread_mutex_lock(&s->mutex);
        if (len < 0) {
            if (ff_neterrno() != AVERROR(EAGAIN) && ff_neterrno() !=
AVERROR(EINTR)) {
                s->circular_buffer_error = ff_neterrno();
                goto end;
            }
            continue;
        }

>
>
> You are right that it would take care of the latency problem of the
> network side, and allow a long timeout. But the latency is a problem too
> for the cancellation. We had a 3 seconds timeout, users did get
> impatient and interrupted it forcefully.
>
> Furthermore, the core of your argument is that "the thread exit check is
> minimal", but as I explained, it is not on embedded devices: anything
> except "block until the kernel wakes up" is too much because it prevents
> deep sleep states and drains the battery.
>

So perhaps keep the existing pthread_cancel functionality and add in the
changes only when pthread_cancel is not available. The user can always
choose not to use the thread stuff if they dont like it by not setting a
fifo_size option etc.

Modern operating systems allow multi-peer applications to work in a
> completely poll-free completely notification-based way. FFmpeg, based on
> very old code without global design, is quite far from achieving that.
> But moving away from that is definitely moving in the wrong direction.
>

Is anyone working on that as your right i definitely think an event system
for all protocols is the best option. Its just that has a larger scope than
i can handle so i was trying to get a simple shorter term fix for udp.

I assume you are not working just on a general distaste about
> pthread_cancel(): you have in mind a specific platform in mind where it
> is not available and you need the UDP thread, have you not?
>

Yep, although im not a huge fan of pthread_cancel, im mainly just trying to
improve functionality with msvc.

Then I suggest working for that platform: find a way of implementing a
> poll-free delay-free interruption mechanism. Maybe using non-portable
> properties of the platform (like closing on another thread like Hendrik
> evoked) or an entirely different API.
>

I havent fully tested Hendriks idea as the msdn docs dont recommend calling
multiple winsock functions at the same time from different threads so i
wasnt sure about how well received a patch that relies on closesocket to
unblock the recv function would be received (although i have seen it
extensively used without issuers). If Hendrik has tested this though with
his local patch without issues then I would accept that as a solution to
fixing my issue. ping Hendrik!