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

[Steve Lhomme]

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.

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
>