Created attachment 179948 [details] Patch

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.

Created attachment 179953 [details] Patch

Sebastian, can you have a look please?

Created attachment 180792 [details] Patch

Comment on attachment 180792 [details] Patch I wonder if it makes sense to make the WebAudio backend depend on GStreamer 1.0?

(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.

(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!

Created attachment 180894 [details] Patch

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.

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

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.

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

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

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

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

(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).

(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.

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

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