Bug 105293 - [GStreamer] Port WebAudio backend to 1.0 APIs
Summary: [GStreamer] Port WebAudio backend to 1.0 APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on: 106182
Blocks: 78883
  Show dependency treegraph
 
Reported: 2012-12-18 07:34 PST by Philippe Normand
Modified: 2014-12-10 09:19 PST (History)
11 users (show)

See Also:


Attachments
Patch (22.73 KB, patch)
2012-12-18 08:32 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (22.80 KB, patch)
2012-12-18 08:43 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (25.69 KB, patch)
2012-12-27 04:49 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (28.34 KB, patch)
2012-12-28 13:00 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-12-18 07:34:46 PST
SSIA
Comment 1 Philippe Normand 2012-12-18 08:32:54 PST
Created attachment 179948 [details]
Patch
Comment 2 WebKit Review Bot 2012-12-18 08:34:18 PST
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.
Comment 3 Philippe Normand 2012-12-18 08:43:09 PST
Created attachment 179953 [details]
Patch
Comment 4 Philippe Normand 2012-12-18 09:01:26 PST
Sebastian, can you have a look please?
Comment 5 Philippe Normand 2012-12-27 04:49:40 PST
Created attachment 180792 [details]
Patch
Comment 6 Martin Robinson 2012-12-27 10:15:12 PST
Comment on attachment 180792 [details]
Patch

I wonder if it makes sense to make the WebAudio backend depend on GStreamer 1.0?
Comment 7 Philippe Normand 2012-12-27 10:37:08 PST
(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.
Comment 8 Martin Robinson 2012-12-27 10:37:48 PST
(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!
Comment 9 Philippe Normand 2012-12-28 13:00:33 PST
Created attachment 180894 [details]
Patch
Comment 10 WebKit Review Bot 2012-12-28 13:03:39 PST
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.
Comment 11 Philippe Normand 2012-12-28 13:17:38 PST
Complying with the style bot is sometimes a bit tricky :)
Comment 12 Martin Robinson 2013-01-03 12:01:18 PST
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.
Comment 13 Eric Seidel (no email) 2013-01-04 00:53:19 PST
Attachment 180894 [details] was posted by a committer and has review+, assigning to Philippe Normand for commit.
Comment 14 Philippe Normand 2013-01-04 02:17:36 PST
Committed r138786: <http://trac.webkit.org/changeset/138786>
Comment 15 Philippe Normand 2013-01-04 02:19:50 PST
Thanks for the review Martin, I applied the changes you suggested and landed the patch.
Comment 16 Chris Dumez 2013-01-05 07:21:54 PST
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
Comment 17 Chris Dumez 2013-01-05 07:35:52 PST
(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).
Comment 18 Martin Robinson 2013-01-05 09:32:19 PST
(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.
Comment 19 Philippe Normand 2013-01-05 12:10:35 PST
Right. Can you please provide a stack trace and GST_DEBUG=webkit*:5 trace?
Comment 20 Philippe Normand 2013-01-05 12:12:19 PST
Some changes of webkitwebsrc are not ifdeffed, I suspect the regression might be related to those.