Bug 150030 – [GStreamer][Mac] Fix WebAudio build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

For now (at least) we should disable Accelerate.framework when building WebAudio for the GTK port on Mac. Unless we can somehow link against a .framework?

[Philippe Normand]

Created attachment 262895 [details]
Patch

[Martin Robinson]

Comment on attachment 262895 [details]
Patch

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

Looks pretty good, but I'm going to suggest a few tweaks.

> Source/WebCore/platform/audio/Biquad.cpp:40
> -#if OS(DARWIN)
> +#if USING_ACCELERATE

Why USING instead of USE_, which is more common in WebKit and would allow the use of the USE(...) macro?

> Source/WebCore/platform/audio/Biquad.h:40
> +#if OS(DARWIN) && !PLATFORM(GTK)
> +#define USING_ACCELERATE 1
> +#else
> +#define USING_ACCELERATE 0
> +#endif

I think it might be worth calling this USE_ACCELERATE_FRAMEWORK since ACCELERATE is such a general term.

> Source/WebCore/platform/audio/DirectConvolver.cpp:77
> -#if OS(DARWIN)
> +#if OS(DARWIN) && !PLATFORM(GTK)
>  #if defined(__ppc__) || defined(__i386__)

Why don't these apply to GTK+? The ChangeLog does not specify. Would USE(ACCELERATE_FRAMEWORK) make sense here as well?

> Source/WebCore/platform/audio/FFTFrame.h:41
> +#elif OS(DARWIN)
> +#include <Accelerate/Accelerate.h>
>  #endif // USE(WEBAUDIO_GSTREAMER)

I think it might make sense to pull this out into its own block, simply because it's a little confusing to mix it with the GStreamer ones.

[Philippe Normand]

Comment on attachment 262895 [details]
Patch

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

>> Source/WebCore/platform/audio/DirectConvolver.cpp:77
>>  #if defined(__ppc__) || defined(__i386__)
> 
> Why don't these apply to GTK+? The ChangeLog does not specify. Would USE(ACCELERATE_FRAMEWORK) make sense here as well?

The vDSP_conv function is part of the Accelerate.framework AFAICS. So yeah I'll update the ifdef accordingly :)

[Philippe Normand]

Created attachment 263684 [details]
Patch

[Darin Adler]

Comment on attachment 263684 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1136
> +/* Disable the Accelerate.framework on Darwin (and Linux obviously) flavors of the GTK and EFL ports. */
> +#if (OS(DARWIN) || OS(LINUX)) && (PLATFORM(GTK) || PLATFORM(EFL))
> +#define USE_ACCELERATE_FRAMEWORK 0
> +#else
> +#define USE_ACCELERATE_FRAMEWORK 1
> +#endif

This conditional seems wrong. I wouldn’t want USE_ACCELERATE_FRAMEWORK to be 1 on Windows, for example.

I think we want:

    #if OS(DARWIN) && !(PLATFORM(GTK) || PLATFORM(EFL))
    #define USE_ACCELERATE_FRAMEWORK 1
    #else
    #define USE_ACCELERATE_FRAMEWORK 0
    #endif

But maybe that’s not quite right either.

> Source/WebCore/platform/audio/DirectConvolver.cpp:355
> +#endif // OS(DARWIN) && !PLATFORM(GTK)

Not right. Please fix.

> Source/WebCore/platform/audio/FFTFrame.h:43
> +#if USE(ACCELERATE_FRAMEWORK)
> +#include <Accelerate/Accelerate.h>
> +#endif // USE(ACCELERATE_FRAMEWORK)

The comment on the #endif here doesn’t seem helpful. I think we should not do these when they are so close to the #if. Just makes things messier and less clear. Probably should mention this in coding style guidelines.

[Philip Chimento]

Comment on attachment 263684 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1131
> +/* Disable the Accelerate.framework on Darwin (and Linux obviously) flavors of the GTK and EFL ports. */

Note, I have something similar on #144560 at https://bugs.webkit.org/attachment.cgi?id=255295&action=diff#a/Source/WTF/wtf/Platform.h_sec1

You might not have to add USE_ACCELERATE_FRAMEWORK, it looks like USE_ACCELERATE already exists.

[Philippe Normand]

Created attachment 263830 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 263830 [details]
Patch

Clearing flags on attachment: 263830

Committed r191844: <http://trac.webkit.org/changeset/191844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.