[Webkit-unassigned] [Bug 69834] [GStreamer] WebAudio AudioFileReader implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 11 08:50:45 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=69834
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #110510|review? |review-
Flag| |
--- Comment #2 from Martin Robinson <mrobinson at webkit.org> 2011-10-11 08:50:45 PST ---
(From update of attachment 110510)
View in context: https://bugs.webkit.org/attachment.cgi?id=110510&action=review
Nice. Below are some initial comments. I have not yet been able to look at the way you are setting up your pipeline in detail, but perhaps sometime later you can walk me through it. I have tried not to Martinize too intensely!
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:27
> +#include "AudioDestinationGStreamer.h"
> +
> +#include "AudioChannel.h"
> +#include "AudioSourceProvider.h"
> +#include "GOwnPtr.h"
This should be one block of headers.
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:38
> + return 44100.0;
Omit the final .0 here.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:35
> +#include "AudioBus.h"
> +#include "AudioFileReader.h"
> +#include "GOwnPtr.h"
> +#include "GRefPtr.h"
> +
> +#include <gio/gio.h>
> +#include <gst/app/gstappsink.h>
> +#include <gst/audio/multichannel.h>
> +#include <gst/base/gstbytereader.h>
> +#include <gst/pbutils/pbutils.h>
Clump these together.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:37
> +#define WEBAUDIO_GST_CAPS_TEMPLATE "audio/x-raw-float, rate=(int)%d, channels=(int)%d, endianness=(int)%d, width=(int)32"
String constants like this are typically defined as static const char*. Since it's only used in getGStreamerAudioCaps it would be better to have it in the body of that method too.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:43
> +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_le
> +#else
> +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_be
> +#endif
Instead of doing it this way, I prefer something like:
static inline bool getFloatFromByteReader(GstByteReader *reader, float *val)
{
#if G_BYTE_ORDER == G_LITTLE_ENDIAN
#else
#endif
}
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:56
> + gpointer buf = g_malloc0(size);
> + float* current = (float*) buf;
I think you can avoid this allocation (see below). If not, please use the WTF allocator here instead of the GLib one and a C++ cast.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:62
> + while (_webaudio_gst_byte_reader_get_float(byteReader, &value))
> + *current++ = (float) value;
> + gst_byte_reader_free(byteReader);
You shouldn't need to cast between float and gfloat. In fact, it might be better to use float here to begin with.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:69
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + reader->handleEndOfStream();
Might as well make this just one line. Alternatively, just accept an AudioFileReader as an argument. I'm almost certain you only need a static_cast here.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:75
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + return reader->handleBuffer(sink);
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:81
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + return reader->handleMessage(message);
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:88
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + reader->handleNewDeinterleavePad(pad);
> +}
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:94
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + reader->deinterleavePadsConfigured();
> +}
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:99
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + reader->plugDeinterleave(pad);
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:105
> + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData);
> + reader->start();
Ditto.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:169
> + const GValue* value = gst_structure_get_value(structure, "channel-positions");
Might want to give this a more descrptive name like channelPositionsValue. I had to look back to figure out what it was.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:173
> + if (channels && value && gst_value_is_fixed(value)) {
Is this an error situation? You don't return GST_FLOW_ERROR in the else case.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:180
> + mergedBuffer = gst_buffer_join(m_bufferFrontLeft, buffer);
> + m_bufferFrontLeft = gst_buffer_ref(mergedBuffer);
Why not just do m_bufferFrontLeft = gst_buffer_ref(gst_buffer_join(m_bufferFrontLeft, buffer)); and dispsense with mergedBuffer altogether?
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:215
> + gchar* padName = gst_pad_get_name(pad);
I recommend a GOwnPtr here.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:233
> + gst_object_unref(GST_OBJECT(sinkPad));
Please use GRefPtr in these cases.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:252
> + GstElement* audioconvert = gst_element_factory_make("audioconvert", 0);
> + GstElement* audioresample = gst_element_factory_make("audioresample", 0);
> + GstElement* capsfilter = gst_element_factory_make("capsfilter", 0);
> + GstElement* deinterleave = gst_element_factory_make("deinterleave", "deinterleave");
Please use camelCase for these names.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:258
> + g_object_set(capsfilter, "caps", getGStreamerAudioCaps(2, m_sampleRate), NULL);
Does gst_caps_from_string return a newly-allocated GstCaps? If so, isn't this a memory leak?
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:313
> + GOwnPtr<GError> error;
> + gstInitialized = gst_init_check(0, 0, &error.outPtr());
> + if (!gstInitialized)
> + return nullptr;
Will this interact nicely with the version of this in MediaPlayerPrivateGStreamer.cpp? Nice use of nullptr!
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:316
> + // properly dispatch its messages to our end.
Nit: dispatches
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319
> + m_context = g_main_context_new();
> + g_main_context_push_thread_default(m_context);
> + m_loop = g_main_loop_new(m_context, FALSE);
I guess this blocks the main loop. :(
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:327
> + g_source_set_callback(timeoutSource, (GSourceFunc) enteredMainLoopCallback, this, 0);
Please use reinterpret_cast instead of a C style cast.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:333
> + guint size = GST_BUFFER_SIZE(m_bufferFrontLeft);
We try not to use the simple Glib wrappers for C types that are local. Thus this can be unsigned.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:344
> + float* bufferLeft = getGStreamerAudioData(m_bufferFrontLeft, size);
> + float* bufferRight = getGStreamerAudioData(m_bufferFrontRight, size);
> +
> + memcpy(audioBus->channel(0)->data(), bufferLeft, size);
> + if (!mixToMono)
> + memcpy(audioBus->channel(1)->data(), bufferRight, size);
> +
I think here you should pass (audioBus->channel(0)->data() directly to getGStreamerAudioData and avoid a memory copy and duplicate allocation entirely.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:360
> + AudioFileReader* reader = new AudioFileReader(filePath);
> + return reader->createBus(sampleRate, mixToMono);
Aren't you leaking the reader here? Why not just stack allocate it?
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:366
> + AudioFileReader* reader = new AudioFileReader(data, dataSize);
> + return reader->createBus(sampleRate, mixToMono);
Ditto. This factory seems to be unused currently...
> Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:37
> + return createBusFromAudioFile(absoluteFilename.get(), false, sampleRate);
You only use the createBusFromAudioFile in one place and it's just two lines. I think it's clearer to just put the code here, unless you plan to use it a lot in followup patches.
--
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