[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