Created attachment 340013 [details] [review]
Patch for gtk-3-22

Patch also applies to current master.

I know there was irc discussion of this issue. What was the outcome ?

(In reply to Matthias Clasen from comment #3)
> I know there was irc discussion of this issue. What was the outcome ?

Emmanuele pointed out we want people to use begin_paint()/end_paint(), but Clutter doesn't and this is precisely for clutter and its subsurface on Wayland.

(All this is for bug 771320, which so far is still wip)

(In reply to Olivier Fourdan from comment #4)
> (In reply to Matthias Clasen from comment #3)
> > I know there was irc discussion of this issue. What was the outcome ?
> 
> Emmanuele pointed out we want people to use begin_paint()/end_paint(), but
> Clutter doesn't and this is precisely for clutter and its subsurface on
> Wayland.

Indeed, Clutter is "special" in that it assumes complete control over the windowing system surface.

The main issue is that begin_draw_frame()/end_draw_frame() redirect rendering to a Pixmap on X11, which is not a valid target for a GLX context, since we cannot call glXSwapBuffers() on that intermediate buffer; for Wayland it's less of a problem because the target is still going to be an EGL surface, and it's assumed that all rendering is client-side. It may be interesting to just add the begin_draw_frame()/end_draw_frame() calls when using the GDK backend on Wayland, and see if that breaks everything or not.

The only other option would be to port Cogl and Clutter to use GdkGLContext, and do their rendering on a separate FBO, but it would be a massive rework of their internals, and it's something I'm not going to commit myself doing — especially since applications are porting away from the whole Clutter API.

The path of least resistance is to update the input and opaque regions immediately after changing them, but I'm not familiar enough with Wayland to determine whether or not it's a good approach.

I suspect there might still be a race somehow because the input shape is not /always/ applied in totem (with the patch from bug 774546 to use gdk subsurfaces in Wayland).

Yet applying the input shape on configure notifications in clutter works, so /something/ is either clearing the input shape or it's not applied when done just after the subsurface creation.

Although it might be mutter because the same works fine in weston, apparently...

Created attachment 341050 [details] [review]
[PATCH gtk-3-22] wayland: apply input shape and opaque region

For subsurfaces, the new state which includes the input shape and opaque
region is not applied by the compositor if the subsurface is in
effective synchronous mode.

So we need to apply the input shape and opaque region once the
subsurface is desynchronized and its parent surface is in effective
desynchronized mode, which is when the parent surface is committed,
otherwise these may never be applied if the widget is not using
being_paint()/end_paint() (like clutter).

  <jadahl> ofourdan: i see, could we make that explicit maybe? instead of 
           sync... we do "wl_surface_set_input_region(empty)" with a comment
           saying why its empty and its fine?
<ofourdan> jadahl: sorry I didn't quite get what you mean about making it
           explicit
  <jadahl> ofourdan: instead of using the sync function, just do it directly
           with the special case (i.e. empty region) so that its obvious that
           it won't break on resize
<ofourdan> I still don't get what you mean, do you mean in gdk or in clutter?
           in clutter, we cannot as there is no api to retrieve the actual
           wayland subsurface from gdk
<ofourdan> oh, you mean, same as now but only for the special case?
  <jadahl> ofourdan: i meant in gdk, yes. make the special case more obvious by
           making it explicit
  <jadahl> ofourdan: since the fact that it'll get set to no-input is hidden 
           from the call site
  <jadahl> ofourdan: as a matter of fact, we could set the input region very 
           early, so the first ever commit catches it (if there is any before
           set_desync)

Created attachment 341478 [details] [review]
[PATCH] wayland: apply empty input shape on parent commit

For subsurfaces, the new state which includes the input shape is not
applied by the compositor if the subsurface is in effective synchronous
mode.

So we need to apply the input shape once parent surface is in effective
desynchronized mode, which is when it's committed, otherwise the input
shape may never be applied if the widget is not using being_paint() /
end_paint() to draw on its subsurface, like clutter does.

We do that only for empty input shape as those won't need update when
the subsurface is resized, for all other non-empty input shape, the
client still has to use begin_paint()/end_paint() for the input shape to
be applied.

Review of attachment 341478 [details] [review]:

Looks good. Should we think out a long term plan? I.e. if clutter wants to use GDK for some things, but still drive the drawing itself, should we add explicit API for that so we still can do the things we do in begin/end_frame()?

Comment on attachment 341478 [details] [review]
[PATCH] wayland: apply empty input shape on parent commit

attachment 341478 [details] [review] pushed to git master as commit 5c3192c - wayland: apply empty input shape on parent commit

attachment 341478 [details] [review] pushed to branch gtk-3-22 as commit 78f8f23 - wayland: apply empty input shape on parent commit