[webkit-reviews] review granted: [Bug 105293] [GStreamer] Port WebAudio backend to 1.0 APIs : [Attachment 180894] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 105293: [GStreamer] Port WebAudio backend to 1.0 APIs
https://bugs.webkit.org/show_bug.cgi?id=105293

Attachment 180894: Patch
https://bugs.webkit.org/attachment.cgi?id=180894&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.