[vlc-devel] [PATCH 1/4] libvlc: Add "fullscreen-monitor" setting
Alexandre Janniaux
ajanni at videolabs.io
Thu Oct 24 18:49:55 CEST 2019
Hi,
At the same time, I'm more or less against adding the
parameter in the window provider implementation. The
argument for adding it there is that it mimics the
audio_output provider behaviour, but basically for me:
+ with vout splitter, it doesn't provide what we want (make
every window fullscreen on the same screen) but it's a bit
an edge case as we want to control the window in this
situation, like I do, because it's currently only one video
output in the end.
+ with multiple video track enabled, there is 100% chance
that it is doing the wrong thing and provide bad UX, as the
window provider would load the settings but doesn't know
about other video outputs.
+ it means something low level (window
providing/interaction) also handle user preferences.
+ it duplicates parameters, which makes life difficult for
the user.
+ there is almost no gain for it, and only in edgecases.
I'm more in favour of something like your patch -- either in
libvlc-modules, meaning the core, or in Qt settings.
The main reason for libvlc-modules.c is that in the general
playback case, user doesn't switch between window providers,
so the setting is meaningful in any case. A fullscreen
device set on a different window provider is exactly like a
fullscreen device set on a device that was unplugged. You
don't want to loose the preference, but the user can change
it for a device on the new window provider.
In addition, you cannot set a device for a window provider
that isn't the current one, except by writing the name by
hand, because it won't report device for the other window
provider. Your only possible guess is that the user
switching window provider also named the different output
the same way.
Finally, the use case is:
+ the user go to preferences -> video.
+ it sets the current fullscreen device according to the
current window providing method (which, as highlighted by
Steve, is the current behaviour but only works with Qt for
now).
+ then the UI will transmit this parameter to the main vout,
and the main vout also inherit from it if wasn't created
yet, so it works as expected from any user point of view.
I agree that module-specific variable would be great in
addition but it seems quite difficult to achieve, to me.
ePirat had made nice suggestions around this, and the
solution might come from a libvlc-module variable for global
settings, combined with the ability to store module-specific
override. Something like --fullscreen-monitor= and
--fullscreen-monitor:win32= parameters. This seems a lot
saner to me, but isn't available and probably needs
refinement or alternative plan.
As conclusion, I think your implementation is fine for a
sane system, or that we're missing another part of the
scheme which is neither interface nor window provider.
Thank you for improving this discussion with your point of
view, I really appreciate your contribution and I hope we
can reach clean consensus.
Regards,
--
Alexandre Janniaux
Videolabs
On Thu, Oct 24, 2019 at 11:39:02AM -0400, Gabriel Luci wrote:
> I think you misunderstood my question. This entire patchset was about
> setting it in the preferences before playback starts. There should be one
> *setting*, yes. But, from what I see, there are two ways to implement it.
> (That's what I meant by "2 options")
>
> In this patch, I defined a global setting in src/libvlc-module.c. Rémi
> objected to that approach and said the setting should be defined in the
> window manager (I assume that means in win32/window.c). But if it's defined
> there, the setting should be *read* in win32/window.c too.
>
> My objection to that is that Rémi himself had added an "id" parameter to
> the vout_ChangeFullscreen() last year, which currently is always NULL. That
> seems to be the place to pass the ID of the monitor to put the fullscreen
> window on. If I'm correct on that, then the setting needs to be read in
> src/video_output/vout_intf.c and thus it should be a global setting.
>
> And that's where my confusion is. If it really should be a
> window-provider-specific setting, then why does vout_ChangeFullscreen()
> have an "id" parameter?
>
> If it should be in the window provider, that's fine, and I'm happy to do
> it. There just doesn't seem to be consensus here, so I don't know which
> direction to go.
>
> On Thu, Oct 24, 2019 at 11:13 AM Steve Lhomme <robux4 at ycbcr.xyz> wrote:
>
> > On 2019-10-24 16:57, Gabriel Luci wrote:
> > > Following up on this: am I correct in my understanding that those are
> > > our only 2 options?
> >
> > IMO there should be only one option. Either you set it in preferences
> > before you start playback or you can change dynamically it from the UI.
> > The second part it not handled for the moment AFAIK.
> >
> > > On Thu, Oct 17, 2019 at 5:26 PM Gabriel Luci <github at luci.ca
> > > <mailto:github at luci.ca>> wrote:
> > >
> > > I do understand the difference between the two, but they are
> > > dependent on each other, no?
> > >
> > > From my inexperienced perspective, I see two options:
> > >
> > > 1. Use a global setting, read it in FullscreenCallback(), and pass
> > > it to vout_ChangeFullscreen() (which is what I did in this patchset)
> > > or,
> > > 2. Define the setting in the window provider plugin, but then that
> > > means (correct me if I'm wrong) the window provider plugin also has
> > > the job of reading that setting in its SetFullscreen function. But
> > > then (I think) that means that there is no purpose for the id
> > > parameter in vout_ChangeFullscreen(), vout_window_SetFullScreen(),
> > > and the window provider SetFullscreen functions.
> > >
> > > That's why I went with option 1, because I saw the id parameter
> > > there doing nothing, so I figured that's what it was supposed to be
> > > for, which led me to believe a global setting was the only way to go.
> > >
> > > On Thu, Oct 17, 2019 at 3:44 PM Rémi Denis-Courmont <remi at remlab.net
> > > <mailto:remi at remlab.net>> wrote:
> > >
> > > Le torstaina 17. lokakuuta 2019, 19.26.56 EEST Gabriel Luci a
> > > écrit :
> > > > I'm happy to make that change. However, then I'm not sure how
> > > to use the
> > > > setting. vout_ChangeFullscreen accepts an id parameter, which
> > > is supposed
> > > > to be the "output identifier", which I assume means the ID of
> > > the monitor?
> > > > (correct me if I'm wrong) So how do we find that value when
> > > calling
> > > > vout_ChangeFullscreen if only the window provider knows? Or
> > am I
> > > > misunderstanding something there?
> > >
> > > I think you're mixing up two things here - changing the
> > > fullscreen output used
> > > by an active window which uses vout_ChangeFullscreen(), versus
> > > changing the
> > > default fullscreen output for future windows which uses window
> > > provider-
> > > dependent mechanism - a setting in Qt and XDG-shell cases.
> > >
> > > --
> > > Rémi Denis-Courmont
> > > 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list