[RFC libwayland] Track protocol object versions inside wl_proxy.

[Pekka Paalanen]

On Sun, 5 Oct 2014 18:48:27 -0700
"Jasper St. Pierre" <jstpierre at mecheye.net> wrote:

> One of my issues is what happens for objects that have multiple parents,
> like wl_buffer or wl_callback. What do we do if another person wants to
> introduce another (like the recently proposed pointer lock extension)? We
> can recommend that users never use wl_callback directly and instead use
> their own types instead because there's no reason *not* to, but what do we
> do about wl_buffer? Or others? Do we prohibit that going forward? Do we
> strongly word against it? Do we invent new versioning rules for those?

My opinion is that:

- wl_buffer is beyond repair; we need it, it cannot be extended by
  version bumps so it is always equivalent to its version 1, and new
  factories will arise which create wl_buffer objects. As long as
  wl_buffer definition version is never bumped, we are ok.

- wl_callback is similarly stuck to version 1, but we can discourage
  new cases trying to use, especially if there is *any* imaginable
  reason one might need more than "done" with an arbitary uint32 arg.
  And we should probably discourage wl_callback in new protocols anyway.

- If someone tries to introduce a new case where an object that had
  only one factory root (the global at the root of the interface
  factory ancestry) will now have more than one, we first just NAK the
  patch. If the submitter claims there is no other way to make it work,
  we have to look closer and find another way. We need to try hard to
  not repeat the wl_buffer design, but I acknowledge that there might
  be a case where the wl_buffer design is the only working design.

> What about use cases much like wl_buffer, which really wants a subtyping
> mechanism? Recommend users use a role mechanism akin to wl_surface? Should
> we consider adding more infrastructure and helpers in libwayland to support
> "role-d objects", much like what Pekka added to Weston recently?

I don't want to propose any generic rules here for now. Let's see case
by case if a pattern emerges later.

In the wl_buffer case, we could not extend wl_buffer itself even with
associate interface (like wl_viewport is for wl_surface, or the role
mechanism), because EGL does not normally expose wl_buffer to client
code.

> On Sun, Oct 5, 2014 at 5:30 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 
> > Ping
> >
> > On Fri, Apr 11, 2014 at 1:36 AM, Jason Ekstrand <jason at jlekstrand.net>
> > wrote:
> >
> >> On Fri, Apr 11, 2014 at 2:03 AM, Pekka Paalanen <ppaalanen at gmail.com>
> >> wrote:
> >>
> >>> On Thu, 10 Apr 2014 09:42:55 -0500
> >>> Jason Ekstrand <jason at jlekstrand.net> wrote:
> >>>
> >>> > On Thu, Apr 10, 2014 at 6:37 AM, Pekka Paalanen <ppaalanen at gmail.com>
> >>> wrote:
> >>> >
> >>> > > Hi Jason,
> >>> > >
> >>> > > thanks for working on this, it does seem very useful, practically a
> >>> > > mandatory feature to support.
> >>> > >
> >>> >
> >>> > Hi Pekka,
> >>> > Yeah, I've been itching to knock this out for a while.  Just finally
> >>> got
> >>> > around to it.  Comments below.
> >>>
> >>> ...
> >>>
> >>> > > On Wed, 2 Apr 2014 09:48:29 -0500
> >>> > > Jason Ekstrand <jason at jlekstrand.net> wrote:
> >>> > >
> >>> > > > It's worth noting that there is one small backwards-compatability
> >>> issue
> >>> > > > here.  Namely, if the client is built against protocol stubs from
> >>> an
> >>> > > > earlier version of libwayland but links against a library built
> >>> against a
> >>> > > > newer version, then all objects created by the client will report a
> >>> > > version
> >>> > > > of 1.  This is because the old api uses
> >>> wl_proxy_marshal_constructor in
> >>> > > > wl_registry_bind so all objects will inherit the protocol version
> >>> of
> >>> > > > wl_display which is 1.  The library the client linked against is
> >>> aware of
> >>> > > > the wl_proxy_version function but has no way of knowing that the
> >>> library
> >>> > > > does not.
> >>> > >
> >>> > > I was about to say that wl_registry_bind() does pass the version to
> >>> > > wl_proxy_marshal_constructor, but that indeed is in the request
> >>> > > arguments and not a mandatory argument.
> >>> > >
> >>> > > But wl_proxy_marshal_constructor() would still have all the
> >>> information
> >>> > > it needs, if we special-case wl_registry.bind inside it. Ugly, but I
> >>> > > guess it'd work for wl_registry. Would that make the
> >>> > > backward-compatibility issue go away? In all other cases you would
> >>> take
> >>> > > the version from the parent wl_proxy, which you always have available
> >>> > > in wl_proxy_marshal_constructor(), and the versioned variant would
> >>> not
> >>> > > be needed?
> >>> > >
> >>> >
> >>> > Yes, we could do that, and I considered it.  However, it would only
> >>> bump
> >>> > the compatibility issue back to wl_proxy_marshal_constructor.  Older
> >>> code
> >>> > that uses wl_proxy_create inside of wl_registry_bind is still in
> >>> trouble.
> >>> >  I don't think it's a huge savings to just bump the compatibility
> >>> issues
> >>> > back to 1.3 rather than 1.4/1.5.  In the long run, I don't think it's
> >>> worth
> >>> > the mess that we would create inside wl_proxy_marshal_constructor.
> >>>
> >>> Ok, I didn't even know to think that far back. Makes sense.
> >>>
> >>> > > But I guess it would still be broken on any other request that used
> >>> the
> >>> > > interfaceless format of new_id?
> >>> > >
> >>> >
> >>> > Nope, that's not a problem.  The wayland-scanner program doesn't
> >>> actually
> >>> > special case wl_registry_bind but interfaceless new_id's in general.
> >>> >  Anything else that specifies a new_id with no interface will hit the
> >>> same
> >>> > code-path and get wl_proxy_marshal_constructor_versioned.
> >>>
> >>> I meant if we special-case wl_registry.bind, then all other requests
> >>> using interfaceless new_ids would still be in trouble. But yes, a moot
> >>> point now.
> >>>
> >>> > > > One possible solution for this is to set the version of wl_display
> >>> to
> >>> > > zero
> >>> > > > and use zero to mean "unversioned".  Then, if a library wants to
> >>> use
> >>> > > > something that's not strictly backwards-compatable, it can check
> >>> for zero
> >>> > > > and use whatever it's non-versioned fallback is.
> >>> > >
> >>> >
> >>> > Thoughts on this? ^^
> >>>
> >>> Well... if you don't know the version, is there a difference between
> >>> assuming it is 1, and knowning it is unknown and then assuming whatever?
> >>>
> >>> As I see it, the only benefit of knowing when you don't know, is that
> >>> you could then explicitly assume a higher version than 1, and then die
> >>> on a protocol error if you are wrong. I'm not sure if that is better
> >>> than assuming 1 which will always work if the application only accepts
> >>> that.
> >>>
> >>
> >> There is a case where I would do something different on unknown vs.
> >> version == 1.  In fact, it's the exact came case that inspired me to
> >> actually sit down and write this.  The wl_surface.damage request, from the
> >> perspective of EGL implementations, is completely broken on wl_surface
> >> versions 2 and 3 (trying to fix it for 4).  An EGL implementation could
> >> check for unknown, 2, or 3 and just do wl_surface.damage(0, 0, INT32_MAX,
> >> INT32_MAX) in both eglSwapBuffers and eglSwapBuffersWithDamageEXT to work
> >> around this.  Then, if the version is 2 or >= 4, they could just damage the
> >> surface correctly.  It's kind of a specific example and the only reason why
> >> we care is because we broke stuff at wl_surface version 2 but it's an
> >> example.

Jason, I can't think of any technical reason to reject that proposition,
and I don't have an alternative to propose, so whatever works, I think.

But, I would advice that we wait before implementing this, because we
at Collabora got an idea that would make verifying the correctness of
any solution easier: automated backwards compatiblity testing. I intend
to file an enchancement bug about it. Essentially it would mean a test
bot would be building and running weston and some test client against
different releases of Wayland, and verifying it actually works. It
should also include building against one version of Wayland and running
against another. I cannot imagine any developer bothering to do that
kind of testing manually.

So how about we wait a bit and see if we can get this kind of testing
rig set up? I can't promise it yet, but it's a too powerful idea to not
try.


Thanks,
pq