webkitgtk: assorted fixes and improvements

PATCH New Adrian Perez de Castro 5 participants 18 messages Oct 9, 2018 – Oct 25, 2018

lore

Share & tools

lore

Adrian Perez de Castro

author

2018-10-09 22:08 UTC #0 lore raw

cover webkitgtk: assorted fixes and improvements

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

Signature · 1 line

2.19.1

Adrian Perez de Castro

author

2018-10-09 22:08 UTC #1 lore raw

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

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 <[email protected]>

---
 package/webkitgtk/Config.in    | 8 ++++++++
 package/webkitgtk/webkitgtk.mk | 4 +---
 2 files changed, 9 insertions(+), 3 deletions(-)

package/webkitgtk/Config.in +8 -0

index 96a7ab0c94..6933248bd6 100644
@@ -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 || \

package/webkitgtk/webkitgtk.mk +1 -3

index f28417ac73..39d681b1d4 100644
@@ -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

Signature · 1 line

2.19.1

Adrian Perez de Castro

author

2018-10-09 22:08 UTC #2 lore raw

PATCH 2/4 webkitgtk: enable package for aarch64

In reply to 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 <[email protected]>

---
 package/webkitgtk/Config.in | 3 +++
 1 file changed, 3 insertions(+)

package/webkitgtk/Config.in +3 -0

index 6933248bd6..85a3af81bf 100644
@@ -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

Signature · 1 line

2.19.1

Adrian Perez de Castro

author

2018-10-09 22:08 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 package/webkitgtk/Config.in | 6 ++++++
 1 file changed, 6 insertions(+)

package/webkitgtk/Config.in +6 -0

index 85a3af81bf..bf0a150251 100644
@@ -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

Signature · 1 line

2.19.1

Adrian Perez de Castro

author

2018-10-09 22:08 UTC #4 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

In reply to 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 <[email protected]>

---
 package/webkitgtk/Config.in    | 1 +
 package/webkitgtk/webkitgtk.mk | 7 +++++++
 2 files changed, 8 insertions(+)

package/webkitgtk/Config.in +1 -0

index bf0a150251..7a83b08b60 100644
@@ -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.

package/webkitgtk/webkitgtk.mk +7 -0

index 39d681b1d4..499715ebfe 100644
@@ -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))

Signature · 1 line

2.19.1

Thomas Petazzoni

2018-10-10 19:26 UTC #5 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Hello,

Quoted 10 lines · 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

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

Signature · 3 lines

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

Yann E. MORIN

2018-10-10 20:24 UTC #6 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

On 2018-10-10 21:26 +0200, Thomas Petazzoni spake thusly:

Quoted 18 lines

Hello,
Quoted 10 lines · 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
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" ?

Quoted 7 lines

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.

Signature · 6 lines

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

Arnout Vandecappelle

2018-10-11 12:39 UTC #7 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Quoted 24 lines · Yann E. MORIN

On 2018-10-10 21:26 +0200, Thomas Petazzoni spake thusly:
Quoted 23 lines
Hello,
Quoted 10 lines · 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
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 2 lines

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

Quoted 21 lines

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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

Signature · 6 lines

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

Peter Korsgaard

2018-10-11 18:16 UTC #8 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> writes:

Hi,

Quoted 9 lines

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

Signature · 1 line

Bye, Peter Korsgaard

Yann E. MORIN

2018-10-11 19:23 UTC #9 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Peter, Arnout, All,

On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:

Quoted 8 lines

Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> 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...

Quoted 2 lines

Quoted 2 lines
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... :-(

Quoted 1 line

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.

Signature · 6 lines

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

Arnout Vandecappelle

2018-10-11 21:56 UTC #10 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Quoted 17 lines · Yann E. MORIN

Peter, Arnout, All,
On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> 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

Quoted 8 lines

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

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
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... :-(
Quoted 1 line
Exactly, which is why I suggested it to Adrian.

Apparently our BDFL disagrees :-)

Regards,
Arnout

Quoted 10 lines

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.

Signature · 6 lines

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

Adrian Perez de Castro

author

2018-10-19 18:25 UTC #11 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Hi everybody,

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

Quoted 25 lines · Arnout Vandecappelle

Quoted 22 lines · Yann E. MORIN
Peter, Arnout, All,
On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> 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

Quoted 6 lines

Quoted 3 lines
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 ?\_(?)_/?

Quoted 6 lines

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

Quoted 7 lines

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

Quoted 9 lines

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
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... :-(
Quoted 1 line
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>

Arnout Vandecappelle

2018-10-20 12:26 UTC #12 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Quoted 71 lines · Adrian Perez de Castro

Hi everybody,
I have some comments more about this topic, please read below...
Quoted 25 lines · Arnout Vandecappelle
Quoted 22 lines · Yann E. MORIN
Peter, Arnout, All,
On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> 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
Quoted 6 lines
Quoted 3 lines
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 ?\_(?)_/?
Quoted 6 lines
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
Quoted 7 lines
        ...
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:

Quoted 4 lines

if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
config BR2_PACKAGE_WEBKITGTK_USE_GSTREAMER_GL
bool "use gstreamer-gl"

Sounds good to me.

Quoted 2 lines

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

Quoted 34 lines

comment "using gstreamer-gl needs gst1-plugins-bad with the gl element enabled"
depends on !BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
endif
WDYT?
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
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... :-(
Quoted 1 line
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

Adrian Perez de Castro

author

2018-10-25 00:39 UTC #13 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Hello,

Quoted 82 lines · Arnout Vandecappelle

Quoted 71 lines · Adrian Perez de Castro
Hi everybody,
I have some comments more about this topic, please read below...
Quoted 25 lines · Arnout Vandecappelle
Quoted 22 lines · Yann E. MORIN
Peter, Arnout, All,
On 2018-10-11 20:16 +0200, Peter Korsgaard spake thusly:
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Arnout" == Arnout Vandecappelle <[email protected]> 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
Quoted 6 lines
Quoted 3 lines
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 ?\_(?)_/?
Quoted 6 lines
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
Quoted 7 lines
        ...
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:
Quoted 4 lines
if BR2_PACKAGE_WEBKITGTK_MULTIMEDIA
config BR2_PACKAGE_WEBKITGTK_USE_GSTREAMER_GL
bool "use gstreamer-gl"
Sounds good to me.
Quoted 2 lines
default y
depends on BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL

Quoted 6 lines

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

Quoted 25 lines

Quoted 25 lines
comment "using gstreamer-gl needs gst1-plugins-bad with the gl element enabled"
depends on !BR2_PACKAGE_GST1_PLUGINS_BAD_PLUGIN_GL
endif
WDYT?
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
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... :-(
Quoted 1 line
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

Quoted 8 lines

Quoted 8 lines
---
[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>

Arnout Vandecappelle

2018-10-10 22:04 UTC #14 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

In reply to Thomas Petazzoni

Quoted 10 lines · Thomas Petazzoni

Hello,
Quoted 7 lines · 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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 5 lines

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.

Quoted 3 lines

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

Signature · 6 lines

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

Thomas Petazzoni

2018-10-11 06:43 UTC #15 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Hello,

Quoted 7 lines · Arnout Vandecappelle

Quoted 3 lines
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.

Quoted 4 lines

Quoted 2 lines
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

Signature · 3 lines

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

Adrian Perez de Castro

author

2018-10-11 13:48 UTC #16 lore raw

PATCH 4/4 webkitgtk: explicitly set USE_GSTREAMER_GL build option

Hi,

Quoted 6 lines · Thomas Petazzoni

Quoted 4 lines · Arnout Vandecappelle
Quoted 3 lines
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" ?

Quoted 7 lines

Quoted 3 lines
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.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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>

Thomas Petazzoni

Applied

2018-10-10 19:23 UTC #17 lore raw

cover webkitgtk: assorted fixes and improvements

In reply to Adrian Perez de Castro

Hello,

Quoted 4 lines · Adrian Perez de Castro

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!

Quoted 1 line

webkitgtk: explicitly set USE_GSTREAMER_GL build option

I'll comment on this one.

Thanks!

Thomas

Signature · 3 lines

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