libweston backend configuration API candidates

[Pekka Paalanen]

On Tue, 16 Feb 2016 13:58:25 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Tue, Feb 16, 2016 at 01:45:03PM +0200, Pekka Paalanen wrote:
> > Hi all,
> > 
> > now we have three API candidates with patches (and my own idea[1]
> > without patches so it doesn't count). Here is what I have gathered, let
> > me know if I got something wrong.
> > 
> > 
> > Giulio's proposal:
> > https://patchwork.freedesktop.org/patch/67547/
> > 
> > It uses transparent structs that get passed through a generic function
> > in libweston to the loaded backend. Configuration format is part of
> > libweston ABI.
> > 
> > My old opinion stays: I am ok with the approach, though we probably
> > want to version the structs. Something like:
> > 
> > ------------------------------- src/compositor.h -------
> >  
> >  /* Configuration struct for a backend.
> >   *
> >   * This struct carries the configuration for a backend, and it's
> >   * passed to the backend's init entry point. The backend will
> >   * likely want to subclass this in order to handle backend specific
> >   * data.
> >   */
> >  struct weston_backend_config {
> > +	/** Major version for the backend-specific config struct
> > +	 *
> > +	 * This version must match exactly what the backend expects, otherwise
> > +	 * the struct is incompatible.
> > +	 */
> > +	uint32_t major_version;
> > +
> > +	/** Minor version of the backend-specific config struct
> > +	 *
> > +	 * This must be set to sizeof(struct backend-specific config).
> > +	 * If the value here is smaller than what the backend expects, the
> > +	 * extra config members will assume their default values.
> > +	 *
> > +	 * A value greater than what the backend expects is incompatible.
> > +	 */
> > +	size_t minor_version;
> >  };
> > 
> > ABI churn is likely, but I am not particularly concerned about it for
> > now.
> > 
> > With such struct versioning, it is possible to add new options without
> > breaking the ABI, as long as all additions are made to the end of a
> > struct and nothing existing is ever modified or removed. If existing
> > things need to be changed, major_version is bumped, which essentially
> > corresponds to an ABI break. Even then, if wanted, a backend could
> > choose to support several major versions. Whether all this flexibility
> > is actually useful, I am not sure. We could probably be fine with only
> > minor_version, and use library version bumps for major.  
> 
> Flexibility is good; it really sucks to be stuck with an unchangeable
> API.  Invariably there's going to be some deficiency or other that gets
> overlooked when the API is stabilized.
> 
> Having a built-in escape hatch to fix these types of things might also
> make reviewing of early-in-life API's simpler since we don't have to
> intensively assure perfection right off the bat.

We already have a backup plan: the parallel-installability of libweston
and no stability promises for a long time. So we wouldn't be stuck even
with the stupidest thing yet.

I would like to avoid over-engineering from the start and see how
things develop while nothing is set in stone forever.

> > Benoit's proposal:
> > https://patchwork.freedesktop.org/patch/73206/
> > 
> > This is based on Giulio's proposal, except the config structs are now
> > opaque. The config structs are created and filled with function calls.
> > These functions are exported in libweston.so, which means it exports
> > many backend-specific functions. However, these functions do not
> > require any backend dependencies to be linked in, so no needless
> > libraries are pulled in.
> > 
> > Config versioning is tied to the library versioning. This makes it
> > harder to support multiple libweston versions where the only difference
> > is adding some configuration options. The compositor must use dlsym()
> > for any functions it can live without but wants to use if available at
> > runtime.
> > 
> > As a detail, Giulio's proposal has a callback for configuring
> > (hotplugged) outputs, while Benoit passes the known output
> > configurations to the backend on start-up. The benefit of the callback
> > is that output default settings are controlled by the compositor, not
> > libweston or the backend. That is why I would prefer to have the
> > callback. The defaults may vary arbitrarily per output, and later
> > layout could perhaps be part of the configuration.  
> 
> Obviously this is going to add complexity.  It makes for a more flexible
> and decoupled design though, potentially allowing improvements to APIs
> without needing version upgrades.  I'm not sure how to weigh the
> trade-off though.

I see it more as essential functionality and a requirement: a
compositor must be allowed to configure outputs based on their
properties (list of video modes, physical size, connector type, etc.)
rather than preload some settings and hope outputs will match them in
the backend.

It's quite possible we have to generalize this away from the backend
config API scope to allow e.g. setting the output layout. So rather
than a config API callback, we'd have a libweston "here is a new
output, please tell me to hook it up" callback, and the intended API to
call from that. It might get as complicated as the compositor popping
up a dialog "you have connected a new monitor, what do you want to do
with it?"

So, whatever we will have here for output hotplug, I expect we'll need
to redo eventually, since it seems we want to have some sort of event
system for libweston to inform of things coming and going, and leaving
the compositor to hook them up. But that's fairly far in the future, I
think.

> > Quentin's proposal:
> > https://patchwork.freedesktop.org/patch/73039/
> > https://patchwork.freedesktop.org/patch/73035/
> > https://patchwork.freedesktop.org/patch/73037/
> > https://patchwork.freedesktop.org/patch/73036/
> > https://patchwork.freedesktop.org/patch/73038/
> > 
> > This proposal first sets out to build a convenience library
> > libweston.la to create more structure in the source tree. It seems we
> > sorely needed that, because the assumptions here are the opposite from
> > what has already landed in upstream. Upstream has compositor.c and
> > struct weston_compositor as libwayland items, from which we slowly
> > extract the things that do not belong in libweston. Quentin's proposal
> > assumes compositor.c to be initally outside of libweston and slowly
> > moving all applicable bits into libweston under the new lib/ directory.
> > This confusion has to be taken into account when looking at the pathces.
> > 
> > The essential idea of this API is, that the compositor will register
> > configuration entry getter functions with libweston. These functions
> > are per config item type: a getter for ints, a getter for strings, etc.
> > Backends will then call these getters to retrieve configuration values
> > one by one.
> > 
> > The configuration options are identified explicitly by { section, key }
> > name tuples rather than a key name alone in the API. The getters get
> > also passed in a default value picked by the backend, in case the
> > compositor does not recognize what it is being asked for.
> > 
> > The benefits of this approach include that the library ABI is very
> > stable, as it includes only per-type getter functions. The compositor
> > implementation is also free to pass unrecognized options through:
> > adding a new option to a backend does not necessarily require updating
> > the compositor to understand it, as a user can make the setting in a
> > configuration file and it will be passed as "data" through to the
> > backend.
> > 
> > Configuring dynamically added outputs is no different: a backend will
> > just query some more options.  
> 
> This gives extreme flexibility and seems to obviate all of the API/ABI
> breakage risks, however I worry it ends up just shifting the problem
> down the stack.  All the same problems of incompatible parameters or
> non-backwards compatible configuration changes still exist, just now
> they have to be handled on more of a case by case basis by each
> compositor within their configuration handling code.
> 
> There is also a trade-off in that the more flexible the configuration
> parameters become, the more testing permutations are required to ensure
> QC coverage (from a community-wide perspective).

Good points, I agree.

Quentin's approach seems over-engineered to me until we find out how
much we will actually need.

> > What I see as the downsides here are the (arguable) complexity of
> > setting up the getters, and not having an explicit way of knowing
> > whether all your options were actually used - you'd have to track that
> > yourself in your compositor. Of course, structs have the same problem,
> > so at least here you could check it if you wanted. Detecting e.g.
> > mispelled configuration items is fairly hard, because the decision of
> > what is a valid key or not is hidden inside libweston.
> > 
> > This is also lacking any explicit notion of a transaction. With structs
> > it's easy: you pass a pointer to a function, and once the function
> > returns, the configuration is in. Here we need to document which
> > function calls query which configurations at what time. This can be
> > awkward with output hotplugging, and the compositor may not even know
> > when libweston/backend has finished querying the options.
> > 
> > I also see the unrecognized option pass-through to the backend as a
> > double-edged sword. It fits perfectly, if users are expecting to be
> > configuring libweston. However, I believe users are primarily using the
> > compositor, not libweston. The compositor may make assumptions on how
> > the backend is configured, and if a user can override those
> > assumptions, it's an opportunity for the user to shoot himself in the
> > foot by blindly copying instructions from shady websites. Of course, a
> > compositor can prevent this, but is there enough reason to support
> > pass-through in the first place?  
> 
> This is a good point.  How many different backends do we expect to see,
> and how heterogeneous do we expect their configuration needs to be?  Do
> we expect that they'll be needing to change configuration frequently, or
> just modestly, or hardly at all?

So far we expect that all libweston backends will live in the weston
repository, and do not design for allowing third party backends.

> If we're just expecting a handful of (known) users, and their
> configuration needs are going to be relatively tame, then possibly the
> simpler the better.
> 
> If we're instead expecting there'll be unknown users (e.g. third parties
> who we want to facilitate doing their own Wayland stuff completely
> independently of the Wayland community), then a more strongly decoupled
> architecture would perhaps be better.
> 
> If we expect a huge amount of variation here, and a lot of different
> implementer/users, then the more stable, robust and flexible we can make
> it the better.
> 
> > In summary, Quentin's proposal seems the most flexible, which naturally
> > makes it the hardest to program for, while also promising a very stable
> > library ABI.
> > 
> > 
> > I can't see an obvious winner in any of the above, they all can be made
> > to work, and do not seem to have particularly huge disadvantages at the
> > moment. We need to decide which properties we value the most.
> > 
> > In that light, I think the person who will be implementing the config
> > API and converting everything over should make the call. It's not like
> > we can't change it later, it would just be more work.  
> 
> If it's feasible for us to change it later, what if we pick the simplest
> solution that we know will solve current needs - the transparent struct
> - and just plan to revisit the decision when conditions change.  That
> would give time to gain experience with the design and maybe help make
> the next design that much better informed?

Yes, nowadays I tend to think this is the best course of action. We
have now seen what some alternatives could be, and that work is not
wasted - we can come back to them if we find the simple approach to
fail.

There could even be a link to this email thread in the code comments.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160217/b40925f6/attachment-0001.sig>