[vlc-devel] [PATCH 02/17] video_output: translate the mouse coordinates to display coordinates early

[Marvin Scholz]

On 2 Nov 2020, at 14:56, Alexandre Janniaux wrote:

> Hi,
>
> On Mon, Nov 02, 2020 at 01:59:51PM +0100, Steve Lhomme wrote:
>> On 2020-11-02 13:11, Rémi Denis-Courmont wrote:
>>> Le maanantaina 2. marraskuuta 2020, 13.46.58 EET 
>>> zhilizhao(赵志立) a écrit :
>>>>> On Nov 2, 2020, at 6:15 PM, Alexandre Janniaux 
>>>>> <ajanni at videolabs.io>
>>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Mon, Nov 02, 2020 at 10:35:09AM +0100, Steve Lhomme wrote:
>>>>>> It seems the issue is that code in macosx.m
>>>>>>
>>>>>>     @synchronized(self) {
>>>>>>         _hasPendingReshape = NO;
>>>>>>     }
>>>>>>
>>>>>> It might use an atomic bool for this variable. Using less locks 
>>>>>> is always
>>>>>> a
>>>>>> good idea when possible. I don't know if/how Objective-C can 
>>>>>> handle
>>>>>> atomics.
>>>>>>
>>>>>> On the other hand the original patch is not mandatory and doesn't 
>>>>>> improve
>>>>>> anything much. So if necessary I'm OK with reverting it.
>>>>>
>>>>> I read the issue as «you cannot get the display lock for mouse 
>>>>> update,
>>>>> since display can currently report the mouse state, thus already 
>>>>> under
>>>>> display lock».
>>>>>
>>>>> Is that different from reality?
>>>>>
>>>>> Using the work from Marvin, we'll be able to remove the mouse API 
>>>>> from
>>>>> display on MacOSX, so finally solve this issue.
>>>>
>>>> So it’s OK to just revert? The master branch is totally unusable 
>>>> now.
>>>
>>> I don't think so. It seems that there's a lock inversion in the 
>>> MacOS video
>>> display plugin that just happens to become (more) visible with the 
>>> patch.
>>
>> The issue is that display callbacks are called under 
>> sys->display_lock
>> (normal). In macos it does a @synchronized on the VLCOpenGLVideoView 
>> in a
>> few places, which is another kind of lock on the Obj-C object. So far 
>> it was
>> either locked in the UI thread or in the display thread, under the
>> display_lock.
>>
>> This commit changed things as vout_MouseState() is called by the UI 
>> thread,
>> under the synchronized VLCOpenGLVideoView, and then it takes the
>> display_lock (this patch).
>>
>> I'm not sure it's possible to remove the synchronized 
>> VLCOpenGLVideoView in
>> the second case. It's mostly there to make sure the module is not 
>> closing.
>> It's never going to change during the lifetime of the module.
>>
>> I'm not sure there is a clean way to handle this. The mouse event is 
>> usually
>> generated in the UI thread on most (all?) platforms. And sent through 
>> this
>> code. But the proper way to do it is via the window_t that gets the 
>> mouse
>> event. This vout_display_SendMouseMovedDisplayCoordinates() seems 
>> like
>> legacy code, like vout_display_SendEventMousePressed,
>> vout_display_SendEventMouseReleased and
>> vout_display_SendEventMouseDoubleClick.
>>
>> The macos code (and other callers of these functions) should store 
>> the
>> window on init and use that to sent the mouse event. The window will 
>> never
>> change nor disappear while the display module exists so it can be 
>> used
>> without a lock.
>>
>> So the solution is either to revert the patch, or to patch these 
>> callers to
>> use the window directly and now the "vd" variable. One is fast, but 
>> the
>> other seems more correct in the long run.
>
> This seems to be a correct solution, but I'm not sure it's completely
> correct as-is. vout_display_Send* already just redirect to the window
> stored in the `vd` so it's quite similar from what you suggest without
> precising how you do it.
>
> The main issue is that the display can be closed while the main thread
> is running, thus you can actually remove the vd behind the main thread
> back. We could wait to have removed the view to ensure we don't 
> receive
> events but then it would deadlock when the player is released in the
> main thread before being stopped, which is an very common case in the
> VLC executable.
>
> Instead, maybe we could just apply your atomic suggestion to have a
> shared state for the vout thread / main thread (could be the
> implementation we declared in this file) containing a killed state.
> It cannot be only an atomic because of the obvious TOCTOU at the
> destruction time so it needs a dedicated locking mechanism that would
> not be shared with other method (unlike the self lock 
> synchronization).
>
> It would not solve the deadlock at Close() if you use the mouse, but
> would solve all other deadlock so it makes it much more reliable and
> usable.
>
> Then, a proper window with Marvin's work would make the final solution
> much more reliable.
>

Hi,
the lock in questions is way too broad and used in way too many places.
This should be solved one I add mouse/key event handling to the window
module and add the new calayer vout, removing the old macosx vout
completely.

>> Marvin, did you touch that part in your rewriting on the window in 
>> macos ?
>> (there are still other modules that need some patching as well)
>>
>>> And if you revert the patch, you'll have to somehow solve the flow 
>>> control of
>>> the video output control queue...
>>>
>>> --
>>> 雷米‧德尼-库尔蒙
>>> http://www.remlab.net/
>>>
>>>
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel