[vlc-devel] [PATCH] vout: fix fullscreen race condition

[Rémi Denis-Courmont]

Le tiistaina 17. huhtikuuta 2018, 19.04.17 EEST Romain Vimont a écrit :
> On Tue, Apr 17, 2018 at 06:16:12PM +0300, Rémi Denis-Courmont wrote:
> > Le tiistaina 17. huhtikuuta 2018, 17.25.28 EEST Romain Vimont a écrit :
> > > On Tue, Apr 17, 2018 at 04:12:49PM +0300, Rémi Denis-Courmont wrote:
> > > > Nack. This is a Qt bug. Don't clobber the full screen flag(s) and
> > > > interfere with other interfaces.
> > > 
> > > Thank you for your review.
> > > 
> > > I think the root cause of the bug is not related to Qt: in
> > > modules/control/hotkeys.c:PutAction(), case ACTIONID_TOGGLE_FULLSCREEN
> > > changes only the playlist variable if p_vout is NULL, e.g. during the
> > > vout creation. In that case, once the vout is created, the playlist
> > > "fullscreen" variable may be different from the vout "fullscreen"
> > > variable, even if Qt is not involved at all.
> > 
> > How is that a bug? Obviously, there has to be a point when the switch
> > happens;
> The problem is that the inconsistency will never be recovered (unless
> you switch the fullscreen flag again).

Even if this were a bug, it has nothing to do with #19697.

> So fullscreen is and remains true
> (resp. false) for the playlist object while it is and remains false
> (resp. true) for the vout object.

That is by design.

Pressing F when there is no video window toggles fullscreen mode for future 
video windows. Pressing F while there is a video window should toggle the 
fullscreen mode of the focused window, or if none, the embedded video window.

Of course, this is racy. User interactions are always racy if widgets can 
change without direct user solicitation.

> More fundamentally, the fullscreen flag is duplicated into several
> objects, while it represents exactly 1 state (so the variables must be
> kept in sync, and they are/were not).

No, it does _not_ represent exactly one state. There can be any number of 
video windows at a time, each with their independent fullscreen flag. The 
input manager flag is *only* the initial value for future video windows.

> > Meanwhile, Qt shows a fullscreen video window, and, at the same time, does
> > not show the FSC. That is totally a bug within Qt GUI.
> 
> Anything using both the playlist "fullscreen" variable and the vout
> "fullscreen" variable would have this inconsistency, since the values
> may be different indefinitely.

If there are no video outputs, then there are no FSC to show, and thus the 
playlist flag is irrelevant. If there are video outputs, then each of them has 
its own fullscreen flag, and there are also no reasons to check the playlist 
flag ever as far as the FSC is concerned.

> The fact that the Qt GUI reveals it is just a consequence.

No, the Qt GUI is just buggy.

-- 
Реми Дёни-Курмон
http://www.remlab.net/