[PATCH weston 08/12 v2] weston: Port Wayland backend to new output handling API
Armin Krezović
krezovic.armin at gmail.com
Wed Aug 17 17:49:06 UTC 2016
On 17.08.2016 17:07, Pekka Paalanen wrote:
> On Sun, 14 Aug 2016 17:28:17 +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().
>>
>> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
>> ---
>> compositor/main.c | 275 +++++++++++++++-------------------
>> libweston/compositor-wayland.c | 327 +++++++++++++++++++++++------------------
>> libweston/compositor-wayland.h | 27 +++-
>> 3 files changed, 325 insertions(+), 304 deletions(-)
>>
>> diff --git a/compositor/main.c b/compositor/main.c
>> index a72c2f9..143ee21 100644
>> --- a/compositor/main.c
>> +++ b/compositor/main.c
>> @@ -1568,218 +1568,177 @@ out:
>> }
>>
>> static void
>> -weston_wayland_output_config_init(struct weston_wayland_backend_output_config *output_config,
>> - struct weston_config_section *config_section,
>> - int option_width, int option_height,
>> - int option_scale)
>> +wayland_backend_output_configure_hotplug(struct wl_listener *listener, void *data)
>
> This new function looks like it matches the old behaviour, good.
>
>> {
>> - char *mode, *t, *str;
>> - unsigned int slen;
>> -
>> - weston_config_section_get_string(config_section, "name", &output_config->name,
>> - NULL);
>> - if (output_config->name) {
>> - slen = strlen(output_config->name);
>> - slen += strlen(WINDOW_TITLE " - ");
>> - str = malloc(slen + 1);
>> - if (str)
>> - snprintf(str, slen + 1, WINDOW_TITLE " - %s",
>> - output_config->name);
>> - free(output_config->name);
>> - output_config->name = str;
>> - }
>> - if (!output_config->name)
>> - output_config->name = strdup(WINDOW_TITLE);
>> -
>> - weston_config_section_get_string(config_section,
>> - "mode", &mode, "1024x600");
>> - if (sscanf(mode, "%dx%d", &output_config->width, &output_config->height) != 2) {
>> - weston_log("Invalid mode \"%s\" for output %s\n",
>> - mode, output_config->name);
>> - output_config->width = 1024;
>> - output_config->height = 640;
>> - }
>> - free(mode);
>> + struct weston_output *output = data;
>>
>> - if (option_width)
>> - output_config->width = option_width;
>> - if (option_height)
>> - output_config->height = option_height;
>> + const struct weston_wayland_output_api *api =
>> + weston_wayland_output_get_api(output->compositor);
>>
>> - weston_config_section_get_int(config_section, "scale", &output_config->scale, 1);
>> + if (!api) {
>> + weston_log("Cannot use weston_wayland_output_api.\n");
>> + return;
>> + }
>>
>> - if (option_scale)
>> - output_config->scale = option_scale;
>> + weston_output_set_scale(output, 1);
>> + weston_output_set_transform(output, WL_OUTPUT_TRANSFORM_NORMAL);
>>
>> - weston_config_section_get_string(config_section,
>> - "transform", &t, "normal");
>> - if (weston_parse_transform(t, &output_config->transform) < 0)
>> - weston_log("Invalid transform \"%s\" for output %s\n",
>> - t, output_config->name);
>> - free(t);
>> + if (api->output_configure(output) < 0) {
>> + weston_log("Cannot configure an output using weston_wayland_output_api.\n");
>> + return;
>> + }
>>
>> + weston_output_enable(output);
>> }
>>
>> static void
>> -weston_wayland_backend_config_release(struct weston_wayland_backend_config *config)
>> -{
>> - int i;
>> -
>> - for (i = 0; i < config->num_outputs; ++i) {
>> - free(config->outputs[i].name);
>> - }
>> - free(config->cursor_theme);
>> - free(config->display_name);
>> - free(config->outputs);
>> -}
>> -
>> -/*
>> - * Append a new output struct at the end of new_config.outputs and return a
>> - * pointer to the newly allocated structure or NULL if fail. The allocated
>> - * structure is NOT cleared nor set to default values.
>> - */
>> -static struct weston_wayland_backend_output_config *
>> -weston_wayland_backend_config_add_new_output(struct weston_wayland_backend_config *config)
>> +wayland_backend_output_configure(struct wl_listener *listener, void *data)
>> {
>> - struct weston_wayland_backend_output_config *outputs;
>> - const size_t element_size = sizeof(struct weston_wayland_backend_output_config);
>> + struct weston_output *output = data;
>> + struct wet_output_config defaults = {
>> + .width = 1024,
>> + .height = 640,
>> + .scale = 1,
>> + .transform = WL_OUTPUT_TRANSFORM_NORMAL
>> + };
>>
>> - outputs = realloc(config->outputs,
>> - (config->num_outputs + 1) * element_size);
>> - if (!outputs)
>> - return NULL;
>> - config->num_outputs += 1;
>> - config->outputs = outputs;
>> - return &(config->outputs[config->num_outputs - 1]);
>> + if (wet_configure_windowed_output_from_config(output, &defaults) < 0)
>> + weston_log("Cannot configure output \"%s\".\n",
>> + output->name ? output->name : "unnamed");
>
> I think wayland_backend_output_configure() is missing a special case
> for --fullscreen. That is, desktop shell (windowed api) but asked to be
> fullscreen...
>
> Actually no, this is fine. It would just need a comment explaining that
> the backend will override the size if --fullscreen was given. I see
> this would be inconvenient to realize otherwise.
>
>
The backend would only set the surface fullscreen. I don't know what it will
do to the size though, the values aren't touched, at least not that I saw it.
>> }
>>
>> 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)
>
> The new load_wayland_backend() looks quite clean, in spite of some
> mixed code and declarations issue, compared to the old
> load_wayland_backend_config(). That's nice.
>
>> {
>> + 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 struct weston_wayland_output_api *wl_api;
>> + int count = 1;
>> + int ret = 0;
>> const char *section_name;
>> - char *name;
>> +
>> + 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)
>> + goto err;
>> +
>> + api = weston_windowed_output_get_api(c);
>> +
>> + if (api == NULL) {
>> + /* Try wayland API */
>> + wl_api = weston_wayland_output_get_api(c);
>> +
>> + if (wl_api == NULL) {
>> + weston_log("Cannot bind to any output configuration API.\n");
>> + ret = -1;
>> + goto err;
>> + }
>> +
>> + 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 (!api) {
>> + weston_log("Cannot use weston_windowed_output_api.\n");
>> + ret = -1;
>> + goto err;
>> + }
>>
>> + if (config.fullscreen) {
>> + if (api->output_create(c, WINDOW_TITLE) < 0) {
>> + ret = -1;
>> + goto err;
>> + }
>> return 0;
>> }
>>
>> section = NULL;
>> while (weston_config_next_section(wc, §ion, §ion_name)) {
>> - if (!section_name || strcmp(section_name, "output") != 0)
>> + char *output_name;
>> +
>> + 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)
>> - return -1;
>> + if (api->output_create(c, output_name) < 0) {
>> + ret = -1;
>> + free(output_name);
>> + goto err;
>> + }
>> + 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)
>> - return -1;
>> -
>> - oc->width = width;
>> - oc->height = height;
>> - oc->name = NULL;
>> - oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
>> - oc->scale = scale;
>> + if (api->output_create(c, WINDOW_TITLE) < 0) {
>
> I don't like too much about creating several outputs with the same
> name, but considering they previously had no name at all, it's ok.
> Ideally all outputs would be uniquely named "WL#" with a number.
>
> Looks like there is some confusion in output names vs. window title
> left over from the previous restructuring, but that's an issue for
> another time. Output name should not include WINDOW_TITLE. The Wayland
> backend will compose the window title from WINDOW_TITLE and output name.
>
Right.
>> + ret = -1;
>> + goto err;
>> + }
>>
>> --count;
>> }
>>
>> return 0;
>> -}
>> +err:
>> + free(parsed_options);
>
> wet_init_parsed_options() put the pointer also into
> wet_compositor::parsed_options. After this free, that pointer will be
> stale.
>
I suppose this was a leftover since parsed_options was made the way it
currently is. Good eye man.
>>
>> -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;
>> }
>>
>> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
>> index 7c12b4c..4b0113f 100644
>> --- a/libweston/compositor-wayland.c
>> +++ b/libweston/compositor-wayland.c
>> @@ -51,6 +51,7 @@
>> #include "fullscreen-shell-unstable-v1-client-protocol.h"
>> #include "presentation-time-server-protocol.h"
>> #include "linux-dmabuf.h"
>> +#include "windowed-output-api.h"
>>
>> #define WINDOW_TITLE "Weston Compositor"
>>
>> @@ -74,6 +75,7 @@ struct wayland_backend {
>>
>> int use_pixman;
>> int sprawl_across_outputs;
>> + int fullscreen;
>>
>> struct theme *theme;
>> cairo_device_t *frame_device;
>> @@ -119,6 +121,9 @@ struct wayland_output {
>>
>> struct weston_mode mode;
>> uint32_t scale;
>> +
>> + struct weston_mode *poutput_mode;
>
> Hmm, can't you use base.current_mode instead of adding this?
>
>> + void *user_data;
>
> This is an internal struct not exposed to any other component, so
> seeing "user_data" is strange. Can't you use the proper pointer type
> with a descriptive name?
>
> Seems like it is only ever used to hold a pointer to a parent output,
> so it should be:
> struct wayland_parent_output *parent_output;
>
>
Yeah, that makes sense. If I do that, I can get rid of poutput_mode, too.
>> };
>>
>> struct wayland_parent_output {
>> @@ -616,21 +621,24 @@ wayland_output_repaint_pixman(struct weston_output *output_base,
>> return 0;
>> }
>>
>> -static void
>> -wayland_output_destroy(struct weston_output *output_base)
>> +static int
>> +wayland_output_disable(struct weston_output *base)
>> {
>> - struct wayland_output *output = to_wayland_output(output_base);
>> - struct wayland_backend *b =
>> - to_wayland_backend(output->base.compositor);
>> + struct wayland_output *output = to_wayland_output(base);
>> + struct wayland_backend *b = to_wayland_backend(base->compositor);
>> +
>> + if (!output->base.enabled)
>> + return 0;
>>
>> if (b->use_pixman) {
>> - pixman_renderer_output_destroy(output_base);
>> + pixman_renderer_output_destroy(&output->base);
>> } else {
>> - gl_renderer->output_destroy(output_base);
>> + gl_renderer->output_destroy(&output->base);
>> }
>>
>> wl_egl_window_destroy(output->gl.egl_window);
>> wl_surface_destroy(output->parent.surface);
>> +
>> if (output->parent.shell_surface)
>> wl_shell_surface_destroy(output->parent.shell_surface);
>>
>> @@ -642,10 +650,18 @@ wayland_output_destroy(struct weston_output *output_base)
>> cairo_surface_destroy(output->gl.border.right);
>> cairo_surface_destroy(output->gl.border.bottom);
>
> Ehheh, looks like someone may have forgot to destroy shm.buffers and
> shm.free_buffers on shutdown. Well, not your problem.
>
>>
>> + return 0;
>> +}
>> +
>> +static void
>> +wayland_output_destroy(struct weston_output *base)
>> +{
>> + struct wayland_output *output = to_wayland_output(base);
>> +
>> + wayland_output_disable(&output->base);
>> weston_output_destroy(&output->base);
>> - free(output);
>>
>> - return;
>> + free(output);
>> }
>>
>> static const struct wl_shell_surface_listener shell_surface_listener;
>> @@ -1002,32 +1018,21 @@ err_output:
>> return -1;
>> }
>>
>> -static struct wayland_output *
>> -wayland_output_create(struct wayland_backend *b, int x, int y,
>> - int width, int height, const char *name, int fullscreen,
>> - uint32_t transform, int32_t scale)
>> +static int
>> +wayland_output_enable(struct weston_output *base)
>> {
>> - struct wayland_output *output;
>> - int output_width, output_height;
>> + struct wayland_output *output = to_wayland_output(base);
>> + struct wayland_backend *b = to_wayland_backend(base->compositor);
>>
>> weston_log("Creating %dx%d wayland output at (%d, %d)\n",
>> - width, height, x, y);
>> -
>> - output = zalloc(sizeof *output);
>> - if (output == NULL)
>> - return NULL;
>> -
>> - output->name = name ? strdup(name) : NULL;
>> - output->base.make = "wayland";
>> - output->base.model = "none";
>> -
>> - output_width = width * scale;
>> - output_height = height * scale;
>> + output->base.mm_width, output->base.mm_height,
>
> This used to be the logical size...
>
> The old weston_output_init() has been called before we get here, so you
> can dig up the logical size from weston_output::width and height IIRC.
>
Thanks for the tip.
>> + output->base.x, output->base.y);
>>
>> output->parent.surface =
>> wl_compositor_create_surface(b->parent.compositor);
>> if (!output->parent.surface)
>> - goto err_name;
>> + return -1;
>> +
>> wl_surface_set_user_data(output->parent.surface, output);
>>
>> output->parent.draw_initial_frame = 1;
>> @@ -1038,118 +1043,169 @@ 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) {
>> + if (!b->sprawl_across_outputs && b->fullscreen && b->parent.shell) {
>
> Hmm, fullscreen was limited to exactly one output, so sprawl with it
> does not make sense... I suppose this hunk is ok, but still looks a
> little out of place in this patch. What prompted
> adding !b->sprawl_across_outputs && ?
>
sprawl_across_outputs is set for both fs_shell and --sprawl, which is handled
down below and a bit different. This is supposed to be a safeguard, since one
can pass both --sprawl and --fullscreen, and all that while on desktop 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;
>> -
>> - weston_compositor_add_output(b->compositor, &output->base);
>> + if (b->sprawl_across_outputs) {
>
> Aha, sprawl_across_outputs is used to indicate the
> weston_output_create_for_parent() case. It doesn't strike obvious to
> me, though it does seem correct.
>
>> + wayland_output_set_fullscreen(output,
>> + WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER,
>> + output->poutput_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->poutput_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->poutput_mode->refresh));
>> + }
>
> All the above looks somehow very fishy, but I see that's how it already
> was, so... not a problem for this time.
>
>> + } else if (b->fullscreen) {
>> + wayland_output_set_fullscreen(output, 0, 0, NULL);
>> + } else {
>> + wayland_output_set_windowed(output);
>> + }
>
> These look ok.
>
>>
>> - 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;
>> }
>>
>> static struct wayland_output *
>> -wayland_output_create_for_config(struct wayland_backend *b,
>> - struct weston_wayland_backend_output_config *oc,
>> - int fullscreen, int32_t x, int32_t y)
>> +wayland_output_create_common(void)
>> {
>> struct wayland_output *output;
>>
>> - output = wayland_output_create(b, x, y, oc->width, oc->height, oc->name,
>> - fullscreen, oc->transform, oc->scale);
>> + output = zalloc(sizeof *output);
>> + if (output == NULL) {
>> + perror("zalloc");
>> + return NULL;
>> + }
>> +
>> + output->base.destroy = wayland_output_destroy;
>> + output->base.disable = wayland_output_disable;
>> + output->base.enable = wayland_output_enable;
>>
>> return output;
>> }
>>
>> -static struct wayland_output *
>> -wayland_output_create_for_parent_output(struct wayland_backend *b,
>> - struct wayland_parent_output *poutput)
>> +static int
>> +wayland_output_create(struct weston_compositor *compositor, const char *name)
>> {
>> - struct wayland_output *output;
>> - struct weston_mode *mode;
>> - int32_t x;
>> + struct wayland_output *output = wayland_output_create_common();
>> +
>> + output->base.name = name ? strdup(name) : NULL;
>> +
>> + weston_output_init_pending(&output->base, compositor);
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +wayland_output_configure(struct weston_output *base, int width, int height)
>> +{
>> + struct wayland_output *output = to_wayland_output(base);
>> + struct wayland_backend *b = to_wayland_backend(base->compositor);
>> +
>> + int output_width, output_height;
>> +
>> + if (width < 1) {
>> + weston_log("Invalid width \"%d\" for output %s\n",
>> + width, output->base.name);
>> + return -1;
>> + }
>> +
>> + if (height < 1) {
>> + weston_log("Invalid height \"%d\" for output %s\n",
>> + height, output->base.name);
>> + return -1;
>> + }
>> +
>> + output_width = width * output->base.scale;
>> + output_height = height * output->base.scale;
>> +
>> + 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 = output->base.scale;
>> + wl_list_init(&output->base.mode_list);
>> + wl_list_insert(&output->base.mode_list, &output->mode.link);
>> +
>> + output->base.mm_width = width;
>> + output->base.mm_height = height;
>> +
>> + if (b->use_pixman)
>> + output->base.repaint = wayland_output_repaint_pixman;
>> + else
>> + output->base.repaint = wayland_output_repaint_gl;
>> +
>> + output->base.start_repaint_loop = wayland_output_start_repaint_loop;
>> + output->base.assign_planes = NULL;
>> + output->base.set_backlight = NULL;
>> + output->base.set_dpms = NULL;
>> + output->base.switch_mode = wayland_output_switch_mode;
>> + output->base.current_mode = &output->mode;
>> + output->base.make = "wayland";
>> + output->base.model = "none";
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +wayland_output_configure_hotplug(struct weston_output *base)
>> +{
>> + struct wayland_output *output = to_wayland_output(base);
>> + struct wayland_parent_output *poutput = output->user_data;
>>
>> if (poutput->current_mode) {
>> - mode = poutput->current_mode;
>> + output->poutput_mode = poutput->current_mode;
>> } else if (poutput->preferred_mode) {
>> - mode = poutput->preferred_mode;
>> + output->poutput_mode = poutput->preferred_mode;
>> } else if (!wl_list_empty(&poutput->mode_list)) {
>> - mode = container_of(poutput->mode_list.next,
>> - struct weston_mode, link);
>> - } else {
>> - weston_log("No valid modes found. Skipping output\n");
>> - return NULL;
>> - }
>> -
>> - if (!wl_list_empty(&b->compositor->output_list)) {
>> - output = container_of(b->compositor->output_list.prev,
>> - struct wayland_output, base.link);
>> - x = output->base.x + output->base.current_mode->width;
>> + output->poutput_mode = container_of(poutput->mode_list.next,
>> + struct weston_mode, link);
>> } else {
>> - x = 0;
>> + weston_log("No valid modes found. Cannot configure an output.\n");
>> + return -1;
>> }
>>
>> - output = wayland_output_create(b, x, 0, mode->width, mode->height,
>> - NULL, 0,
>> - WL_OUTPUT_TRANSFORM_NORMAL, 1);
>> - if (!output)
>> - return NULL;
>> + if (wayland_output_configure(&output->base, output->poutput_mode->width,
>> + output->poutput_mode->height) < 0)
>
> If wayland_output_configure() multiplies by scale and applies
> transformation, then passing mode->width and mode->height directly here
> is wrong.
>
> The "video mode" is given in output pixels, so it is already multiplied
> by scale and rotated by transform.
>
> I see the old code was equally wrong, particularly if the output was
> rotated 90 degrees it would create a window with inverted aspect ratio.
> Ok, so if this keeps the old broken behaviour, it's fine.
>
> Testing the upstream x11-backend, it looks like we expect to put the
> logical output size (in global coordinates) in weston.ini, and then
> expect the scale and transform to be applied on that to get the output
> resolution in output pixels.
>
This is for parent output (fs shell and sprawl). You can't change the size
here. Scale and transform are also hardcoded to "scale = 1, transform = WL_OUTPUT_TRANSFORM_NORMAL"
(well, they used to be and I let them remain that way).
See:
https://cgit.freedesktop.org/wayland/weston/tree/libweston/compositor-wayland.c#n1148
and:
https://github.com/krezovic/weston/blob/gsoc-v10/compositor/main.c#L1545
Those values are set just for sake of it, it's not that anything uses them.
>> + return -1;
>>
>> output->parent.output = poutput->global;
>>
>> @@ -1159,27 +1215,20 @@ wayland_output_create_for_parent_output(struct wayland_backend *b,
>> wl_list_insert_list(&output->base.mode_list, &poutput->mode_list);
>> wl_list_init(&poutput->mode_list);
>>
>> - wayland_output_set_fullscreen(output,
>> - WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER,
>> - mode->refresh, poutput->global);
>> + return 0;
>> +}
>>
>> - if (output->parent.shell_surface) {
>> - wl_shell_surface_set_fullscreen(output->parent.shell_surface,
>> - WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER,
>> - mode->refresh, poutput->global);
>> - } else if (b->parent.fshell) {
>> - zwp_fullscreen_shell_v1_present_surface(b->parent.fshell,
>> - output->parent.surface,
>> - ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_CENTER,
>> - poutput->global);
>> - zwp_fullscreen_shell_mode_feedback_v1_destroy(
>> - zwp_fullscreen_shell_v1_present_surface_for_mode(b->parent.fshell,
>> - output->parent.surface,
>> - poutput->global,
>> - mode->refresh));
>> - }
>> +static int
>> +wayland_output_create_for_parent_output(struct wayland_backend *b,
>> + struct wayland_parent_output *poutput)
>> +{
>> + struct wayland_output *output = wayland_output_create_common();
>> + output->user_data = poutput;
>> + output->base.name = NULL;
>>
>> - return output;
>> + weston_output_init_pending(&output->base, b->compositor);
>> +
>> + return 0;
>> }
>>
>> static void
>> @@ -2211,6 +2260,7 @@ wayland_backend_create(struct weston_compositor *compositor,
>> create_cursor(b, new_config);
>>
>> b->use_pixman = new_config->use_pixman;
>> + b->fullscreen = new_config->fullscreen;
>>
>> if (!b->use_pixman) {
>> gl_renderer = weston_load_module("gl-renderer.so",
>> @@ -2284,6 +2334,15 @@ wayland_backend_destroy(struct wayland_backend *b)
>> free(b);
>> }
>>
>> +static const struct weston_windowed_output_api windowed_api = {
>> + wayland_output_configure,
>
> Rename to *_set_size.
>
>
>> + wayland_output_create,
>> +};
>> +
>> +static const struct weston_wayland_output_api wayland_api = {
>> + wayland_output_configure_hotplug,
>> +};
>> +
>> static void
>> config_init_to_defaults(struct weston_wayland_backend_config *config)
>> {
>> @@ -2294,10 +2353,9 @@ backend_init(struct weston_compositor *compositor,
>> struct weston_backend_config *config_base)
>> {
>> struct wayland_backend *b;
>> - struct wayland_output *output;
>> struct wayland_parent_output *poutput;
>> struct weston_wayland_backend_config new_config;
>> - int x, count;
>> + int ret;
>>
>> if (config_base == NULL ||
>> config_base->struct_version != WESTON_WAYLAND_BACKEND_CONFIG_VERSION ||
>> @@ -2321,41 +2379,30 @@ backend_init(struct weston_compositor *compositor,
>> wl_list_for_each(poutput, &b->parent.output_list, link)
>> wayland_output_create_for_parent_output(b, poutput);
>>
>> - 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_WAYLAND_OUTPUT_API_NAME,
>> + &wayland_api, sizeof(wayland_api));
>>
>> - output = wayland_output_create_for_config(b,
>> - &new_config.outputs[0],
>> - 1, 0, 0);
>> - if (!output)
>> - goto err_outputs;
>> + if (ret < 0) {
>> + weston_log("Failed to register output API.\n");
>> + wayland_backend_destroy(b);
>> + return -1;
>> + }
>>
>> - 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;
>> + ret = weston_plugin_api_register(compositor, WESTON_WINDOWED_OUTPUT_API_NAME,
>> + &windowed_api, sizeof(windowed_api));
>>
>> - 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..3164db6 100644
>> --- a/libweston/compositor-wayland.h
>> +++ b/libweston/compositor-wayland.h
>> @@ -27,6 +27,7 @@
>> #define WESTON_COMPOSITOR_WAYLAND_H
>>
>> #include "compositor.h"
>> +#include "plugin-registry.h"
>>
>> #ifdef __cplusplus
>> extern "C" {
>> @@ -36,14 +37,28 @@ 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;
>> +
>> +#define WESTON_WAYLAND_OUTPUT_API_NAME "weston_wayland_output_api_v1"
>> +
>> +struct weston_wayland_output_api {
>> + /** Configure a fullscreen/sprawl-ed output so it can
>> + * be enabled.
>> + *
>> + * Returns 0 on success, -1 on failure.
>> + */
>> + int (*output_configure)(struct weston_output *output);
>> };
>
> I think we discussed in IRC that this API won't be necessary after all.
>
> Splitting weston_output_init_pending() to the separate signal emission
> step should fix it.
>
Indeed.
>>
>> +static inline const struct weston_wayland_output_api *
>> +weston_wayland_output_get_api(struct weston_compositor *compositor)
>> +{
>> + const void *api;
>> + api = weston_plugin_api_get(compositor, WESTON_WAYLAND_OUTPUT_API_NAME,
>> + sizeof(struct weston_wayland_output_api));
>> +
>> + return (const struct weston_wayland_output_api *)api;
>> +}
>> +
>> struct weston_wayland_backend_config {
>> struct weston_backend_config base;
>> int use_pixman;
>
>
> I think the overall structure is fairly good already, but reading
> through this I did discover a bunch of existing issues. The Wayland
> backend would need more love than just keeping up with API changes...
>
>
> Thanks,
> pq
>
Thanks for the review. The rest of issues in this patch I didn't reply to
individually will also be taken care of (obviously).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 855 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160817/aa55dec9/attachment-0001.sig>
More information about the wayland-devel
mailing list