[PATCH weston v4 08/14] weston: Port Wayland backend to new output handling API

[Pekka Paalanen]

On Fri, 30 Sep 2016 14:11:09 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> This is a complete port of the Wayland backend that
> uses the 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().
> 
> v4:
> 
>  - Drop unused fields from weston_wayland_backend_config
>    and bump WESTON_WAYLAND_BACKEND_CONFIG_VERSION to 2.
>  - Move output creation to backend itself when
>    --fullscreen is used.
>  - Prevent possible duplicated output names by assigning
>    a different name to outputs created without any
>    configuration specified.
> 
> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
>  compositor/main.c              | 251 +++++++++-----------------
>  libweston/compositor-wayland.c | 399 +++++++++++++++++++++++++++--------------
>  libweston/compositor-wayland.h |  12 +-
>  3 files changed, 350 insertions(+), 312 deletions(-)
> 

> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
> index 0375a11..da0c4fe 100644
> --- a/libweston/compositor-wayland.c
> +++ b/libweston/compositor-wayland.c
> @@ -26,6 +26,7 @@
>  
>  #include "config.h"
>  
> +#include <assert.h>
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -51,6 +52,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 +76,7 @@ struct wayland_backend {
>  
>  	int use_pixman;
>  	int sprawl_across_outputs;
> +	int fullscreen;
>  
>  	struct theme *theme;
>  	cairo_device_t *frame_device;
> @@ -617,22 +620,34 @@ wayland_output_repaint_pixman(struct weston_output *output_base,
>  }
>  
>  static void
> -wayland_output_destroy(struct weston_output *output_base)
> +wayland_backend_destroy_output_surface(struct wayland_output *output)
>  {
> -	struct wayland_output *output = to_wayland_output(output_base);
> -	struct wayland_backend *b =
> -		to_wayland_backend(output->base.compositor);
> +	if (output->parent.shell_surface)
> +		wl_shell_surface_destroy(output->parent.shell_surface);
> +
> +	wl_surface_destroy(output->parent.surface);
> +}
> +
> +static int
> +wayland_output_disable(struct weston_output *base)
> +{
> +	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);
> +	/* Done on output->enable when not fullscreen, otherwise
> +	 * done in output_create, to get the proper mode */

Hi

Done in output_create? Do you mean:
Created on output->enable() when not fullscreen, and on
wayland_output_create_fullscreen() when fullscreen to get the proper
size.

> +	if (!b->fullscreen)
> +		wayland_backend_destroy_output_surface(output);

It doesn't feel right to use b->fullscreen as the condition here, would
it not work by checking output->parent.surface?

I'd expect the wl_surface et al. to be destroyed always if they exist.

>  
>  	if (output->frame)
>  		frame_destroy(output->frame);
> @@ -642,10 +657,23 @@ wayland_output_destroy(struct weston_output *output_base)
>  	cairo_surface_destroy(output->gl.border.right);
>  	cairo_surface_destroy(output->gl.border.bottom);
>  
> +	return 0;
> +}
> +
> +static void
> +wayland_output_destroy(struct weston_output *base)
> +{
> +	struct wayland_output *output = to_wayland_output(base);
> +	struct wayland_backend *b = to_wayland_backend(base->compositor);
> +
> +	wayland_output_disable(&output->base);
> +
> +	if (b->fullscreen)
> +		wayland_backend_destroy_output_surface(output);

Especially here b->fullscreen is a surprising condition.
If disable() ensured the surface is destroyed, you wouldn't need this
hunk.

I think you were probably looking for symmetry, destroying the thing in
the place corresponding to where it was created. In most cases that
would be desireable, but here I'm not sure.

The odd thing would be to call disable() when the user specified
--fullscreen, which case there would be no output at all. Then calling
enable() to start the output later after all would... work?

I think we can assume it won't happen at the moment. There is a lot
more to fix and streamline in the wayland-backend that I don't want
that to hold up the merging of this series.

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

> +
>  	weston_output_destroy(&output->base);
> -	free(output);
>  
> -	return;
> +	free(output);
>  }

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/20161005/fc3bc2c4/attachment.sig>