On 05.10.2016 14:35, Pekka Paalanen wrote: > On Fri, 30 Sep 2016 14:11:09 +0200 > Armin Krezović <[email protected]> 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ć <[email protected]> >> --- >> 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 <[email protected]>
>
>> +
>> weston_output_destroy(&output->base);
>> - free(output);
>>
>> - return;
>> + free(output);
>> }
>
> Thanks,
> pq
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
