Bug 78688 – [EFL] Implement Web Audio API.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dongwoo Joshua Im (dwim)]

Implement Web Audio API on EFL port.
(https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html)

I'm implementing this patch now.
I'll upload my first patch in a week.

[Dongwoo Joshua Im (dwim)]

Created attachment 133428 [details]
Patch

This is the first patch of the Web Audio API on EFL port.

[Philippe Normand]

Comment on attachment 133428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133428&action=review

> LayoutTests/platform/efl/Skipped:-1074
> -# The EFL port has no support for webaudio
> -webaudio

You got all the tests running? I'm a bit surprised.

> Source/WebKit/efl/ewk/ewk_view.cpp:202
> +#if ENABLE(WEB_AUDIO)
> +        bool webAudio : 1;
> +#endif

Does that mean the feature is enabled by default? If so, I don't recommend this... WebAudio is still quite experimental.

[Peter Beverloo]

Keep in mind that the Web Audio API implementation moved to Source/WebCore/Modules/webaudio/ a few days ago: http://wkrev.com/111474. This patch won't apply as it still refers some old paths.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #3)
> Keep in mind that the Web Audio API implementation moved to Source/WebCore/Modules/webaudio/ a few days ago: http://wkrev.com/111474. This patch won't apply as it still refers some old paths.

Oh. I can see the changes now. I will fix the patch.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #2)
> (From update of attachment 133428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133428&action=review
> 
> > LayoutTests/platform/efl/Skipped:-1074
> > -# The EFL port has no support for webaudio
> > -webaudio
> 
> You got all the tests running? I'm a bit surprised.
> 

Not all...
I remove webaudio directory from this file because we "need" to pass this test suites.
I will add the failed cases in the Skipped file.

How about GTK port? How many test are failed currently?



> > Source/WebKit/efl/ewk/ewk_view.cpp:202
> > +#if ENABLE(WEB_AUDIO)
> > +        bool webAudio : 1;
> > +#endif
> 
> Does that mean the feature is enabled by default? If so, I don't recommend this... WebAudio is still quite experimental.

Uhm.. ok. I agree with you. I will disable webaudio by default.

[Dongwoo Joshua Im (dwim)]

Created attachment 134520 [details]
Patch

I fixed the patch regarding the comments.

[Philippe Normand]

Comment on attachment 134520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134520&action=review

Looks good apart from the following one:

> Source/WebCore/PlatformEfl.cmake:288
> +    platform/graphics/gstreamer/GRefPtrGStreamer.cpp

That's used by both VIDEO and WEB_AUDIO. Maybe it's in the wrong section of the file?

[Dongwoo Joshua Im (dwim)]

Created attachment 138755 [details]
Patch

I've fixed the patch regarding Philippe's comment.

[Philippe Normand]

Comment on attachment 138755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138755&action=review

I think a new iteration of this patch would be needed. Looks good though.

> Source/WebCore/PlatformEfl.cmake:223
>      ${GStreamer-Video_INCLUDE_DIRS}

That's only needed for VIDEO.

> Source/WebCore/PlatformEfl.cmake:232
> +    ${GStreamer-Video_LIBRARIES}

That's only needed for VIDEO.

> Source/WebCore/PlatformEfl.cmake:239
> +    platform/graphics/gstreamer/GStreamerUtilities.cpp

This is needed by both VIDEO and WEB_AUDIO.

> Source/WebCore/PlatformEfl.cmake:240
> +    platform/graphics/gstreamer/GStreamerVersioning.cpp

This is only used by VIDEO but might be needed for both when the WebAudio implementation is ported to GStreamer-1.0... So might be good to move it now too.

> Source/WebKit/efl/ChangeLog:15
> +        (ewk_view_setting_web_audio_set): Querie if the Web Audio API feature is enabled.

typo: Querie

> Source/cmake/OptionsEfl.cmake:135
> +IF (ENABLE_WEB_AUDIO)
> +  IF (NOT ENABLE_VIDEO)
> +    FIND_PACKAGE(GStreamer REQUIRED)
> +    FIND_PACKAGE(GStreamer-App REQUIRED)
> +    FIND_PACKAGE(GStreamer-Base REQUIRED)
> +    FIND_PACKAGE(GStreamer-Interfaces REQUIRED)
> +    FIND_PACKAGE(GStreamer-Pbutils REQUIRED)
> +    FIND_PACKAGE(GStreamer-Plugins-Base REQUIRED)
> +    SET(WTF_USE_GSTREAMER 1)
> +    ADD_DEFINITIONS(-DWTF_USE_GSTREAMER=1)
> +  ENDIF ()
> +  FIND_PACKAGE(GStreamer-Audio REQUIRED)
> +  FIND_PACKAGE(GStreamer-FFT REQUIRED)
> +  ADD_DEFINITIONS(-DWTF_USE_WEBAUDIO_GSTREAMER=1)
> +ENDIF ()

I'm no CMake expert but maybe there's a cleaner way to perform the checks depending on the enabled features?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #9)
> (From update of attachment 138755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138755&action=review
> 
> I think a new iteration of this patch would be needed. Looks good though.
> 
> > Source/WebCore/PlatformEfl.cmake:223
> >      ${GStreamer-Video_INCLUDE_DIRS}
> 
> That's only needed for VIDEO.
> 

I will move it.

> > Source/WebCore/PlatformEfl.cmake:232
> > +    ${GStreamer-Video_LIBRARIES}
> 
> That's only needed for VIDEO.
> 

I will move it, as well.

> > Source/WebCore/PlatformEfl.cmake:239
> > +    platform/graphics/gstreamer/GStreamerUtilities.cpp
> 
> This is needed by both VIDEO and WEB_AUDIO.
> 

I will move it to right place.

> > Source/WebCore/PlatformEfl.cmake:240
> > +    platform/graphics/gstreamer/GStreamerVersioning.cpp
> 
> This is only used by VIDEO but might be needed for both when the WebAudio implementation is ported to GStreamer-1.0... So might be good to move it now too.
> 

I will move it to right place, as well.

> > Source/WebKit/efl/ChangeLog:15
> > +        (ewk_view_setting_web_audio_set): Querie if the Web Audio API feature is enabled.
> 
> typo: Querie

I will fix it.

> 
> > Source/cmake/OptionsEfl.cmake:135
> > +IF (ENABLE_WEB_AUDIO)
> > +  IF (NOT ENABLE_VIDEO)
> > +    FIND_PACKAGE(GStreamer REQUIRED)
> > +    FIND_PACKAGE(GStreamer-App REQUIRED)
> > +    FIND_PACKAGE(GStreamer-Base REQUIRED)
> > +    FIND_PACKAGE(GStreamer-Interfaces REQUIRED)
> > +    FIND_PACKAGE(GStreamer-Pbutils REQUIRED)
> > +    FIND_PACKAGE(GStreamer-Plugins-Base REQUIRED)
> > +    SET(WTF_USE_GSTREAMER 1)
> > +    ADD_DEFINITIONS(-DWTF_USE_GSTREAMER=1)
> > +  ENDIF ()
> > +  FIND_PACKAGE(GStreamer-Audio REQUIRED)
> > +  FIND_PACKAGE(GStreamer-FFT REQUIRED)
> > +  ADD_DEFINITIONS(-DWTF_USE_WEBAUDIO_GSTREAMER=1)
> > +ENDIF ()
> 
> I'm no CMake expert but maybe there's a cleaner way to perform the checks depending on the enabled features?

I think I can make it little bit simpler than this. I'll try.

[Dongwoo Joshua Im (dwim)]

Created attachment 139745 [details]
Patch

I've fixed the patch regarding Philippe's comments.

[Philippe Normand]

Comment on attachment 139745 [details]
Patch

Looks good now!

[Dongwoo Joshua Im (dwim)]

(In reply to comment #12)
> (From update of attachment 139745 [details])
> Looks good now!

Thank you for your kind review!!

[WebKit Review Bot]

Comment on attachment 139745 [details]
Patch

Clearing flags on attachment: 139745

Committed r115800: <http://trac.webkit.org/changeset/115800>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

I've posted a follow-up patch in bug 86982 removing the override for ENABLE_WEB_AUDIO from OptionsEfl.cmake.