[GStreamer][GTK][WPE] Move common things to GStreamer cmake files
Created attachment 318251 [details] Patch
Comment on attachment 318251 [details] Patch Setting r- because patch doesn't apply. We should test this on the EWS before landing.
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
Created attachment 318261 [details] Patch
(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.
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?
(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
(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.
(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)
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
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
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?
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?
(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.
(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
(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.
(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.
Created attachment 318381 [details] Patch
(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.
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
Created attachment 318490 [details] Patch Addressed annulen's comment
(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 :)
LGTM
Comment on attachment 318490 [details] Patch Clearing flags on attachment: 318490 Committed r221006: <http://trac.webkit.org/changeset/221006>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34009174>
Re-opened since this is blocked by bug 175821
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.
Comment on attachment 318863 [details] Patch Clearing flags on attachment: 318863 Committed r221072: <http://trac.webkit.org/changeset/221072>