[PATCH weston 04/11] make the backends compositor-neutral

[Jon A. Cruz]

On 07/10/2015 04:32 AM, Pekka Paalanen wrote:
> 
> As I see it, the main reason the backends are .so files instead of
> built into libweston or the main executable is that we can restrict the
> dependencies.
> 
> I can well imagine a packager splitting Weston into several packages,
> for example: core, gl-renderer, x11-backend, drm-backend, xwayland
> 
> - gl-renderer pulls in dependency to libEGL and GLESv2
> - x11-backend and xwayland pull in X11 libs
> - drm-backend pulls in libdrm and libgbm (which probably pulls in Mesa
>   or something)
> 
> In all other aspects, I consider backends an integral part of
> libweston, and could just be built in to libweston instead of
> separate .so files.
> 
> It should also improve startup time if you don't have to load all
> possible libraries but only the ones you are going to need.
> 
>>> One design choice would be to have backends parse out what they need.
> 
> That is precisely where I would not want to go.
> 
>>> Another would be to remove all knowledge of parsing and parameters and
>>> leave that up to the hosting code.
> 
> That is what I believe should work best, even if it causes some burden
> on the hosting code.
> 
>>> This patch had started on the route of moving things out of the parser,
>>> but then we end up with a lot in main.c/weston.c. If this was chosen
>>> then I'd suggest breaking those parts out to a separate file so the ugly
>>> guts of backend-specific hardcoding was more encapsulated.
>>>
>>> However... I personally would prefer to see more follow-up on the API
>>> added in patch 2.
>>>
>>> One way to facilitate this would be to add a call for fetching the
>>> option-parser struct details from a backend. Then adding argc and argv
>>> to the *init() entrypoint would remove the need for all the hardcoding
>>> in weston.c/main.c. The main code could pull out known parameters from
>>> the argv list and pass a controlled (or entirely synthesized) list to
>>> the backend.
>>
>> If the main code needs to check the argv and remove unknown parameters
>> i don't see what we gain, to be honest, since we'll still have code
>> dealing with the backend parameters outside of the backends code.
>> And letting the backends parse out their options from the original
>> argv instead is not an option imho. For example, the drm backend has
>> the --use-pixman option, but that may collide with a compositor's
>> requirement to use opengl.
> 
> I agree with Giulio here. We cannot avoid the "main.c" knowing about
> all possible parameters a backend might take. Main needs to know about
> them, and main chooses which parameters it exposes to the user via
> command line and/or config file options.
> 
> Thinking about the goal of an API-stable libweston, having a
> backend-specific struct with all parameters might not be the best way;
> an alternative is the libinput way of exposing configuration options
> which has AFAIU three functions for each knob. Adding new knobs only
> adds to the API/ABI without breaking it.
> 
> We could implement the same with structs, too. Like this:
> struct weston_backend_config {
> 	enum backend_type type;
> 	int magic;
> };
> 
> struct foo_backend_config {
> 	struct weston_backend_config base;
> 	...
> };
> 
> The magic would be sizeof(struct foo_backend_config). The type would
> indicate for which backend the caller prepared this, and is just an
> additional safety.
> 
> An old main with a new libweston which added new options for a backend
> would have old magic: the backend would know to ignore the new entries.
> 
> These are only suggestions, but I believe the right approach is to have
> the main in charge of all options exposed to the user. I do not think
> letting libweston and backends to define arbitrary new options without
> the main ever knowing about them is a good idea. If you accept this,
> then the remaining question is what should the option-setting API of
> libweston look like. We can very well leave that question open for now,
> and just use whatever works at the moment.
> 
> Manufacturing an argv-like array of strings seems silly to me. It only
> leads to "snprintf" followed by "sscanf" on the other side, while we
> could just pass already parsed values through. Main needs to parse them
> anyway if it wants to filter them.
> 
> There is also the question of runtime-appearing objects that need
> configuration. Outputs are a perfect example: a hotplug may create a
> new output, and then you may want to have a special configuration for
> it. Can a main predict everything and encode all configurations a
> priori during the compositor init, or we allow the main to make up a
> configuraton when something new appears?
> 
> The latter cannot really be implemented without some kind of callbacks.
> The benefit is that a main can e.g. detect a class of outputs and apply
> particular configuration on them, without the need to list every
> possible output name in advance to achieve the same (if that is even
> possible).
> 
> One more question are parameters that a main might want to change after
> initialization. I think those are out of scope for this discussion, as
> we are replacing config file and command line options which were only
> ever set at startup. If an option becomes more dynamic than that, we
> need new API for it, maybe a la libinput.

This sounds like very solid analysis of the factors, and addresses most
of my concerns. I also discussed things a bit with Giulio on IRC to get
some more clarification on intent. (To call this out up front, once he
clarified some of his intent that removed any of my blocking concerns on
this patchset).

Point 1:

It could be inferred from some of the implementation details that the
different backends were being encapsulated away from libweston and the
main executable to allow for arbitrary ones to be added in the future.
However, that appears to be counter to the current design intent. The
follow up discussions have indicated that the focus is just more on
modularity and ease of loading rather than future-proof support for
arbitrary back-ends.

Passing the back-end specific config struct thus seems like a solid
implementation for now. However the one aspect of passing it as a void*
parameter to a common overloaded function stood out as a red flag. If
instead each back-end had a back-end specific init function that takes
the corresponding struct directly (without wrapping it in a void *) then
compile-time safety would be regained. However that in turn does
complicate the loading API a bit.

The addition of the backend_type and magic members on the init struct
would go to mitigate most of the risk from using void*, so that winds up
things nicely for the immediate timeframe.

Point 2:

Finally, there is a second design question that Giulio flagged for me:
is libweston intended to be "a library any compositor can use to
implement Wayland" or instead is it to be "a collection of related yet
independent building blocks a program can use to build a Wayland
compositor"?

This doesn't have to be set in stone immediately. However it does go to
address the points where there is code not "in libweston" but "in
main.c" that is doing work that some would consider the domain of
libweston. Starting to settle out what functionality should remain in a
main() function (command-line parameter parsing and config parsing
definitely fall into this area) and what functionality should eventually
migrate to live "inside libweston" can benefit from clarity on this
design intent.

-- 
Jon A. Cruz - Senior Open Source Developer
Samsung Open Source Group
jonc at osg.samsung.com