[vlc-devel] [PATCH v3 00/18] Get rid of the control command API

Fri Aug 21 12:55:21 CEST 2020

Hi,

There's probably no point in keeping the filter lock when doing
the filtering. Adding a state machine allowing some change only
in an idle state and a condition variable to wait for the state
to transition to idle is probably enough.

With regard to filters, if both mouse and filter callbacks can
be called at the same time, they'll need their own internal
state synchronization mechanism which might not lock during the
rendering itself.

If we look at Qt UI, it is always syncing state from player,
playlist, etc, which are notified from different thread, and
sending request from the UI thread. The main point for making
the UI reactive is not that it shouldn't lock or shouldn't
process but that what it's doing under lock in both side is
only cheap state update or state retrieval.

That being said, the current design might be evolved through a
filter wrapper simulating the previous threading behaviour and
a different mouse callback name for the time of the transition,
so as to clean the video output part without bother with the
filters first, and then incrementally port the filters to the
new behaviour.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Aug 21, 2020 at 09:53:07AM +0200, Rémi Denis-Courmont wrote:
> Hi,
>
> Mouse coordinates have to be translated to original video coordinates for menus. We've done it, without filter callbacks, for years. No way we're removing that, regardless of which thread does it. If some component uses the video coordinates as window coordinates, it is buggy. Conversely, if some filters translate wrong or fail to translate, they're already buggy now and this makes no difference to this patchset.
>
> And maybe it's not currently practical to filter the mouse events on the video window thread. That's not an excuse for regressing the mouse event processing. Currently it is mostly immediate, not deferred to the next picture, even if it's not literally synchronous.
>
> And I'm not convinced that doing heavy mouse event processing on the render thread is any better than on the UI thread. Doing heavy processing on mouse event at all sounds like a bad idea. I don't think we should base the architecture on badly designed and rarely used filters.
>
> With that said, sure, we need to define what the locking rule is for mouse events. Taking the whole filter chain lock on the UI thread is not ideal.
>
> Le 21 août 2020 09:34:10 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
> >What we're talking about here is opening a can of worms. This is a
> >whole
> >new behavior the code wasn't designed for. The whole mouse filtering
> >could do with some love, but this is not the scope of this patchset.
> >
> >Example of worm we're talking about: the software deinterlace filters
> >mouse for when the picture is half height. None of the hardware filters
> >
> >do that.
> >There is no consistency between filters. (BTW rotate doesn't handle
> >mouse events, and transform does)
> >
> >I think, when it comes to UI, the responsiveness should matter the
> >most.
> >That means do as little "heavy" processing in the UI thread. Locking
> >"sys->filter.loc"k in the UI thread would be quite heavy. That lock is
> >used to process the pixels. It can take quite a while.
> >
> >In the end the mouse events are transformed according to the filter
> >dimension changes. But it's not to reflect something in the UI. It's to
> >
> >reflect something in the video rendering. Either modifying the
> >rendering
> >based on the mouse position or by sending "mouse-xxx" events. In the
> >former case, there is no use having that in the UI thread. In the
> >latter
> >case, there are things like gestures/hotkeys which might benefit from
> >being in the UI thread. There is also the callback set by some
> >access/demuxer modules via ES_OUT_VOUT_SET_MOUSE_EVENT. It sounds like
> >it could benefit from being in the UI thread. In practice when the user
> >
> >clicks it doesn't necessarily match the position the module think it's
> >at, because of the buffering in between. Doing it one frame later is
> >probably not a big deal.
> >
> >So the real benefit would be for gestures/hotkeys.
> >
> >The gestures only check for a movement in (all) directions, based on a
> >threshold. But if a deinterlace shows the video at double/half size,
> >that means the user actually has to moved twice more/less to reach that
> >
> >threshold, when in fact the threshold is based on the visible area, not
> >
> >the original picture location. So this is in fact wrong to handle
> >filtered mouse values here. (it would be worse in puzzle if you happen
> >to cross a square and the next virtual position is totally different
> >from where the mouse is actually on the screen)
> >
> >The hotkeys module has 2 things. Some "intf-xxx" toggles, which can
> >popup a menu over the video. This should definitely not be filtered
> >according to the coordinates in the unfiltered picture but should match
> >
> >the screen coordinates. It also calls vlc_player_UpdateViewpoint() with
> >
> >the new mouse (filtered) coordinates, to change the viewpoint. Here I
> >don't think it makes sense to translate to filtered coordinates. For
> >example it the video is zoomed 2x a mouse movement of 10 pixels should
> >result in a movement of 10 pixels in the rendered video, not 5 pixels
> >in
> >the source video. In the end the viewpoint value is only applied to
> >video and audio outputs, not the source material.
> >
> >So it seems the "mouse-xxx" calls should not be filtered. They should
> >be
> >called before the filtering is applied. That means they could be called
> >
> >when the mouse event in received, in the UI thread. That would make the
> >
> >gesture/hotkey more responsive. (the double click fullscreen toggle
> >should fall in that category too)
> >
> >The filtered coordinates end up being used only for the few modules
> >that
> >actually need to know exactly where in the source video the user
> >clicked/moved (using ES_OUT_VOUT_SET_MOUSE_EVENT). As explained above,
> >I
> >think it should remain in the rendering thread for now.
> >
> >> On 20 Aug 2020, at 21:46, Rémi Denis-Courmont <remi at remlab.net>
> >wrote:
> >>
> >> Le jeudi 20 août 2020, 21:29:48 EEST Steve Lhomme a écrit :
> >>>> On 2020-08-20 18:11, Rémi Denis-Courmont wrote:
> >>>> Hi,
> >>>>
> >>>> Badly designed or implemented plugins are what they are. Just as
> >big
> >>>> loops in the UI thread are bad for interactivity, big loops in the
> >>>> render thread are bad for timeliness. And besides those egregiously
> >>>> broken plugins you have, e.g., rotate and transform who actually
> >>>> translate the mouse event, and should obviously run synchronously -
> >not
> >>>> at the next render.
> >>>
> >>> Synchronously ? I have seen nothing in the UI thread, the one that
> >will
> >>> display the rendered result, that says a frame is allowed to be
> >>> displayed ASAP. It will wait until the next loop, in that thread.
> >>> Putting that in the UI thread is just pretending the rendering will
> >be
> >>> more responsive.
> >>
> >> Pretending? How do you need the rendering to translate mouse
> >coordinates
> >> through the transform filter? and eventually to pass mouse events to
> >the input?
> >>
> >> Even plotting the cursor does not need the VLC rendering. Only thing
> >that does
> >> is updating the Puzzle and such, not exactly everyday use cases.
> >>
> >> --
> >> Rémi Denis-Courmont
> >>
> >>
> >> _______________________________________________
> >> 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
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel