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

[Robert Bragg]

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.

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.

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.

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.

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.

--
Regards,
Robert

>
> - Owen