[PATCH weston 08/14 v3] weston: Port Wayland backend to new output handling API
Armin Krezović
krezovic.armin at gmail.com
Wed Sep 28 20:03:45 UTC 2016
On 27.09.2016 16:04, Pekka Paalanen wrote:
> 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,
>
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/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.
>
Sure.
>>
>> 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?
>
>
Yeah, it can be done and it should be done, as user can't manipulate
such output in any way. And in the current code, only one output is
created when --fullscreen is specified, so no need for creation API
either.
>
>> 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.
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). 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.
>
> 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 --^
>> - 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'.
>
Ooops. I'll fix that.
> Everything else looks good. Very close to landable, I think.
>
Thanks for the review!
>
> Thanks,
> pq
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 837 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160928/6e9e47fe/attachment-0001.sig>
More information about the wayland-devel
mailing list