[PATCH weston 08/14 v3] weston: Port Wayland backend to new output handling API
Pekka Paalanen
ppaalanen at gmail.com
Tue Sep 27 14:04:46 UTC 2016
On Thu, 18 Aug 2016 18:42:36 +0200
Armin Krezović <krezovic.armin at 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.armin at gmail.com>
> ---
> compositor/main.c | 257 ++++++++++++--------------------
> libweston/compositor-wayland.c | 324 +++++++++++++++++++++++------------------
> libweston/compositor-wayland.h | 8 -
> 3 files changed, 279 insertions(+), 310 deletions(-)
Hi,
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.
> diff --git a/compositor/main.c b/compositor/main.c
> index 7007901..d2df568 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> static int
> -load_wayland_backend_config(struct weston_compositor *compositor, int *argc,
> - char *argv[], struct weston_config *wc,
> - struct weston_wayland_backend_config *config)
> +load_wayland_backend(struct weston_compositor *c,
> + int *argc, char **argv, struct weston_config *wc)
> {
> + struct weston_wayland_backend_config config = {{ 0, }};
> struct weston_config_section *section;
> - struct weston_wayland_backend_output_config *oc;
> - int count, width, height, scale;
> + const struct weston_windowed_output_api *api;
> const char *section_name;
> - char *name;
> + char *output_name = NULL;
> + int count = 1;
> + int ret = 0;
> + int i;
> +
> + struct wet_output_config *parsed_options = wet_init_parsed_options(c);
> + if (!parsed_options)
> + return -1;
> +
> + config.cursor_size = 32;
> + config.cursor_theme = NULL;
> + config.display_name = NULL;
> + config.fullscreen = 0;
> + config.sprawl = 0;
> + config.use_pixman = 0;
>
> const struct weston_option wayland_options[] = {
> - { WESTON_OPTION_INTEGER, "width", 0, &width },
> - { WESTON_OPTION_INTEGER, "height", 0, &height },
> - { WESTON_OPTION_INTEGER, "scale", 0, &scale },
> - { WESTON_OPTION_STRING, "display", 0, &config->display_name },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config->use_pixman },
> + { WESTON_OPTION_INTEGER, "width", 0, &parsed_options->width },
> + { WESTON_OPTION_INTEGER, "height", 0, &parsed_options->height },
> + { WESTON_OPTION_INTEGER, "scale", 0, &parsed_options->scale },
> + { WESTON_OPTION_STRING, "display", 0, &config.display_name },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> - { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &config->fullscreen },
> - { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config->sprawl },
> + { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &config.fullscreen },
> + { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config.sprawl },
> };
>
> - width = 0;
> - height = 0;
> - scale = 0;
> - config->display_name = NULL;
> - config->use_pixman = 0;
> - count = 1;
> - config->fullscreen = 0;
> - config->sprawl = 0;
> - parse_options(wayland_options,
> - ARRAY_LENGTH(wayland_options), argc, argv);
> -
> - config->cursor_size = 32;
> - config->cursor_theme = NULL;
> - config->base.struct_size = sizeof(struct weston_wayland_backend_config);
> - config->base.struct_version = WESTON_WAYLAND_BACKEND_CONFIG_VERSION;
> + parse_options(wayland_options, ARRAY_LENGTH(wayland_options), argc, argv);
>
> section = weston_config_get_section(wc, "shell", NULL, NULL);
> weston_config_section_get_string(section, "cursor-theme",
> - &config->cursor_theme, NULL);
> + &config.cursor_theme, NULL);
> weston_config_section_get_int(section, "cursor-size",
> - &config->cursor_size, 32);
> + &config.cursor_size, 32);
> +
> + config.base.struct_size = sizeof(struct weston_wayland_backend_config);
> + config.base.struct_version = WESTON_WAYLAND_BACKEND_CONFIG_VERSION;
> +
> + /* load the actual wayland backend and configure it */
> + ret = weston_compositor_load_backend(c, WESTON_BACKEND_WAYLAND,
> + &config.base);
> +
> + free(config.cursor_theme);
> + free(config.display_name);
> +
> + if (ret < 0)
> + return ret;
> +
> + api = weston_windowed_output_get_api(c);
> +
> + if (api == NULL) {
> + /* We will just assume if load_backend() finished cleanly and
> + * windowed_output_api is not present that wayland backend is
> + * started with --sprawl or runs on fullscreen-shell. */
Ah yes, the fullscreen shell must be detected by the backend. A very
good point.
> + wet_set_pending_output_handler(c, wayland_backend_output_configure_hotplug);
>
> - if (config->sprawl) {
> - /* do nothing, everything is already set */
> return 0;
> }
>
> - if (config->fullscreen) {
> - oc = weston_wayland_backend_config_add_new_output(config);
> - if (!oc)
> - return -1;
> + wet_set_pending_output_handler(c, wayland_backend_output_configure);
>
> - oc->width = width;
> - oc->height = height;
> - oc->name = NULL;
> - oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> - oc->scale = 1;
> + if (config.fullscreen) {
> + if (api->output_create(c, "wayland-fullscreen") < 0) {
> + weston_log("Cannot create wayland fullscreen output.\n");
> + return -1;
> + }
Using --fullscreen, there is actually nothing to be set via the
windowed output API, and it seems like
wayland_backend_output_configure() even causes some confusion by
calling the set_size entry. We could just use
wayland_backend_output_configure_hotplug() for the fullscreen case too.
>
> return 0;
> }
>
> section = NULL;
> while (weston_config_next_section(wc, §ion, §ion_name)) {
> - if (!section_name || strcmp(section_name, "output") != 0)
> + if (count == 0)
> + break;
> +
> + if (strcmp(section_name, "output") != 0) {
> continue;
> - weston_config_section_get_string(section, "name", &name, NULL);
> - if (name == NULL)
> + }
> +
> + weston_config_section_get_string(section, "name", &output_name, NULL);
> +
> + if (output_name == NULL)
> continue;
>
> - if (name[0] != 'W' || name[1] != 'L') {
> - free(name);
> + if (output_name[0] != 'W' || output_name[1] != 'L') {
> + free(output_name);
> continue;
> }
> - free(name);
>
> - oc = weston_wayland_backend_config_add_new_output(config);
> - if (!oc)
> + if (api->output_create(c, output_name) < 0) {
> + free(output_name);
Ah, this is using the windowed output API to create the single
fullscreen output. Could that be left to be done automatically in the
backend, so it can not advertise the API when fullscreen?
> return -1;
> + }
> + free(output_name);
>
> - weston_wayland_output_config_init(oc, section, width,
> - height, scale);
> --count;
> }
>
> - if (!width)
> - width = 1024;
> - if (!height)
> - height = 640;
> - if (!scale)
> - scale = 1;
> -
> - while (count > 0) {
> - oc = weston_wayland_backend_config_add_new_output(config);
> - if (!oc)
> + for (i = 0; i < count; i++) {
> + if (asprintf(&output_name, "WL%d", i) < 0)
I think this results very often in duplicate output names, but it
doesn't matter yet. Previously they didn't have a name at all.
> return -1;
>
> - oc->width = width;
> - oc->height = height;
> - oc->name = NULL;
> - oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> - oc->scale = scale;
> -
> - --count;
> + if (api->output_create(c, output_name) < 0) {
> + free(output_name);
> + return -1;
> + }
> + free(output_name);
> }
>
> return 0;
> }
>
> -static int
> -load_wayland_backend(struct weston_compositor *c,
> - int *argc, char **argv, struct weston_config *wc)
> -{
> - struct weston_wayland_backend_config config = {{ 0, }};
> - int ret = 0;
> -
> - ret = load_wayland_backend_config(c, argc, argv, wc, &config);
> - if (ret < 0) {
> - weston_wayland_backend_config_release(&config);
> - return ret;
> - }
> -
> - /* load the actual wayland backend and configure it */
> - ret = weston_compositor_load_backend(c, WESTON_BACKEND_WAYLAND,
> - &config.base);
> - weston_wayland_backend_config_release(&config);
> - return ret;
> -}
> -
>
> static int
> load_backend(struct weston_compositor *compositor, const char *backend,
> 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.
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.
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.
>
> - 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);
> - return 0;
> - }
> -
> - x = 0;
> - for (count = 0; count < new_config.num_outputs; ++count) {
> - output = wayland_output_create_for_config(b, &new_config.outputs[count],
> - 0, x, 0);
> - if (!output)
> - goto err_outputs;
> - if (wayland_output_set_windowed(output))
> - goto err_outputs;
> -
> - x += output->base.width;
> + if (ret < 0) {
> + weston_log("Failed to register output API.\n");
> + wayland_backend_destroy(b);
> + return -1;
> }
>
> weston_compositor_add_key_binding(compositor, KEY_F,
> MODIFIER_CTRL | MODIFIER_ALT,
> fullscreen_binding, b);
> return 0;
> -
> -err_outputs:
> - wayland_backend_destroy(b);
> - return -1;
> }
> diff --git a/libweston/compositor-wayland.h b/libweston/compositor-wayland.h
> index b705dee..68e7b0b 100644
> --- a/libweston/compositor-wayland.h
> +++ b/libweston/compositor-wayland.h
> @@ -36,14 +36,6 @@ extern "C" {
>
> #define WESTON_WAYLAND_BACKEND_CONFIG_VERSION 1
>
> -struct weston_wayland_backend_output_config {
> - int width;
> - int height;
> - char *name;
> - uint32_t transform;
> - int32_t scale;
> -};
> -
> struct weston_wayland_backend_config {
> struct weston_backend_config base;
> int use_pixman;
Seems you forgot to remove the 'outputs' member from this struct and
then bump WESTON_WAYLAND_BACKEND_CONFIG_VERSION. And 'num_outputs'.
Everything else looks good. Very close to landable, I think.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160927/133932c2/attachment.sig>
More information about the wayland-devel
mailing list