Bug 173406 – [GTK] Enable GStreamer GL by default on production builds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

Enable GStreamer GL by default on production builds, only if the gstreamer version is higher than 1.10.

[Carlos Garcia Campos]

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

[Miguel Gomez]

Created attachment 312961 [details]
Patch

[Miguel Gomez]

Created attachment 312962 [details]
Patch

[Miguel Gomez]

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

[Carlos Garcia Campos]

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?

[Miguel Gomez]

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

[Michael Catanzaro]

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.

[Miguel Gomez]

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

[Miguel Gomez]

Created attachment 313504 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313504 [details]
Patch

Clearing flags on attachment: 313504

Committed r218630: <http://trac.webkit.org/changeset/218630>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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