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

[Jon A. Cruz]

On 06/22/2015 10:51 PM, Giulio Camuffo wrote:
> 2015-06-23 3:52 GMT+03:00 Bill Spitzak <spitzak at gmail.com>:
>> It seems like prev
>>
>> On Mon, Jun 22, 2015 at 1:02 PM, Giulio Camuffo <giuliocamuffo at gmail.com>
>> wrote:
>>>
>>> The backends used to have lots of code dealing with weston specific
>>> configs.
>>> To allow them to be used by other compositors with their own
>>> configurations
>>> remove all the usage of weston_config from the backends code and move it
>>> in weston.c.
>>
>>
>> It seems like the previous version allowed the backend to decide on the
>> name, type, and number of config options that it understood, while this
>> requires the main program to have all that knowledge. Thus it does not seem
>> as good.
> 
> Not really. The previous versions had quite frankestein plugins, in
> that they also had an entry point specific for the compositor weston.
> Other compositors still needed to handle config in the compositor
> itself.
> By making all even for all compositors i think the new version is better.
> 

Yes... I really like that we don't have the
"I-call-you-now-you-call-me-again" loops anymore.

>>
>> Just because it is a weston_config structure does not mean it has to contain
>> the contents of a weston configuration. A different compositor could just
>> extract any backend-specific details from it's own configuration database
>> and write a dummy weston_config containing them.
>>
> 
> I'm not sre what you mean. Other compositors will not use
> weston_config, many needed functions for it are not even exported.
> And there is not just weston_config, there also is the command line
> options handling which can vary between compositors.


Aside from working on various plugin systems, I was just addressing some
of the command-line parsing/config issues for the newer testing
framework so certain concepts jumped out at me.


First, the aspect that the backends are half-dynamic and half-hardcoded
is an aspect that raised some flags in my mind right away. Since patch 2
added an API to manage instances, it seems good to try to go with that
all the way. And then minimizing the use of void* would help, since that
is often a source of problems. (to clarify, using 'void *' for an opaque
value to be returned for callbacks is good, but using one as a key input
that must be of the correct type for the situation gets to be very
problematic).


One design choice would be to have backends parse out what they need.
Another would be to remove all knowledge of parsing and parameters and
leave that up to 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.

For processing help messages, returning the info from a backend call
would help. Add a member to the struct for help and then all hardcoding
could be removed from the main code. This is a separate issue, but could
remove a lot of string duplication and chance for maintenance error.
(this was what Bryce mentioned on the thread "[PATCH weston] compositor:
Separate hardcoded backend related code from compositor"


There are many different ways to implement the details of param handling
that avoid the explicit structs added here. Backends could parse out or
the hosting weston.c/main.c could.

Anyway... since this patchset looks to be at least 80% of the way to
nicely abstracting the backends, I think it would be quite worth it to
complete the final 20% and not punt by hardcoding.



Secondary issue is that we've now ended up with overloading the term
"compositor" in our code to have "weston_compositor" and
"compositor_x11" belonging to different sets. However that does look to
me like something we can clean up in a later patchset.


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