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

[Steve Lhomme]

On 2019-06-14 4:48, Rémi Denis-Courmont wrote:
> 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.

Maybe "bad practice" is the wrong term. But it's bound to create 
deadlock if not carefully handled. There's nothing in the code that 
guarantees the order will always be the same. Maybe some helpers to 
avoid issues could help. C++11 has this for example:

https://en.cppreference.com/w/cpp/thread/lock


>> 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
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>