[GStreamer] Make codecparsers optionnal
Created attachment 345766 [details] Patch
Comment on attachment 345766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345766&action=review > Source/WebCore/platform/GStreamer.cmake:111 > + message(WARNING "WebRTC or MediaStream enabled but GStreamer < 1.10 - Support won't be built.") Nobody is going to see this warning... use a fatal error if you want the user to see a problem. And this is definitely a problem worth seeing. Also, follow the format of our comparable error messages: message(FATAL_ERROR "GStreamer 1.10 is needed for ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC") > Source/cmake/OptionsGTK.cmake:429 > include(GStreamerChecks) > +if (ENABLE_MEDIA_STREAM OR ENABLE_WEB_RTC) > + if (PC_GSTREAMER_VERSION VERSION_LESS "1.10") > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) > + else () > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD TRUE) > + endif () > +else () > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) > +endif () Surely this should be done in GStreamerChecks.cmake? > Source/cmake/OptionsWPE.cmake:158 > +if (ENABLE_MEDIA_STREAM OR ENABLE_WEB_RTC) > + if (PC_GSTREAMER_VERSION VERSION_LESS "1.10") > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) > + else () > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD TRUE) > + endif () > +else () > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) > +endif () Ditto
Comment on attachment 345766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345766&action=review >> Source/WebCore/platform/GStreamer.cmake:111 >> + message(WARNING "WebRTC or MediaStream enabled but GStreamer < 1.10 - Support won't be built.") > > Nobody is going to see this warning... use a fatal error if you want the user to see a problem. And this is definitely a problem worth seeing. Also, follow the format of our comparable error messages: > > message(FATAL_ERROR "GStreamer 1.10 is needed for ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC") I decided to go with a warning because I expect ubuntu/debian buildbot use the build-webkit script which would ENABLE_WEB_RTC... and thus fail without that check. >> Source/cmake/OptionsGTK.cmake:429 >> +endif () > > Surely this should be done in GStreamerChecks.cmake? For some reason I don't get (as it often happens to me with CMake), using `SET_AND_EXPOSE_TO_BUILD` there doesn't work (ie. the value is not added to cmakeconfig.h - I definitely wanted and tried to have that code there.
Created attachment 345770 [details] Patch Hard fail when MEDIA_STREAM or WEBRTC or enabled but GStreamer < 1.10
(In reply to Thibault Saunier from comment #4) > Created attachment 345770 [details] > Patch > > Hard fail when MEDIA_STREAM or WEBRTC or enabled but GStreamer < 1.10 But this will keep the Ubuntu LTS build broken, no? I disagree with Michael. A warning would be better, in my opinion.
(In reply to Philippe Normand from comment #5) > (In reply to Thibault Saunier from comment #4) > > Created attachment 345770 [details] > > Patch > > > > Hard fail when MEDIA_STREAM or WEBRTC or enabled but GStreamer < 1.10 > > But this will keep the Ubuntu LTS build broken, no? > > I disagree with Michael. A warning would be better, in my opinion. Sorry should have explicit here, I checked in https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20(Build)/builds/14304/steps/compile-webkit/logs/stdio and found: ``` argv: ['perl', './Tools/Scripts/build-webkit', '--release', '--no-experimental-features', '--gtk'] environment: BUILD_WEBKIT_ARGS=--cmakeargs=-DENABLE_WEB_CRYPTO=OFF --cmakeargs=-DUSE_WOFF2=OFF --cmakeargs=-DUSE_GSTREAMER_GL=OFF ``` `--no-experimental-features` should make it so it won't break iiuc :-)
Ah, ok! Well then you can land this I suppose, Michael reviewed the previous iteration already :)
(In reply to Thibault Saunier from comment #3) > For some reason I don't get (as it often happens to me with CMake), using > `SET_AND_EXPOSE_TO_BUILD` there doesn't work (ie. the value is not added to > cmakeconfig.h - I definitely wanted and tried to have that code there. Good that you've checked this carefully. Still, that macro is used all over the place in GStreamerChecks.cmake. I think we need to understand this better... if it's not working, that would be a disaster.
(In reply to Michael Catanzaro from comment #8) > (In reply to Thibault Saunier from comment #3) > > For some reason I don't get (as it often happens to me with CMake), using > > `SET_AND_EXPOSE_TO_BUILD` there doesn't work (ie. the value is not added to > > cmakeconfig.h - I definitely wanted and tried to have that code there. > > Good that you've checked this carefully. > > Still, that macro is used all over the place in GStreamerChecks.cmake. I > think we need to understand this better... if it's not working, that would > be a disaster. SET_AND_EXPOSE_TO_BUILD is not used in GStreamer.cmake afaics.
GStreamerChecks.cmake. It's used for USE_WEBAUDIO_GSTREAMER. Is that always turned off by mistake, then?
Created attachment 345948 [details] Patch Move OptionXX check to GStreamerCheck. Sorry Michael I miundesrstood you on that one.
Comment on attachment 345948 [details] Patch Rejecting attachment 345948 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 345948, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/8679911
Comment on attachment 345948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345948&action=review > Source/WebCore/ChangeLog:3 > + [GStreamer] Make codecparsers optionnal Nit: optional > Source/cmake/GStreamerChecks.cmake:51 > + if (PC_GSTREAMER_VERSION VERSION_LESS "1.10") > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) I wonder if we should print an explicit warning here. I say this because just disabling something that was commanded to be on can make you lose some time if you don't explicitly check the options. > Source/cmake/OptionsGTK.cmake:-105 > -WEBKIT_OPTION_DEPEND(USE_LIBWEBRTC ENABLE_WEB_RTC) I think I wouldn't remove this, I might add it to GStreamerDependencies.cmake. It is still a dependency though we are enforcing it some other way. Anyway, I don't have a strong opinion on this one.
Comment on attachment 345948 [details] Patch Attachment 345948 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8682687 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 345992 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Xabier Rodríguez Calvar from comment #13) > Comment on attachment 345948 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=345948&action=review > > > Source/WebCore/ChangeLog:3 > > + [GStreamer] Make codecparsers optionnal > > Nit: optional > > > Source/cmake/GStreamerChecks.cmake:51 > > + if (PC_GSTREAMER_VERSION VERSION_LESS "1.10") > > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE) > > + SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE) > > I wonder if we should print an explicit warning here. I say this because > just disabling something that was commanded to be on can make you lose some > time if you don't explicitly check the options. It is already an error in `Source/WebCore/platform/GStreamer.cmake` if needed. > > Source/cmake/OptionsGTK.cmake:-105 > > -WEBKIT_OPTION_DEPEND(USE_LIBWEBRTC ENABLE_WEB_RTC) > > I think I wouldn't remove this, I might add it to > GStreamerDependencies.cmake. It is still a dependency though we are > enforcing it some other way. > > Anyway, I don't have a strong opinion on this one. LibWebRTC is built as third party and we do not have any other WebRTC backend (yet) so I do not see any point of having an option for that.
Created attachment 346058 [details] Patch Add missing 'Reviewed by NOBODY' not
Comment on attachment 346058 [details] Patch Clearing flags on attachment: 346058 Committed r234362: <https://trac.webkit.org/changeset/234362>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42729765>