[vlc-devel] [PATCH 05/13] vout: set display filled synchronously

[Rémi Denis-Courmont]

Le jeudi 13 juin 2019, 16:04:53 EEST Steve Lhomme a écrit :
> On 2019-06-13 14:39, Rémi Denis-Courmont wrote:
> > Hi,
> > 
> > T1 sets, say, A/R, which updates the window size. Then T2 does the same.
> > Then T2 sends the A/R control to vout thread, then T1 sends the same.
> > 
> > -> window uses T2 A/R, display uses T1 A/R.
> 
> OK, so about those intertwined locks. It doesn't seem to be a good
> practice when using locks.

I fail to see how that's a bad practice. The lock order is constant here.

I do see a bad practice of keeping locks while calling to external components, 
but that is a preexisting problem, that seems impossible to avoid here.

> It may work if always done the same way (lock
> A while B is locked, then unlock B). But that's very risky IMO.

"Very risky" compared to what? We cannot do away with locking here, seen as 
the window and display are used by different unsynchronized threads (decoder, 
user interfaces).

We can also not merge the display lock and window lock, seen as the window 
event callbacks may need to lock the display, but may be taken with or without 
the window lock.

> It's
> very easy to make a mistake. For example what if you lock A, do a lot of
> things, including locking B inside a function you didn't think of ? At
> the very least this behavior should be documented as such.

Nothing new here. Even with a single lock, you can already accidentally lock 
recursively. Examples include such stupid things as reporting the window size 
from a display method (instead of from the window provider).

-- 
Rémi Denis-Courmont