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

[Giulio Camuffo]

2015-06-24 4:46 GMT+03:00 Jon A. Cruz <jonc at osg.samsung.com>:
> 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.

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.

>
> 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.

That probably is the final 20% that takes al long as the 80%, though. ;)

>
>
>
> 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