Bug 188010 – [GStreamer] Make codecparsers optionnal

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

[GStreamer] Make codecparsers optionnal

[Thibault Saunier]

Created attachment 345766 [details]
Patch

[Michael Catanzaro]

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

[Thibault Saunier]

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.

[Thibault Saunier]

Created attachment 345770 [details]
Patch

Hard fail when MEDIA_STREAM or WEBRTC or enabled but GStreamer < 1.10

[Philippe Normand]

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

[Thibault Saunier]

(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 :-)

[Philippe Normand]

Ah, ok! Well then you can land this I suppose, Michael reviewed the previous iteration already :)

[Michael Catanzaro]

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

[Thibault Saunier]

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

[Michael Catanzaro]

GStreamerChecks.cmake. It's used for USE_WEBAUDIO_GSTREAMER. Is that always turned off by mistake, then?

[Thibault Saunier]

Created attachment 345948 [details]
Patch

Move OptionXX check to GStreamerCheck. Sorry Michael I miundesrstood you on that one.

[WebKit Commit Bot]

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

[Xabier Rodríguez Calvar]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Thibault Saunier]

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

[Thibault Saunier]

Created attachment 346058 [details]
Patch

Add missing 'Reviewed by NOBODY' not

[WebKit Commit Bot]

Comment on attachment 346058 [details]
Patch

Clearing flags on attachment: 346058

Committed r234362: <https://trac.webkit.org/changeset/234362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42729765>