[vlc-devel] [PATCH] input: fix crash when flushing the decoder

[Felix Abecassis]

Ping.

Given my explanations, does the above patch now make more sense for you Rémi?

Thank you,

2014-08-12 11:01 GMT+02:00 Felix Abecassis <felix.abecassis at gmail.com>:
> 2014-08-11 21:58 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
>> Le lundi 11 août 2014, 11:56:42 Felix Abecassis a écrit :
>>> The crash can occur in the following situation:
>>> 1) A video decoder is created and starts waiting for the first picture:
>>>    b_waiting = true, b_first = true
>>>
>>> 2) The first picture is received but the decoder is still in waiting mode:
>>>    b_waiting = true, b_first = false
>>
>> I think that's a bug already. The code in input_DecoderWait() does not seem to
>> make sense:
>>
>>     while( p_owner->b_waiting && !p_owner->b_has_data )
>>         ...
>>
>> I don't see how p_owner->b_waiting can ever change from true to false while
>> the loop iterates. Note that input_DecoderStopWait() is called by the same
>> thread (input or demux) that calls input_DecoderWait() !
>>
>
> I think you are right here, the check on b_waiting is not needed,
> b_waiting cannot be set to "false" from the decoder thread.
> After receiving the first picture (which will be forced display), we
> are waiting for another picture.
>
> However, the issue is arising earlier, neither input_DecoderWait or
> DecoderStopWait are called.  We have:
> 08-12 10:37:07.859: D/VLC(15900): core input: EOF reached
> 08-12 10:37:07.859: D/VLC(15900): core input: control type=3
> And then events 4) and 5) from my list, leading to the crash.
> In EsOutDecodersStopBuffering, we haven't reached message "Stream
> buffering done" yet.
>
>> In my opinion, the real problem is that Wait and StopWait are separate (and
>> accordingly b_first and b_waiting are distinct). So the decoder ends up in a
>> seemingly invalid intermediate state, b_wait && !b_first, even though the lock
>> released between those two function calls.
>>
>
> I agree, but so far I think that's a different issue.
>
>>> 3) A second picture is received, the decoder is now waiting in
>>> DecoderWaitUnblock().
>>
>> That function makes the decoder thread wait for the owner thread. I don't see
>> the point, but there may be some nasty side effects and non-trivial
>> interactions with the clock code.
>>
>
> I think we need the clocks to be properly initialized in order to be
> able to assign a system PTS to the pictures.
>
>> Even in the other direction, the input thread waits for the decoder thread
>> only for crappy buffering time estimation and to release the old vout (if none
>> of the active decoder reallocated it).
>>
>>> 4) From the input thread, an EOF event is received, EsOutChangePosition is
>>> called and sets the decoder in flushing mode.
>>>
>>> 5) The decoder thread wakes up and exits DecoderWaitUnblock because
>>> b_flushing is true. However this triggers the assertion in
>>> DecoderDecodeVideo since we have b_waiting && !b_first.
>>
>> I think the assertion is valid; it boils down to the intended relation between
>> b_waiting and b_first. As far as I understand, the real problem occurs earlier.
>>
>
> The assertion is valid indeed, maybe a bit extreme to crash for a
> situation that I think is harmless (but still inconsistent regarding
> the internal decoder state machine).
> I'm questioning in which cases we test this assertion, but not
> questioning the existence of this assertion.
>
> Thank you,
>
>> --
>> Rémi Denis-Courmont
>> http://www.remlab.net/
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>
>
> --
> Félix Abecassis
> http://felix.abecassis.me



-- 
Félix Abecassis
http://felix.abecassis.me