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

[Felix Abecassis]

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