Two issues were present with the previous implementation for interface
handling.
- Contrary to bin/vlc.c, darwinvlc.m needs to handle the CFRunLoop in
the main thread, but libvlc was released only after the runloop was
terminated, meaning that interfaces could not use the runloop on
close.
- Some application-specific runloop stop code was present to stop the
NSApp for the main macosx.m interface, with conditionals to check
whether the NSApp was started or not. darwinvlc.m shouldn't need to
know whether it needs to do that and should only handle the
termination of its own CFRunLoopRun() call, even if an interface
nests the runloop afterwards. As with the point above, the interface
should terminate its NSApp before darwinvlc terminates its runloop.
This commit reorder the resource deallocation to first remove the
interfaces, by releasing the libvlc instance, and only then destroy the
runloop. It uses previous changes in the macosx interface to handle the
stopping through libvlc_Quit.
By reordering this way, it's now possible to use SIGINT to stop the Qt
interface also.
On MacOS, the Quit shortcut (Command+Q/Ctrl+Q) is intercepted directly
by AppKit which emits an event on the NSApplicationDelegate bound to
NSApp.delegate. The event is caught by Qt Cocoa integration which sends
a closeEvent to the QApplication, and stops the Qt eventloop if the
closeEvent pass through.
This is problematic since we don't want to kill the eventloop right
away, and it provides a different behaviour when closing than other
ways (SIGINT/closing the window) where we redirect to libvlc_Quit() and
handle the termination of interfaces from their Deactivation callback
directly.
Instead this commit install a handler to catch Qt's closeEvent, on
every platforms, so as to redirect it to libvlc_Quit();
Co-authored-by: Pierre Lamot <pierre@videolabs.io>
The Qt window is created when code from qt.cpp's WindowOpen request the
initialisation of the vlc_window instance to the compositor, using
p_intf->p_compositor->setupVoutWindow( p_wnd, &WindowCloseCb );
WindowOpen, and the code in qt.cpp in general, handles the reference
counting of Qt's resources, hence why a destructor callback, WindowCloseCb,
is forwarded to the compositor. The compositor is unable to handle the
reference counting by itself.
When the window is destroyed, it goes through CompositorVideo callbacks
first, leading to WindowCloseCb being called from the compositor.
However, WindowCloseCb will release a reference and trigger the
destruction of the Compositor instance, and thus CompositorVideo, while
being in CompositorVideo::windowDestroy method.
In this case, it triggered use-after-free by writing m_wnd and m_destroyCb
on the CompositorVideo instance.
To prevent this, we can separate the window state from the
CompositorVideo state and modify the window destroy operation to first
notify the CompositorVideo that the window has been destroyed by its
client, and only then trigger the WindowCloseCB function from callback.
The separation of the window state is made by introducing a new
CompositorWindow structure, which is currently allocated statically in
the file since there is only ever a single window allocated, but we can
move to a dynamically allocated window state as soon as a dynamically
defined number of window is required just by changing the initialization
site and release the state in the window destroy operation.
This change simplify the constraints on CompositorVideo which doesn't
need to handle re-entrancy through its destructor, or state corruption
in the middle of the call to CompositorVideo::windowDestroy. It also
align the state handling so that a window being allocated triggers
reference increase from the window, and its destruction triggers
reference decrease from the window operation and state directly,
instead of another component tied to the reference count.
Co-authored-by: Fatih Uzunoglu <fuzun54@outlook.com>
Original discovery and fix for the UAF in MR !7241.
When binding the window to the compositor, there is no error code to
signal that allocation failed, but the code is already exception safe,
so handle it through exception handlers to avoid specific memory
handling path.
Avoids deadlocks when handling WindowEnable callback and setting of fullscreen variable on the windows' vout thread
Signed-off-by: Claudio Cambra <developer@claudiocambra.com>
It appears that d4c114ea revealed this bug in `ExpandGridView`.
I have not observed this before, because it was still dormant in
the home page without b1f3e4ef, which was added in the last minute.
Nevertheless, this should have been observable in the browse
page, but for some reason I have not observed it there either.
The new scrolling behavior has caused a lot of frustration, and
it completely broke scrolling when there are sections.
The old behavior with deceleration set to 10000 feels much better,
and fixes scrolling when there are view sections.
So that the fading edge effect does not clip the view. In fact, it still
"clips" but only outside the display margins, which I assume is wanted
anyway (see the last paragraph).
Due to the fundamental working of item layering, clipping is inevitable.
This is because in order to render an item offscreen, it is necessary to
know the boundaries of the offscreen buffer. Well, unless the buffer can
have infinite size, which I assume is not technically possible as that
would require infinite memory.
As a side note, the vertical artists list view stacked above in the left
side, and the playlist stacked above in the right side both have non-
opaque background so as to reveal window's background blur effect, but
they currently use a hack to not reveal any Qt Quick item rendered behind,
and even if this is changed in the future, we still do not want the items
in the horizontal view to appear behind. So, having the fading edge effect's
clipping is beneficial here. However, if in the future we stop using that
said hack, we need to have explicit clipping here even though the fading edge
effect clips the view, because the fading edge effect is disabled when
neither the beginning nor the end fade is enabled, which may have many
reasons including not having enough size to have fading effect, or, the
current item is the beginning item (so beginning fade is disabled) and
the user hovers the end item (so end fade is disabled).
The display "margin" grows outwards the viewport, this can also be seen in `FadingEdge`
where its `ShaderEffectSource` use the negative of the display margin for its position
(left and top margins). The hover areas must perfectly overlap with the fading areas,
we need this change to satisfy that.
I also fix mistakenly using beginning margin for the end hover area.
Normally we are not using preserve aspect crop in vertical list views, but
in this case since we want a perfect circle, we need to crop when necessary.
This is relevant for cover art that are not square.
This is to align with the grid views of other pages, where preserve aspect crop fill
mode is already used. Even though this is a horizontal list view, it uses the same
`GridItem` that is used in standard grid views, so the user naturally expects a
similar look here.
When `VLCStyle.isScreenSmall` is true, the artists pane can
not be resized. However since the horizontal resize handle is
still visible, the cursor changes and this makes it confusing
for the user whether the pane can actually be resized or not.
When `VLCStyle.isScreenSmall` is true, the playlist pane can
not be resized. However since the horizontal resize handle is
still visible, the cursor changes and this makes it confusing
for the user whether the pane can actually be resized or not.
Alexandre Janniaux
François Cartegnie
Claudio Cambra
Fatih Uzunoglu
Tristan Matthews
Astrom Cheng
Prince Gupta
Steve Lhomme
Pierre Lamot