[Buildroot] [PATCH 0/4] webkitgtk: assorted fixes and improvements

[Buildroot] [PATCH 0/4] webkitgtk: assorted fixes and improvements

[Adrian Perez de Castro]

Hi all,

This patch set takes some of the changes proposed along with the update to
WebKitGTK+ 2.22.x, and improves them to address the review comments. In
particular, moving the logic to determine whether to enable the JavaScriptCore
JIT support is much nicer to have in the Kconfig files.

Best regards,



Adrian Perez de Castro (4):
  webkitgtk: move JSC JIT selection logic to kconfig
  webkitgtk: enable package for aarch64
  webkitgtk: enable JIT support on 32-bit MIPS
  webkitgtk: explicitly set USE_GSTREAMER_GL build option

 package/webkitgtk/Config.in    | 18 ++++++++++++++++++
 package/webkitgtk/webkitgtk.mk | 11 ++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

-- 
2.19.1

[Buildroot] [PATCH 1/4] webkitgtk: move JSC JIT selection logic to kconfig

[Adrian Perez de Castro]

This is done in preparation to enable the JavaScriptCore JIT support
for more platforms. Having the logic in Config.in scales better than
checking in the .mk file.

Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
---
 package/webkitgtk/Config.in    | 8 ++++++++
 package/webkitgtk/webkitgtk.mk | 4 +---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/package/webkitgtk/Config.in b/package/webkitgtk/Config.in
index 96a7ab0c94..6933248bd6 100644
--- a/package/webkitgtk/Config.in
+++ b/package/webkitgtk/Config.in
@@ -11,6 +11,14 @@ config BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS
 	depends on BR2_TOOLCHAIN_HAS_SYNC_4
 	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libgcrypt
 
+config BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS_JIT
+	bool
+	# ARM needs NEON for JIT.
+	default y if BR2_ARM_CPU_HAS_NEON
+	# i386 & x86_64 don't have any special requirements.
+	default y if BR2_i386
+	default y if BR2_x86_64
+
 comment "webkitgtk needs libgtk3 and a glibc toolchain w/ C++, gcc >= 6, host gcc >= 4.8"
 	depends on BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS
 	depends on !BR2_PACKAGE_LIBGTK3 || !BR2_INSTALL_LIBSTDCPP || \
diff --git a/package/webkitgtk/webkitgtk.mk b/package/webkitgtk/webkitgtk.mk
index f28417ac73..39d681b1d4 100644
--- a/package/webkitgtk/webkitgtk.mk
+++ b/package/webkitgtk/webkitgtk.mk
@@ -27,9 +27,7 @@ WEBKITGTK_CONF_OPTS = \
 	-DUSE_LIBNOTIFY=OFF \
 	-DUSE_LIBHYPHEN=OFF
 
-# ARM needs NEON for JIT
-# i386 & x86_64 don't seem to have any special requirements
-ifeq ($(BR2_ARM_CPU_HAS_NEON)$(BR2_i386)$(BR2_x86_64),y)
+ifeq ($(BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS_JIT),y)
 WEBKITGTK_CONF_OPTS += -DENABLE_JIT=ON
 else
 WEBKITGTK_CONF_OPTS += -DENABLE_JIT=OFF
-- 
2.19.1

[Buildroot] [PATCH 2/4] webkitgtk: enable package for aarch64

[Adrian Perez de Castro]

64-bit ARM is well supported, particularly in little-endian
configurations, where JavaScriptCore JIT can be enabled as well.

Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
---
 package/webkitgtk/Config.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/webkitgtk/Config.in b/package/webkitgtk/Config.in
index 6933248bd6..85a3af81bf 100644
--- a/package/webkitgtk/Config.in
+++ b/package/webkitgtk/Config.in
@@ -2,6 +2,7 @@ config BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS
 	bool
 	# ARM needs BLX, so v5t+, BE completely untested so disabled
 	default y if BR2_arm && !BR2_ARM_CPU_ARMV4
+	default y if BR2_aarch64 || BR2_aarch64_be
 	default y if BR2_i386 || BR2_x86_64
 	# Disabled on MIPS big endian due to sigbus
 	default y if BR2_mipsel || BR2_mips64el
@@ -15,6 +16,8 @@ config BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS_JIT
 	bool
 	# ARM needs NEON for JIT.
 	default y if BR2_ARM_CPU_HAS_NEON
+	# AArch64 is supported upstream but not well tested on big-endian mode.
+	default y if BR2_aarch64
 	# i386 & x86_64 don't have any special requirements.
 	default y if BR2_i386
 	default y if BR2_x86_64
-- 
2.19.1

[Buildroot] [PATCH 3/4] webkitgtk: enable JIT support on 32-bit MIPS

[Adrian Perez de Castro]

WebKitGTK+ is known to work on all 32-bit MIPS R2 processors
or newer, in little-endian mode.

Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
---
 package/webkitgtk/Config.in | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/package/webkitgtk/Config.in b/package/webkitgtk/Config.in
index 85a3af81bf..bf0a150251 100644
--- a/package/webkitgtk/Config.in
+++ b/package/webkitgtk/Config.in
@@ -21,6 +21,12 @@ config BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS_JIT
 	# i386 & x86_64 don't have any special requirements.
 	default y if BR2_i386
 	default y if BR2_x86_64
+	# JIT is known not to work on MIPS64.
+	# Plain MIPS32 (pre R2) is not well tested and likely broken.
+	# The MIPS support is completely untested in big-endian mode.
+	default y if BR2_mipsel && BR2_MIPS_CPU_MIPS32R2
+	default y if BR2_mipsel && BR2_MIPS_CPU_MIPS32R5
+	default y if BR2_mipsel && BR2_MIPS_CPU_MIPS32R6
 
 comment "webkitgtk needs libgtk3 and a glibc toolchain w/ C++, gcc >= 6, host gcc >= 4.8"
 	depends on BR2_PACKAGE_WEBKITGTK_ARCH_SUPPORTS
-- 
2.19.1

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Adrian Perez de Castro]

This covers the case where GL/GLES is available (so -DENABLE_OPENGL=ON
gets passed), which makes the webkitgtk build system assume GStreamer-GL
is available, while actually it is not.

Also, use "imply" to select the needed GStreamer-GL component, because
in general it is preferred due to better performance.

This fixes some autobuilder failures like the following:

  http://autobuild.buildroot.net/results/187796535af53ece426641ff7d88aabada281674

Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
---
 package/webkitgtk/Config.in    | 1 +
 package/webkitgtk/webkitgtk.mk | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/package/webkitgtk/Config.in b/package/webkitgtk/Config.in
index bf0a150251..7a83b08b60 100644
--- a/package/webkitgtk/Config.in
+++ b/package/webkitgtk/Config.in
@@ -101,6 +101,7 @@ config BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
 	select BR2_PACKAGE_GST1_PLUGINS_GOOD_PLUGIN_ISOMP4
 	select BR2_PACKAGE_GST1_PLUGINS_GOOD_PLUGIN_RTSP
 	select BR2_PACKAGE_GST1_LIBAV
+	imply BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
 	help
 	  This option pulls in all of the required dependencies
 	  to enable multimedia (video/audio) support.
diff --git a/package/webkitgtk/webkitgtk.mk b/package/webkitgtk/webkitgtk.mk
index 39d681b1d4..499715ebfe 100644
--- a/package/webkitgtk/webkitgtk.mk
+++ b/package/webkitgtk/webkitgtk.mk
@@ -95,4 +95,11 @@ WEBKITGTK_CONF_OPTS += -DENABLE_WAYLAND_TARGET=ON
 endif
 endif
 
+ifeq ($(BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL),y)
+WEBKITGTK_CONF_OPTS += -DUSE_GSTREAMER_GL=ON
+WEBKITGTK_DEPENDENCIES += gst1-plugins-bad
+else
+WEBKITGTK_CONF_OPTS += -DUSE_GSTREAMER_GL=OFF
+endif
+
 $(eval $(cmake-package))
-- 
2.19.1

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Thomas Petazzoni]

Hello,

On Wed, 10 Oct 2018 01:08:52 +0300, Adrian Perez de Castro wrote:
> This covers the case where GL/GLES is available (so -DENABLE_OPENGL=ON
> gets passed), which makes the webkitgtk build system assume GStreamer-GL
> is available, while actually it is not.
> 
> Also, use "imply" to select the needed GStreamer-GL component, because
> in general it is preferred due to better performance.
> 
> This fixes some autobuilder failures like the following:
> 
>   http://autobuild.buildroot.net/results/187796535af53ece426641ff7d88aabada281674

What is fixing the autobuilder issue exactly? Explicitly disabling
-DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
enabled by the "imply" ?

In the former case, then I believe we need two patches: one fixing the
autobuilder issue (i.e just the part in the .mk file) that we can
backport to older Buildroot versions as needed, and another one adding
the imply.

This usage of "imply" would be the first in Buildroot. Peter, Arnout,
Yann, anything against it ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Yann E. MORIN]

On 2018-10-10 21:26 +0200, Thomas Petazzoni spake thusly:
> Hello,
> 
> On Wed, 10 Oct 2018 01:08:52 +0300, Adrian Perez de Castro wrote:
> > This covers the case where GL/GLES is available (so -DENABLE_OPENGL=ON
> > gets passed), which makes the webkitgtk build system assume GStreamer-GL
> > is available, while actually it is not.
> > 
> > Also, use "imply" to select the needed GStreamer-GL component, because
> > in general it is preferred due to better performance.
> > 
> > This fixes some autobuilder failures like the following:
> > 
> >   http://autobuild.buildroot.net/results/187796535af53ece426641ff7d88aabada281674
> 
> What is fixing the autobuilder issue exactly? Explicitly disabling
> -DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
> enabled by the "imply" ?
> 
> In the former case, then I believe we need two patches: one fixing the
> autobuilder issue (i.e just the part in the .mk file) that we can
> backport to older Buildroot versions as needed, and another one adding
> the imply.
> 
> This usage of "imply" would be the first in Buildroot. Peter, Arnout,
> Yann, anything against it ?

I don't like it, because it makes it more complex to follow what is
going on. 'imply' is a weak 'select', as the implied symbol can still be
changed to 'n'. Where 'imply' is suefull, is with tristates, where it
ensures that a tristate can't be 'M' when the implier is 'y'. From the
kernel's doc:

  config FOO
        tristate
        imply BAZ

  config BAZ
        tristate
        depends on BAR

  The following values are possible:

        FOO             BAR             BAZ's default   choice for BAZ
        ---             ---             -------------   --------------
        n               y               n               N/m/y
        m               y               m               M/y/n
        y               y               y               Y/n
        y               n               *               N

And we do not even use tristates at all in Buildroot.

And basically, from a Buildroot point of view: either we need something,
in which case we select it (or depend on it), or it is optional, and we
let the user decide for themselves.

Optionally, if package A can optionaly use package B (or a sub-option of
it), and we want to entice the user to select it, then we just add a
sub-option to package A, something like "use B's feature", with proper
select or depends. We already do such things in many places.

So, no, I am absolutely not in favour of using 'imply' at all in Buildroot.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Arnout Vandecappelle]

On 10/10/18 22:24, Yann E. MORIN wrote:
> On 2018-10-10 21:26 +0200, Thomas Petazzoni spake thusly:
>> Hello,
>>
>> On Wed, 10 Oct 2018 01:08:52 +0300, Adrian Perez de Castro wrote:
>>> This covers the case where GL/GLES is available (so -DENABLE_OPENGL=ON
>>> gets passed), which makes the webkitgtk build system assume GStreamer-GL
>>> is available, while actually it is not.
>>>
>>> Also, use "imply" to select the needed GStreamer-GL component, because
>>> in general it is preferred due to better performance.
>>>
>>> This fixes some autobuilder failures like the following:
>>>
>>>   http://autobuild.buildroot.net/results/187796535af53ece426641ff7d88aabada281674
>>
>> What is fixing the autobuilder issue exactly? Explicitly disabling
>> -DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
>> enabled by the "imply" ?
>>
>> In the former case, then I believe we need two patches: one fixing the
>> autobuilder issue (i.e just the part in the .mk file) that we can
>> backport to older Buildroot versions as needed, and another one adding
>> the imply.
>>
>> This usage of "imply" would be the first in Buildroot. Peter, Arnout,
>> Yann, anything against it ?
> 
> I don't like it, because it makes it more complex to follow what is
> going on.

 I think this is only true because you're not used to it yet.

> 'imply' is a weak 'select', as the implied symbol can still be
> changed to 'n'.

 'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
is the place where you put the imply. (To be exact, it is the same as 'default
...' because it propagates the m/y/n state to the default, not just y/n. But
since we don't use m, it doesn't matter.)

 For this specific case, it is pretty obvious that putting a 'default y if
BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
concise and clear.

 Note also that setting a default means that in the typical use case of:

defconfig
menuconfig (add some options)
build
menuconfig (enable BR2_PACKAGE_WEBKITGTK_MULTIMEDIA)

the default will *no longer* be used (because the
BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL is not a new option), so it will *not* be
enabled. Perhaps that is what you mean with "more complex to follow what is
going on"?

 Or maybe your statement is: we shouldn't use 'default y if ...' constructs and
it was a mistake to add them to BR2_TARGET_ROOTFS_JFFS2_NOCLEANMARKER,
BR2_PACKAGE_IFUPDOWN_SCRIPTS, BR2_PACKAGE_LUA_32BITS,
BR2_TOOLCHAIN_EXTERNAL_HAS_SSP and BR2_TOOLCHAIN_EXTERNAL_INET_RPC?


> Where 'imply' is suefull, is with tristates, where it
> ensures that a tristate can't be 'M' when the implier is 'y'. From the
> kernel's doc:
> 
>   config FOO
>         tristate
>         imply BAZ
> 
>   config BAZ
>         tristate
>         depends on BAR
> 
>   The following values are possible:
> 
>         FOO             BAR             BAZ's default   choice for BAZ
>         ---             ---             -------------   --------------
>         n               y               n               N/m/y
>         m               y               m               M/y/n
>         y               y               y               Y/n
>         y               n               *               N
> 
> And we do not even use tristates at all in Buildroot.
> 
> And basically, from a Buildroot point of view: either we need something,
> in which case we select it (or depend on it), or it is optional, and we
> let the user decide for themselves.

 In recent years, we have been evolving from defaulting to the absolute minimum
towards defaulting to something sane (with the possibility to remove it again).
imply is going to be a huge step forward in that evolution, because it allows us
enable a lot of things that you really want automatically in a simple and
concise way.


> Optionally, if package A can optionaly use package B (or a sub-option of
> it), and we want to entice the user to select it, then we just add a
> sub-option to package A, something like "use B's feature", with proper
> select or depends. We already do such things in many places.

 No we don't. We try to limit such cases because it makes the Kconfig menus so
much bigger.


> So, no, I am absolutely not in favour of using 'imply' at all in Buildroot.

 So it will be up to our BFDL to decide.

 One thing is very clear though: the patch should be split!

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
 > is the place where you put the imply. (To be exact, it is the same as 'default
 > ...' because it propagates the m/y/n state to the default, not just y/n. But
 > since we don't use m, it doesn't matter.)

 >  For this specific case, it is pretty obvious that putting a 'default y if
 > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
 > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
 > concise and clear.

Exactly, which is why I suggested it to Adrian.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Yann E. MORIN]

Peter, Arnout, All,

On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
> >>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
>  >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
>  > is the place where you put the imply. (To be exact, it is the same as 'default
>  > ...' because it propagates the m/y/n state to the default, not just y/n. But
>  > since we don't use m, it doesn't matter.)
> 
>  >  For this specific case, it is pretty obvious that putting a 'default y if
>  > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

As the submitter said "in general it is preferred due to better
performance", why don't we just have, in webkitgtk:

    select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if BR2_PACKAGE_GST1_PLUGINS_BAD

Or even further:

    select BR2_PACKAGE_GST1_PLUGINS_BAD
    select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

Of course, that requires propagating the required dependencies...
But since webkitgtk already depends on libgtk3, which itself depends on
LIBEGL_WAYLAND or LIBGL, we're not too far off...

>  > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
>  > concise and clear.

Sorry, I am still not convinced that using 'imply' in Buildroot is a
good idea overall... :-(

> Exactly, which is why I suggested it to Adrian.

On the other hand, this is webkitgtk we're speaking here. It's already
huge, really huge. What would be the problem with 'select'ing
GST1_PLUGINS_BAD_PLUGIN_GL if it is known to be the 'best' solution
(even if not strictly required) ?

But I'm done speaking about it now.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Arnout Vandecappelle]

On 11/10/18 21:23, Yann E. MORIN wrote:
> Peter, Arnout, All,
> 
> On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
>>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
>>  >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
>>  > is the place where you put the imply. (To be exact, it is the same as 'default
>>  > ...' because it propagates the m/y/n state to the default, not just y/n. But
>>  > since we don't use m, it doesn't matter.)
>>
>>  >  For this specific case, it is pretty obvious that putting a 'default y if
>>  > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> 
> As the submitter said "in general it is preferred due to better
> performance", why don't we just have, in webkitgtk:
> 
>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if BR2_PACKAGE_GST1_PLUGINS_BAD
> 
> Or even further:
> 
>     select BR2_PACKAGE_GST1_PLUGINS_BAD
>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> 
> Of course, that requires propagating the required dependencies...
> But since webkitgtk already depends on libgtk3, which itself depends on
> LIBEGL_WAYLAND or LIBGL, we're not too far off...

 For this specific case, that's a very good idea indeed. Something like (to be
tested!):

config BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
        bool "multimedia support"
        select BR2_PACKAGE_GSTREAMER1
        select BR2_PACKAGE_GST1_PLUGINS_BAD
        select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if
BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
        ...

 Ideally we would also select the plugins that make sure HAS_LIB_OPENGL becomes
true, but unfortunately that's really complicated. Selecting
BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL would help, but that has the
disadvantage that it might not actually make the GL plugin available (depending
on the state of X11/wayland/...

> 
>>  > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
>>  > concise and clear.
> 
> Sorry, I am still not convinced that using 'imply' in Buildroot is a
> good idea overall... :-(
> 
>> Exactly, which is why I suggested it to Adrian.

 Apparently our BDFL disagrees :-)


 Regards,
 Arnout

> On the other hand, this is webkitgtk we're speaking here. It's already
> huge, really huge. What would be the problem with 'select'ing
> GST1_PLUGINS_BAD_PLUGIN_GL if it is known to be the 'best' solution
> (even if not strictly required) ?
> 
> But I'm done speaking about it now.
> 
> Regards,
> Yann E. MORIN.
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Adrian Perez de Castro]

Hi everybody,

I have some comments more about this topic, please read below...

On Thu, 11 Oct 2018 23:56:36 +0200, Arnout Vandecappelle <arnout@mind.be> wrote:
> 
> 
> On 11/10/18 21:23, Yann E. MORIN wrote:
> > Peter, Arnout, All,
> > 
> > On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
> >>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
> >>  >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
> >>  > is the place where you put the imply. (To be exact, it is the same as 'default
> >>  > ...' because it propagates the m/y/n state to the default, not just y/n. But
> >>  > since we don't use m, it doesn't matter.)
> >>
> >>  >  For this specific case, it is pretty obvious that putting a 'default y if
> >>  > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> > 
> > As the submitter said "in general it is preferred due to better
> > performance", why don't we just have, in webkitgtk:
> > 
> >     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if BR2_PACKAGE_GST1_PLUGINS_BAD
> > 
> > Or even further:
> > 
> >     select BR2_PACKAGE_GST1_PLUGINS_BAD
> >     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> > 
> > Of course, that requires propagating the required dependencies...
> > But since webkitgtk already depends on libgtk3, which itself depends on
> > LIBEGL_WAYLAND or LIBGL, we're not too far off...
> 
>  For this specific case, that's a very good idea indeed. Something like (to be
> tested!):

While something like this would probably work, there can be cases in which
GStreamer-GL cannot be built (for example when there is no windowing system
supported by GStreamer-GL), but WebKitGTK+ may still build and work fine with
GL support enabled and GStreamer-GL support disabled. The GL code in WebKit
has a few different methods used at runtime as fall-back to create the GL
context [1].  On the other hand GStreamer-GL has a few more requirements on
the GL implementation than WebKit for building [2] ? so I do see value in
allowing to build without GStreamer-GL ?\_(?)_/? 

> config BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
>         bool "multimedia support"
>         select BR2_PACKAGE_GSTREAMER1
>         select BR2_PACKAGE_GST1_PLUGINS_BAD
>         select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if
> BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
>         ...
> 
>  Ideally we would also select the plugins that make sure HAS_LIB_OPENGL becomes
> true, but unfortunately that's really complicated. Selecting
> BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL would help, but that has the
> disadvantage that it might not actually make the GL plugin available (depending
> on the state of X11/wayland/...

I am a bit reluctant of doing this, due to the complications in making sure
that ?BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL? gets selected for all
configuration where it's supported, and the also complicated alternative of
using ?BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL?.

There's one more issue, though: I can't remember right now the precise case
(driver, GPU, GStreamer versions, etc), but some work mates from Igalia found
a couple of cases where using GStreamer-GL in WebKit results in garbled and/or
tinted video output, so it can actually be handy if we allow to manually
disable using it. So I'm leaning towards:

  if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA

  config BR2_PACKAGE_WEBKITGTK_USE_GSTREAMER_GL
      bool "use gstreamer-gl"
	  default y
	  depends on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

  comment "using gstreamer-gl needs gst1-plugins-bad with the gl element enabled"
      depends on !BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

  endif

WDYT?

> >>  > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
> >>  > concise and clear.
> > 
> > Sorry, I am still not convinced that using 'imply' in Buildroot is a
> > good idea overall... :-(
> > 
> >> Exactly, which is why I suggested it to Adrian.
> 
>  Apparently our BDFL disagrees :-)

I guess we will stay away from using ?imply? for now :)

Best regards,


-Adri?n


---
[1] In some cases it can even be enough that the driver supports Pbuffer
    contexts which with many drivers do not even require any windowing
	system running.
[2] Now that it's going to be Halloween, I could tell you some horror stories
    about how we have managed to run WebKit on some GL/EGL ?frankendriver?
	implementations :D
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20181019/23f9ca25/attachment.asc>

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Arnout Vandecappelle]

On 19/10/2018 19:25, Adrian Perez de Castro wrote:
> Hi everybody,
> 
> I have some comments more about this topic, please read below...
> 
> On Thu, 11 Oct 2018 23:56:36 +0200, Arnout Vandecappelle <arnout@mind.be> wrote:
>>
>>
>> On 11/10/18 21:23, Yann E. MORIN wrote:
>>> Peter, Arnout, All,
>>>
>>> On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
>>>>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
>>>>  >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
>>>>  > is the place where you put the imply. (To be exact, it is the same as 'default
>>>>  > ...' because it propagates the m/y/n state to the default, not just y/n. But
>>>>  > since we don't use m, it doesn't matter.)
>>>>
>>>>  >  For this specific case, it is pretty obvious that putting a 'default y if
>>>>  > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
>>>
>>> As the submitter said "in general it is preferred due to better
>>> performance", why don't we just have, in webkitgtk:
>>>
>>>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if BR2_PACKAGE_GST1_PLUGINS_BAD
>>>
>>> Or even further:
>>>
>>>     select BR2_PACKAGE_GST1_PLUGINS_BAD
>>>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
>>>
>>> Of course, that requires propagating the required dependencies...
>>> But since webkitgtk already depends on libgtk3, which itself depends on
>>> LIBEGL_WAYLAND or LIBGL, we're not too far off...
>>
>>  For this specific case, that's a very good idea indeed. Something like (to be
>> tested!):
> 
> While something like this would probably work, there can be cases in which
> GStreamer-GL cannot be built (for example when there is no windowing system
> supported by GStreamer-GL), but WebKitGTK+ may still build and work fine with
> GL support enabled and GStreamer-GL support disabled. The GL code in WebKit
> has a few different methods used at runtime as fall-back to create the GL
> context [1].  On the other hand GStreamer-GL has a few more requirements on
> the GL implementation than WebKit for building [2] ? so I do see value in
> allowing to build without GStreamer-GL ?\_(?)_/? 
> 
>> config BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
>>         bool "multimedia support"
>>         select BR2_PACKAGE_GSTREAMER1
>>         select BR2_PACKAGE_GST1_PLUGINS_BAD
>>         select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if
>> BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
>>         ...
>>
>>  Ideally we would also select the plugins that make sure HAS_LIB_OPENGL becomes
>> true, but unfortunately that's really complicated. Selecting
>> BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL would help, but that has the
>> disadvantage that it might not actually make the GL plugin available (depending
>> on the state of X11/wayland/...
> 
> I am a bit reluctant of doing this, due to the complications in making sure
> that ?BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL? gets selected for all
> configuration where it's supported, and the also complicated alternative of
> using ?BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL?.
> 
> There's one more issue, though: I can't remember right now the precise case
> (driver, GPU, GStreamer versions, etc), but some work mates from Igalia found
> a couple of cases where using GStreamer-GL in WebKit results in garbled and/or
> tinted video output, so it can actually be handy if we allow to manually
> disable using it. So I'm leaning towards:
> 
>   if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
> 
>   config BR2_PACKAGE_WEBKITGTK_USE_GSTREAMER_GL
>       bool "use gstreamer-gl"

 Sounds good to me.

> 	  default y
> 	  depends on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

 But here, I'd prefer to have

	depends on BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
	select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

to make it slightly easier for the user to do the right thing.


 Regards,
 Arnout

> 
>   comment "using gstreamer-gl needs gst1-plugins-bad with the gl element enabled"
>       depends on !BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> 
>   endif
> 
> WDYT?
> 
>>>>  > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
>>>>  > concise and clear.
>>>
>>> Sorry, I am still not convinced that using 'imply' in Buildroot is a
>>> good idea overall... :-(
>>>
>>>> Exactly, which is why I suggested it to Adrian.
>>
>>  Apparently our BDFL disagrees :-)
> 
> I guess we will stay away from using ?imply? for now :)
> 
> Best regards,
> 
> 
> -Adri?n
> 
> 
> ---
> [1] In some cases it can even be enough that the driver supports Pbuffer
>     contexts which with many drivers do not even require any windowing
> 	system running.
> [2] Now that it's going to be Halloween, I could tell you some horror stories
>     about how we have managed to run WebKit on some GL/EGL ?frankendriver?
> 	implementations :D
> 

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Adrian Perez de Castro]

Hello,

On Sat, 20 Oct 2018 13:26:47 +0100, Arnout Vandecappelle <arnout@mind.be> wrote:
 
> On 19/10/2018 19:25, Adrian Perez de Castro wrote:
> > Hi everybody,
> > 
> > I have some comments more about this topic, please read below...
> > 
> > On Thu, 11 Oct 2018 23:56:36 +0200, Arnout Vandecappelle <arnout@mind.be> wrote:
> >>
> >>
> >> On 11/10/18 21:23, Yann E. MORIN wrote:
> >>> Peter, Arnout, All,
> >>>
> >>> On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
> >>>>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
> >>>>  >  'imply' is syntactic sugar for 'default y if ...'. The *only* thing it changes
> >>>>  > is the place where you put the imply. (To be exact, it is the same as 'default
> >>>>  > ...' because it propagates the m/y/n state to the default, not just y/n. But
> >>>>  > since we don't use m, it doesn't matter.)
> >>>>
> >>>>  >  For this specific case, it is pretty obvious that putting a 'default y if
> >>>>  > BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> >>>
> >>> As the submitter said "in general it is preferred due to better
> >>> performance", why don't we just have, in webkitgtk:
> >>>
> >>>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if BR2_PACKAGE_GST1_PLUGINS_BAD
> >>>
> >>> Or even further:
> >>>
> >>>     select BR2_PACKAGE_GST1_PLUGINS_BAD
> >>>     select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> >>>
> >>> Of course, that requires propagating the required dependencies...
> >>> But since webkitgtk already depends on libgtk3, which itself depends on
> >>> LIBEGL_WAYLAND or LIBGL, we're not too far off...
> >>
> >>  For this specific case, that's a very good idea indeed. Something like (to be
> >> tested!):
> > 
> > While something like this would probably work, there can be cases in which
> > GStreamer-GL cannot be built (for example when there is no windowing system
> > supported by GStreamer-GL), but WebKitGTK+ may still build and work fine with
> > GL support enabled and GStreamer-GL support disabled. The GL code in WebKit
> > has a few different methods used at runtime as fall-back to create the GL
> > context [1].  On the other hand GStreamer-GL has a few more requirements on
> > the GL implementation than WebKit for building [2] ? so I do see value in
> > allowing to build without GStreamer-GL ?\_(?)_/? 
> > 
> >> config BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
> >>         bool "multimedia support"
> >>         select BR2_PACKAGE_GSTREAMER1
> >>         select BR2_PACKAGE_GST1_PLUGINS_BAD
> >>         select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL if
> >> BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
> >>         ...
> >>
> >>  Ideally we would also select the plugins that make sure HAS_LIB_OPENGL becomes
> >> true, but unfortunately that's really complicated. Selecting
> >> BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL would help, but that has the
> >> disadvantage that it might not actually make the GL plugin available (depending
> >> on the state of X11/wayland/...
> > 
> > I am a bit reluctant of doing this, due to the complications in making sure
> > that ?BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL? gets selected for all
> > configuration where it's supported, and the also complicated alternative of
> > using ?BR2_PACKAGE_GST1_PLUGINS_BASE_LIB_OPENGL?.
> > 
> > There's one more issue, though: I can't remember right now the precise case
> > (driver, GPU, GStreamer versions, etc), but some work mates from Igalia found
> > a couple of cases where using GStreamer-GL in WebKit results in garbled and/or
> > tinted video output, so it can actually be handy if we allow to manually
> > disable using it. So I'm leaning towards:
> > 
> >   if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
> > 
> >   config BR2_PACKAGE_WEBKITGTK_USE_GSTREAMER_GL
> >       bool "use gstreamer-gl"
> 
>  Sounds good to me.
> 
> > 	  default y
> > 	  depends on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> 
>  But here, I'd prefer to have
> 
> 	depends on BR2_PACKAGE_GST1_PLUGINS_BASE_HAS_LIB_OPENGL
> 	select BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> 
> to make it slightly easier for the user to do the right thing.

I liked this suggestion, so I have submitted a different patch that uses
the approach above:

  https://patchwork.ozlabs.org/patch/988847/

Thanks!

-Adri?n


> >   comment "using gstreamer-gl needs gst1-plugins-bad with the gl element enabled"
> >       depends on !BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
> > 
> >   endif
> > 
> > WDYT?
> > 
> >>>>  > would be crazy. On the other hand, adding the imply in webkitgtk looks nice,
> >>>>  > concise and clear.
> >>>
> >>> Sorry, I am still not convinced that using 'imply' in Buildroot is a
> >>> good idea overall... :-(
> >>>
> >>>> Exactly, which is why I suggested it to Adrian.
> >>
> >>  Apparently our BDFL disagrees :-)
> > 
> > I guess we will stay away from using ?imply? for now :)
> > 
> > Best regards,
> > 
> > 
> > -Adri?n
> > 
> > 
> > ---
> > [1] In some cases it can even be enough that the driver supports Pbuffer
> >     contexts which with many drivers do not even require any windowing
> > 	system running.
> > [2] Now that it's going to be Halloween, I could tell you some horror stories
> >     about how we have managed to run WebKit on some GL/EGL ?frankendriver?
> > 	implementations :D
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20181025/271343d6/attachment-0001.asc>

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Arnout Vandecappelle]

On 10/10/18 21:26, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 10 Oct 2018 01:08:52 +0300, Adrian Perez de Castro wrote:
>> This covers the case where GL/GLES is available (so -DENABLE_OPENGL=ON
>> gets passed), which makes the webkitgtk build system assume GStreamer-GL
>> is available, while actually it is not.
>>
>> Also, use "imply" to select the needed GStreamer-GL component, because
>> in general it is preferred due to better performance.
>>
>> This fixes some autobuilder failures like the following:
>>
>>   http://autobuild.buildroot.net/results/187796535af53ece426641ff7d88aabada281674
> 
> What is fixing the autobuilder issue exactly? Explicitly disabling
> -DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
> enabled by the "imply" ?

 imply doesn't really enable it, it just changes the default. It is the
equivalent of adding 'default y if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' to
BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL. So imply can never fix the issue.

> 
> In the former case, then I believe we need two patches: one fixing the
> autobuilder issue (i.e just the part in the .mk file) that we can
> backport to older Buildroot versions as needed, and another one adding
> the imply.

 Indeed, it should be split.

> 
> This usage of "imply" would be the first in Buildroot. Peter, Arnout,
> Yann, anything against it ?

 Absolute not, this is a great way to use imply.

 Regards,
 Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Thomas Petazzoni]

Hello,

On Thu, 11 Oct 2018 00:04:31 +0200, Arnout Vandecappelle wrote:

> > What is fixing the autobuilder issue exactly? Explicitly disabling
> > -DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
> > enabled by the "imply" ?  
> 
>  imply doesn't really enable it, it just changes the default. It is the
> equivalent of adding 'default y if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' to
> BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL. So imply can never fix the issue.

Yes, of course, I also don't think imply can fix the issue, but that
was not clear in the commit log, and the fact that the use of imply is
mixed with the actual bug fix made it even more confusing.

> > This usage of "imply" would be the first in Buildroot. Peter, Arnout,
> > Yann, anything against it ?  
> 
>  Absolute not, this is a great way to use imply.

Seems like you and Yann disagree :-)

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 4/4] webkitgtk: explicitly set USE_GSTREAMER_GL build option

[Adrian Perez de Castro]

Hi,

On Thu, 11 Oct 2018 08:43:58 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
 
> On Thu, 11 Oct 2018 00:04:31 +0200, Arnout Vandecappelle wrote:
> 
> > > What is fixing the autobuilder issue exactly? Explicitly disabling
> > > -DUSE_GSTREAMER_GL=OFF when the plugin is not available ? Or having it
> > > enabled by the "imply" ?  
> > 
> >  imply doesn't really enable it, it just changes the default. It is the
> > equivalent of adding 'default y if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA' to
> > BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL. So imply can never fix the issue.
> 
> Yes, of course, I also don't think imply can fix the issue, but that
> was not clear in the commit log, and the fact that the use of imply is
> mixed with the actual bug fix made it even more confusing.

Indeed. What fixes the autobuilder issue is the changes to the .mk file; the
?imply? is not really needed. I'll re-submit the patch with only that change.

> > > This usage of "imply" would be the first in Buildroot. Peter, Arnout,
> > > Yann, anything against it ?  
> > 
> >  Absolute not, this is a great way to use imply.
> 
> Seems like you and Yann disagree :-)

...and once some agreement about whether to start using ?imply? or not in
Buildroot is done, we can always add that bit later :)

Cheers,


-Adri?n
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20181011/e2b06612/attachment.asc>

[Buildroot] [PATCH 0/4] webkitgtk: assorted fixes and improvements

[Thomas Petazzoni]

Hello,

On Wed, 10 Oct 2018 01:08:48 +0300, Adrian Perez de Castro wrote:

> Adrian Perez de Castro (4):
>   webkitgtk: move JSC JIT selection logic to kconfig
>   webkitgtk: enable package for aarch64
>   webkitgtk: enable JIT support on 32-bit MIPS

I've applied those first three patches. Thanks!

>   webkitgtk: explicitly set USE_GSTREAMER_GL build option

I'll comment on this one.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com