[Webkit-unassigned] [Bug 69835] [GStreamer] WebAudio AudioDestination

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Philippe Normand <pnormand at igalia.com>  2011-11-11 02:50:26 PST ---
(In reply to comment #16)
> (From update of attachment 114487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114487&action=review
> 
> Just a couple more nitpicky things. I'll try to do a real review soon!
> 
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:35
> > +const unsigned framesToPull = 128;
> 
> This definitely goes beyond what I suggested, but I think it's an interesting approach. I was just interested in having a small comment explaining why you pull 128 frames at a time.
> 

When you create an AudioBus with a specific size you can only pull that number of frames at a time, from my understanding. That's why I made it constant in AudioDestination.

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> > +    GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,
> 
> Sorry maybe just format like this:
> 
> ...g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,
>                 "rate", sampleRate,
>                 "bus",  &m_renderBus,
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:69
> > +enum {
> > +    PROP_RATE = 1,
> > +    PROP_BUS = 2,
> > +    PROP_PROVIDER = 3,
> > +    PROP_FRAMES = 4
> > +};
> 
> I think you can omit the values here...but I'm not certain.
> 

The first one no but the others yes indeed :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:134
> > +    GObjectClass* oklass = G_OBJECT_CLASS(klass);
> > +    GstElementClass* eklass = GST_ELEMENT_CLASS(klass);
> 
> I think I'd prefer objectClass and elementClass here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:220
> > +    // gst_object_ref_sink(GST_OBJECT(priv->interleave));
> > +    // gst_object_ref_sink(GST_OBJECT(priv->wavEncoder));
> 
> Was this left in by mistake?
> 

Heh. Yes :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:26
> > +#define WEBKIT_TYPE_WEB_AUDIO_SRC (webkit_web_audio_src_get_type())
> > +#define WEBKIT_WEB_AUDIO_SRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))
> 
> Nit. You you should remove the extra space before the parenthesis.
> 

What extra space?

> >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30
> >> +GType webkit_web_audio_src_get_type(void);
> > 
> > webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> You can omit the "void" from the argument list.

Sure!

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