The build error is as follows: In file included from DerivedSources/WebCore/unified-sources/UnifiedSource287.cpp:8:0: ../Source/WebCore/page/DeprecatedGlobalSettings.cpp: In static member function ‘static void WebCore::DeprecatedGlobalSettings::setGStreamerEnabled(bool)’: ../Source/WebCore/page/DeprecatedGlobalSettings.cpp:144:23: error: incomplete type ‘WebCore::HTMLMediaElement’ used in nested name specifier HTMLMediaElement::resetMediaEngines(); ^~~~~~~~~~~~~~~~~ The part of the code referenced above is guarded with “USE(GSTREAMER)”, but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not be using GStreamer at all.
Setting both “ENABLE_VIDEO=OFF” and “ENABLE_WEB_AUDIO=OFF” effectively makes CMake skip checking for GStreamer, and the “USE(GSTREAMER)” guard evaluates to false. I see the following options: 1.) Fixing things using ENABLE_VIDEO=OFF and ENABLE_WEB_AUDIO=ON combination works. 2.) Make ENABLE_WEB_AUDIO depend on ENABLE_VIDEO. 3.) Have a new ENABLE_MULTIMEDIA on which both ENABLE_VIDEO and ENABLE_WEB_AUDIO depend. 4.) ??? What should we do here?
(In reply to Adrian Perez from comment #0) > The build error is as follows: > > In file included from > DerivedSources/WebCore/unified-sources/UnifiedSource287.cpp:8:0: > ../Source/WebCore/page/DeprecatedGlobalSettings.cpp: In static member > function ‘static void > WebCore::DeprecatedGlobalSettings::setGStreamerEnabled(bool)’: > ../Source/WebCore/page/DeprecatedGlobalSettings.cpp:144:23: error: > incomplete type ‘WebCore::HTMLMediaElement’ used in nested name specifier > HTMLMediaElement::resetMediaEngines(); > ^~~~~~~~~~~~~~~~~ > > The part of the code referenced above is guarded with “USE(GSTREAMER)”, > but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not > be using GStreamer at all. ENABLE_WEB_AUDIO uses GStreamer too. I think the code should probably be guarded by #if ENABLE(VIDEO) || ENABLE(WEB_AUDIO). But please ask the multimedia team what they prefer.
> The part of the code referenced above is guarded with “USE(GSTREAMER)”, > but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not > be using GStreamer at all. I think the WEB_AUDIO feature depends on GStreamer.
(In reply to Carlos Alberto Lopez Perez from comment #3) > > The part of the code referenced above is guarded with “USE(GSTREAMER)”, > > but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not > > be using GStreamer at all. > > I think the WEB_AUDIO feature depends on GStreamer. I have been checking the source code: WebAudio also depends on ENABLE_VIDEO. The correct solution here seems to be adding the feature dependency, which is option (2.) of the ones I was suggesting.
(In reply to Adrian Perez from comment #4) > (In reply to Carlos Alberto Lopez Perez from comment #3) > > > The part of the code referenced above is guarded with “USE(GSTREAMER)”, > > > but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not > > > be using GStreamer at all. > > > > I think the WEB_AUDIO feature depends on GStreamer. > > I have been checking the source code: WebAudio also depends on ENABLE_VIDEO. > The correct solution here seems to be adding the feature dependency, which > is option (2.) of the ones I was suggesting. mmm... isn't hat a bug? Why (or where) WebAudio needs to depend on ENABLE_VIDEO?
Created attachment 326326 [details] Patch
(In reply to Carlos Alberto Lopez Perez from comment #5) > (In reply to Adrian Perez from comment #4) > > (In reply to Carlos Alberto Lopez Perez from comment #3) > > > > The part of the code referenced above is guarded with “USE(GSTREAMER)”, > > > > but IIRC it used to be that building with “ENABLE_VIDEO=OFF” should not > > > > be using GStreamer at all. > > > > > > I think the WEB_AUDIO feature depends on GStreamer. > > > > I have been checking the source code: WebAudio also depends on ENABLE_VIDEO. > > The correct solution here seems to be adding the feature dependency, which > > is option (2.) of the ones I was suggesting. > > mmm... isn't hat a bug? Why (or where) WebAudio needs to depend on > ENABLE_VIDEO? The *whole* implementation of “HTMLMediaElement” (which is used for <video> and <audio> tags) is guarded with “ENABLE(VIDEO)”, and WebAudio requires it. My guess is that ENABLE_VIDEO was originally used to guard the support for <video>, then when <audio> was implemented (and later the WebAudio JS API) but the guard was kept. Probably the guard should be renamed to ENABLE_MULTIMEDIA_PLAYBACK to make it better reflect what it does.
Comment on attachment 326326 [details] Patch OK, wow. That is a surprise to me. I'm OK with this, assuming multimedia team agrees. (Please find a multimedia team reviewer.) But if we do it, we need to update existing code that assumes ENABLE_WEB_AUDIO does not depend on ENABLE_VIDEO. For example, in GStreamerChecks.cmake, the entire file is guarded by if (ENABLE_VIDEO OR ENABLE_WEB_AUDIO). That doesn't make sense after this commit, since ENABLE_VIDEO now implies ENABLE_WEB_AUDIO, the ENABLE_WEB_AUDIO check should be dropped. I grepped the codebase for 'ENABLE(VIDEO) || ENABLE(WEB_AUDIO)' and found hits in PlatformMediaSession.cpp and PlatformMediaSessionManager.cpp that will need to be removed.
(In reply to Michael Catanzaro from comment #8) > Comment on attachment 326326 [details] > Patch > > OK, wow. That is a surprise to me. Same here when I found out. Interestingly enough, in Buildroot the recipe has a “Enable multimedia support” configuration option which ends up toggling both ENABLE_VIDEO and ENABLE_WEB_AUDIO: https://git.busybox.net/buildroot/tree/package/webkitgtk/webkitgtk.mk#n37 ...which from the repo history I see they do that way because setting only ENABLE_VIDEO=OFF doesn't build :-) > I'm OK with this, assuming multimedia team agrees. (Please find a multimedia > team reviewer.) But if we do it, we need to update existing code that > assumes ENABLE_WEB_AUDIO does not depend on ENABLE_VIDEO. For example, in > GStreamerChecks.cmake, the entire file is guarded by if (ENABLE_VIDEO OR > ENABLE_WEB_AUDIO). That doesn't make sense after this commit, since > ENABLE_VIDEO now implies ENABLE_WEB_AUDIO, the ENABLE_WEB_AUDIO check should > be dropped. I grepped the codebase for 'ENABLE(VIDEO) || ENABLE(WEB_AUDIO)' > and found hits in PlatformMediaSession.cpp and > PlatformMediaSessionManager.cpp that will need to be removed. Good catch, I'll comb our CMake build files accordingly.
(In reply to Michael Catanzaro from comment #8) > I'm OK with this, assuming multimedia team agrees. (Please find a multimedia > team reviewer.) [...] I am CC'ing Philippe, Alex, and Xabier. Let's see what's their opinion.
Comment on attachment 326326 [details] Patch Let's check what happens in Mac first. If there's the dependency too then we land, otherwise we should go for option 1.
Mmm.... Are you sure this dependency can be untangled easily? I was able to build with WebKitGTK+ with -DENABLE_VIDEO=OFF -DENABLE_WEB_AUDIO=ON with the patch below diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp b/Source/WebCore/page/DeprecatedGlobalSettings.cpp index 348d93833fd..2cb1c9c20e1 100644 --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp @@ -141,7 +141,9 @@ void DeprecatedGlobalSettings::setGStreamerEnabled(bool enabled) return; gGStreamerEnabled = enabled; +#if ENABLE(VIDEO) HTMLMediaElement::resetMediaEngines(); +#endif } #endif And I tested the minibrowser with this test and it worked fine Tools/Scripts/run-minibrowser --gtk --enable-webaudio=true http://webaudiodemos.appspot.com/oscilloscope/index.html Also tested to run the 111 webaudio/ layout tests. They pass fine, except the 5 below Regressions: Unexpected text-only failures (4) webaudio/createMediaStreamSource-null.html [ Failure ] webaudio/mediaelementaudiosourcenode-gc.html [ Failure ] webaudio/mediaelementaudiosourcenode.html [ Failure ] webaudio/mediastreamaudiodestinationnode.html [ Failure ] Regressions: Unexpected timeouts (1) webaudio/audiocontext-state-interrupted.html [ Timeout ]
I think the confusion here is that ENABLE_WEB_AUDIO doesn't have anything to do with HTMLAudioElement. HTMLAudioElement really does depend on ENABLE_VIDEO (for HTMLMediaElement), but ENABLE_WEB_AUDIO seems to be a separate feature.
Comment on attachment 326326 [details] Patch Let's go with Carlos Lopez's approach instead.
(In reply to Carlos Alberto Lopez Perez from comment #12) > Mmm.... > > > Are you sure this dependency can be untangled easily? > > I was able to build with WebKitGTK+ with -DENABLE_VIDEO=OFF > -DENABLE_WEB_AUDIO=ON with the patch below > > diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > index 348d93833fd..2cb1c9c20e1 100644 > --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > @@ -141,7 +141,9 @@ void DeprecatedGlobalSettings::setGStreamerEnabled(bool > enabled) > return; > > gGStreamerEnabled = enabled; > +#if ENABLE(VIDEO) > HTMLMediaElement::resetMediaEngines(); > +#endif > } > #endif > > > > > And I tested the minibrowser with this test and it worked fine > > Tools/Scripts/run-minibrowser --gtk --enable-webaudio=true > http://webaudiodemos.appspot.com/oscilloscope/index.html > > > Also tested to run the 111 webaudio/ layout tests. They pass fine, except > the 5 below > > Regressions: Unexpected text-only failures (4) > webaudio/createMediaStreamSource-null.html [ Failure ] > webaudio/mediaelementaudiosourcenode-gc.html [ Failure ] > webaudio/mediaelementaudiosourcenode.html [ Failure ] > webaudio/mediastreamaudiodestinationnode.html [ Failure ] > > Regressions: Unexpected timeouts (1) > webaudio/audiocontext-state-interrupted.html [ Timeout ] This might be because “HTMLMediaElement::resetMediaEngines()” in turn calls “MediaPlayer::resetMediaEngines()”. With the guards around the latter never gets called. I am now trying something like the following instead: diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp b/Source/WebCore/page/DeprecatedGlobalSettings.cpp index 348d93833fd..3d56163753b 100644 --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp @@ -141,7 +141,13 @@ void DeprecatedGlobalSettings::setGStreamerEnabled(bool enabled) return; gGStreamerEnabled = enabled; +#if ENABLE(VIDEO) HTMLMediaElement::resetMediaEngines(); +#else + // HTMLMediaElement is not available, but we can do here what it does, + // to make sure things like WebAudio work. + MediaPlayer::resetMediaEngines(); +#endif } #endif I'll see how this goes regarding the test failures (the build is progressing as I write this :D).
(In reply to Adrian Perez from comment #15) > (In reply to Carlos Alberto Lopez Perez from comment #12) > > Mmm.... > > > > > > Are you sure this dependency can be untangled easily? > > > > I was able to build with WebKitGTK+ with -DENABLE_VIDEO=OFF > > -DENABLE_WEB_AUDIO=ON with the patch below > > > > diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > > b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > > index 348d93833fd..2cb1c9c20e1 100644 > > --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > > +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > > @@ -141,7 +141,9 @@ void DeprecatedGlobalSettings::setGStreamerEnabled(bool > > enabled) > > return; > > > > gGStreamerEnabled = enabled; > > +#if ENABLE(VIDEO) > > HTMLMediaElement::resetMediaEngines(); > > +#endif > > } > > #endif > > > > > > > > > > And I tested the minibrowser with this test and it worked fine > > > > Tools/Scripts/run-minibrowser --gtk --enable-webaudio=true > > http://webaudiodemos.appspot.com/oscilloscope/index.html > > > > > > Also tested to run the 111 webaudio/ layout tests. They pass fine, except > > the 5 below > > > > Regressions: Unexpected text-only failures (4) > > webaudio/createMediaStreamSource-null.html [ Failure ] > > webaudio/mediaelementaudiosourcenode-gc.html [ Failure ] > > webaudio/mediaelementaudiosourcenode.html [ Failure ] > > webaudio/mediastreamaudiodestinationnode.html [ Failure ] > > > > Regressions: Unexpected timeouts (1) > > webaudio/audiocontext-state-interrupted.html [ Timeout ] > > This might be because “HTMLMediaElement::resetMediaEngines()” in turn > calls “MediaPlayer::resetMediaEngines()”. With the guards around the > latter never gets called. I am now trying something like the following > instead: > > diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > index 348d93833fd..3d56163753b 100644 > --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > @@ -141,7 +141,13 @@ void > DeprecatedGlobalSettings::setGStreamerEnabled(bool enabled) > return; > > gGStreamerEnabled = enabled; > +#if ENABLE(VIDEO) > HTMLMediaElement::resetMediaEngines(); > +#else > + // HTMLMediaElement is not available, but we can do here what it does, > + // to make sure things like WebAudio work. > + MediaPlayer::resetMediaEngines(); > +#endif > } > #endif > > I'll see how this goes regarding the test failures (the build is progressing > as I write this :D). Noup. MediaPlayer is it all also guarded with “ENABLE(VIDEO)”. This means that even if the WebAudio API *may* work e.g. for processing sound samples, there is no way WebKit will ever be able of playing back sound to the user. Do we really want to allow that? What would be the point? I still think making ENABLE_WEB_AUDIO depend on ENABLE_VIDEO is the best thing to do: otherwise we allow to make builds which advertise the WebAudio API, which cannot produce any kind of multimedia output... that seems like a really bad idea to me :-\
(In reply to Adrian Perez from comment #16) > Noup. MediaPlayer is it all also guarded with “ENABLE(VIDEO)”. This means > that even if the WebAudio API *may* work e.g. for processing sound samples, > there is no way WebKit will ever be able of playing back sound to the user. I'm able to hear the sound on this demo <http://webaudiodemos.appspot.com/oscilloscope/index.html> with a WebKitGTK+/MiniBrowser built with ENABLE_VIDEO=OFF and ENABLE_WEB_AUDIO=ON (NOTE: you need to pass the run-time flag --enable-webaudio=true to the MB)
(In reply to Carlos Alberto Lopez Perez from comment #12) > I was able to build with WebKitGTK+ with -DENABLE_VIDEO=OFF > -DENABLE_WEB_AUDIO=ON with the patch below > > diff --git a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > index 348d93833fd..2cb1c9c20e1 100644 > --- a/Source/WebCore/page/DeprecatedGlobalSettings.cpp > +++ b/Source/WebCore/page/DeprecatedGlobalSettings.cpp > @@ -141,7 +141,9 @@ void DeprecatedGlobalSettings::setGStreamerEnabled(bool > enabled) > return; > > gGStreamerEnabled = enabled; > +#if ENABLE(VIDEO) > HTMLMediaElement::resetMediaEngines(); > +#endif > } > #endif Want to submit a patch for this?
Created attachment 327961 [details] Patch
Comment on attachment 327961 [details] Patch Attachment 327961 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5415953 New failing tests: imported/w3c/web-platform-tests/html/browsers/offline/appcache/workers/appcache-worker.html
Created attachment 327965 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 327961 [details] Patch Clearing flags on attachment: 327961 Committed r225323: <https://trac.webkit.org/changeset/225323>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35770196>
I forgot, you had suggested this: +#if ENABLE(VIDEO) HTMLMediaElement::resetMediaEngines(); +#else + // HTMLMediaElement is not available, but we can do here what it does, + // to make sure things like WebAudio work. + MediaPlayer::resetMediaEngines(); +#endif Was there a problem with that, or just forgot?
(In reply to Michael Catanzaro from comment #25) > I forgot, you had suggested this: > > +#if ENABLE(VIDEO) > HTMLMediaElement::resetMediaEngines(); > +#else > + // HTMLMediaElement is not available, but we can do here what it does, > + // to make sure things like WebAudio work. > + MediaPlayer::resetMediaEngines(); > +#endif > > Was there a problem with that, or just forgot? It didn't work: MediaPlayer is also guarded by ENABLE(VIDEO).