Re: [PATCH 6/6] window: Add a simple getenv to force SHM rendering

2014-04-30 Thread Kristian Høgsberg
On Mon, Apr 28, 2014 at 11:19:32AM -0400, Jasper St. Pierre wrote:

Yeah, that's useful, applied.

Kristian

> ---
>  clients/window.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index d822af7..3897440 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -4338,11 +4338,11 @@ surface_create(struct window *window)
>   return surface;
>  }
>  
> -static window_buffer_type
> +static enum window_buffer_type
>  get_preferred_buffer_type(struct display *display)
>  {
>  #ifdef HAVE_CAIRO_EGL
> - if (display->argb_device)
> + if (display->argb_device && !getenv("TOYTOOLKIT_NO_EGL"))
>   return WINDOW_BUFFER_TYPE_EGL_WINDOW;
>  #endif
>  
> -- 
> 1.9.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH 4/6] cairo-util: Don't show a resize cursor on edges when we're maximized

2014-04-30 Thread Kristian Høgsberg
On Mon, Apr 28, 2014 at 11:19:30AM -0400, Jasper St. Pierre wrote:
> This is substantially confusing to users, namely me.

That is indeed confusing.  However, I edited the patch to just set grip_size
to 0 if we're maximized.

Kristian

> ---
>  shared/cairo-util.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/shared/cairo-util.c b/shared/cairo-util.c
> index a1568ff..a77d0b6 100644
> --- a/shared/cairo-util.c
> +++ b/shared/cairo-util.c
> @@ -487,11 +487,14 @@ enum theme_location
>  theme_get_location(struct theme *t, int x, int y,
>   int width, int height, int flags)
>  {
> + int maximized;
>   int vlocation, hlocation, location;
>   const int grip_size = 8;
>   int margin, top_margin;
>  
> - margin = (flags & THEME_FRAME_MAXIMIZED) ? 0 : t->margin;
> + maximized = (flags & THEME_FRAME_MAXIMIZED);
> +
> + margin = maximized ? 0 : t->margin;
>  
>   if (flags & THEME_FRAME_NO_TITLE)
>   top_margin = t->width;
> @@ -500,22 +503,22 @@ theme_get_location(struct theme *t, int x, int y,
>  
>   if (x < margin)
>   hlocation = THEME_LOCATION_EXTERIOR;
> - else if (x < margin + grip_size)
> + else if (!maximized && x < margin + grip_size)
>   hlocation = THEME_LOCATION_RESIZING_LEFT;
>   else if (x < width - margin - grip_size)
>   hlocation = THEME_LOCATION_INTERIOR;
> - else if (x < width - margin)
> + else if (!maximized && x < width - margin)
>   hlocation = THEME_LOCATION_RESIZING_RIGHT;
>   else
>   hlocation = THEME_LOCATION_EXTERIOR;
>  
>   if (y < margin)
>   vlocation = THEME_LOCATION_EXTERIOR;
> - else if (y < margin + grip_size)
> + else if (!maximized && y < margin + grip_size)
>   vlocation = THEME_LOCATION_RESIZING_TOP;
>   else if (y < height - margin - grip_size)
>   vlocation = THEME_LOCATION_INTERIOR;
> - else if (y < height - margin)
> + else if (!maximized && y < height - margin)
>   vlocation = THEME_LOCATION_RESIZING_BOTTOM;
>   else
>   vlocation = THEME_LOCATION_EXTERIOR;
> -- 
> 1.9.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH 1/6] compositor: Fix the documentation for surface->configure

2014-04-30 Thread Kristian Høgsberg
On Mon, Apr 28, 2014 at 11:19:27AM -0400, Jasper St. Pierre wrote:
> It's called on commit, not on attach. Additionally, correct the
> interface name to be wl_surface, not surface.
> ---
>  src/compositor.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compositor.h b/src/compositor.h
> index 03d8992..c913f54 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -908,9 +908,9 @@ struct weston_surface {
>   } pending;
>  
>   /*
> -  * If non-NULL, this function will be called on surface::attach after
> +  * If non-NULL, this function will be called on wl_surface::commit after
>* a new buffer has been set up for this surface. The integer params
> -  * are the sx and sy paramerters supplied to surface::attach .
> +  * are the sx and sy paramerters supplied to wl_surface::attach .

Applied and I squashed in a typo fix for "paramerters".

Kristian

>*/
>   void (*configure)(struct weston_surface *es, int32_t sx, int32_t sy);
>   void *configure_private;
> -- 
> 1.9.0
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston v2] screen-share: Don't unset server environment variables

2014-04-30 Thread Kristian Høgsberg
On Wed, Apr 30, 2014 at 10:53:30AM -0500, Jason Ekstrand wrote:
> Looks good to me.
> 
> Reviewed-by: Jason Ekstrand 

Thanks, committed.

Kristian

> On Apr 30, 2014 3:52 AM, "Andrew Wedgbury" 
> wrote:
> 
> > There is no need to unset WAYLAND_DISPLAY and WAYLAND_SOCKET when
> > screen-share
> > launches the fullscreen shell server. This was done originally in case the
> > launched server decided to use the wayland backend based on the presence of
> > these. However, we pass a command line argument telling it to use the RDP
> > backend, which overrides the automatic backend selection based on the
> > environment.
> >
> > Keeping these environment variables allows the launched fullscreen shell
> > server
> > to know the original server's display name, which it may need in order to
> > show
> > a configuration UI.
> >
> > Signed-off-by: Andrew Wedgbury 
> > ---
> >  src/screen-share.c | 4 
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/src/screen-share.c b/src/screen-share.c
> > index d3e3f05..6f60b81 100644
> > --- a/src/screen-share.c
> > +++ b/src/screen-share.c
> > @@ -1005,10 +1005,6 @@ weston_output_share(struct weston_output *output,
> > }
> >
> > if (pid == 0) {
> > -   /* We don't want anything circular */
> > -   unsetenv("WAYLAND_DISPLAY");
> > -   unsetenv("WAYLAND_SOCKET");
> > -
> > /* do not give our signal mask to the new process */
> > sigfillset(&allsigs);
> > sigprocmask(SIG_UNBLOCK, &allsigs, NULL);
> > --
> > 1.9.2
> >
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >

> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston v2] screen-share: Don't unset server environment variables

2014-04-30 Thread Jason Ekstrand
Looks good to me.

Reviewed-by: Jason Ekstrand 
On Apr 30, 2014 3:52 AM, "Andrew Wedgbury" 
wrote:

> There is no need to unset WAYLAND_DISPLAY and WAYLAND_SOCKET when
> screen-share
> launches the fullscreen shell server. This was done originally in case the
> launched server decided to use the wayland backend based on the presence of
> these. However, we pass a command line argument telling it to use the RDP
> backend, which overrides the automatic backend selection based on the
> environment.
>
> Keeping these environment variables allows the launched fullscreen shell
> server
> to know the original server's display name, which it may need in order to
> show
> a configuration UI.
>
> Signed-off-by: Andrew Wedgbury 
> ---
>  src/screen-share.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/src/screen-share.c b/src/screen-share.c
> index d3e3f05..6f60b81 100644
> --- a/src/screen-share.c
> +++ b/src/screen-share.c
> @@ -1005,10 +1005,6 @@ weston_output_share(struct weston_output *output,
> }
>
> if (pid == 0) {
> -   /* We don't want anything circular */
> -   unsetenv("WAYLAND_DISPLAY");
> -   unsetenv("WAYLAND_SOCKET");
> -
> /* do not give our signal mask to the new process */
> sigfillset(&allsigs);
> sigprocmask(SIG_UNBLOCK, &allsigs, NULL);
> --
> 1.9.2
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH weston 3/3] weston-info: Display the name from the new wl_output.name event

2014-04-30 Thread Neil Roberts
If the compositor supports version 3 of the wl_output interface and
sends a name event then this will now be displayed in the info.
---
 clients/weston-info.c | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/clients/weston-info.c b/clients/weston-info.c
index df869e3..05edd21 100644
--- a/clients/weston-info.c
+++ b/clients/weston-info.c
@@ -59,6 +59,8 @@ struct output_info {
 
struct wl_output *output;
 
+   char *name;
+
struct {
int32_t x, y;
int32_t physical_width, physical_height;
@@ -160,6 +162,9 @@ print_output_info(void *data)
 
print_global_info(data);
 
+   if (output->name)
+   printf("\tname: %s\n", output->name);
+
switch (output->geometry.subpixel) {
case WL_OUTPUT_SUBPIXEL_UNKNOWN:
subpixel_orientation = "unknown";
@@ -423,9 +428,35 @@ output_handle_mode(void *data, struct wl_output *wl_output,
wl_list_insert(output->modes.prev, &mode->link);
 }
 
+static void
+output_handle_name(void *data,
+struct wl_output *wl_output,
+const char *name)
+{
+   struct output_info *output = data;
+
+   output->name = xstrdup(name);
+}
+
+static void
+output_handle_done(void *data,
+  struct wl_output *wl_output)
+{
+}
+
+static void
+output_handle_scale(void *data,
+   struct wl_output *wl_output,
+   int32_t factor)
+{
+}
+
 static const struct wl_output_listener output_listener = {
output_handle_geometry,
output_handle_mode,
+   output_handle_done,
+   output_handle_scale,
+   output_handle_name,
 };
 
 static void
@@ -440,6 +471,8 @@ destroy_output_info(void *data)
free(output->geometry.make);
if (output->geometry.model != NULL)
free(output->geometry.model);
+   if (output->name != NULL)
+   free(output->name);
 
wl_list_for_each_safe(mode, tmp, &output->modes, link) {
wl_list_remove(&mode->link);
@@ -458,8 +491,12 @@ add_output_info(struct weston_info *info, uint32_t id, 
uint32_t version)
 
wl_list_init(&output->modes);
 
+   if (version > 3)
+   version = 3;
+
output->output = wl_registry_bind(info->registry, id,
- &wl_output_interface, 1);
+ &wl_output_interface,
+ version);
wl_output_add_listener(output->output, &output_listener,
   output);
 
-- 
1.9.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH weston 2/3] Send the new wl_output.name event when binding an output

2014-04-30 Thread Neil Roberts
This version for the wl_output interface has been changed to 3 and it
now sends out the name event when a client binds an output.
---
 src/compositor.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/compositor.c b/src/compositor.c
index ee8dc24..6a333df 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -3081,6 +3081,9 @@ bind_output(struct wl_client *client,
mode->refresh);
}
 
+   if (version >= 3 && output->name)
+   wl_output_send_name(resource, output->name);
+
if (version >= 2)
wl_output_send_done(resource);
 }
@@ -3321,7 +3324,7 @@ weston_output_init(struct weston_output *output, struct 
weston_compositor *c,
output->compositor->output_id_pool |= 1 << output->id;
 
output->global =
-   wl_global_create(c->wl_display, &wl_output_interface, 2,
+   wl_global_create(c->wl_display, &wl_output_interface, 3,
 output, bind_output);
wl_signal_emit(&c->output_created_signal, output);
 }
-- 
1.9.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH 1/3] protocol: Add an event to specify the name of an output

2014-04-30 Thread Neil Roberts
This bumps the version of the wl_output interface to 3 and adds a
separate event to report the output's name.
---
 protocol/wayland.xml | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 330f8ab..60fa81e 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1645,7 +1645,7 @@
 
   
 
-  
+  
 
   An output describes part of the compositor geometry.  The
   compositor works in the 'compositor coordinate system' and an
@@ -1783,6 +1783,17 @@
   
   
 
+
+
+
+
+  
+The name event contains a human-readable name for the output.
+If the output has a name then this event is sent after binding
+to the output object.
+  
+  
+
   
 
   
-- 
1.9.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH weston] shell: Don't map surfaces of type SHELL_SURFACE_NONE

2014-04-30 Thread Ander Conselvan de Oliveira
When commit 07926d90 factored out the code that chooses in which layer
a surface is added to, it changed the behavior for surfaces with no
type. Instead of not adding it to any layer, the surface is added to
the current workspace.

This patch restores the old behavior.

https://bugs.freedesktop.org/show_bug.cgi?id=77527
---
 desktop-shell/shell.c | 27 ++-
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index 7631f1b..7e4ceed 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -2127,6 +2127,12 @@ shell_surface_calculate_layer_link (struct shell_surface 
*shsurf)
struct weston_view *parent;
 
switch (shsurf->type) {
+   case SHELL_SURFACE_XWAYLAND:
+   return &shsurf->shell->fullscreen_layer.view_list;
+
+   case SHELL_SURFACE_NONE:
+   return NULL;
+
case SHELL_SURFACE_POPUP:
case SHELL_SURFACE_TOPLEVEL:
if (shsurf->state.fullscreen && !shsurf->state.lowered) {
@@ -2142,22 +2148,15 @@ shell_surface_calculate_layer_link (struct 
shell_surface *shsurf)
if (parent)
return parent->layer_link.prev;
}
-   break;
 
-   case SHELL_SURFACE_XWAYLAND:
-   return &shsurf->shell->fullscreen_layer.view_list;
-
-   case SHELL_SURFACE_NONE:
-   default:
-   /* Go to the fallback, below. */
-   break;
+   /* Move the surface to a normal workspace layer so that surfaces
+* which were previously fullscreen or transient are no longer
+* rendered on top. */
+   ws = get_current_workspace(shsurf->shell);
+   return &ws->layer.view_list;
}
 
-   /* Move the surface to a normal workspace layer so that surfaces
-* which were previously fullscreen or transient are no longer
-* rendered on top. */
-   ws = get_current_workspace(shsurf->shell);
-   return &ws->layer.view_list;
+   assert(0 && "Unknown shell surface type");
 }
 
 static void
@@ -2198,6 +2197,8 @@ shell_surface_update_layer(struct shell_surface *shsurf)
 
new_layer_link = shell_surface_calculate_layer_link(shsurf);
 
+   if (new_layer_link == NULL)
+   return;
if (new_layer_link == &shsurf->view->layer_link)
return;
 
-- 
1.8.3.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH] xwayland: free xcb reply value when malloc returns null

2014-04-30 Thread vivek
Signed-off-by: vivek 
---
 xwayland/selection.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xwayland/selection.c b/xwayland/selection.c
index b694477..d1f6361 100644
--- a/xwayland/selection.c
+++ b/xwayland/selection.c
@@ -186,8 +186,10 @@ weston_wm_get_selection_targets(struct weston_wm *wm)
}
 
source = malloc(sizeof *source);
-   if (source == NULL)
+   if (source == NULL) {
+   free(reply);
return;
+   }
 
wl_signal_init(&source->base.destroy_signal);
source->base.accept = data_source_accept;
-- 
1.7.9.5

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH weston v2] screen-share: Don't unset server environment variables

2014-04-30 Thread Andrew Wedgbury
There is no need to unset WAYLAND_DISPLAY and WAYLAND_SOCKET when screen-share 
launches the fullscreen shell server. This was done originally in case the
launched server decided to use the wayland backend based on the presence of
these. However, we pass a command line argument telling it to use the RDP 
backend, which overrides the automatic backend selection based on the 
environment.

Keeping these environment variables allows the launched fullscreen shell server
to know the original server's display name, which it may need in order to show
a configuration UI.

Signed-off-by: Andrew Wedgbury 
---
 src/screen-share.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/src/screen-share.c b/src/screen-share.c
index d3e3f05..6f60b81 100644
--- a/src/screen-share.c
+++ b/src/screen-share.c
@@ -1005,10 +1005,6 @@ weston_output_share(struct weston_output *output,
}
 
if (pid == 0) {
-   /* We don't want anything circular */
-   unsetenv("WAYLAND_DISPLAY");
-   unsetenv("WAYLAND_SOCKET");
-
/* do not give our signal mask to the new process */
sigfillset(&allsigs);
sigprocmask(SIG_UNBLOCK, &allsigs, NULL);
-- 
1.9.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH] client: extend error handling

2014-04-30 Thread Pekka Paalanen
On Wed, 30 Apr 2014 09:40:19 +0200
Marek Chalupa  wrote:

> On 24 April 2014 14:45, Pekka Paalanen  wrote:
> 
> > On Wed, 23 Apr 2014 14:39:48 +0200
> > Marek Chalupa  wrote:
> >
> > > +/**
> > > + * This function is called for error events
> > > + * and idicates that in some object occured an error.
> > > + * Difference between this function and display_fatal_error()
> > > + * is that this one handles errors that will come in wire, whereas
> > > + * display_fatal_error() is called for local errors.
> > > + *
> > > + * \param display
> > > + * \param codeerror code
> > > + * \param id  id of the object that generated the error
> > > + * \param intfprotocol interface
> > > + */
> > >  static void
> > > -wl_display_fatal_error(struct wl_display *display, int error)
> > > +display_protocol_error(struct wl_display *display, int code,
> > > +unsigned int id, const struct wl_interface *intf)
> > >  {
> > > + struct wl_event_queue *iter;
> > > + int err;
> > > +
> > > + /* set correct errno */
> > > + if (wl_interface_equal(intf, &wl_display_interface)) {
> > > + switch (code) {
> > > + case WL_DISPLAY_ERROR_INVALID_OBJECT:
> > > + case WL_DISPLAY_ERROR_INVALID_METHOD:
> > > + err = EINVAL;
> > > + break;
> > > + case WL_DISPLAY_ERROR_NO_MEMORY:
> > > + err = ENOMEM;
> > > + break;
> > > + default:
> > > + err = EFAULT;
> > > + }
> > > + } else {
> > > + err = EPROTO;
> > > + }
> > > +
> > > + if (display->last_error)
> > > + return;
> >
> > This 'if' could be the very first thing. Doesn't matter.
> >
> > > +
> > >   pthread_mutex_lock(&display->mutex);
> > > - display_fatal_error(display, error);
> > > +
> > > + display->last_error = err;
> > > +
> > > + display->protocol_error.code = code;
> > > + display->protocol_error.id = id;
> > > + display->protocol_error.interface = intf;
> > > +
> > > + wl_list_for_each(iter, &display->event_queue_list, link)
> > > + pthread_cond_broadcast(&iter->cond);
> >
> > What is the purpose of this pthread_cond_broadcast()? I am not too
> > familiar with the logic here, but looks like this is something new you
> > added, and I'm not sure it belongs with the addition of
> > wl_display_get_protocol_error().
> >
> > If this is fixing something, I feel like it should be a separate patch,
> > since I can't see how adding wl_display_get_protocol_error() would add
> > the need for this. Either the need was there to begin with and this
> > bug fix should get into the stable releases, or it's not needed. Am I
> > missing something?
> >
> >
> This broadcast is not new, so I didn't touch it. So far as I see into it it

I was confused about it because your patch never removes any lines
doing pthread_cond_broadcast(), but it does add it.

Ok, now I see it. Originally, display_fatal_error() does it, and
wl_display_fatal_error() called it. Your patch renames
wl_display_fatal_error to display_protocol_error and removes the call
to display_fatal_error(), so you copy back the relevant parts of
display_fatal_error().

> should
> wake up all the sleeping event queues (when there's more of them than the
> default one - in multi-threaded environment) so that they can receive the
> error
> and notify the client.
> 
> relevant commits:
> 33b7637b4500a682018b503837b8aca9afae36f2
> 385fe30e8b144a968aa88c6546c2ef247771b3d7
> 
> However, I don't see any pthread_cond_{wait, timedwait} for this condition
> anywhere in the code and that's weird...

Marking the 'cond' member deprecated, the compiler tells me it is used
in the following places in current master:

  CC   src/libwayland_client_la-wayland-client.lo
src/wayland-client.c: In function 'display_fatal_error':
src/wayland-client.c:113:3: warning: 'cond' is deprecated (declared at 
src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_broadcast(&iter->cond);

src/wayland-client.c: In function 'wl_event_queue_init':
src/wayland-client.c:128:2: warning: 'cond' is deprecated (declared at 
src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_init(&queue->cond, NULL);

src/wayland-client.c: In function 'wl_event_queue_release':
src/wayland-client.c:143:2: warning: 'cond' is deprecated (declared at 
src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_destroy(&queue->cond);

src/wayland-client.c: In function 'queue_event':
src/wayland-client.c:979:3: warning: 'cond' is deprecated (declared at 
src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_signal(&queue->cond);

You're right, and I cannot see a related mutex, either. Maybe it is
unneeded or we have a hard to trigger bug here.

Nice find.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lis

[PATCH v3] client: extend error handling

2014-04-30 Thread Marek Chalupa
When an error occurs, wl_display_get_error() does not
provide any way of getting know if it was a local error or if it was
an error event, respectively what object caused the error and what
the error was.

This patch introduces a new function wl_display_get_protocol_error()
which will return error code, interface and id of the object that
generated the error.
wl_display_get_error() will work the same way as before.

wl_display_get_protocol_error() DOES NOT indicate that a non-protocol
error happened. It returns valid information only in that case that
(protocol) error occurred, so it should be used after calling
wl_display_get_error() with positive result.

Thanks to Pekka Paalanen  for pointing out
issues in the first versions of this patch.
---
 src/wayland-client.c | 138 ---
 src/wayland-client.h |   3 ++
 2 files changed, 123 insertions(+), 18 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index bd40313..8dd8804 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -78,7 +78,24 @@ struct wl_event_queue {
 struct wl_display {
struct wl_proxy proxy;
struct wl_connection *connection;
+
+   /* errno of the last wl_display error */
int last_error;
+
+   /* When display gets an error event from some object, it stores
+* information about it here, so that client can get this
+* information afterwards */
+   struct {
+   /* Code of the error. It can be compared to
+* the interface's errors enumeration. */
+   int code;
+   /* interface (protocol) in which the error occurred */
+   const struct wl_interface *interface;
+   /* id of the proxy that caused the error. There's no warranty
+* that the proxy is still valid. It's up to client how it will
+* use it */
+   uint32_t id;
+   } protocol_error;
int fd;
pthread_t display_thread;
struct wl_map objects;
@@ -96,6 +113,14 @@ struct wl_display {
 
 static int debug_client = 0;
 
+/**
+ * This function is called for local errors (no memory, server hung up)
+ *
+ * \param display
+ * \param errorerror value (EINVAL, EFAULT, ...)
+ *
+ * \note this function is called with display mutex locked
+ */
 static void
 display_fatal_error(struct wl_display *display, int error)
 {
@@ -105,7 +130,7 @@ display_fatal_error(struct wl_display *display, int error)
return;
 
if (!error)
-   error = 1;
+   error = EFAULT;
 
display->last_error = error;
 
@@ -113,11 +138,56 @@ display_fatal_error(struct wl_display *display, int error)
pthread_cond_broadcast(&iter->cond);
 }
 
+/**
+ * This function is called for error events
+ * and idicates that in some object occured an error.
+ * Difference between this function and display_fatal_error()
+ * is that this one handles errors that will come in wire, whereas
+ * display_fatal_error() is called for local errors.
+ *
+ * \param display
+ * \param codeerror code
+ * \param id  id of the object that generated the error
+ * \param intfprotocol interface
+ */
 static void
-wl_display_fatal_error(struct wl_display *display, int error)
+display_protocol_error(struct wl_display *display, int code,
+  unsigned int id, const struct wl_interface *intf)
 {
+   struct wl_event_queue *iter;
+   int err;
+
+   if (display->last_error)
+   return;
+
+   /* set correct errno */
+   if (wl_interface_equal(intf, &wl_display_interface)) {
+   switch (code) {
+   case WL_DISPLAY_ERROR_INVALID_OBJECT:
+   case WL_DISPLAY_ERROR_INVALID_METHOD:
+   err = EINVAL;
+   break;
+   case WL_DISPLAY_ERROR_NO_MEMORY:
+   err = ENOMEM;
+   break;
+   default:
+   err = EFAULT;
+   }
+   } else {
+   err = EPROTO;
+   }
+
pthread_mutex_lock(&display->mutex);
-   display_fatal_error(display, error);
+
+   display->last_error = err;
+
+   display->protocol_error.code = code;
+   display->protocol_error.id = id;
+   display->protocol_error.interface = intf;
+
+   wl_list_for_each(iter, &display->event_queue_list, link)
+   pthread_cond_broadcast(&iter->cond);
+
pthread_mutex_unlock(&display->mutex);
 }
 
@@ -579,25 +649,12 @@ display_handle_error(void *data,
 uint32_t code, const char *message)
 {
struct wl_proxy *proxy = object;
-   int err;
 
wl_log("%s@%u: error %d: %s\n",
   proxy->object.interface->name, proxy->object.id, code, message);
 
-   switch (code) {
-   case WL_DISPLAY_ERROR_INVALID_OBJECT:
-   case WL_DISPLAY_ERROR_INVALID_METHOD:
- 

Re: [PATCH] client: extend error handling

2014-04-30 Thread Marek Chalupa
On 24 April 2014 14:45, Pekka Paalanen  wrote:

> On Wed, 23 Apr 2014 14:39:48 +0200
> Marek Chalupa  wrote:
>
> > When an error occurres, than wl_display_get_error() do not
>
> ... occurs, [then] ... does not
>
> > provide any way of getting know if it was a local error or if it was
> > an error event, respectively what object caused the error and what
> > the error was.
> >
> > This patch introduces a new function wl_display_get_protocol_error()
> > which will return error code, interface and id of the object that
> > generated the error.
> > wl_display_get_error() will work the same way as before.
> >
> > wl_display_get_protcol_error() DO NOT indicate that the error happed.
>
> +o, a non-protocol error happened?
>
> > It returns valid information only in that case that (protocol) error
> > occurred, so it should be used after calling wl_display_get_error()
> > with positive result.
> >
> > Thanks to Pekka Paalanen  for pointing out
> > issues in the first versions of this patch.
> > ---
> >  src/wayland-client.c | 138
> ---
> >  src/wayland-client.h |   3 ++
> >  2 files changed, 123 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index bd40313..f8146e7 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -78,7 +78,24 @@ struct wl_event_queue {
> >  struct wl_display {
> >   struct wl_proxy proxy;
> >   struct wl_connection *connection;
> > +
> > + /* errno of the last wl_display error */
> >   int last_error;
> > +
> > + /* When display gets an error event from some object, it stores
> > +  * information about it here, so that client can get this
> > +  * information afterwards */
> > + struct {
> > + /* Code of the error. It can be compared to
> > +  * the interface's errors enumeration. */
> > + int code;
> > + /* interface (protocol) in wich the error occured */
>
> which, occurred
>
> > + const struct wl_interface *interface;
> > + /* id of the proxy that caused the error. There's no
> warranty
> > +  * that the proxy is still valid. It's up to client how it
> will
> > +  * use it */
> > + unsigned int id;
>
> I think we use uint32_t for ids specifically.
>

Yes, uint32_t, I'll fix that.


>
> > + } protocol_error;
> >   int fd;
> >   pthread_t display_thread;
> >   struct wl_map objects;
> > @@ -96,6 +113,14 @@ struct wl_display {
> >
> >  static int debug_client = 0;
> >
> > +/**
> > + * This function is called for local errors (no memory, server hung up)
> > + *
> > + * \param display
> > + * \param errorerror value (EINVAL, EFAULT, ...)
> > + *
> > + * \note this function is called with display mutex locked
> > + */
> >  static void
> >  display_fatal_error(struct wl_display *display, int error)
> >  {
> > @@ -105,7 +130,7 @@ display_fatal_error(struct wl_display *display, int
> error)
> >   return;
> >
> >   if (!error)
> > - error = 1;
> > + error = EFAULT;
>
> Yup, makes sense. Better EFAULT than EPERM for the case where errno is
> still 0, I guess. :-)
>
> >
> >   display->last_error = error;
> >
> > @@ -113,11 +138,56 @@ display_fatal_error(struct wl_display *display,
> int error)
> >   pthread_cond_broadcast(&iter->cond);
> >  }
> >
> > +/**
> > + * This function is called for error events
> > + * and idicates that in some object occured an error.
> > + * Difference between this function and display_fatal_error()
> > + * is that this one handles errors that will come in wire, whereas
> > + * display_fatal_error() is called for local errors.
> > + *
> > + * \param display
> > + * \param codeerror code
> > + * \param id  id of the object that generated the error
> > + * \param intfprotocol interface
> > + */
> >  static void
> > -wl_display_fatal_error(struct wl_display *display, int error)
> > +display_protocol_error(struct wl_display *display, int code,
> > +unsigned int id, const struct wl_interface *intf)
> >  {
> > + struct wl_event_queue *iter;
> > + int err;
> > +
> > + /* set correct errno */
> > + if (wl_interface_equal(intf, &wl_display_interface)) {
> > + switch (code) {
> > + case WL_DISPLAY_ERROR_INVALID_OBJECT:
> > + case WL_DISPLAY_ERROR_INVALID_METHOD:
> > + err = EINVAL;
> > + break;
> > + case WL_DISPLAY_ERROR_NO_MEMORY:
> > + err = ENOMEM;
> > + break;
> > + default:
> > + err = EFAULT;
> > + }
> > + } else {
> > + err = EPROTO;
> > + }
> > +
> > + if (display->last_error)
> > + return;
>
> This 'if' could be the very first thing. Doesn't matter.
>
> > +
> >   pthread_mutex_lock(&disp

Re: [PATCH 6/6] window: Add a simple getenv to force SHM rendering

2014-04-30 Thread Pekka Paalanen
On Tue, 29 Apr 2014 16:18:33 -0400
"Jasper St. Pierre"  wrote:

> Can we at least do this work independently of this patch? I was debugging
> EGL stack issues, so I figured I might as well give my envvar hack to debug
> SHM issues.

Of course, I never intended to block this patch. I just wanted to start
a conversation about cairo-egl in toytoolkit in general.

> On Tue, Apr 29, 2014 at 4:07 PM, Kristian Høgsberg wrote:
> 
> > On Tue, Apr 29, 2014 at 5:35 AM, Pekka Paalanen 
> > wrote:
> > > On Mon, 28 Apr 2014 11:19:32 -0400
> > > "Jasper St. Pierre"  wrote:
> > >
> > >> ---
> > >>  clients/window.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/clients/window.c b/clients/window.c
> > >> index d822af7..3897440 100644
> > >> --- a/clients/window.c
> > >> +++ b/clients/window.c
> > >> @@ -4338,11 +4338,11 @@ surface_create(struct window *window)
> > >>   return surface;
> > >>  }
> > >>
> > >> -static window_buffer_type
> > >> +static enum window_buffer_type
> > >>  get_preferred_buffer_type(struct display *display)
> > >>  {
> > >>  #ifdef HAVE_CAIRO_EGL
> > >> - if (display->argb_device)
> > >> + if (display->argb_device && !getenv("TOYTOOLKIT_NO_EGL"))
> > >>   return WINDOW_BUFFER_TYPE_EGL_WINDOW;
> > >>  #endif
> > >>
> > >
> > > Nice.
> > >
> > > I wonder, would it be time to finally just drop cairo-egl completely?
> > > It does raise the question on what to do with weston-gears and
> > > weston-screensaver. ISTR that krh was not fond of using sub-surfaces
> > > for them, so... just turn them into simple-egl kind of clients? Did we
> > > already have the decorations code easily integrable to non-cairo GL
> > > apps, maybe stemming from the wayland-backend work?
> >
> > I'd like to drop cairo-egl, but not in a way that makes window.c shm
> > only.  I'd like it to use GL by default and use cairo for rendering
> > assets, for example, render the frame in cairo, but use gl to stretch
> > and scale it and composite the title on top.

Any particular reason why you want to do that?

For all toytoolkit apps that do not explicitly need GL, I see using GL
as an unneeded overhead. That is especially true for e.g.
weston-desktop-shell and the VKB apps which run always.

You would still keep the shm-only path as an alternative, right? That
would mean no simplification inside toytoolkit as a bonus from getting
rid of cairo-egl.

If toytookit uses GL or GLESv2, we won't get rid of the incompatibility
we have right now, where if you compile with cairo-gles, you simply
cannot run weston-gears or weston-screensaver, because those explicitly
use desktop GL. If you compile with cairo-gl, you cannot get
subsurfaces demo's GLESv2 widget, because again you cannot mix GL and
GLESv2 in the same process.

We'd have to use something like libepoxy for window.c and write both GL
and GLESv2 paths in it, which I think would be going a bit too far for
the use cases it is intended for.

OTOH, I suppose we could mandate that all toytoolkit apps must be
desktop GL apps, iff the app itself wants to use any GL. That would
then exclude things like running subsurfaces demo on RPi, which might
otherwise be useful for testing. If we went GLESv2 only, we need to
throw weston-screensaver out, but that can be replaced if someone wants
to, and switch to GLESv2 gears which is probably just a small case of
copying some code.

Instead what I would propose is, make window.c shm-only, but also add
helpers similar to what compositor-wayland.c does to render window.c
decorations into a EGLSurface. That way window.c would not restrict
what GL an app can use, but would still support apps using any flavour
of GL.

Or would you rather have window.c support everything? As part of
window.c init, app tells it which GL flavour has been linked in if any,
and then it adapts at runtime?

I think I'd rather see window.c going in the simpler direction when
possible, after all it is not supposed to be a real versatile toolkit.
Is it?


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel