[PATCH weston v3 01/13] protocol: add sub-surfaces

Pekka Paalanen ppaalanen at gmail.com
Fri Apr 26 02:02:52 PDT 2013


On Thu, 25 Apr 2013 16:47:15 +0200
David Herrmann <dh.herrmann at gmail.com> wrote:

> Hi Pekka
> 
> On Thu, Apr 25, 2013 at 12:57 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > Add protocol for sub-surfaces, wl_subcompositor as the global interface,
> > and wl_subsurface as the per-surface interface extension.
> >
> > This patch is meant to be reverted, once sub-surfaces are moved into
> > Wayland core.
> >
> > Changes in v2:
> >
> > - Rewrite wl_subcompositor.get_subsurface description, and move mapping
> >   and commit details into wl_subsurface description. Check the wording
> >   in wl_subsurface.set_position description.
> >
> > - Add wl_subsurface.set_commit_mode request, and document it, with the
> >   commit_mode enum. Add bad_value error code for wl_subsurface.
> >
> > - Moved the protocol into Weston repository so we can land it upstream
> >   sooner for public exposure. It is to be moved into Wayland core later.
> >
> > - Add destroy requests to both wl_subcompositor and wl_subsurface, and
> >   document them. Experience has showed, that interfaces should always
> >   have a destructor unless there is a good and future-proof reason to not
> >   have it.
> >
> > Changes in v3:
> >
> > - Specify, that wl_subsurface will become inert, if the corresponding
> >   wl_surface is destroyed, instead of requiring a certain destruction
> >   order.
> >
> > - Replaced wl_subsurface.set_commit_mode with wl_subsurface.set_sync and
> >   wl_subsurface.set_desync. Parent-cached commit mode is now called
> >   synchronized, and independent mode is desynchronized. Removed
> >   commit_mode enum, and bad_vBut if we introduce other protocol operations that cause state to be applied, a recursive definition is alue error.
> >
> > - Added support for nested sub-surfaces.
> 
> Nice! \o/
> 
> > Signed-off-by: Pekka Paalanen <ppaalanen at gmail.com>
> > ---
> >  clients/.gitignore      |   2 +
> >  clients/Makefile.am     |   4 +
> >  clients/window.h        |   1 +
> >  protocol/subsurface.xml | 236 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/.gitignore          |   3 +
> >  src/Makefile.am         |   4 +
> >  src/compositor.h        |   1 +
> >  tests/.gitignore        |   2 +
> >  tests/Makefile.am       |   4 +
> >  9 files changed, 257 insertions(+)
> >  create mode 100644 protocol/subsurface.xml
> >
> > diff --git a/clients/.gitignore b/clients/.gitignore
> > index dcd4564..16088e8 100644
> > --- a/clients/.gitignore
> > +++ b/clients/.gitignore
> > @@ -20,6 +20,8 @@ simple-egl
> >  simple-shm
> >  simple-touch
> >  smoke
> > +subsurface-client-protocol.h
> > +subsurface-protocol.c
> >  tablet-shell-client-protocol.h
> >  tablet-shell-protocol.c
> >  text-client-protocol.h
> > diff --git a/clients/Makefile.am b/clients/Makefile.am
> > index 8c9bcd4..5f83acd 100644
> > --- a/clients/Makefile.am
> > +++ b/clients/Makefile.am
> > @@ -81,6 +81,8 @@ libtoytoolkit_la_SOURCES =                    \
> >         window.h                                \
> >         text-cursor-position-protocol.c         \
> >         text-cursor-position-client-protocol.h  \
> > +       subsurface-protocol.c                   \
> > +       subsurface-client-protocol.h            \
> >         workspaces-protocol.c                   \
> >         workspaces-client-protocol.h
> >
> > @@ -185,6 +187,8 @@ BUILT_SOURCES =                                     \
> >         desktop-shell-protocol.c                \
> >         tablet-shell-client-protocol.h          \
> >         tablet-shell-protocol.c                 \
> > +       subsurface-client-protocol.h            \
> > +       subsurface-protocol.c                   \
> >         workspaces-client-protocol.h            \
> >         workspaces-protocol.c
> >
> > diff --git a/clients/window.h b/clients/window.h
> > index c2946d8..815b3f1 100644
> > --- a/clients/window.h
> > +++ b/clients/window.h
> > @@ -27,6 +27,7 @@
> >  #include <wayland-client.h>
> >  #include <cairo.h>
> >  #include "../shared/config-parser.h"
> > +#include "subsurface-client-protocol.h"
> >
> >  #define ARRAY_LENGTH(a) (sizeof (a) / sizeof (a)[0])
> >
> > diff --git a/protocol/subsurface.xml b/protocol/subsurface.xml
> > new file mode 100644
> > index 0000000..60b4002
> > --- /dev/null
> > +++ b/protocol/subsurface.xml
> > @@ -0,0 +1,236 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="subsurface">
> > +
> > +  <copyright>
> > +    Copyright © 2012-2013 Collabora, Ltd.
> > +
> > +    Permission to use, copy, modify, distribute, and sell this
> > +    software and its documentation for any purpose is hereby granted
> > +    without fee, provided that the above copyright notice appear in
> > +    all copies and that both that copyright notice and this permission
> > +    notice appear in supporting documentation, and that the name of
> > +    the copyright holders not be used in advertising or publicity
> > +    pertaining to distribution of the software without specific,
> > +    written prior permission.  The copyright holders make no
> > +    representations about the suitability of this software for any
> > +    purpose.  It is provided "as is" without express or implied
> > +    warranty.
> > +
> > +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> > +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> > +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> > +    THIS SOFTWARE.
> > +  </copyright>
> > +
> > +  <interface name="wl_subcompositor" version="1">
> > +    <description summary="sub-surface compositing">
> > +      The global interface exposing sub-surface compositing capabilities.
> > +      A wl_surface, that has sub-surfaces associated, is called the
> > +      parent surface. Sub-surfaces can be arbitrarily nested and create
> > +      a tree of sub-surfaces.
> > +
> > +      The root surface in a tree of sub-surfaces is the main
> > +      surface. The main surface cannot be a sub-surface, because
> > +      sub-surfaces must always have a parent.
> > +
> > +      A main surface with its sub-surfaces forms a (compound) window.
> > +      For window management purposes, this set of wl_surface objects is
> > +      to be considered as a single window, and it should also behave as
> > +      such.
> > +
> > +      The aim of sub-surfaces is to offload some of the compositing work
> > +      within a window from clients to the compositor. A prime example is
> > +      a video player with decorations and video in separate wl_surface
> > +      objects. This should allow the compositor to pass YUV video buffer
> > +      processing to dedicated overlay hardware when possible.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="unbind from the subcompositor interface">
> > +       Informs the server that the client will not be using this
> > +       protocol object anymore. This does not affect any other
> > +       objects, wl_subsurface objects included.
> > +      </description>
> > +    </request>
> > +
> > +    <enum name="error">
> > +      <entry name="bad_surface" value="0"
> > +             summary="the to-be sub-surface is invalid"/>
> > +      <entry name="bad_parent" value="1"
> > +             summary="the given parent is a sub-surface"/>
> > +    </enum>
> > +
> > +    <request name="get_subsurface">
> > +      <description summary="give a surface the role sub-surface">
> > +       Create a sub-surface interface for the given surface, and
> > +       associate it with the given parent surface. This turns a
> > +       plain wl_surface into a sub-surface.
> > +
> > +       The to-be sub-surface must not already have a dedicated
> > +       purpose, like any shell surface type, cursor image, drag icon,
> > +       or sub-surface. Otherwise a protocol error is raised.
> > +      </description>
> > +
> > +      <arg name="id" type="new_id" interface="wl_subsurface"
> > +           summary="the new subsurface object id"/>
> > +      <arg name="surface" type="object" interface="wl_surface"
> > +           summary="the surface to be turned into a sub-surface"/>
> > +      <arg name="parent" type="object" interface="wl_surface"
> > +           summary="the parent surface"/>
> > +    </request>
> > +  </interface>
> > +
> > +  <interface name="wl_subsurface" version="1">
> > +    <description summary="sub-surface interface to a wl_surface">
> > +      An additional interface to a wl_surface object, which has been
> > +      made a sub-surface. A sub-surface has one parent surface.
> > +
> > +      A sub-surface becomes mapped, when a non-NULL wl_buffer is applied
> > +      and the parent surface is mapped. The order of which one happens
> > +      first is irrelevant. A sub-surface is hidden if the parent becomes
> > +      hidden, or if a NULL wl_buffer is applied. These rules apply
> > +      recursively through the tree of surfaces.
> > +
> > +      The behaviour of wl_surface.commit request on a sub-surface
> > +      depends on the sub-surface's mode. The possible modes are
> > +      synchronized and desynchronized, see methods
> > +      wl_subsurface.set_sync and wl_subsurface.set_desync. Synchronized
> > +      mode caches wl_surface state to be applied on the next parent
> > +      surface's commit, and desynchronized mode applies the pending
> 
> I had to look at the code to actually understand which implementation
> you chose. It is not clear to me (from that description) what happens
> in the case that you described earlier:
> 
> C.commit
> B.commit
> C.commit
> A.commit
> 
> (assuming A<-B<-C stacking)

That really depends on the commit modes of each surface, but if we
assume A is main, and B and C are synchronized (IIRC that was the
example), then:

C.commit(buffer C1)
- C1 becomes cached in C
B.commit(buffer B1)
- B1 becomes cached in B
C.commit(buffer C2)
- C2 becomes cached in C, and C1 is released
A.commit(buffer A1)
- A1 becomes current in A, B1 becomes current in B, C2 becomes current
  in C

Actually, C's commit mode is irrelevant. As long as B is synchronized,
C will behave as synchronized.

> According to your implementation you apply a sub-surface state if
> wl_surface.commit is called on _any_ ancestor. I think you should
> mention that explicitly. This description could mean both, but your
> description below is definitely misleading.

Hmm, no. I only apply the new state in children, if the children are
effectively in synchronized mode.

A sub-surface has a commit mode, which is either desynchronized or
synchronized. However, the effective commit mode is synchronized, if
any of sub-surface's ancestors are in synchronized mode.

Maybe I should explain the difference between the commit mode and the
effective commit mode here?

I'm uncertain how to clarify what you are asking. Can you propose
something here?

> > +      wl_surface state directly. A sub-surface is initially in the
> > +      synchronized mode.
> > +
> > +      Sub-surfaces have also other kind of state, which is managed by
> > +      wl_subsurface requests, as opposed to wl_surface requests. This
> > +      state includes the sub-surface position relative to the parent
> > +      surface (wl_subsurface.set_position), and the stacking order of
> > +      the parent and its sub-surfaces (wl_subsurface.place_above and
> > +      .place_below). This state is applied when the parent surface's
> > +      wl_surface state is applied, regardless of the sub-surface's mode.
> > +      As the exception, set_sync and set_desync are effective immediately.
> > +
> > +      The main surface can thought to be always in desynchronized mode,
> > +      since it does not have a parent in the sub-surfaces sense.
> > +
> > +      Even if a sub-surface is in desynchronized mode, it will behave as
> > +      in synchronized mode, if its parent surface behaves as in
> > +      synchronized mode. This rule is applied recursively throughout the
> > +      tree of surfaces. This means, that one can set a sub-surface into
> > +      synchronized mode, and then assume that all its child sub-surfaces
> > +      are synchronized, too, without explicitly setting them.
> > +
> > +      If the wl_surface associated with the wl_subsurface is destroyed, the
> > +      wl_subsurface object becomes inert. Note, that destroying either object
> > +      takes effect immediately. If you need to synchronize the removal
> > +      of a sub-surface to the parent surface update, unmap the sub-surface
> > +      first by attaching a NULL wl_buffer, update parent, and then destroy
> > +      the sub-surface.
> > +
> > +      If the parent wl_surface object is destroyed, the sub-surface is
> > +      unmapped.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="remove sub-surface interface">
> > +       The sub-surface interface is removed from the wl_surface object
> > +       that was turned into a sub-surface with
> > +       wl_subcompositor.get_subsurface request. The wl_surface's association
> > +       to the parent is deleted, and the wl_surface loses its role as
> > +       a sub-surface. The wl_surface is unmapped.
> > +      </description>
> > +    </request>
> > +
> > +    <enum name="error">
> > +      <entry name="bad_surface" value="0"
> > +             summary="wl_surface is not a sibling or the parent"/>
> > +    </enum>
> > +
> > +    <request name="set_position">
> > +      <description summary="reposition the sub-surface">
> > +       This schedules a sub-surface position change.
> > +       The sub-surface will be moved so, that its origin (top-left
> > +       corner pixel) will be at the location x, y of the parent surface.
> > +
> > +       The next wl_surface.commit on the parent surface will reset
> > +       the sub-surface's position to the scheduled coordinates.
> > +
> > +       The initial position is 0, 0.
> 
> Your patch doesn't mention what happens if the parent doesn't fully
> include the sub-surface. I think it should be clear whether the child
> is clipped or not.
> 
> I know you postponed the rotation/clipping extension, but we should
> still define the behavior for raw sub-surfaces. I guess "no clipping"
> is the best option. Or is that implied by not mentioning it?

I thought not mentioning anything implies no clipping, and that is how
it is implemented. Sub-surface area is in no way restricted by the
parent or ancestors.

No clipping is essential for stitching window decorations from
sub-surfaces, while eliminating surface overlap, for instance.

I'll add a note here.

I never suggested rotation. It is the clipping and scaling extension
that will follow separately, and is applicable to any wl_surface.

> > +      </description>
> > +
> > +      <arg name="x" type="int" summary="coordinate in the parent surface"/>
> > +      <arg name="y" type="int" summary="coordinate in the parent surface"/>
> > +    </request>
> > +
> > +    <request name="place_above">
> > +      <description summary="restack the sub-surface">
> > +       This sub-surface is taken from the stack, and put back just
> > +       above the reference surface, changing the z-order of the sub-surfaces.
> > +       The reference surface must be one of the sibling surfaces, or the
> > +       parent surface. Using any other surface, including this sub-surface,
> > +       will cause a protocol error.
> > +
> > +       The z-order is double-buffered state, and will be applied on the
> > +       next commit of the parent surface.
> > +       See wl_surface.commit and wl_subcompositor.get_subsurface.
> > +      </description>
> > +
> 
> Maybe I missed it, but what's the initial z-order for new
> sub-surfaces? I guess it's "top-most" but I think it should be
> mentioned either here or in the wl_subsurface description.

Yeah, it is top-most of the particular parent's children. The logical
place to document it is in wl_subsurface.place_above, right?

I'll add it.

> > +      <arg name="sibling" type="object" interface="wl_surface"
> > +           summary="the reference surface"/>
> > +    </request>
> > +
> > +    <request name="place_below">
> > +      <description summary="restack the sub-surface">
> > +       The sub-surface is placed just below of the reference surface.
> > +       See wl_subsurface.place_above.
> > +      </description>
> > +
> > +      <arg name="sibling" type="object" interface="wl_surface"
> > +           summary="the reference surface"/>
> > +    </request>
> > +
> > +    <request name="set_sync">
> > +      <description summary="set sub-surface to synchronized mode">
> > +       Change the commit behaviour of the sub-surface to synchronized
> > +       mode, also described as the parent dependant mode.
> > +
> > +       In synchronized mode, wl_surface.commit on a sub-surface will
> > +       accumulate the committed state in a cache, but the state will
> > +       not be applied and hence will not change the compositor output.
> > +       The cached state is applied to the sub-surface when
> > +       wl_surface.commit is called on the parent surface, after the
> 
> This is the description that I was talking about. It's misleading. A
> sub-surface state is _not_ necessarily applied _only_ when
> wl_surface.commit is called on the parent. Instead it is applied if
> the parent's state is applied.

Right, I need to fix that. Left-overs from before nesting support.

> On the first sight this seems to be identical scenarios, but it's not:
> 
> wl_surface.commit causes a state and all synchronized sub-surface
> states to be applied. That means, a state of a surface is applied when
> wl_surface.commit is called on _any_ ancestor (direct or indirect
> parent) not only the direct parent.
> With the current implementation this is identical to:
> wl_surface.commit is called on the top-most synchronized parent. All
> other commits in between are invisible to the client.

Yeah.

> So whether "wl_surface.commit is called" or whether a "surface's state
> is applied" are no longer identical. The first implies the second, but
> not vice versa.

Actually, neither implies the other. Btw. is it clear enough, that
commit and apply are separate steps now?

> Other than these few documentation details, the extension looks really
> nice. I doubt that we need the multi-layer-cache that we were talking
> about with v2 so I like this proposal a lot!

Oh yeah, the multi-layer cache would be madness on resource usage.


Thank you, David!
- pq


More information about the wayland-devel mailing list