Bug 94806 – [GStreamer][Qt] WebAudio support

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

[Philippe Normand]

I plan to do this only for WK2, as I understand wk1 is not the main focus anymore:

- fiddle with qmake
- AudioBusQt implementation
- enable-webaudio websetting
- WKTR support

[Philippe Normand]

I'll post WKTR support in a separate patch, which should be port-independant, hopefully.

[Philippe Normand]

Created attachment 160686 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 160686 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:30
> +#if PLATFORM(QT)
> +#undef signals
> +#endif

Deserves comment I think

> Source/WebCore/platform/audio/qt/AudioBusQt.cpp:38
> +        QString environmentPath = QString::fromUtf8(qgetenv("AUDIO_RESOURCES_PATH"));
> +        if (environmentPath.isEmpty()) {

Why not just use String?

[Mikhail Pozdnyakov]

Comment on attachment 160686 [details]
Patch

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

> Source/WebCore/platform/audio/qt/AudioBusQt.cpp:34
> +

Just a proposal: QString absoluteFilename = QString(":/webkit/resources/audio/%1").arg(QLatin1String(name));

> Source/WebCore/platform/audio/qt/AudioBusQt.cpp:42
> +        const String filename = String(name) + ".wav";

and: const String& filename = String("%1.wav").arg(name);

[Philippe Normand]

Thanks Kenneth and Mikhail for the quick feedback! I'll update the patch.

[Simon Hausmann]

Comment on attachment 160686 [details]
Patch

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

Looks good in general! A few small comments.

> Source/WebCore/platform/audio/HRTFElevation.cpp:63
> +#if PLATFORM(GTK) || PLATFORM(MAC) || PLATFORM(EFL) || PLATFORM(QT)

Should this be PLATFORM(MAC) || USE(WEBAUDIO_STUFF_WITH_STREAMER_OR_SOMETHING)?

>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:30
>> +#endif
> 
> Deserves comment I think

I'm surprised this is still needed anyway. I thought Lauro removed all uses of "signals:" in WebKit and the Qt headers should be clean, i.e. use Q_SIGNALS. Are you sure this is still needed?

> Source/WebCore/platform/audio/qt/AudioBusQt.cpp:30
> +PassOwnPtr<AudioBus> AudioBus::loadPlatformResource(const char* name, float sampleRate)

I suggest to rewrite the entire function to be a lot simpler. There is no need to support AUDIO_RESOURCES_PATH, because there is no "uninstalled case". Qt resources are compiled into the library and are therefore always available at run-time. createBusFromAudioFile() however won't work, because it uses filesrc in gstreamer, which does not support Qt resources. Therefore I suggest to boil this down to
something along the lines of this:

QString path = QStringLiteral(":/webkit/resources/audio/");
path.append(QLatin1String(name));
QResource resource(path);
return createBusFromInMemoryAudioFile(resource.data(), resource.size(), /* mixToMono */false, sampleRate);

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:160
> +        clean_env['AUDIO_RESOURCES_PATH'] = self._filesystem.join(self._config.webkit_base_dir(),
> +                                                                  'Source', 'WebCore', 'platform', 'audio', 'resources')

I don't think that this is needed, given that qt resources are compiled into the webkit library.

[Philippe Normand]

Here's the error if I don't undef signals. I'm using r126740.

In file included from /usr/include/glib-2.0/gio/gio.h:54:0,
                from /home/phil/gst/jhbuild/build/WebKit/Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:27:
/usr/include/glib-2.0/gio/gdbusintrospection.h:157:21: error: expected unqualified-id before ‘public’
/usr/include/glib-2.0/gio/gdbusintrospection.h:157:20: error: expected ‘;’ at end of member declaration
/usr/include/glib-2.0/gio/gdbusintrospection.h:157:27: error: expected ‘:’ before ‘;’ token

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 160686 [details])
> 
> > Source/WebCore/platform/audio/qt/AudioBusQt.cpp:30
> > +PassOwnPtr<AudioBus> AudioBus::loadPlatformResource(const char* name, float sampleRate)
> 
> I suggest to rewrite the entire function to be a lot simpler. There is no need to support AUDIO_RESOURCES_PATH, because there is no "uninstalled case". Qt resources are compiled into the library and are therefore always available at run-time. createBusFromAudioFile() however won't work, because it uses filesrc in gstreamer, which does not support Qt resources. Therefore I suggest to boil this down to
> something along the lines of this:
> 
> QString path = QStringLiteral(":/webkit/resources/audio/");
> path.append(QLatin1String(name));
> QResource resource(path);
> return createBusFromInMemoryAudioFile(resource.data(), resource.size(), /* mixToMono */false, sampleRate);
> 

Hum that sounds like a great idea yes but the Composite resource is not found in the binary I build, should I pass any specific option apart from the .qrc file update?

[Philippe Normand]

Sorry the path in the qrc file was wrong :)

[Philippe Normand]

Created attachment 160708 [details]
Patch

[Simon Hausmann]

Comment on attachment 160708 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:31
> +#if PLATFORM(QT)
> +// Clear out offending Qt macro so the following header, gio.h, can be included.
> +#undef signals
> +#endif

I think that's okay for the moment. It would be great if you could file a bug report and assign it to me to fix this and maybe add a link to it in a comment. But either way this can be fixed at a later point.

[Philippe Normand]

Committed r126756: <http://trac.webkit.org/changeset/126756>