[Cogl] [PATCH 3/7] CoglTexturePixmapX11: add support for stereo content

[Owen Taylor]

On Sun, 2014-07-06 at 04:56 +0100, Robert Bragg wrote:
> On Sat, Jul 5, 2014 at 3:37 PM, Owen Taylor <otaylor at fishsoup.net> wrote:
> > On Thu, Jul 3, 2014 at 9:54 PM, Robert Bragg <robert at sixbynine.org> wrote:
> >
> >>
> >> A nice idea that Neil had that avoids the need for the ref-counted
> >> pixmap registry could be to have a cogl_texture_pixmap_x11_new_right()
> >> that takes a CoglTexturePixmapX11 *left argument whereby you can keep
> >> a reference to the left eye and share as much or as little state from
> >> the left eye tfp as you need.
> >>
> >> As a way to make the apis pair up a bit more clearly, I was also
> >> thinking it could make sense to rename
> >> cogl_texture_pixmap_x11_new_stereo() to
> >> cogl_texture_pixmap_x11_new_left().
> >>
> >> It looks like this lets us avoid having a new texture object, lets us
> >> share resources and should be fairly straight forward too I think.
> >
> >
> > Hmm, so the idea is that cogl_texture_pixmap_x11_new_right() returns a
> > CoglTexturePixmapX11? It's a bit ugly in terms of internal detail,
> > since a lot of the internals would be along the lines of:
> >
> >  if (tfp->is_right)
> >    {
> >      /* forward to tfp->left */
> >    }
> >  else
> >    {
> >      /* do the normal stuff */
> >    }
> >
> > but as long as you are OK with that sort of constrained ugliness in
> > cogl-texture-pixmap-x11.c, it's certainly workable and probably ends up as
> > less lines of code than the current patch because there's no need for the
> > boilerplate for a new texture type.
> 
> I suppose it's subjective to say that adding such if statements would
> be ugly per se, and I wasn't saying your approach was ugly from my
> pov. I was more just surprised at the need for a new texture type when
> the texture conceptually seems (to me at least) to be of the same type
> only with some shared resources. We don't currently have a precedent
> for using new texture types just for internal code organisation; they
> separate distinctly different kinds of textures in the public api.

To me the right eye texture seemed different because the only public
operation on it was to use it as a texture for drawing ... but there are
many deviations in most API's from the idea that you can call all class
members on all instances of a class. OO design is never as pretty in
reality :-)

> If it were clearer to me that it really makes the code more
> maintainable to abstract things so we can split some of the tfp code
> into a separate file maybe that would be fine I just didn't get that
> impression.

It's certainly shorter your way - my original patch:

 9 files changed, 576 insertions(+), 67 deletions(-)

Without CoglTexturePixmapX11Right:

 7 files changed, 250 insertions(+), 44 deletions(-)

> I'm not sure, but something else to consider here is that using one
> texture type could remove much of the conditional code in the glx
> winsys code. It looks like the only real difference the winsys code
> needs to handle is the GLX_FRONT_LEFT_EXT vs GLX_FRONT_RIGHT_EXT
> buffer id. It could remove the need to introduce the
> CoglPixmapTextureInfoGLX struct as a means to reduce conditional
> winsys code.

I don't see this as attractive code organization. The winsys code is
also a place where we strongly have resource sharing (the GLXPixmap)
between the left and right textures... I think the code reads much
more attractively if there is a single object that has the Pixmap
and both textures, rather than a pair of objects pointing at each
other.

So I'll post a patch with the changes to the public API to use
a CoglTexturePixmapX11 for both eyes but without trying to push the
public API down to the winsys level and you can see what that looks
like.

> Looking to see where we need left vs right conditional code based on
> what you put in the right eye texture object; it looks like the
> majority of the right texture methods were forwarding to the
> corresponding left texture method as a way to share the
> implementation, and if we instead used the same type for both eyes
> then the same code should work for both with no condition.
> 
> >
> > do you imagine:
> >
> >  cogl_texture_pixmap_x11_update_area()
> >  cogl_texture_pixmap_x11_set_damage_object()
> >
> > as disallowed on the right texture, or would calling them be equivalent to
> > calling them on the left texture?
> 
> If there's no current use case for separate damage objects, it would
> seem ok to initially document that it's not allowed to set a damage
> object on a right eye. If a use case ever arises, we should hopefully
> be able to allow that without breaking things.
> 
> It seems that conceptually the damage for the two eyes isn't
> implicitly related (so it could be a meaningful and useful
> optimization to have a way to report separate damage data to a
> compositor for each eye) even though that may not technically be
> possible a.t.m. It looks like we can allow for this possibility, being
> careful with what semantics we define for these two apis.

Well, with GLX, it's simply not possible and won't be possible. With
some other protocol (Wayland, say), it might be possible. I think it's
pretty much only a conceptual optimization and not a real one, because
when a scene changes, it's going to change for both eyes.

> I'd imagine calling cogl_texture_pixmap_x11_update_area() for the
> right eye would be allowed and when it enters into the
> _x11_damage_notify in the glx winsys, it can queue a binding for both
> eyes as your patch does currently. So that an _x11_damage_notify can
> easily queue for both eyes, I'd imagine adding a *other_eye member to
> CoglTexturePixmapX11.
> 
> B.t.w out of curiosity, do you know of any plan for anyone to add
> GLX_EXT_stereo_tree support to Mesa? I have a slight concern that
> without being able to write Cogl tests for this that we can test with
> Mesa, it'll become awkward to make changes to
> cogl-texture-pixmap-x11.c in the future for fear of breaking this
> functionality. Although we don't have tfp tests currently, I expect
> we'd know pretty quickly if we broke it. With less people using this
> feature we might not find out so quickly.

I don't know of any current plans. It's a pretty big project, even
though the Intel kernel driver has support for 3D now.

 * Make stereo in Mesa work for KMS/EGL - there's some nods to stereo
   support in the code, but AFAIK it's never been actually used.
 * Add stereo support to the Present extension, hook that up in Mesa
 * Add code in the X server to track and propagate stereo status
   from subwindows to toplevel windows
 * Add stereo support to the mesa TFP code and write code for the
   GLX EXT_stereo_tree extension

Probably 1-2 months of work, depending on exactly how familiar someone
was with the complete stack.

- Owen