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

Wed Oct 5 12:46:33 UTC 2016

On 05.10.2016 14:35, Pekka Paalanen wrote:
> 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
> 

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.
> 

Yes, exactly that.

>> +	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.
> 

I suppose that could work, yes. I haven't tried it though.

>>  
>>  	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?
> 

Well, it's supposed to. Currently, we have output->disable() undo what
output->enable() does.

For output created when using --fullscreen, output->enable() does not
create the surface, so output->disable() can't destroy it, as it would
be wrong thing to do, because the current conditional won't recreate the
surface on next output->enable() if --fullscreen is used.

Now, that you've given me an idea about checking for output->parent.surface,
I can share output->enable() and output->disable() for all 3 cases once
again.

> 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.
> 

Thanks. We can do a follow-up if needed to fix some more problems.

> 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: signature.asc
Type: application/pgp-signature
Size: 837 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161005/1132240d/attachment.sig>