Enable GStreamer GL by default on production builds, only if the gstreamer version is higher than 1.10.
(In reply to Miguel Gomez from comment #0) > Enable GStreamer GL by default on production builds, only if the gstreamer > version is higher than 1.10. or equal, no?
Created attachment 312961 [details] Patch
Created attachment 312962 [details] Patch
(In reply to Carlos Garcia Campos from comment #1) > (In reply to Miguel Gomez from comment #0) > > Enable GStreamer GL by default on production builds, only if the gstreamer > > version is higher than 1.10. > > or equal, no? Yep. Corrected the comment in the patch, as I was already checking for >= 1.10.
Comment on attachment 312962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312962&action=review > Source/cmake/OptionsGTK.cmake:95 > -WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF) > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE ON) I think we will have to make this public, because we will require users to pass USE_GSTREAMER_GL=OFF if they are building with gst < 1.10. Or maybe we should not make this fatal and simply enable/disable it depending on the gst version. This is different than libnotify or libhyphen in that the dep is not missing but old. What do you think?
(In reply to Carlos Garcia Campos from comment #5) > Comment on attachment 312962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312962&action=review > > > Source/cmake/OptionsGTK.cmake:95 > > -WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF) > > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE ON) > > I think we will have to make this public, because we will require users to > pass USE_GSTREAMER_GL=OFF if they are building with gst < 1.10. Or maybe we > should not make this fatal and simply enable/disable it depending on the gst > version. This is different than libnotify or libhyphen in that the dep is > not missing but old. What do you think? I would make it public. Even if we default to ON, there could be distros that meet the dependencies but wanted to keep it disabled.
Comment on attachment 312962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312962&action=review Awesome! Thanks Miguel. I don't think it should be public. GStreamer is an old requirement and OpenGL is an old requirement. They can both be disabled via other flags. It already has dependencies on ENABLE_OPENGL and ENABLE_VIDEO, so just changing the flag to ON should make it good to go. I have no clue why a distro would want to keep it disabled. We should keep all distros using the default flags, or this isn't going to be maintainable. > Source/cmake/OptionsGTK.cmake:368 > + if (PC_GSTREAMER_VERSION VERSION_LESS "1.10") > + message(FATAL_ERROR "GStreamer 1.10 is required to use USE_GSTREAMER_GL.") > + endif () I would silently disable it in this case. I don't normally approve of silently disabling features, because it's easy to miss a dependency that needs to be installed and therefore miss the feature. But in this case, it's just a version check, and there's no way you want GStreamerGL if you have older GStreamer, so I think this is fine.
> I would silently disable it in this case. I don't normally approve of > silently disabling features, because it's easy to miss a dependency that > needs to be installed and therefore miss the feature. But in this case, it's > just a version check, and there's no way you want GStreamerGL if you have > older GStreamer, so I think this is fine. Sounds good. I'll follow that approach then. But I'll wait a bit for it, as I found that there are problems with gl rendering in wayland, and if we enabled gst-gl, video rendering will break there. I'll fix the gl issue first, then update this.
Created attachment 313504 [details] Patch
Comment on attachment 313504 [details] Patch Clearing flags on attachment: 313504 Committed r218630: <http://trac.webkit.org/changeset/218630>
All reviewed patches have been landed. Closing bug.
Awesome, thanks Miguel! (I notice now that I wrote this a couple comments up, but I forgot about this bug, and this really is pretty great, so I'm going to write it here again.)