Bug 105293 – [GStreamer] Port WebAudio backend to 1.0 APIs

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

SSIA

[Philippe Normand]

Created attachment 179948 [details]
Patch

[WebKit Review Bot]

Attachment 179948 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:101:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:111:  Missing space before ( in for(  [whitespace/parens] [5]
Source/WebCore/platform/audio/FFTFrame.h:57:  "gst/fft/gstfftf32.h" already included at Source/WebCore/platform/audio/FFTFrame.h:54  [build/include] [4]
Total errors found: 7 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 179953 [details]
Patch

[Philippe Normand]

Sebastian, can you have a look please?

[Philippe Normand]

Created attachment 180792 [details]
Patch

[Martin Robinson]

Comment on attachment 180792 [details]
Patch

I wonder if it makes sense to make the WebAudio backend depend on GStreamer 1.0?

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 180792 [details])
> I wonder if it makes sense to make the WebAudio backend depend on GStreamer 1.0?

Well, no strong opinion on that, it'd be a bit sad though now that we support both versions. I was planning to remove the 0.10 support after gst 1.2.

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 180792 [details] [details])
> > I wonder if it makes sense to make the WebAudio backend depend on GStreamer 1.0?
> 
> Well, no strong opinion on that, it'd be a bit sad though now that we support both versions. I was planning to remove the 0.10 support after gst 1.2.

Okay, let's do that!

[Philippe Normand]

Created attachment 180894 [details]
Patch

[WebKit Review Bot]

Attachment 180894 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:101:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:106:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:158:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:163:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Complying with the style bot is sometimes a bit tricky :)

[Martin Robinson]

Comment on attachment 180894 [details]
Patch

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

Okay. This all looks pretty reasonable to me, with no obvious problems. The GStreamer code we have is becoming very complicated so perhaps it makes sense to only use GStreamer 1.0 for WebAudio soon.

> Source/WebCore/platform/audio/FFTFrame.h:58
> +#ifndef GST_API_VERSION_1
>  G_BEGIN_DECLS
> +#endif
>  #include <gst/fft/gstfftf32.h>
> +#ifndef GST_API_VERSION_1
>  G_END_DECLS
> +#endif

There's no harm in having nested extern "C" { as far as I know, so you can probably just omit the #ifdefs here and make this more readable.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:41
> +#ifdef GST_API_VERSION_1
> +#include <gst/audio/audio.h>
> +#else
>  #include <gst/audio/multichannel.h>
> +#endif

If possible, move #includes that are guarded by #ifdefs to the end of the include block in a new section.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:111
> +        GstBuffer* buffer = gst_buffer_list_get(buffers, i);
> +        if (buffer) {
> +            GstMapInfo info;
> +            gst_buffer_map(buffer, &info, GST_MAP_READ);
> +            memcpy(audioChannel->mutableData() + offset, reinterpret_cast<float*>(info.data), info.size);
> +            offset += info.size / sizeof(float);
> +            gst_buffer_unmap(buffer, &info);
> +        }

I think I'd prefer a continue here rather than another level of nesting.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:252
> +        GstAudioChannelPosition channelPosition = webKitWebAudioGStreamerChannelPosition(channelIndex);
> +        GstAudioInfo info;
> +        gst_audio_info_from_caps(&info, monoCaps.get());
> +        GST_AUDIO_INFO_POSITION(&info, 0) = channelPosition;

Why not just:

GST_AUDIO_INFO_POSITION(&info, 0) = webKitWebAudioGStreamerChannelPosition(channelIndex);

?

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:29
> +#ifdef GST_API_VERSION_1
> +#include <gst/audio/audio.h>
> +#else
> +#include <gst/audio/multichannel.h>
> +#endif

#includes that are interspersed with #ifdefs should be after the main include block, generally.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:34
>  
> +
>  void webkitGstObjectRefSink(GstObject* gstObject)

Extra newline here.

[Eric Seidel (no email)]

Attachment 180894 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.

[Philippe Normand]

Committed r138786: <http://trac.webkit.org/changeset/138786>

[Philippe Normand]

Thanks for the review Martin, I applied the changes you suggested and landed the patch.

[Chris Dumez]

It appears this patch caused 8 webaudio tests to crash on WK2 EFL:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138842%20%287689%29/results.html

[Chris Dumez]

(In reply to comment #16)
> It appears this patch caused 8 webaudio tests to crash on WK2 EFL:
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138842%20%287689%29/results.html

Hmm. GTK port has gstreamer 1.0.4 installed via jhbuild but EFL does not. We are using gstreamer 0.10.x from the distro. I guess we will need to add gstreamer 1.0.4 to EFL port's jhbuild (Bug 106178).

[Martin Robinson]

(In reply to comment #17)
> (In reply to comment #16)
> > It appears this patch caused 8 webaudio tests to crash on WK2 EFL:
> > http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138842%20%287689%29/results.html
> 
> Hmm. GTK port has gstreamer 1.0.4 installed via jhbuild but EFL does not. We are using gstreamer 0.10.x from the distro. I guess we will need to add gstreamer 1.0.4 to EFL port's jhbuild (Bug 106178).

Right now the webaudio backend should work with both GStreamer 0.10 and GStreamer 1.x so this seems like a real regression.

[Philippe Normand]

Right. Can you please provide a stack trace and GST_DEBUG=webkit*:5 trace?

[Philippe Normand]

Some changes of webkitwebsrc are not ifdeffed, I suspect the regression might be related to those.