[Webkit-unassigned] [Bug 105293] [GStreamer] Port WebAudio backend to 1.0 APIs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=105293


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #180894|review?                     |review+
               Flag|                            |




--- Comment #12 from Martin Robinson <mrobinson at webkit.org>  2013-01-03 12:01:18 PST ---
(From update of attachment 180894)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.