On Wed, 28 Sep 2016 22:03:45 +0200 Armin Krezović <krezovic.ar...@gmail.com> wrote:
> On 27.09.2016 16:04, Pekka Paalanen wrote:
> > On Thu, 18 Aug 2016 18:42:36 +0200
> > Armin Krezović <krezovic.ar...@gmail.com> wrote:
> >
> >> This is a complete port of the Wayland backend that
> >> uses recently added output handling API for output
> >> configuration.
> >>
> >> - Output can be configured at runtime by passing the
> >> necessary configuration parameters, which can be
> >> filled in manually, obtained from the configuration
> >> file or obtained from the command line using
> >> previously added functionality. It is required that
> >> the scale and transform values are set using the
> >> previously added functionality.
> >>
> >> - Output can be created at runtime using the output
> >> API. The output creation only creates a pending
> >> output, which needs to be configured the same way as
> >> mentioned above.
> >>
> >> However, the backend can behave both as windowed backend
> >> and as a backend that issues "hotplug" events, when
> >> running under fullscreen shell or with --sprawl command
> >> line option. The first case was covered by reusing
> >> previously added functionality. The second case required
> >> another API to be introduced and implemented into both
> >> the backend and compositor for handling output setup.
> >>
> >> After everything has been set, output needs to be
> >> enabled manually using weston_output_enable().
> >>
> >> v2:
> >>
> >> - Fix wet_configure_windowed_output_from_config() usage.
> >> - Call wayland_output_disable() explicitly from
> >> wayland_output_destroy().
> >>
> >> v3:
> >>
> >> - Get rid of weston_wayland_output_api and rework output
> >> creation and configuration in case wayland backend is
> >> started with --sprawl or on fullscreen-shell.
> >> - Remove unneeded free().
> >> - Disallow calling wayland_output_configure more than once.
> >> - Remove unneeded checks for output->name == NULL as that
> >> has been disallowed.
> >> - Use weston_compositor_add_pending_output().
> >>
> >> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
> >> ---
> >> compositor/main.c | 257 ++++++++++++--------------------
> >> libweston/compositor-wayland.c | 324
> >> +++++++++++++++++++++++------------------
> >> libweston/compositor-wayland.h | 8 -
> >> 3 files changed, 279 insertions(+), 310 deletions(-)
> >
> > Hi,
> >
>
> Salut,
>
> > I tested this quickly, hosted on weston/x11 with desktop-shell since
> > fullscreen-shell does not manage to show anything either before or
> > after these patches.
> >
> > While --sprawl works fine (tried with parent output-count=2),
> > --fullscreen is getting a wrong size. More comments on that inline.
> >
>
> Thanks for the review and testing! I couldn't catch all the cases it seems!
> >> diff --git a/libweston/compositor-wayland.c
> >> b/libweston/compositor-wayland.c
> >> index 7c12b4c..5fa6cfd 100644
> >> --- a/libweston/compositor-wayland.c
> >> +++ b/libweston/compositor-wayland.c
> >
> >> @@ -1038,92 +1042,167 @@ wayland_output_create(struct wayland_backend *b,
> >> int x, int y,
> >> output->parent.surface);
> >> if (!output->parent.shell_surface)
> >> goto err_surface;
> >> +
> >> wl_shell_surface_add_listener(output->parent.shell_surface,
> >> &shell_surface_listener, output);
> >> }
> >>
> >> - if (fullscreen && b->parent.shell) {
> >> - wl_shell_surface_set_fullscreen(output->parent.shell_surface,
> >> - 0, 0, NULL);
> >> - wl_display_roundtrip(b->parent.wl_display);
> >> - if (!width)
> >> - output_width = output->parent.configure_width;
> >> - if (!height)
> >> - output_height = output->parent.configure_height;
> >> - }
> >> -
> >> - output->mode.flags =
> >> - WL_OUTPUT_MODE_CURRENT | WL_OUTPUT_MODE_PREFERRED;
> >> - output->mode.width = output_width;
> >> - output->mode.height = output_height;
> >> - output->mode.refresh = 60000;
> >> - output->scale = scale;
> >> - wl_list_init(&output->base.mode_list);
> >> - wl_list_insert(&output->base.mode_list, &output->mode.link);
> >> - output->base.current_mode = &output->mode;
> >> -
> >> wl_list_init(&output->shm.buffers);
> >> wl_list_init(&output->shm.free_buffers);
> >>
> >> - weston_output_init(&output->base, b->compositor, x, y, width, height,
> >> - transform, scale);
> >> -
> >> if (b->use_pixman) {
> >> if (wayland_output_init_pixman_renderer(output) < 0)
> >> goto err_output;
> >> - output->base.repaint = wayland_output_repaint_pixman;
> >> } else {
> >> if (wayland_output_init_gl_renderer(output) < 0)
> >> goto err_output;
> >> - output->base.repaint = wayland_output_repaint_gl;
> >> }
> >>
> >> - output->base.start_repaint_loop = wayland_output_start_repaint_loop;
> >> - output->base.destroy = wayland_output_destroy;
> >> - output->base.assign_planes = NULL;
> >> - output->base.set_backlight = NULL;
> >> - output->base.set_dpms = NULL;
> >> - output->base.switch_mode = wayland_output_switch_mode;
> >> + if (b->sprawl_across_outputs) {
> >> + wayland_output_set_fullscreen(output,
> >> +
> >> WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER,
> >> + output->mode.refresh,
> >> output->parent.output);
> >> +
> >> + if (output->parent.shell_surface) {
> >> +
> >> wl_shell_surface_set_fullscreen(output->parent.shell_surface,
> >> +
> >> WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER,
> >> + output->mode.refresh,
> >> output->parent.output);
> >> + } else if (b->parent.fshell) {
> >> +
> >> zwp_fullscreen_shell_v1_present_surface(b->parent.fshell,
> >> +
> >> output->parent.surface,
> >> +
> >> ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_CENTER,
> >> +
> >> output->parent.output);
> >> + zwp_fullscreen_shell_mode_feedback_v1_destroy(
> >> +
> >> zwp_fullscreen_shell_v1_present_surface_for_mode(b->parent.fshell,
> >> +
> >> output->parent.surface,
> >> +
> >> output->parent.output,
> >> +
> >> output->mode.refresh));
> >> + }
> >> + } else if (b->fullscreen) {
> >> + wayland_output_set_fullscreen(output, 0, 0, NULL);
> >>
> >> - weston_compositor_add_output(b->compositor, &output->base);
> >> + if (output->parent.shell_surface) {
> >> +
> >> wl_shell_surface_set_fullscreen(output->parent.shell_surface,
> >> + 0, 0, NULL);
> >> + wl_display_roundtrip(b->parent.wl_display);
> >> + }
> >
> > Something here does not quite add up. I cannot find where the
> > wayland_output_set_fullscreen() call came from.
>
>
> See below (**).
>
> >
> > In the old code, the fullscreening worked probably something like this:
> > 1. create a wl_surface
> > 2. create a shell_surface for the wl_surface
> > 3. make the shell_surface fullscreen
> > 4. wait for the configure event to come with the size
> > (wl_display_roundtrip())
> > 5. Use the size given by the parent compositor
> > (output->parent.configure_width,height)
> >
> > That is different from the fullscreening-in-flight code, which is what
> > wayland_output_set_fullscreen() is for, I believe.
> > Fullscreening-in-flight is essentially the same except it does not wait
> > for the configure event and so the using the new size happens some time
> > later.
> >
> > Calling wayland_output_set_fullscreen(output, 0, 0, NULL) here is
> > trying to resize to the current mode and then it does the same call to
> > wl_shell_surface_set_fullscreen() as written here.
> >
> > Since we are not doing any on-the-fly enablement, I think we should
> > keep the old behaviour as close as possible. Either in
> > wayland_output_enable() or right before calling
> > weston_compositor_add_pending_output() you could do the protocol setup
> > and wait for the configure event when fullscreen. Well, the wait is
> > here, it's just the call to wayland_output_set_fullscreen() that seems
> > redundant to me.
>
> I'm not sure about redundant wayland_output_set_fullscreen() call, as I've
> closely tried to replicate the old behaviour which included the call
> (although,
> on a different place).
Hi!
Oh, I missed that.
> I've certainly failed to fetch the output size from fs
> surface size, as it was done before, but that will be fixed in the next
> iteration.
>
> Besides that, I believe I've swapped wl_shell_surface_set_fullscreen()
> and wayland_output_set_fullscreen() calls.
Hmm, ok. Let's see what the next round looks like.
> >
> > Sprawl or fullscreen-shell is not affected because it uses the
> > wl_output information, which is waited for in the backend init already.
> >
> >> + } else {
> >> + wayland_output_set_windowed(output);
> >> + }
> >>
> >> - return output;
> >> + return 0;
> >>
> >> err_output:
> >> - weston_output_destroy(&output->base);
> >> if (output->parent.shell_surface)
> >> wl_shell_surface_destroy(output->parent.shell_surface);
> >> err_surface:
> >> wl_surface_destroy(output->parent.surface);
> >> -err_name:
> >> - free(output->name);
> >>
> >> - /* FIXME: cleanup weston_output */
> >> - free(output);
> >> -
> >> - return NULL;
> >> + return -1;
> >> }
> >
> >> @@ -2324,38 +2393,17 @@ backend_init(struct weston_compositor *compositor,
> >> return 0;
> >> }
> >>
> >> - if (new_config.fullscreen) {
> >> - if (new_config.num_outputs != 1 || !new_config.outputs)
> >> - goto err_outputs;
> >> + ret = weston_plugin_api_register(compositor,
> >> WESTON_WINDOWED_OUTPUT_API_NAME,
> >> + &windowed_api, sizeof(windowed_api));
> >
> > If fullscreen, the windowed output API does not actually offer anything
> > usable, so might as well not register it in that case. We haven't
> > supported adding multiple outputs in the fullscreen case before either.
> >
>
> Yeah, we've discussed this on IRC already, and have agreed to let the backend
> create and configure an output when --fullscreen is specified (same case as
> when started with --sprawl).
>
> >>
> >> - output = wayland_output_create_for_config(b,
> >> -
> >> &new_config.outputs[0],
> >> - 1, 0, 0);
> >
> > That means creating the single fullscreen output should stay.
> >
> >> - if (!output)
> >> - goto err_outputs;
> >> -
> >> - wayland_output_set_fullscreen(output, 0, 0, NULL);
>
> (**) See here --^
So this is where it came from. I suspect the old code could already be
slightly confused, because part of wayland_output_set_fullscreen()
makes the direct call to wl_shell_surface_set_fullscreen() redundant.
But, we do need to wl_display_roundtrip() to ensure that
output->parent.configure_width and _height are populated, so the
redundancy is probably intentional.
I think there might a an issue with the execution order.
wayland_output_set_fullscreen() calls wayland_output_resize_surface(),
but we don't have the right size until after the wl_display_roundtrip().
The old code handled it in wayland_output_create() directly doing the
wl_shell_surface_set_fullscreen() and wl_display_roundtrip() before
initializing the mode for the output, so that the following call to
resize via wayland_output_set_fullscreen() actually has the right size.
So I think what you said in IRC is right: the call to
wayland_output_set_fullscreen() in the enable function should be after
the call to wl_shell_surface_set_fullscreen(), *and* it was missing the
size assignment.
Thanks,
pq
pgpoCokHrx3KV.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
