[vlc-devel] [PATCH] video_output: explicit vlc_testcancel

[Quentin Chateau]

On 26/02/2020 15:54, Thomas Guillem wrote:
>
>
> On Wed, Feb 26, 2020, at 15:34, Quentin Chateau wrote:
>>
>> Thanks for the hindsight, I dug further:
>>
>> vlc_cond_timedwait is called, and so is pthread_cond_timedwait. The 
>> issue happens when the deadline parameter is in the past: when this 
>> happens, pthread_cond_timedwait does not act as a cancellation point 
>> anymore...
>>
>> In my case deadline happens to be in the past because 
>> ThreadDisplayPicture keeps failing (due to one of my video filters 
>> failing on the last frame)
>>
>> I could not find this behavior in the pthread documentation, but it 
>> seems to be the case anyway. Do you think that we need to:
>>
>>  1. Fix vlc_cond_timedwait so it will check for cancellation if the
>>     deadline is in the past
>>  2. Acknowledge the fact that vlc_cond_timedwait is not always a
>>     cancellation point and fix the issue higher in the stack, either
>>     in vout_control_Pop, or similarly to the patch I sent, directly
>>     in Thread
>>
>
> Or 3. Investigate why deadline is in the past.

That I can tell: I have a video filter that happens to fail on the last 
frame each video.

Everything goes as normal until the call to vout_ConvertForDisplay in 
ThreadDisplayRenderPicture, which returns NULL (result of my failing 
video filter) and causes an early return.

     todisplay = vout_ConvertForDisplay(vd, todisplay);
     if (todisplay == NULL) {
         vlc_mutex_unlock(&sys->display_lock);
         if (subpic != NULL)
             subpicture_Delete(subpic);
         return VLC_EGENERIC;
     }

This bypasses the update of sys->displayed.date that would have happened 
50 lines below.

Because of that, sys->displayed.date never changes, in turn causing the 
deadtime variable to keep the same value (updated in ThreadDisplayPicture)

     vlc_tick_t date_refresh = VLC_TICK_INVALID;
     if (sys->displayed.date != VLC_TICK_INVALID) {
         date_refresh = sys->displayed.date + VOUT_REDISPLAY_DELAY - 
render_delay;
         refresh = date_refresh <= system_now;
     }
     bool force_refresh = !drop_next_frame && refresh;

     if (!frame_by_frame) {
         if (date_refresh != VLC_TICK_INVALID)
             *deadline = date_refresh;
         if (date_next != VLC_TICK_INVALID && date_next < *deadline)
             *deadline = date_next;
     }

As time goes on, the value can get "in the past" before the thread is 
cancelled. From there the thread is never cancelled because of the 
behavior of pthread_cond_timedwait.

>
> Rémi, any preference ?
>
>>
>> On 26/02/2020 11:11, Thomas Guillem wrote:
>>> Why is it not canceled from vlc_cond_timedwait() called from vout_control_Pop() ?
>>>
>>> Is ctrl->can_sleep false in your case ?
>>>
>>> Anyway, I would prefer a fix in vout_control_Pop() like this:
>>>
>>>      if (ctrl->cmd.i_size <= 0 && deadline != VLC_TICK_INVALID && ctrl->can_sleep) {
>>>          /* Spurious wakeups are perfectly fine */
>>>          ctrl->is_waiting = true;
>>>          vlc_cond_signal(&ctrl->wait_available);
>>>          vlc_cond_timedwait(&ctrl->wait_request, &ctrl->lock, deadline);
>>>          ctrl->is_waiting = false;
>>>      }
>>>      else
>>>          vlc_testcancel();
>>>
>>> On Tue, Feb 25, 2020, at 12:47,quentin.chateau at deepskycorp.com  <mailto:quentin.chateau at deepskycorp.com>  wrote:
>>>
>>>> From: Quentin Chateau<quentin.chateau at deepskycorp.com>  <mailto:quentin.chateau at deepskycorp.com>
>>>>
>>>> When ThreadDisplayPicture does not return VLC_SUCCESS,
>>>> explicitely check for thread cancellation.
>>>>
>>>> There are cases where a video filter can fail
>>>> systematically without hitting any cancellation point
>>>> after the video output thread is cancelled, resulting
>>>> in an infinite loop if there is no explicit check.
>>>> ---
>>>>   src/video_output/video_output.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
>>>> index 229b010a96..f7aac554e4 100644
>>>> --- a/src/video_output/video_output.c
>>>> +++ b/src/video_output/video_output.c
>>>> @@ -1703,6 +1703,7 @@ noreturn static void *Thread(void *object)
>>>>   
>>>>           if (wait)
>>>>           {
>>>> +            vlc_testcancel();
>>>>               const vlc_tick_t max_deadline = vlc_tick_now() +
>>>> VLC_TICK_FROM_MS(100);
>>>>               deadline = deadline == VLC_TICK_INVALID ? max_deadline :
>>>> __MIN(deadline, max_deadline);
>>>>           } else {
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200226/6a3c2519/attachment.html>