[Webkit-unassigned] [Bug 69835] [GStreamer] WebAudio AudioDestination
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 10 06:25:40 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=69835
--- Comment #13 from Philippe Normand <pnormand at igalia.com> 2011-11-10 06:25:40 PST ---
(In reply to comment #12)
> (From update of attachment 114025 [details])
> 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.
>
Done
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:31
> > +#include "WebKitWebAudioSourceGStreamer.h"
> > +
> > +#include <gst/gst.h>
> > +#include <gst/pbutils/pbutils.h>
>
> No newline here.
>
Ok!
> > 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?
>
Hum. The issue I see with that is that AudioDestination and the FileReader can run in different threads, we really need at least two calls of gst_init_check on each side and avoid additional ones, not sure how to implement that in a separate function.
> > 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.
>
Ok!
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> > + GstElement* wavparse = gst_element_factory_make("wavparse", 0);
>
> wavParser?
>
Sure!
> > 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?
>
Sure, I'll send a separate patch for GstPad support in GRefPtrGStreamer though.
> > 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.
>
wavparse is in gst-plugins-good which is a runtime dependency. Not having -good will make this useless anyway. I thought asserting was the way to go in this case but I'm ok with removing it :)
> > 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.
>
Sure!
> > 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.
Hum, ok. Trying this with help from Carlos.
>
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:42
> > +struct _WebKitWebAudioSrcPrivate {
>
> _WebKitWebAudioSrcPrivate -> _WebKitWebAudioSourcePrivate
>
OK.
> > 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?
>
Ok!
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:50
> > + GstElement* wavenc;
>
> wavEncoder?
>
Ok!
> > 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.
>
Ok!
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:56
> > + GstPad* srcpad; // src pad of the element, interleaved wav data is pushed to it.
>
> sourcePad?
>
Ok!
> > 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.
>
Ok!
> > 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.
>
Ok, but style bot won't like it! :)
> > 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.
>
Sure.
> > 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*.
>
Ok!
> > 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.
>
Done!
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:175
> > + priv->task = gst_task_create((GstTaskFunction) webKitWebAudioSrcLoop, src);
>
> Please use a C++ style cast here.
>
Ok!
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:179
> > +static void webKitWebAudioSrcConstructed(GObject *object)
>
> The asterisk is in the wrong place here.
>
Oops.
> > 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.
>
Hum, this will need support in GRefPtrGStreamer as well :( Ok... :)
> > 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.
>
Well it turns out I do :(
Spent an hour figuring it out when I added this.
> > 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.
>
Alright.
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:257
> > + GST_CALL_PARENT(G_OBJECT_CLASS, finalize, ((GObject* )(src)));
>
> G_OBJECT(src) here?
>
Sure.
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:265
> > + switch (propID) {
>
> Nit: propID = propId
>
I made it propertyId.
> > 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.
>
Ok.
> > 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.
>
Alright I'll make it a constant in the AudioDestination implementation and pass it as new property to the src element, if that's OK with you.
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:312
> > + guint bufferSize = requestedFrames * sizeof(float);
>
> Please use unsigned instead of guint.
>
Ok.
> > 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.
>
I tried a GRefPtr here but it doesn't work when I need to update the GstStructure just below.
> > 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?
>
In the macro the (0) text argument is used like this: gchar *__txt = _gst_element_error_printf text;
So here it translates to gchar *__txt = _gst_element_error_printf(0);
I can add a more relevant value if necessary :)
> > 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.
>
Okeys ^ :)
> > 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.
>
Sure.
> > 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.
>
Hum. OK!
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:40
> > + WebKitWebAudioSrcPrivate *priv;
>
> The asterisk is in the wrong place here.
Oh, my! :)
Thanks for the review Martin!
--
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