[Webkit-unassigned] [Bug 69835] [GStreamer] WebAudio AudioDestination
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 9 23:40:28 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=69835
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #114025|review? |review-
Flag| |
--- Comment #12 from Martin Robinson <mrobinson at webkit.org> 2011-11-09 23:40:28 PST ---
(From update of attachment 114025)
View in context: https://bugs.webkit.org/attachment.cgi?id=114025&action=review
Just a lot of nitpicky stuff below. I'll try to get a moment later to get a deeper understanding of this patch. Since GStreamer experts have already looked at it though, I'm not too worried.
> Source/WebCore/ChangeLog:31
> + * GNUmakefile.list.am:
> + * platform/audio/gstreamer/AudioDestinationGStreamer.cpp:
> + (WebCore::onGStreamerWavparsePadAddedCallback):
> + (WebCore::AudioDestinationGStreamer::AudioDestinationGStreamer):
> + (WebCore::AudioDestinationGStreamer::~AudioDestinationGStreamer):
> + (WebCore::AudioDestinationGStreamer::plugSink):
> + (WebCore::AudioDestinationGStreamer::start):
> + (WebCore::AudioDestinationGStreamer::stop):
> + * platform/audio/gstreamer/AudioDestinationGStreamer.h:
> + * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: Added.
> + (getGStreamerMonoAudioCaps):
> + (webKitWebAudioGStreamerChannelPosition):
> + (webkit_web_audio_src_class_init):
> + (webkit_web_audio_src_init):
> + (webKitWebAudioSrcConstructed):
> + (webKitWebAudioSrcFinalize):
> + (webKitWebAudioSrcSetProperty):
> + (webKitWebAudioSrcGetProperty):
> + (webKitWebAudioSrcLoop):
> + (webKitWebAudioSrcChangeState):
> + * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h: Added.
Please fill in this information.
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:31
> +#include "WebKitWebAudioSourceGStreamer.h"
> +
> +#include <gst/gst.h>
> +#include <gst/pbutils/pbutils.h>
No newline here.
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:58
> + static bool gstInitialized = false;
> + if (!gstInitialized)
> + gstInitialized = gst_init_check(0, 0, 0);
This code seems to be the same as the block in AudioFileReader::createBus. Is there any way to abstract it to a shared function somewhere?
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:63
> + GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC, "rate", static_cast<gfloat>(sampleRate), "bus", static_cast<gpointer>(&m_renderBus), "provider", &m_provider, NULL));
I think it would be good to break up this line in the way we often break up g_object_new and g_object_set calls. You don't need to cast float to gfloat nor do I think you need to cast &m_renderBus to gpointer. gfloat is just a thin veil over float and gpointer is just a thin veil over void*. In general, pointers to data types never need to be cast to void *, unless it's a const pointer, in which case you should use const_cast.
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> + GstElement* wavparse = gst_element_factory_make("wavparse", 0);
wavParser?
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:114
> + GstPad* sinkPad = gst_element_get_static_pad(audioconvert, "sink");
> + gst_pad_link(pad, sinkPad);
> + gst_object_unref(GST_OBJECT(sinkPad));
RefPtr here?
> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:126
> + ASSERT(m_wavParserAvailable);
> + if (!m_wavParserAvailable)
> + return;
Could the wav parser not be avilable for a valid reason (missing codec, for instance)? If that's the case I don't think an assert is the right thing here. That will cause a debug build to crash immediately, of course. That shouldn't happen in a valid error condition.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:30
> +#include "GOwnPtr.h"
> +
> +#include <gst/audio/multichannel.h>
> +#include <gst/pbutils/pbutils.h>
No blank line here.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:34
> +#define WEBKIT_WEB_AUDIO_SRC_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrcPrivate))
Ideally the private data would be allocated via the WTF allocator and not via the GLib allocator.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:42
> +struct _WebKitWebAudioSrcPrivate {
_WebKitWebAudioSrcPrivate -> _WebKitWebAudioSourcePrivate
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:47
> + guint64 offset; // current buffer offset.
Instead of making a comment why not just call the member current_buffer_offset?
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:50
> + GstElement* wavenc;
wavEncoder?
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:55
> + GSList* pads; // list of queue src pads. One queue for each planar audio channel.
'list' should be capitalized.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:56
> + GstPad* srcpad; // src pad of the element, interleaved wav data is pushed to it.
sourcePad?
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:71
> +static void webKitWebAudioSrcSetProperty(GObject*, guint, const GValue*, GParamSpec*);
> +static void webKitWebAudioSrcGetProperty(GObject*, guint, GValue*, GParamSpec*);
The guint parameters should have names.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:77
> + return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, 1, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);
I think it will make this easier to read if you break it up.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:123
> + gst_element_class_add_pad_template(eklass,
> + gst_static_pad_template_get(&srcTemplate));
This can be one line.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:129
> + (gchar*) "WebKit WebAudio source element",
> + (gchar*) "Source",
> + (gchar*) "Handles WebAudio data from WebCore",
> + (gchar*) "Philippe Normand <pnormand at igalia.com>");
> +
You don't need to cast const char* to char*.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:142
> + (GParamFlags) (G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE)));
Please use static_cast here. In the API layer we typically save this to a temporary to avoid all the chatter.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:175
> + priv->task = gst_task_create((GstTaskFunction) webKitWebAudioSrcLoop, src);
Please use a C++ style cast here.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:179
> +static void webKitWebAudioSrcConstructed(GObject *object)
The asterisk is in the wrong place here.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:217
> + GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
> + g_object_set(capsfilter, "caps", monoCaps, NULL);
> + gst_caps_unref(monoCaps);
> +
monoCaps should be a smart pointer.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:219
> + g_object_set(queue, "max-size-buffers", static_cast<guint>(1), NULL);
You should not need to cast 1 to a uint.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:251
> + if (priv->interleave)
> + gst_object_unref(GST_OBJECT(priv->interleave));
> + if (priv->wavenc)
> + gst_object_unref(GST_OBJECT(priv->wavenc));
> +
If your private data structure was a C++ object you could use a the destructor and smart pointers could take care of most of this cleanup. We use this pattern quite a bit in the API layer.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:257
> + GST_CALL_PARENT(G_OBJECT_CLASS, finalize, ((GObject* )(src)));
G_OBJECT(src) here?
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:265
> + switch (propID) {
Nit: propID = propId
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:270
> + priv->bus = reinterpret_cast<AudioBus*>(g_value_get_pointer(value));
You only need a staic cast here.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:273
> + priv->provider = reinterpret_cast<AudioSourceProvider*>(g_value_get_pointer(value));
Ditto.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:281
> +static void webKitWebAudioSrcGetProperty(GObject* object, guint propID, GValue* value, GParamSpec* pspec)
propID -> propId.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:311
> + size_t requestedFrames = 128;
This should probably be static with a comment explaining why you chose 128 here.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:312
> + guint bufferSize = requestedFrames * sizeof(float);
Please use unsigned instead of guint.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:315
> + for (guint index = 0; index < g_slist_length(priv->pads); index++) {
Ditto.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:324
> + GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
This should be asmart pointer.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:340
> + GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
ret -> returnValue
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:347
> + gst_element_post_message(element,
> + gst_missing_element_message_new(element, "interleave"));
This can be one line
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:348
> + GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no interleave"));
Why the parenthesis around 0?
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:354
> + gst_element_post_message(element,
> + gst_missing_element_message_new(element, "wavenc"));
> + GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no wavenc"));
Dittos.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:363
> + if (G_UNLIKELY(ret == GST_STATE_CHANGE_FAILURE)) {
There's a WTF version of LIKELY and UNLIKELY. I think we should use those in WebCore code.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:25
> +G_BEGIN_DECLS
Is this public API? G_BEGIN_DECLS ensures that these symbols are unmangled so that they can be called from a foriegn C compilation unit. Is that necessary? A lot of this header looks like stuff you can omit actually. A lot of it can probably be private to the implementation file and the dead code (uncalled) should probably be removed. I think we should make this change for all private GObjects. Basically just trim it down to what you need in some other file.
> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:40
> + WebKitWebAudioSrcPrivate *priv;
The asterisk is in the wrong place here.
--
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