| Message ID | 20181009220852.4309-5-aperez@igalia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | webkitgtk: assorted fixes and improvements | expand |
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
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.
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
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
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
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
>>>>> "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.
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.
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. >
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
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 >
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 > >
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))
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(+)