diff mbox

[v8,21/28] xbmc: Add X.org/OpenGL support

Message ID 1400342276-10303-22-git-send-email-bernd.kuhls@t-online.de
State Superseded
Headers show

Commit Message

Bernd Kuhls May 17, 2014, 3:57 p.m. UTC
Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
---
 package/xbmc/Config.in |   24 ++++++++++++++++++++----
 package/xbmc/xbmc.mk   |   40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 54 insertions(+), 10 deletions(-)

Comments

Yann E. MORIN May 17, 2014, 8:55 p.m. UTC | #1
Bernd, All,

On 2014-05-17 17:57 +0200, Bernd Kuhls spake thusly:
> Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>

There are quite a few changes in that patch. A ciommit log explaining
what is being done would be good. ;-)

> ---
>  package/xbmc/Config.in |   24 ++++++++++++++++++++----
>  package/xbmc/xbmc.mk   |   40 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/package/xbmc/Config.in b/package/xbmc/Config.in
> index f001209..40a1f63 100644
> --- a/package/xbmc/Config.in
> +++ b/package/xbmc/Config.in
> @@ -2,9 +2,12 @@ comment "xbmc needs a toolchain w/ C++, IPv6, largefile, threads, wchar"
>  	depends on BR2_arm || BR2_i386 || BR2_x86_64
>  	depends on !BR2_INET_IPV6 || !BR2_INSTALL_LIBSTDCPP || !BR2_LARGEFILE || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR
>  
> -comment "xbmc requires an OpenGL ES and EGL backend"
> -	depends on BR2_arm || BR2_i386 || BR2_x86_64
> -	depends on !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES
> +comment "xbmc needs an OpenGL backend"
> +	depends on (!BR2_arm && (BR2_i386 || BR2_x86_64)) && \
> +		!BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES)
> +
> +comment "xbmc needs EGL & GLES support"

Please keep the original comment. Besides, this change makes the two
comments incoherent:

    comment "xbmc needs an OpenGL backend"
    comment "xbmc needs EGL & GLES support"

Also, I doubt the GL dependency is tied to x86, or the EGL/GLES
dependency is tied to ARM. What if an ARM platform has a full openGL
implementation? Probably XBMC would be able to use that, no?

So, I'd suggest something along those lines:

    comment "XBMC needs an openGL backend, or an OpenGL ES and EGL backend"
        depends on BR2_arm || BR2_i386 || BR2_x86_64
        depends on !BR2_PACKAGE_HAS_LIBGL || !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES

> +	depends on BR2_arm && !BR2_PACKAGE_HAS_LIBEGL && !BR2_PACKAGE_HAS_LIBGLES

In anycase, this dependency line is incorrect, it would show only when
both of libgles and libegl are not selected, while we would want it to
show when either or both are not selected.
 
> -	depends on BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES
> +	depends on BR2_PACKAGE_HAS_LIBGL || (BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES)

Parenthesis are unneeded: '&&' has precedence over '||' anyway.

> diff --git a/package/xbmc/xbmc.mk b/package/xbmc/xbmc.mk
> index 5a72dab..7fdaafc 100644
> --- a/package/xbmc/xbmc.mk
> +++ b/package/xbmc/xbmc.mk
> @@ -61,6 +56,39 @@ ifeq ($(BR2_PACKAGE_DBUS),y)
>  XBMC_DEPENDENCIES += dbus
>  endif
>  
> +ifeq ($(BR2_PACKAGE_HAS_LIBGL),y)
> +XBMC_DEPENDENCIES += \
> +	libglew \
> +	libglu \
> +	libgl \
> +	sdl_image \
> +	xlib_libX11 \
> +	xlib_libXext \
> +	xlib_libXmu \
> +	xlib_libXrandr \
> +	xlib_libXt
> +XBMC_CONF_OPT += \
> +	--enable-x11 \
> +	--enable-xrandr \
> +	--enable-gl \
> +	--enable-sdl
> +else
> +XBMC_CONF_OPT += \
> +	--disable-x11 \
> +	--disable-xrandr \
> +	--disable-gl \
> +	--disable-sdl
> +endif

Could you join dependencies so they fit on one or two lines, instead of
one pre line?

Ditto the configure options.

> +ifeq ($(BR2_PACKAGE_HAS_LIBEGL)$(BR2_PACKAGE_HAS_LIBGLES),yy)
> +XBMC_DEPENDENCIES += libegl libgles
> +XBMC_CONF_OPT += --enable-gles
> +XBMC_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags egl)" \
> +	CXXFLAGS="$(TARGET_CXXFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags egl)"

He! This XBMC_CONF_ENV did not pre-exist, and did not seem to be needed.

Since this change is part of the EGL/GLES if-block, it has no impact on
the GL case, so is not due to adding GL support.

Why do you need to add it?

If it really is needed, then:
  - either it's due to the bump to Gotham, and should go in the bump
    patch, or
  - it should be a separate patch.

Regards,
Yann E. MORIN.

> +else
> +XBMC_CONF_OPT += --disable-gles
> +endif
> +
>  ifeq ($(BR2_PACKAGE_XBMC_LIBUSB),y)
>  XBMC_DEPENDENCIES += libusb-compat
>  XBMC_CONF_OPT += --enable-libusb
> -- 
> 1.7.10.4
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Bernd Kuhls May 17, 2014, 9:01 p.m. UTC | #2
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
news:20140517205500.GL3459@free.fr: 

> Also, I doubt the GL dependency is tied to x86, or the EGL/GLES
> dependency is tied to ARM. What if an ARM platform has a full openGL
> implementation? Probably XBMC would be able to use that, no?

Hi,

xbmc depends on GLES on ARM:
https://github.com/xbmc/xbmc/blob/Gotham/configure.in#L695

I consider this patch as one of the most complicated ones regarding Kconfig, 
for which I am really not an expert ;) I will have a look at the rest of your 
comment tomorrow.

Regards, Bernd
Yann E. MORIN May 17, 2014, 9:17 p.m. UTC | #3
Bernd, All,

On 2014-05-17 23:01 +0200, Bernd Kuhls spake thusly:
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
> news:20140517205500.GL3459@free.fr: 
> 
> > Also, I doubt the GL dependency is tied to x86, or the EGL/GLES
> > dependency is tied to ARM. What if an ARM platform has a full openGL
> > implementation? Probably XBMC would be able to use that, no?
> 
> Hi,
> 
> xbmc depends on GLES on ARM:
> https://github.com/xbmc/xbmc/blob/Gotham/configure.in#L695

Ah, it's XBMC ./configure that has this constraint. OK.

> I consider this patch as one of the most complicated ones regarding Kconfig, 
> for which I am really not an expert ;)

OK, we can work on this together. ;-)

> I will have a look at the rest of your 
> comment tomorrow.

Good night, see ya!

Regards,
Yann E. MORIN.
Bernd Kuhls May 18, 2014, 11:58 a.m. UTC | #4
Hi,

"Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
news:20140517205500.GL3459@free.fr: 

>> -comment "xbmc requires an OpenGL ES and EGL backend"
>> -     depends on BR2_arm || BR2_i386 || BR2_x86_64
>> -     depends on !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES
>> +comment "xbmc needs an OpenGL backend"
>> +     depends on (!BR2_arm && (BR2_i386 || BR2_x86_64)) && \
>> +          !BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL &&
>> BR2_PACKAGE_HAS_LIBGLES) +
>> +comment "xbmc needs EGL & GLES support"
> 
> Please keep the original comment. Besides, this change makes the two
> comments incoherent:
> 
>     comment "xbmc needs an OpenGL backend"
>     comment "xbmc needs EGL & GLES support"

as I already stated ARM depends on EGL/GLES so my plan was to display

comment "xbmc needs EGL & GLES support"

on ARM, so users know what to enable, and

comment "xbmc needs an OpenGL backend"

on BR2_i386 || BR2_x86_64 only, here EGL/GLES or GL are accepted by xbmc.

> Could you join dependencies so they fit on one or two lines, instead of
> one pre line?
> 
> Ditto the configure options.

Done.

>> +XBMC_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) $(shell
>> $(PKG_CONFIG_HOST_BINARY) --cflags egl)" \ +    
>> CXXFLAGS="$(TARGET_CXXFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags
>> egl)" 
> 
> He! This XBMC_CONF_ENV did not pre-exist, and did not seem to be needed.

For Gotham it fixes a build error, this section will be a preparatory patch 
in v9.

Regards, Bernd
Yann E. MORIN May 18, 2014, 4:29 p.m. UTC | #5
Bernd,

On 2014-05-18 13:58 +0200, Bernd Kuhls spake thusly:
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
> news:20140517205500.GL3459@free.fr: 
> 
> >> -comment "xbmc requires an OpenGL ES and EGL backend"
> >> -     depends on BR2_arm || BR2_i386 || BR2_x86_64
> >> -     depends on !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES
> >> +comment "xbmc needs an OpenGL backend"
> >> +     depends on (!BR2_arm && (BR2_i386 || BR2_x86_64)) && \
> >> +          !BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL &&
> >> BR2_PACKAGE_HAS_LIBGLES) +
> >> +comment "xbmc needs EGL & GLES support"
> > 
> > Please keep the original comment. Besides, this change makes the two
> > comments incoherent:
> > 
> >     comment "xbmc needs an OpenGL backend"
> >     comment "xbmc needs EGL & GLES support"
> 
> as I already stated ARM depends on EGL/GLES

Yep, now I saw the light.

> so my plan was to display
> 
> comment "xbmc needs EGL & GLES support"
> 
> on ARM, so users know what to enable, and
> 
> comment "xbmc needs an OpenGL backend"
> 
> on BR2_i386 || BR2_x86_64 only, here EGL/GLES or GL are accepted by xbmc.

Something like:

    comment "xbmc needs an OpenGL ES and EGL backend"
        depends on BR2_arm
        depends on !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES

    comment "xbmc needs an OpenGL backend"
        depends on BR2_i386 || BR2_x86_64
        depends on !BR2_PACKAGE_HAS_LIBGL

should do the trick.

There's no need to play tricks with mutiple archs on the same depends:
        (i386 || x86_64) && !arm

is exactly the same of
        i386 || x86_64

since arm can't be set when either i386 or x86_64 are set: it's the
architecture, only one can be set at any one time. ;-)

[--SNIP--]
> >> +XBMC_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) $(shell
> >> $(PKG_CONFIG_HOST_BINARY) --cflags egl)" \ +    
> >> CXXFLAGS="$(TARGET_CXXFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags
> >> egl)" 
> > 
> > He! This XBMC_CONF_ENV did not pre-exist, and did not seem to be needed.
> 
> For Gotham it fixes a build error, this section will be a preparatory patch 
> in v9.

OK, good.

Thank you! :-)

Regards,
Yann E. MORIN.
Bernd Kuhls May 18, 2014, 4:52 p.m. UTC | #6
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
news:20140518162900.GA3631@free.fr: 

>     comment "xbmc needs an OpenGL backend"
>         depends on BR2_i386 || BR2_x86_64
>         depends on !BR2_PACKAGE_HAS_LIBGL

Hi,

BR2_i386 || BR2_x86_64 both support libgl and egl/gles, so I typed this:

comment "xbmc needs an OpenGL backend"
       depends on BR2_i386 || BR2_x86_64
       depends on !BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL && BR2
_PACKAGE_HAS_LIBGLES)

which seems to do the trick.

Regards, Bernd
Bernd Kuhls May 18, 2014, 4:58 p.m. UTC | #7
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
news:20140518162900.GA3631@free.fr: 

> Something like:
[...]
> should do the trick.

Hi,

for further comments I posted my current patch here:
http://pastebin.com/zv7jueLB (expires in one week).

Especially the last "depends on" block looks crude, but seems to work.

Regards, Bernd
Yann E. MORIN May 18, 2014, 5:07 p.m. UTC | #8
Bernd, All,

On 2014-05-18 18:52 +0200, Bernd Kuhls spake thusly:
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
> news:20140518162900.GA3631@free.fr: 
> 
> >     comment "xbmc needs an OpenGL backend"
> >         depends on BR2_i386 || BR2_x86_64
> >         depends on !BR2_PACKAGE_HAS_LIBGL
> 
> Hi,
> 
> BR2_i386 || BR2_x86_64 both support libgl and egl/gles, so I typed this:
> 
> comment "xbmc needs an OpenGL backend"

    comment "xbmc needs an OpenGL backend, or an openGL ES and EGL backend"

>        depends on BR2_i386 || BR2_x86_64
>        depends on !BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL && BR2
> _PACKAGE_HAS_LIBGLES)
> 
> which seems to do the trick.

Yep, good. :-)

Regards,
Yann E. MORIN.
Yann E. MORIN May 18, 2014, 5:29 p.m. UTC | #9
Bernd, All,

On 2014-05-18 18:58 +0200, Bernd Kuhls spake thusly:
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
> news:20140518162900.GA3631@free.fr: 
> 
> > Something like:
> [...]
> > should do the trick.
> 
> Hi,
> 
> for further comments I posted my current patch here:
> http://pastebin.com/zv7jueLB (expires in one week).
> 
> Especially the last "depends on" block looks crude, but seems to work.

Yep, it looks too dense. I think we can factor it into:

    depends on BR2_arm || BR2_i386 || BR2_x86_64
    depends on HAS_LIBEGL && HAS_LIBGLES \
            || (BR2_i386 || BR2_x86_64) && HAS_LIBGL

So, it first ensures that it is only visible for ARM or x86 (the only
archs we currently support SBMC on.)

Then, it is available only for EGL+GLES or x86+GL, since EGL+GLES is
posible on ARM and x86 alike, but full GL is only possible on x86.

I think it is much cleaner to separate the architectures dependencies
from the GL dependencies.

Of course, this would probably not scale if we were to support it on
other archs (eg. PPC), but it will be time to revisit this if/when we
happen to add that support.

Regards,
Yann E. MORIN.
Bernd Kuhls May 18, 2014, 5:40 p.m. UTC | #10
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote in 
news:20140518172928.GD3631@free.fr:

> Of course, this would probably not scale if we were to support it on
> other archs (eg. PPC), but it will be time to revisit this if/when we
> happen to add that support.

Hi,

compiling for ppc and mips was already successful here, it takes only some 
extensions for the xbmc build system and one patch for ldt_keeper.c.

I do not plan to extend my patch series with these archs now, but we should 
have them in mind, it may happen earlier then you might have thaught ;)

Regards, Bernd
Arnout Vandecappelle May 20, 2014, 4:33 p.m. UTC | #11
On 18/05/14 19:29, Yann E. MORIN wrote:
> Bernd, All,
> 
> On 2014-05-18 18:58 +0200, Bernd Kuhls spake thusly:
>> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
>> news:20140518162900.GA3631@free.fr: 
>>
>>> Something like:
>> [...]
>>> should do the trick.
>>
>> Hi,
>>
>> for further comments I posted my current patch here:
>> http://pastebin.com/zv7jueLB (expires in one week).
>>
>> Especially the last "depends on" block looks crude, but seems to work.
> 
> Yep, it looks too dense. I think we can factor it into:
> 
>     depends on BR2_arm || BR2_i386 || BR2_x86_64
>     depends on HAS_LIBEGL && HAS_LIBGLES \
>             || (BR2_i386 || BR2_x86_64) && HAS_LIBGL

 I don't agree with you Yann that the () should be left out here. Not many
people know enough about the Kconfig logic to be able to be sure about the
precedence here.

> 
> So, it first ensures that it is only visible for ARM or x86 (the only
> archs we currently support SBMC on.)
> 
> Then, it is available only for EGL+GLES or x86+GL, since EGL+GLES is
> posible on ARM and x86 alike, but full GL is only possible on x86.

 I think there's something more fundamentally inappropriate about the approach
taken here. I think the way out could be to add auxiliary symbols to identify
which provider is used. Something like:

config BR2_PACKAGE_XBMC_EGL_GLES
	bool
	default y
	depends on BR2_PACKAGE_HAS_LIBEGL
	depends on BR2_PACKAGE_HAS_LIBGLES
	depends on !BR2_PACKAGE_XBMC_GL # prefer GL if available

config BR2_PACKAGE_XBMC_GL
	bool
	depends on BR2_PACKAGE_XORG7
	depends on BR2_PACKAGE_HAS_LIBGL

comment "xbmc requires an OpenGL-capable backend"
	depends on BR2_USE_MMU
	depends on BR2_arm || BR2_i386 || BR2_x86_64
	depends on !BR2_PACKAGE_XBMC_EGL_GLES
	depends on !BR2_PACKAGE_XBMC_GL

config BR2_PACKAGE_XBMC
	...
	depends on BR2_PACKAGE_XBMC_EGL_GLES || BR2_PACKAGE_XBMC_GL


 That also allows you to use these new symbols in the .mk file instead of the
_HAS_LIBGL which IMHO is not sufficiently accurate and future-safe.


 BTW, I don't understand this comment:

# mesa3d provides libgl only with dri drivers which depend on xorg

 If that is the case, then it's up to mesa3d to add the required dependencies, no?


 Regards,
 Arnout


> 
> I think it is much cleaner to separate the architectures dependencies
> from the GL dependencies.
> 
> Of course, this would probably not scale if we were to support it on
> other archs (eg. PPC), but it will be time to revisit this if/when we
> happen to add that support.
> 
> Regards,
> Yann E. MORIN.
>
Yann E. MORIN May 20, 2014, 4:49 p.m. UTC | #12
Arnout, All,

On 2014-05-20 18:33 +0200, Arnout Vandecappelle spake thusly:
> On 18/05/14 19:29, Yann E. MORIN wrote:
> > Bernd, All,
> > 
> > On 2014-05-18 18:58 +0200, Bernd Kuhls spake thusly:
> >> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
> >> news:20140518162900.GA3631@free.fr: 
> >>
> >>> Something like:
> >> [...]
> >>> should do the trick.
> >>
> >> Hi,
> >>
> >> for further comments I posted my current patch here:
> >> http://pastebin.com/zv7jueLB (expires in one week).
> >>
> >> Especially the last "depends on" block looks crude, but seems to work.
> > 
> > Yep, it looks too dense. I think we can factor it into:
> > 
> >     depends on BR2_arm || BR2_i386 || BR2_x86_64
> >     depends on HAS_LIBEGL && HAS_LIBGLES \
> >             || (BR2_i386 || BR2_x86_64) && HAS_LIBGL
> 
>  I don't agree with you Yann that the () should be left out here. Not many
> people know enough about the Kconfig logic to be able to be sure about the
> precedence here.

Well, the precedence of the || and && operators is always the same in
virtually all languages: C, shell, java, you-name-it. kconfig does just
abide by the usual standards, here, and it's clearly stated in the
kconfig grammar.

Furthermore, leaving the () would falsely imply that || would have
precedence over &&, which is not the case.

> > So, it first ensures that it is only visible for ARM or x86 (the only
> > archs we currently support SBMC on.)
> > 
> > Then, it is available only for EGL+GLES or x86+GL, since EGL+GLES is
> > posible on ARM and x86 alike, but full GL is only possible on x86.
> 
>  I think there's something more fundamentally inappropriate about the approach
> taken here. I think the way out could be to add auxiliary symbols to identify
> which provider is used. Something like:
> 
> config BR2_PACKAGE_XBMC_EGL_GLES
> 	bool
> 	default y
> 	depends on BR2_PACKAGE_HAS_LIBEGL
> 	depends on BR2_PACKAGE_HAS_LIBGLES
> 	depends on !BR2_PACKAGE_XBMC_GL # prefer GL if available
> 
> config BR2_PACKAGE_XBMC_GL
> 	bool

Missing 'default y' here too, I guess. ;-)

> 	depends on BR2_PACKAGE_XORG7
> 	depends on BR2_PACKAGE_HAS_LIBGL

And as Bernd said, full openGL support in XBMC is not possible on ARM,
so it is missing a dependency on !BR2_arm. But the basis are here,
granted.

> comment "xbmc requires an OpenGL-capable backend"

I think we want to differentiate the full-openGL vs. openGL EGL/GLES
cases, here, since ARM can only work with EGL/GLES, while x86 can use
either. So we'd need a comment for ARM (or any arch that requires
GL/GLES), and another comment for x86 (or any arch that supports both.)

> 	depends on BR2_USE_MMU
> 	depends on BR2_arm || BR2_i386 || BR2_x86_64
> 	depends on !BR2_PACKAGE_XBMC_EGL_GLES
> 	depends on !BR2_PACKAGE_XBMC_GL
> 
> config BR2_PACKAGE_XBMC
> 	...
> 	depends on BR2_PACKAGE_XBMC_EGL_GLES || BR2_PACKAGE_XBMC_GL

Yes, this seems more sound, indeed. I tried to come up with something
like that, but I failed to properly abstract the situation. Your
solution is pretty good, I like it. ;-)

>  That also allows you to use these new symbols in the .mk file instead of the
> _HAS_LIBGL which IMHO is not sufficiently accurate and future-safe.

I disagree. _HAS_LIBGL is just that: we do have a libGL implementation
available.

Now, I never managed to grasp all those lib*GL* stuff. What about libGLX?

Regards,
Yann E. MORIN.
Peter Korsgaard May 20, 2014, 9:10 p.m. UTC | #13
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> I think there's something more fundamentally inappropriate about the approach
 >> taken here. I think the way out could be to add auxiliary symbols to identify
 >> which provider is used. Something like:
 >> 
 >> config BR2_PACKAGE_XBMC_EGL_GLES
 >> bool
 >> default y
 >> depends on BR2_PACKAGE_HAS_LIBEGL
 >> depends on BR2_PACKAGE_HAS_LIBGLES
 >> depends on !BR2_PACKAGE_XBMC_GL # prefer GL if available
 >> 

I like these.

 >> config BR2_PACKAGE_XBMC_GL
 >> bool

 > Missing 'default y' here too, I guess. ;-)

 >> depends on BR2_PACKAGE_XORG7
 >> depends on BR2_PACKAGE_HAS_LIBGL

 > And as Bernd said, full openGL support in XBMC is not possible on ARM,
 > so it is missing a dependency on !BR2_arm. But the basis are here,
 > granted.

But _HAS_LIBGL only gets selected by the (currently x86 only) mesa dri
drivers, so you don't need to specify it here (and update it if there's
ever a non-x86 mesa dri driver).


 >> comment "xbmc requires an OpenGL-capable backend"

 > I think we want to differentiate the full-openGL vs. openGL EGL/GLES
 > cases, here, since ARM can only work with EGL/GLES, while x86 can use
 > either. So we'd need a comment for ARM (or any arch that requires
 > GL/GLES), and another comment for x86 (or any arch that supports both.)

How about just saying:

"xbmc requires an OpenGL(-ES) capable backend"
Arnout Vandecappelle May 20, 2014, 11:16 p.m. UTC | #14
On 20/05/14 18:49, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2014-05-20 18:33 +0200, Arnout Vandecappelle spake thusly:
>> On 18/05/14 19:29, Yann E. MORIN wrote:
>>> Bernd, All,
>>>
>>> On 2014-05-18 18:58 +0200, Bernd Kuhls spake thusly:
>>>> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote in
>>>> news:20140518162900.GA3631@free.fr: 
>>>>
>>>>> Something like:
>>>> [...]
>>>>> should do the trick.
>>>>
>>>> Hi,
>>>>
>>>> for further comments I posted my current patch here:
>>>> http://pastebin.com/zv7jueLB (expires in one week).
>>>>
>>>> Especially the last "depends on" block looks crude, but seems to work.
>>>
>>> Yep, it looks too dense. I think we can factor it into:
>>>
>>>     depends on BR2_arm || BR2_i386 || BR2_x86_64
>>>     depends on HAS_LIBEGL && HAS_LIBGLES \
>>>             || (BR2_i386 || BR2_x86_64) && HAS_LIBGL
>>
>>  I don't agree with you Yann that the () should be left out here. Not many
>> people know enough about the Kconfig logic to be able to be sure about the
>> precedence here.
> 
> Well, the precedence of the || and && operators is always the same in
> virtually all languages: C, shell, java, you-name-it. kconfig does just
> abide by the usual standards, here, and it's clearly stated in the
> kconfig grammar.

 In shell, || and && have equal precedence. So I disagree here.

> 
> Furthermore, leaving the () would falsely imply that || would have
> precedence over &&, which is not the case.
> 
>>> So, it first ensures that it is only visible for ARM or x86 (the only
>>> archs we currently support SBMC on.)
>>>
>>> Then, it is available only for EGL+GLES or x86+GL, since EGL+GLES is
>>> posible on ARM and x86 alike, but full GL is only possible on x86.
>>
>>  I think there's something more fundamentally inappropriate about the approach
>> taken here. I think the way out could be to add auxiliary symbols to identify
>> which provider is used. Something like:
>>
>> config BR2_PACKAGE_XBMC_EGL_GLES
>> 	bool
>> 	default y
>> 	depends on BR2_PACKAGE_HAS_LIBEGL
>> 	depends on BR2_PACKAGE_HAS_LIBGLES
>> 	depends on !BR2_PACKAGE_XBMC_GL # prefer GL if available
>>
>> config BR2_PACKAGE_XBMC_GL
>> 	bool
> 
> Missing 'default y' here too, I guess. ;-)
> 
>> 	depends on BR2_PACKAGE_XORG7
>> 	depends on BR2_PACKAGE_HAS_LIBGL
> 
> And as Bernd said, full openGL support in XBMC is not possible on ARM,
> so it is missing a dependency on !BR2_arm. But the basis are here,
> granted.

 Agreed if there is a specific ARM dependency in XBMC itself. I don't agree if
this is just because only mesa3d on x86 implements libgl.

>> comment "xbmc requires an OpenGL-capable backend"
> 
> I think we want to differentiate the full-openGL vs. openGL EGL/GLES
> cases, here, since ARM can only work with EGL/GLES, while x86 can use
> either. So we'd need a comment for ARM (or any arch that requires
> GL/GLES), and another comment for x86 (or any arch that supports both.)

 Other packages use "requires an OpenGL-capable backend" even if they need
EGL/GLES. But admittedly these others are all qt5.

> 
>> 	depends on BR2_USE_MMU
>> 	depends on BR2_arm || BR2_i386 || BR2_x86_64
>> 	depends on !BR2_PACKAGE_XBMC_EGL_GLES
>> 	depends on !BR2_PACKAGE_XBMC_GL
>>
>> config BR2_PACKAGE_XBMC
>> 	...
>> 	depends on BR2_PACKAGE_XBMC_EGL_GLES || BR2_PACKAGE_XBMC_GL
> 
> Yes, this seems more sound, indeed. I tried to come up with something
> like that, but I failed to properly abstract the situation. Your
> solution is pretty good, I like it. ;-)
> 
>>  That also allows you to use these new symbols in the .mk file instead of the
>> _HAS_LIBGL which IMHO is not sufficiently accurate and future-safe.
> 
> I disagree. _HAS_LIBGL is just that: we do have a libGL implementation
> available.

 Well, if we get another libgl provider that doesn't require X, then the
_HAS_LIBGL condition will be incorrect...


 Regards,
 Arnout


> Now, I never managed to grasp all those lib*GL* stuff. What about libGLX?
> 
> Regards,
> Yann E. MORIN.
>
Bernd Kuhls May 21, 2014, 7:39 a.m. UTC | #15
Hi,

Arnout Vandecappelle <arnout@mind.be> wrote in
news:537BE23F.602@mind.be: 

> On 20/05/14 18:49, Yann E. MORIN wrote:
>> Arnout, All,

>> And as Bernd said, full openGL support in XBMC is not possible on ARM,
>> so it is missing a dependency on !BR2_arm. But the basis are here,
>> granted.
> 
>  Agreed if there is a specific ARM dependency in XBMC itself. I don't
>  agree if this is just because only mesa3d on x86 implements libgl.

xbmc needs GLES on ARM:
https://github.com/xbmc/xbmc/blob/Gotham/configure.in#L695
http://article.gmane.org/gmane.comp.lib.uclibc.buildroot/84408

>  Well, if we get another libgl provider that doesn't require X, then the
> _HAS_LIBGL condition will be incorrect...

I think it is safe to remove

depends on BR2_PACKAGE_XORG7

from BR2_PACKAGE_XBMC_GL because BR2_PACKAGE_HAS_LIBGL is currently only 
provided, when a DRI drivers was activated in mesa3d, which is only possible 
with activated xorg, v10 will contain the updated patch.

Regards, Bernd
diff mbox

Patch

diff --git a/package/xbmc/Config.in b/package/xbmc/Config.in
index f001209..40a1f63 100644
--- a/package/xbmc/Config.in
+++ b/package/xbmc/Config.in
@@ -2,9 +2,12 @@  comment "xbmc needs a toolchain w/ C++, IPv6, largefile, threads, wchar"
 	depends on BR2_arm || BR2_i386 || BR2_x86_64
 	depends on !BR2_INET_IPV6 || !BR2_INSTALL_LIBSTDCPP || !BR2_LARGEFILE || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR
 
-comment "xbmc requires an OpenGL ES and EGL backend"
-	depends on BR2_arm || BR2_i386 || BR2_x86_64
-	depends on !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES
+comment "xbmc needs an OpenGL backend"
+	depends on (!BR2_arm && (BR2_i386 || BR2_x86_64)) && \
+		!BR2_PACKAGE_HAS_LIBGL && !(BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES)
+
+comment "xbmc needs EGL & GLES support"
+	depends on BR2_arm && !BR2_PACKAGE_HAS_LIBEGL && !BR2_PACKAGE_HAS_LIBGLES
 
 menuconfig BR2_PACKAGE_XBMC
 	bool "xbmc"
@@ -22,6 +25,8 @@  menuconfig BR2_PACKAGE_XBMC
 	select BR2_PACKAGE_LIBCDIO
 	select BR2_PACKAGE_LIBCURL
 	select BR2_PACKAGE_LIBFRIBIDI
+	select BR2_PACKAGE_LIBGLEW if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_LIBGLU if BR2_PACKAGE_HAS_LIBGL
 	select BR2_PACKAGE_LIBGCRYPT
 	select BR2_PACKAGE_LIBID3TAG
 	select BR2_PACKAGE_LIBMAD
@@ -50,17 +55,28 @@  menuconfig BR2_PACKAGE_XBMC
 	select BR2_PACKAGE_PYTHON_UNICODEDATA
 	select BR2_PACKAGE_PYTHON_ZLIB
 	select BR2_PACKAGE_READLINE
+	select BR2_PACKAGE_SDL if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_SDL_X11 if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_SDL_IMAGE if BR2_PACKAGE_HAS_LIBGL
 	select BR2_PACKAGE_SQLITE
 	select BR2_PACKAGE_TAGLIB
 	select BR2_PACKAGE_TIFF
 	select BR2_PACKAGE_TINYXML
+
+	# mesa3d provides libgl only with dri drivers which depend on xorg
+	select BR2_PACKAGE_XLIB_LIBX11 if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_XLIB_LIBXEXT if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_XLIB_XMU if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_XLIB_XRANDR if BR2_PACKAGE_HAS_LIBGL
+	select BR2_PACKAGE_XLIB_XT if BR2_PACKAGE_HAS_LIBGL
+
 	select BR2_PACKAGE_YAJL
 	select BR2_PACKAGE_ZLIB
 	depends on BR2_INET_IPV6
 	depends on BR2_INSTALL_LIBSTDCPP
 	depends on BR2_LARGEFILE
 	depends on BR2_TOOLCHAIN_HAS_THREADS
-	depends on BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES
+	depends on BR2_PACKAGE_HAS_LIBGL || (BR2_PACKAGE_HAS_LIBEGL && BR2_PACKAGE_HAS_LIBGLES)
 	depends on BR2_USE_MMU # python
 	depends on BR2_USE_WCHAR
 	depends on BR2_arm || BR2_i386 || BR2_x86_64
diff --git a/package/xbmc/xbmc.mk b/package/xbmc/xbmc.mk
index 5a72dab..7fdaafc 100644
--- a/package/xbmc/xbmc.mk
+++ b/package/xbmc/xbmc.mk
@@ -14,7 +14,7 @@  XBMC_LICENSE_FILES = LICENSE.GPL
 # http://wiki.xbmc.org/index.php?title=TexturePacker
 XBMC_DEPENDENCIES = host-gawk host-gettext host-gperf host-infozip host-lzo host-sdl_image host-swig
 XBMC_DEPENDENCIES += boost bzip2 expat flac fontconfig freetype jasper jpeg \
-	libass libcdio libcurl libegl libfribidi libgcrypt libgles libmad libmodplug libmpeg2 \
+	libass libcdio libcurl libfribidi libgcrypt libmad libmodplug libmpeg2 \
 	libogg libplist libpng libsamplerate libungif libvorbis libxml2 libxslt lzo ncurses \
 	openssl pcre python readline sqlite taglib tiff tinyxml yajl zlib
 
@@ -32,7 +32,6 @@  XBMC_CONF_OPT +=  \
 	--disable-crystalhd \
 	--disable-debug \
 	--disable-dvdcss \
-	--disable-gl \
 	--disable-hal \
 	--disable-joystick \
 	--disable-mysql \
@@ -40,14 +39,10 @@  XBMC_CONF_OPT +=  \
 	--disable-optical-drive \
 	--disable-projectm \
 	--disable-pulse \
-	--disable-sdl \
 	--disable-ssh \
 	--disable-vaapi \
 	--disable-vdpau \
 	--disable-vtbdecoder \
-	--disable-x11 \
-	--disable-xrandr \
-	--enable-gles \
 	--enable-optimizations
 
 ifeq ($(BR2_PACKAGE_RPI_USERLAND),y)
@@ -61,6 +56,39 @@  ifeq ($(BR2_PACKAGE_DBUS),y)
 XBMC_DEPENDENCIES += dbus
 endif
 
+ifeq ($(BR2_PACKAGE_HAS_LIBGL),y)
+XBMC_DEPENDENCIES += \
+	libglew \
+	libglu \
+	libgl \
+	sdl_image \
+	xlib_libX11 \
+	xlib_libXext \
+	xlib_libXmu \
+	xlib_libXrandr \
+	xlib_libXt
+XBMC_CONF_OPT += \
+	--enable-x11 \
+	--enable-xrandr \
+	--enable-gl \
+	--enable-sdl
+else
+XBMC_CONF_OPT += \
+	--disable-x11 \
+	--disable-xrandr \
+	--disable-gl \
+	--disable-sdl
+endif
+
+ifeq ($(BR2_PACKAGE_HAS_LIBEGL)$(BR2_PACKAGE_HAS_LIBGLES),yy)
+XBMC_DEPENDENCIES += libegl libgles
+XBMC_CONF_OPT += --enable-gles
+XBMC_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags egl)" \
+	CXXFLAGS="$(TARGET_CXXFLAGS) $(shell $(PKG_CONFIG_HOST_BINARY) --cflags egl)"
+else
+XBMC_CONF_OPT += --disable-gles
+endif
+
 ifeq ($(BR2_PACKAGE_XBMC_LIBUSB),y)
 XBMC_DEPENDENCIES += libusb-compat
 XBMC_CONF_OPT += --enable-libusb