Bug 175625 – [GStreamer][GTK][WPE] Move common things to GStreamer cmake files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GStreamer][GTK][WPE] Move common things to GStreamer cmake files

[Xabier Rodríguez Calvar]

Created attachment 318251 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 318251 [details]
Patch

Setting r- because patch doesn't apply.
We should test this on the EWS before landing.

[Carlos Alberto Lopez Perez]

Comment on attachment 318251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318251&action=review

> Source/cmake/OptionsGStreamerDefinitions.cmake:5
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF)

Maybe we should turn this on by default also for WPE? Or is there a reason to have different default values for this option between ports?
Therefore we can change here the default value to ON and remove the line
" WEBKIT_OPTION_DEFAULT_PORT_VALUE(USE_GSTREAMER_GL PRIVATE ON) "
from Source/cmake/OptionsGTK.cmake

[Xabier Rodríguez Calvar]

Created attachment 318261 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Carlos Alberto Lopez Perez from comment #3)
> > Source/cmake/OptionsGStreamerDefinitions.cmake:5
> > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF)
> 
> Maybe we should turn this on by default also for WPE? Or is there a reason
> to have different default values for this option between ports?
> Therefore we can change here the default value to ON and remove the line
> " WEBKIT_OPTION_DEFAULT_PORT_VALUE(USE_GSTREAMER_GL PRIVATE ON) "
> from Source/cmake/OptionsGTK.cmake

I didn't touch this and I won't unless Žan or Miguel gives a green light to it.

I did rebase the patch and made VIDEO_TRACK private for WPE.

[Michael Catanzaro]

Comment on attachment 318251 [details]
Patch

I think I would prefer to duplicate the contents of OptionsGStreamerDefinitions.cmake in both port files... it's just easier to read without having to open up a new file. But I like OptionsGStreamerChecks.cmake. Maybe rename it to GStreamerChecks.cmake?

[Carlos Alberto Lopez Perez]

(In reply to Xabier Rodríguez Calvar from comment #5)
> (In reply to Carlos Alberto Lopez Perez from comment #3)
> > > Source/cmake/OptionsGStreamerDefinitions.cmake:5
> > > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF)
> > 
> > Maybe we should turn this on by default also for WPE? Or is there a reason
> > to have different default values for this option between ports?
> > Therefore we can change here the default value to ON and remove the line
> > " WEBKIT_OPTION_DEFAULT_PORT_VALUE(USE_GSTREAMER_GL PRIVATE ON) "
> > from Source/cmake/OptionsGTK.cmake
> 
> I didn't touch this and I won't unless Žan or Miguel gives a green light to
> it.

Miguel was the one proposing to turn this on by default on GTK+. Check bug 173406


> 
> I did rebase the patch and made VIDEO_TRACK private for WPE.

Good

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 318251 [details]
> Patch
> 
> I think I would prefer to duplicate the contents of
> OptionsGStreamerDefinitions.cmake in both port files... it's just easier to
> read without having to open up a new file. 

I think avoiding this duplication is needed if we want to ensure that the consistency regarding gstreamer build options between GTK+ and WPE ports remains true.

Otherwise in the future is likely that some developer will update the options of one port but forgets about the other.

I think this is more important than making things easier to read.

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #6)
> But I like OptionsGStreamerChecks.cmake

Me too (except for "audio" which crept into GSTREAMER_COMPONENTS by default)

[Build Bot]

Comment on attachment 318261 [details]
Patch

Attachment 318261 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4325446

New failing tests:
http/tests/security/contentSecurityPolicy/report-status-code-zero-when-using-https.html

[Build Bot]

Created attachment 318280 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Michael Catanzaro]

Comment on attachment 318261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318261&action=review

OK, but please rename OptionsGStreamerDefinitions, etc. to GStreamerOptionsDefinitions. That way these new files are still sorted together, but not alongside the port options files.

> Source/cmake/OptionsGStreamerDefinitions.cmake:5
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF)

I'm going to reverse your statement. I don't want to turn off GStreamerGL unless Miguel agrees to do that. We already agreed it would be ON for 2.18, so I really doubt he wants to turn it off now. If for some reason GStreamerGL is not yet ready for WPE, then perhaps it's too soon to have a unified set of options definitions, right? :)

Perhaps it's just an oversight that it was not already enabled for WPE?

[Michael Catanzaro]

Comment on attachment 318261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318261&action=review

> Source/cmake/OptionsGStreamerDependencies.cmake:3
> +if (DEFINED ENABLE_OPENGL)
> +  WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL ENABLE_OPENGL)
> +endif ()

Does WPE not have an ENABLE_OPENGL setting?

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 318261 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318261&action=review
> 
> > Source/cmake/OptionsGStreamerDependencies.cmake:3
> > +if (DEFINED ENABLE_OPENGL)
> > +  WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL ENABLE_OPENGL)
> > +endif ()
> 
> Does WPE not have an ENABLE_OPENGL setting?

OPENGL/EGL is mandatory on WPE AFAIK.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 318261 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318261&action=review
> 
> OK, but please rename OptionsGStreamerDefinitions, etc. to
> GStreamerOptionsDefinitions. That way these new files are still sorted
> together, but not alongside the port options files.
> 
> > Source/cmake/OptionsGStreamerDefinitions.cmake:5
> > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL" PRIVATE OFF)
> 
> I'm going to reverse your statement. I don't want to turn off GStreamerGL
> unless Miguel agrees to do that. We already agreed it would be ON for 2.18,
> so I really doubt he wants to turn it off now. If for some reason
> GStreamerGL is not yet ready for WPE, then perhaps it's too soon to have a
> unified set of options definitions, right? :)
> 
> Perhaps it's just an oversight that it was not already enabled for WPE?

I'm having a hard time thinking what possibly could be any reason to disable this on WPE by default and not on GTK+.

But let's wait for their input if you insist

[Xabier Rodríguez Calvar]

(In reply to Konstantin Tokarev from comment #9)
> Me too (except for "audio" which crept into GSTREAMER_COMPONENTS by default)

Ok, I'll move this.

(In reply to Carlos Alberto Lopez Perez from comment #15)
> I'm having a hard time thinking what possibly could be any reason to disable
> this on WPE by default and not on GTK+.
> 
> But let's wait for their input if you insist

I don't know why it's disabled, but I also prefer to keep it as it is now.

(In reply to Carlos Alberto Lopez Perez from comment #14)
> (In reply to Michael Catanzaro from comment #13)
> > Does WPE not have an ENABLE_OPENGL setting?
> 
> OPENGL/EGL is mandatory on WPE AFAIK.

If it is, then I'll fix that too. We need their input then.

(In reply to Michael Catanzaro from comment #12)
> OK, but please rename OptionsGStreamerDefinitions, etc. to
> GStreamerOptionsDefinitions. That way these new files are still sorted
> together, but not alongside the port options files.

Removing the files and leaving the options back in the ports defeats not totally but a big part of the purpose of unifying things involving code of multimedia/GStreamer. I want to have them controlled. Actually, I think I should add a big warning on multimedia things that are overridden in ports.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #16)
> (In reply to Michael Catanzaro from comment #12)
> > OK, but please rename OptionsGStreamerDefinitions, etc. to
> > GStreamerOptionsDefinitions. That way these new files are still sorted
> > together, but not alongside the port options files.
> 
> Removing the files and leaving the options back in the ports defeats not
> totally but a big part of the purpose of unifying things involving code of
> multimedia/GStreamer. I want to have them controlled. Actually, I think I
> should add a big warning on multimedia things that are overridden in ports.

I forgot to mention that I think your renaming proposal sounds good and I'll do it in the next iteration.

[Xabier Rodríguez Calvar]

Created attachment 318381 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #17)
> I forgot to mention that I think your renaming proposal sounds good and I'll
> do it in the next iteration.

Done.

(In reply to Xabier Rodríguez Calvar from comment #16)
> (In reply to Konstantin Tokarev from comment #9)
> > Me too (except for "audio" which crept into GSTREAMER_COMPONENTS by default)
> 
> Ok, I'll move this.

Done.

> (In reply to Carlos Alberto Lopez Perez from comment #15)
> > I'm having a hard time thinking what possibly could be any reason to disable
> > this on WPE by default and not on GTK+.
> > 
> > But let's wait for their input if you insist
> 
> I don't know why it's disabled, but I also prefer to keep it as it is now.

Enabled it as per input of Žan and Miguel.

Also set VIDEO_TRACK as PRIVATE ON.

[Konstantin Tokarev]

Comment on attachment 318381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318381&action=review

> Source/cmake/GStreamerDependencies.cmake:1
> +WEBKIT_OPTION_DEPEND(ENABLE_VIDEO_TRACK ENABLE_VIDEO)

This line should be in WebKitFeatures.cmake

[Xabier Rodríguez Calvar]

Created attachment 318490 [details]
Patch

Addressed annulen's comment

[Xabier Rodríguez Calvar]

(In reply to Konstantin Tokarev from comment #20)
> > Source/cmake/GStreamerDependencies.cmake:1
> > +WEBKIT_OPTION_DEPEND(ENABLE_VIDEO_TRACK ENABLE_VIDEO)
> 
> This line should be in WebKitFeatures.cmake

It was already actually :)

[Konstantin Tokarev]

LGTM

[WebKit Commit Bot]

Comment on attachment 318490 [details]
Patch

Clearing flags on attachment: 318490

Committed r221006: <http://trac.webkit.org/changeset/221006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34009174>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 175821

[Xabier Rodríguez Calvar]

Created attachment 318863 [details]
Patch

Enabled VIDEO_TRACK by default as it was in both ports which was making Debian and Ubuntu GTK+ bots fail to build.

[WebKit Commit Bot]

Comment on attachment 318863 [details]
Patch

Clearing flags on attachment: 318863

Committed r221072: <http://trac.webkit.org/changeset/221072>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.