[Webkit-unassigned] [Bug 69834] [GStreamer] WebAudio AudioFileReader implementation

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 13 14:34:19 PDT 2011


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





--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2011-10-13 14:34:19 PST ---
(From update of attachment 110867)
View in context: https://bugs.webkit.org/attachment.cgi?id=110867&action=review

I'm not sure I understand why you export the AudioFileReader class in AudioFileReaderGStreamer.h. Will you be using from another class in a later patch? if not, might be better to simply stick the class definition in AudioFileReaderGstreamer.cpp and delete AudioFileReaderGStreamer.h.

Looking good in general. I'm tempted to r+ this, but I'll let you respond to my latest comments. Perhaps someone with more GStreamer experience can take a peak too.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:138
> +    if (channels && channelPositionsValue && gst_value_is_fixed(channelPositionsValue)) {
> +        GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);

It'd be better to ues an early return here for the error case.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:164
> +    GOwnPtr<GError> err;

err should be called "error"

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:165
> +    GOwnPtr<gchar> debug;

debug is unused, so you can just pass 0 (null) to gst_message_parse_warning as the last argument.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:259
> +static bool gstInitialized = false;

This variable should be declared locally to method it's used in.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:268
> +    gstInitialized = gst_init_check(0, 0, &error.outPtr());
> +    if (!gstInitialized)
> +        return nullptr;

You never check the value of gstInitialized before running gst_init_check. What's the point of making it static?

>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:302

> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Please fix.

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.h:34
> +PassOwnPtr<AudioBus> createBusFromAudioFile(const char* filePath, bool mixToMono, float sampleRate);
> +PassOwnPtr<AudioBus> createBusFromInMemoryAudioFile(const void* data, size_t dataSize, bool mixToMono, float sampleRate);

If you don't delete this header, I think it would be better to simply include AudioFileReader.h, instead of redclaring these factories. It's weird to have the declarations in two places.

> Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:29
> +#include "AudioFileReaderGStreamer.h"
> +#include "GOwnPtr.h"
> +
> +#include <gio/gio.h>
> +#include <glib.h>

These can be one clump.  Wouldn't it make more sense to just include AudioFileReader.h?

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



More information about the webkit-unassigned mailing list