[PATCH weston] compositor: Separate hardcoded backend related code from compositor

Bryce Harrington bryce at osg.samsung.com
Tue Jun 23 15:58:40 PDT 2015 

On Tue, Jun 23, 2015 at 12:48:13PM -0700, Jon A. Cruz wrote:
> Actually... yesterday I was just bouncing off of Bryce some ideas in
> regards to tuning up the options parser to clean up a bit of the current
> mess in regards to params, help messages, keeping multiple areas in
> sync, etc.

Yeah at the time I added all those ifdef's I wondered if it'd be better
to make the options help text live with the backend code itself, to
improve modularity.  So I think this patch is a step in the right
direction.  I have some further suggestions though.
 
> I *think* I've been starting to see some of the same issues that
> JoonCheol is trying to address. It also looks to me that Giulio's work
> and this are not completely blocking of each other... and that some
> clarification might help facilitate covering both sets of needs.

It's mainly patch 4 of libweston v2 that conflicts with this patch.
Bill's review makes a good point that moving the backend init code into
weston.c makes the backend code less well organized.  I tend to agree
with him, and think keeping the backend code compartmentalized will be
more beneficial to us in the long run.

The backend init code sets up the command line options, and it makes
sense to move the backend-specific option help text to live in the
backend next to the setup code, as this patch proposes.

Bryce

> The problems seem very similar to plugin architecture issues I've worked
> on in the past, so after some review it should not be too hard to put
> forth some technical details on different ways to reconcile these needs.
> There are also a few different approaches to parameter handling that
> could be leveraged. At least I won't have to code support for Unicode
> params on Win95 again :-)
> 
> 
> On 06/23/2015 12:22 PM, Giulio Camuffo wrote:
> > 2015-06-23 21:57 GMT+03:00 JoonCheol Park <jooncheol at gmail.com>:
> >>
> >>
> >> 2015-06-24 2:49 GMT+09:00 Giulio Camuffo <giuliocamuffo at gmail.com>:
> >>>
> >>> 2015-06-23 20:40 GMT+03:00 JoonCheol Park <jooncheol at gmail.com>:
> >>>> Hi Giulio, pq!
> >>>>
> >>>>
> >>>> 2015-06-23 20:57 GMT+09:00 Pekka Paalanen <ppaalanen at gmail.com>:
> >>>>>
> >>>>> On Tue, 23 Jun 2015 14:14:53 +0300
> >>>>> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> >>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> This kinda goes in the opposite direction to my libweston patches. In
> >>>>>> that series i move the command line handling away from the backends
> >>>>>> code to make them work in multiple compositors. This moves even more
> >>>>>> compositor-specific stuff in the backends so we need to decide if we
> >>>>>> want this or libweston.
> >>>>>
> >>>>> We want libweston.
> >>>>
> >>>>
> >>>> I just quick review the libweston on the github
> >>>> (https://github.com/giucam/weston/). When it will be applied to
> >>>> mainstream ?
> >>>>
> >>>> My understanding about libweston is for reuse the x11/drm related code
> >>>> for
> >>>> other compositor project (like libinput). Yes, It is everyone want :-)
> >>>> But, since the backend structure which manages multiple plugins and load
> >>>> symbol 'backend_xxx()'  is weston compositor specific stuff and it is
> >>>> not in
> >>>> the scope of libweston, I believe my proposal is not the opposite idea
> >>>> to
> >>>> the libweston what you are preparing.
> >>>> (In your repository, my proposal may change the src/weston.c to remove
> >>>> hardcoded backend option outputs. It is not part of libweston library. )
> >>>
> >>
> >>
> >> Which branch should I see on your repository? libweston ? libweston-v2 ?
> >> I found that you mentioned structure in branch "libweston-v2". (right ?)
> > 
> > libweston-v2 was slighly old, i updated it now.
> > 
> >>
> >>>
> >>> Well, but you are adding weston specific stuff into the backends code,
> >>> that is the options handling.
> >>
> >>
> >> backend plugin (and option handling) is the *weston* compositor specific.
> >> so this is not the problem I think.
> > 
> > The backends are not weston specific, that is one of the point of
> > libweston, to share them. Only the initialization is compositor
> > specific.
> > 
> >>
> >>
> >>>
> >>> In libweston the backends do not parse
> >>> the command line or the config file, so all the backend options are
> >>> weston specific.
> >>>
> >> In your code (src/weston.c, branch libweston-v2), as you explained, the
> >> weston (compositor main) have the code for parse options for all known
> >> backend plugin before init it. But, there are many hard code option outputs
> >> for only known backend plugins. In this case, we don't have to load plugin
> >> file dynamically.
> >>
> >> And it use the "weston_compositor_init_backend" of the libweston.so which
> >> has hard coded backend list.
> >> I think this function should not be included in the libweston.so if its goal
> >> is to be reused for other compositor. Since the
> >> "weston_compositor_init_backend" is for weston specific backend plugin
> >> initialization, it is not necessary function for others.
> > 
> > actually the version i sent to the list doesn't have the backends
> > enum, they are loaded like now by using the filename of the .so or the
> > absolute path.
> > and as i said above the backends are meant to be shared between compositor.
> > 
> >>
> >>
> >>
> >>>
> >>>>
> >>>>
> >>>>>
> >>>
> >>>>>> 2015-06-23 13:29 GMT+03:00 JoonCheol Park <jooncheol at gmail.com>:
> >>>>>>> Instead of adding available backends and usage outputs at build
> >>>>>>> time,
> >>>>>>> this patch finds all available backend plugins in MODULEDIR and
> >>>>>>> prints
> >>>>>>> them. It also prints usage output for selected backend by calling
> >>>>>>> "backend_usage()" in the plugin.
> >>>>>>>
> >>>>>>> By this patch, we can remove all hardcode for backends from
> >>>>>>> compositor.
> >>>>>>> 3rd party backend plugin can be listed in help output. Backend
> >>>>>>> developer
> >>>>>>> can freely add additional description for backend.
> >>>>>>>
> >>>>>>> Signed-off-by: JoonCheol Park <jooncheol at gmail.com>
> >>>>>>> ---
> >>>>>>>  src/compositor-drm.c      |  11 ++++
> >>>>>>>  src/compositor-fbdev.c    |   9 +++
> >>>>>>>  src/compositor-headless.c |  11 ++++
> >>>>>>>  src/compositor-rdp.c      |  15 +++++
> >>>>>>>  src/compositor-rpi.c      |  12 ++++
> >>>>>>>  src/compositor-wayland.c  |  14 +++++
> >>>>>>>  src/compositor-x11.c      |  13 +++++
> >>>>>>>  src/compositor.c          | 141
> >>>>>>> ++++++++++++----------------------------------
> >>>>>>>  src/compositor.h          |   3 +
> >>>>>>>  9 files changed, 125 insertions(+), 104 deletions(-)
> >>>>>
> >>>>> JoonCheol, any particular reason you are proposing this?
> >>>>> Are you building external backends?
> >>>>
> >>>>
> >>>> Yeap, I'm trying to make some fun experiments based on headless backend
> >>>> code.
> >>>> When I tried to add new options for backend plugin, I had to add option
> >>>> outputs for my backend into main compositor code. not in the plugin
> >>>> itself.
> >>>> Since it is kind of hardcode, I thought it doesn't look good. I think
> >>>> that
> >>>> plugin should have such information of itself and compositor should load
> >>>> such information from plugin file.
> >>>>
> >>>> (similar case: In case of NPAPI, Web browser can show plugin's
> >>>> information
> >>>> by getting/calling "NP_GetMIMEDescription()" from each plugin .so file.
> >>>> e.g
> >>>> - about:config in Mozilla)
> >>>>
> >>>> So, I created this patch for better (weston specific) backend plugin
> >>>> management structure.
> >>>>
> >>>>>
> >>>>> AFAIK the Weston SDK (the installed headers) for external plugins never
> >>>>> supported external backends, so I'm curious.
> >>>>>
> >>>>
> >>>> Since this kind of plugin just need header files for building and
> >>>> weston.pc
> >>>> is also already supported, I thought that building external backend
> >>>> plugin
> >>>> is supported (and ideally possible in current version of weston)... but
> >>>> wasn't it??
> >>>> If it can be supported, it would be good for like my case. Developer can
> >>>> create the backend plugin without build all weston source.. (like other
> >>>> '-dev' pkg supported program)
> >>>>
> >>>>
> >>>>> We are going in the direction of backends becoming libweston internal
> >>>>> details, not something you can plug and switch arbitrarily, at least
> >>>>> for the middle-term.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>> pq
> >>>>
> >>>>
> >>
> >>
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > 
> 
> -- 
> Jon A. Cruz - Senior Open Source Developer
> Samsung Open Source Group
> jonc at osg.samsung.com
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

More information about the wayland-devel mailing list