Bug 179373 – [GStreamer] Builds fails with ENABLE_VIDEO=OFF due to GStreamer usage

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adrian Perez]

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.

[Adrian Perez]

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?

[Michael Catanzaro]

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

[Carlos Alberto Lopez Perez]

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

[Adrian Perez]

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

[Carlos Alberto Lopez Perez]

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

[Adrian Perez]

Created attachment 326326 [details]
Patch

[Adrian Perez]

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

[Michael Catanzaro]

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.

[Adrian Perez]

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

[Adrian Perez]

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

[Xabier Rodríguez Calvar]

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.

[Carlos Alberto Lopez Perez]

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 ]

[Michael Catanzaro]

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.

[Michael Catanzaro]

Comment on attachment 326326 [details]
Patch

Let's go with Carlos Lopez's approach instead.

[Adrian Perez]

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

[Adrian Perez]

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

[Carlos Alberto Lopez Perez]

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

[Michael Catanzaro]

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

[Adrian Perez]

Created attachment 327961 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 327961 [details]
Patch

Clearing flags on attachment: 327961

Committed r225323: <https://trac.webkit.org/changeset/225323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35770196>

[Michael Catanzaro]

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?

[Adrian Perez]

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