[PATCH v2 1/2] shell & compositor: add parameters for set_fullsceen
Pekka Paalanen
ppaalanen at gmail.com
Tue Jan 10 03:41:13 PST 2012
Hi,
here a just some bits that caught my eye on a quick look, inline.
We might prefer to have compositor and client changes in separate
patches, after a minimal patch that fixes breakage from protocol
changes. Should be easy to fix just the protocol bits and retain current
behaviour, and then implement the new semantics separately in Weston
and clients.
Disclaimer: again, just my personal comments. Krh is the authority.
On Mon, 9 Jan 2012 22:54:19 +0800
juan.j.zhao at linux.intel.com wrote:
> From: Alex Wu <zhiwen.wu at linux.intel.com>
>
> For some appliation(e.g. xbmc) wanting actual fullscreen effect, compositor should change the
> display mode to match the app UI rectangle. We add 2 parameters to set_fullscreen in order to
> let client to choose the appropriate way to deal with dismatch between the client's surface and
> display mode of output.
> For "WL_SHELL_SURFACE_FULLSCREEN_MOTHED_SCALE", compositor will rescale the surface rectangle
> to match the current output. For "WESTON_SURFACE_FULLSCREEN_FORCE", compositor will change to
> a appropriate display mode to match the client's surface rectangle. For "WESTON_SURFACE_FULLSCREEN_FILL",
> make the surface center on output.
>
> We add a function pointer "set_mode" to struct weston_output which will be implemented by backend,
> for now, just compositor-drm.c implemented this virtaul method.
>
> In addition, we modified window.c to coordinate with new set_fullscreen api and added the simple-fullscreen
> client for testing.
> F12, F11, and F10 to set fullscreen by different method, F9 to set maximised, F8 to set toplevel to fallback
>
> Signed-off-by: Juan Zhao <juan.j.zhao at linux.intel.com>
> Signed-off-by: Alex Wu <zhiwen.wu at linux.intel.com>
>
> ---
> clients/Makefile.am | 5 +-
> clients/simple-fullscreen.c | 614 +++++++++++++++++++++++++++++++++++++++++++
> clients/window.c | 5 +-
> src/compositor-drm.c | 169 ++++++++++++
> src/compositor.c | 243 +++++++++++++++--
> src/compositor.h | 21 ++
> src/shell.c | 69 ++++--
> 7 files changed, 1077 insertions(+), 49 deletions(-)
> create mode 100644 clients/simple-fullscreen.c
...
> diff --git a/clients/window.c b/clients/window.c
> index a101204..bee15ea 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -782,12 +782,15 @@ window_get_resize_dx_dy(struct window *window, int *x, int *y)
> static void
> window_set_type(struct window *window)
> {
> + uint32_t fs_mode = WL_SHELL_SURFACE_FULLSCREEN_MOTHED_NONE;
> +
> if (!window->shell_surface)
> return;
>
> +#define FRAMERATE 60
Use the don't-care value.
> switch (window->type) {
> case TYPE_FULLSCREEN:
> - wl_shell_surface_set_fullscreen(window->shell_surface);
> + wl_shell_surface_set_fullscreen(window->shell_surface, FRAMERATE, fs_mode);
> break;
> case TYPE_TOPLEVEL:
> wl_shell_surface_set_toplevel(window->shell_surface);
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index a6cedd5..4a4c70d 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -78,6 +78,30 @@ struct drm_output {
> uint32_t pending_fs_surf_fb_id;
> };
>
> +static struct drm_mode *
> +choose_mode (struct drm_output *output, char *resolution, uint32_t refresh)
> +{
> + struct drm_mode *tmp_mode = NULL, *mode, *next;
> + printf ("%s: mode to choose: resolution=%s, refresh=%u\n", __FUNCTION__, resolution, refresh);
> +
> + wl_list_for_each_safe(mode, next,
> + &output->base.mode_list,base.link) {
> + if (!strcmp (mode->mode_info.name, resolution)) {
> + if (mode->mode_info.vrefresh == refresh)
> + return mode;
> + else if (!tmp_mode)
> + tmp_mode = mode;
> + }
> + }
> +
> + if (tmp_mode)
> + printf ("%s: no matched refresh, choose mode: resolution=%s, refresh=%u\n",
> + __FUNCTION__, tmp_mode->mode_info.name, tmp_mode->mode_info.vrefresh);
> + else
> + printf ("%s: no matched mode\n");
> + return tmp_mode;
> +}
I don't think it is a good way to choose the mode by comparing strings,
but I don't know what guarantees there are for mode names. I suspect
they are meant as human-readable, not machine parseable. I'd prefer
matching the mode attributes from struct weston_mode or drmModeModeInfo.
> +
> static int
> drm_output_prepare_render(struct weston_output *output_base)
> {
> @@ -258,6 +282,147 @@ out:
> return ret;
> }
>
> +static int
> +drm_output_set_mode (struct weston_output *output_base, char *resolution, uint32_t refresh)
> +{
> + struct drm_output *output;
> + struct drm_mode *drm_mode, *next;
> + drmModeEncoder *encoder;
> + int i, ret;
> + unsigned handle, stride;
> + struct drm_compositor *ec;
> +
> + ec = (struct drm_compositor *)output_base->compositor;
Reference before NULL-check, but...
> + output = (struct drm_output *)output_base;
> + if (output == NULL) {
this can never be NULL, right?
> + return -1;
> + }
> +
> + drm_mode = (struct drm_mode *)(output->base.current);
> +
> + printf ("current mode: %s, w=%d, h=%d, refresh=%u, flags=%u\n",
> + drm_mode->mode_info.name, drm_mode->base.width,
> + drm_mode->base.height, drm_mode->base.refresh,
> + drm_mode->base.flags);
> +
> + drm_mode = choose_mode (output, resolution, refresh);
> +
> + if (!drm_mode) {
> + printf ("%s, invalid resolution:%s\n", __FUNCTION__, resolution);
> + return -1;
> + }
...
> diff --git a/src/compositor.c b/src/compositor.c
> index b33ed8a..2b01a65 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
...
> @@ -773,12 +897,11 @@ weston_output_repaint(struct weston_output *output)
> {
> struct weston_compositor *ec = output->compositor;
> struct weston_surface *es;
> + struct weston__surface *cursorsurface = NULL, *panelsurface = NULL;
> pixman_region32_t opaque, new_damage, total_damage, repaint;
>
> output->prepare_render(output);
>
> - glViewport(0, 0, output->current->width, output->current->height);
> -
> glUseProgram(ec->texture_shader.program);
> glUniformMatrix4fv(ec->texture_shader.proj_uniform,
> 1, GL_FALSE, output->matrix.d);
> @@ -807,29 +930,88 @@ weston_output_repaint(struct weston_output *output)
>
> es = container_of(ec->surface_list.next, struct weston_surface, link);
>
> + struct wl_list *tlist=(struct wl_list *)ec->surface_list.next;
> + int count=0;
> +
> + while((es->type != WESTON_SURFACE_TYPE_GENERAL) && count <1024){
> + tlist=tlist->next;
> + if((tlist == (struct wl_list *)ec->surface_list.next) &&
> + count!=0)
> + break;
> + switch(es->type){
> + case WESTON_SURFACE_TYPE_CURSOR:
> + cursorsurface = es;
> + break;
> + case WESTON_SURFACE_TYPE_PANEL:
> + panelsurface = es;
> + break;
> + default:
> + fprintf(stderr,"unkown surface type!\n");
> + return;
> + }
> + es = container_of(tlist->next, struct weston_surface, link);
> + count++;
> + }
> + if(count == 1024) {
> + fprintf(stderr,"too many surfaces!\n");
> + return -1;
> + }
What is this supposed to do?
Why is there an arbitrary limit on 'count'?
Are you forgetting there can be several cursors?
What is PANEL doing here?
Maybe all this just stems from the fact that a single list of rendered
surfaces is not sufficient, and we should use a more appropriate data
structures supporting layers?
> +
> if (setup_scanout_surface(output, es) == 0)
> /* We're drawing nothing, just let the damage accumulate */
> return;
>
> - if (es->fullscreen_output == output) {
> - if (es->width < output->current->width ||
> - es->height < output->current->height)
> - glClear(GL_COLOR_BUFFER_BIT);
> + if (es->fs_support.fullscreen_output == output ) {
> + if (es->width != output->current->width ||
> + es->height != output->current->height) {
> + weston_surface_set_fullscreen(output, es);
> + output->prepare_render(output);
> + glViewport( 0, 0, (GLint) output->current->width,
> + (GLint) output->current->height);
> + glClear(GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT);
> + }
> weston_surface_draw(es, output, &total_damage);
> + if(cursorsurface)
> + weston_surface_draw(cursorsurface, output, &total_damage);
> } else {
> - wl_list_for_each(es, &ec->surface_list, link) {
> - pixman_region32_copy(&es->damage, &total_damage);
> - pixman_region32_subtract(&total_damage, &total_damage, &es->opaque);
> - }
> -
> - wl_list_for_each_reverse(es, &ec->surface_list, link) {
> - pixman_region32_init(&repaint);
> - pixman_region32_intersect(&repaint, &output->region,
> - &es->damage);
> - weston_surface_draw(es, output, &repaint);
> - pixman_region32_subtract(&es->damage,
> - &es->damage, &output->region);
> - pixman_region32_fini(&repaint);
> + if( output->saved_mode &&
> + (output->saved_mode->width!=output->current->width ||
> + output->saved_mode->height!=output->current->height) ) {
> + weston_output_restore_mode(output);
> + output->saved_mode = NULL;
> + output->fs_dirty = NULL;
> + output->prepare_render(output);
> + glViewport( 0, 0,
> + (GLint) output->current->width,
> + (GLint) output->current->height);
> + glClear(GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT);
> + weston_output_damage(output);
> + weston_output_repaint(output);
> + } else if (output->fs_dirty) {
> + output->fs_dirty = NULL;
> + weston_output_damage(output);
> + weston_output_repaint(output);
> + } else {
> + glViewport(0, 0,
> + output->current->width,
> + output->current->height);
> + wl_list_for_each(es, &ec->surface_list, link) {
> + pixman_region32_copy(&es->damage,
> + &total_damage);
> + pixman_region32_subtract(&total_damage,
> + &total_damage,
> + &es->opaque);
> + }
> + wl_list_for_each_reverse(es, &ec->surface_list, link) {
> + pixman_region32_init(&repaint);
> + pixman_region32_intersect(&repaint,
> + &output->region,
> + &es->damage);
> + weston_surface_draw(es, output, &repaint);
> + pixman_region32_subtract(&es->damage,
> + &es->damage,
> + &output->region);
> + }
> }
> }
>
> @@ -1554,6 +1736,7 @@ input_device_attach(struct wl_client *client,
> weston_surface_create(compositor,
> device->input_device.x,
> device->input_device.y, 32, 32);
> + device->sprite->type = WESTON_SURFACE_TYPE_CURSOR;
> wl_list_init(&device->sprite->link);
> }
>
> diff --git a/src/compositor.h b/src/compositor.h
> index 031b7d4..9907abb 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -32,6 +32,15 @@
> #include <EGL/egl.h>
> #include <EGL/eglext.h>
>
> +#define WESTON_SURFACE_FULLSCREEN_NONE 0
> +#define WESTON_SURFACE_FULLSCREEN_SCALE 1
> +#define WESTON_SURFACE_FULLSCREEN_FORCE 2
> +#define WESTON_SURFACE_FULLSCREEN_FILL 3
> +#define WESTON_SURFACE_TYPE_CURSOR 0
> +#define WESTON_SURFACE_TYPE_PANEL 1
> +#define WESTON_SURFACE_TYPE_GENERAL 2
Why is PANEL here? Panel as a concept should not exist here, it is a
shell thing.
> +
> +
> struct weston_matrix {
> GLfloat d[16];
> };
> @@ -79,8 +88,10 @@ struct weston_output {
>
> char *make, *model;
> uint32_t subpixel;
> + uint32_t fs_dirty;
>
> struct weston_mode *current;
> + struct weston_mode *saved_mode;
> struct wl_list mode_list;
> struct wl_buffer *scanout_buffer;
> struct wl_listener scanout_buffer_destroy_listener;
> @@ -94,6 +105,7 @@ struct weston_output {
> int (*set_hardware_cursor)(struct weston_output *output,
> struct weston_input_device *input);
> void (*destroy)(struct weston_output *output);
> + int (*set_mode)(struct weston_output *output_base, char *mode, uint32_t refresh);
I believe the 'mode' argument should not be a string, but either w, h,
or maybe even just struct weston_mode *. Mode selection is not
backend specific, if you only match fields in weston_mode. Would that
be sufficient?
> };
>
> struct weston_input_device {
> @@ -242,6 +254,15 @@ struct weston_surface {
> struct weston_transform *transform;
> uint32_t alpha;
> uint32_t visual;
> + uint32_t type;
> +
> + struct {
> + uint32_t framerate;
> + uint32_t fs_method;
> + struct weston_transform *fs_transform;
> + struct weston_output *fullscreen_output;
> + } fs_support;
Forgot to remove the existing fullscreen_output member?
> +
>
> /*
> * Which output to vsync this surface to.
> diff --git a/src/shell.c b/src/shell.c
...
Cheers,
pq
More information about the wayland-devel
mailing list