[Webkit-unassigned] [Bug 78883] [GStreamer] AudioSourceProvider support in the MediaPlayer

Thu Dec 27 10:32:19 PST 2012

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

--- Comment #10 from Philippe Normand <pnormand at igalia.com>  2012-12-27 10:34:25 PST ---
(In reply to comment #9)
> (From update of attachment 180805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180805&action=review
> 
> Okay. There's a great deal of this that I don't understand yet, so I can't really do a full review. Here are some questions and suggestions though.
> 

Thanks :)

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:23
> > +#include "AudioSourceProviderGStreamer.h"
> 
> AudioSourceProviderGStreamer.h also has the ENABLE(WEB_AUDIO) guard so maybe it can go above the guard here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> > +using namespace std;
> 
> This is actually a violation of style rule 3 in the "using" Statement section:
> 
> In C++ implementation files, do not use "using" declarations of any kind to import names in the standard template library. Directly qualify the names at the point they're used instead.
> 
> You basically need to use "std::function" everywhere.
> 

Hum I didn't know that rule, I suspect other code I wrote breaks the law :)
Anyway, not sure I actually use any std:: function, I'll check.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:46
> > +    return gst_caps_new_simple("audio/x-raw", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "format", G_TYPE_STRING, gst_audio_format_to_string(GST_AUDIO_FORMAT_F32), "layout", G_TYPE_STRING, "interleaved", NULL);
> > +#else
> > +    return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);
> > +#endif
> > +}
> 
> This is a situation where I think having a single line for each pair makes this more readable.
> 

Yes but the style bot will complain. I'd like to move this function to GStreamerVersioning or Utilities btw as it's a copy/paste of the one in AudioFileReader.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:58
> > +static void copyGstreamerBuffersToAudioChannel(GstBufferList* buffers, AudioBus* bus , int channelNumber, size_t framesToProcess)
> 
> Looks like you have an extra space after "bus". Is framesToProcess totally unused here?
> 

Yes unused but we should ASSERT its value I think, because the constants used in this file depend on it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:71
> > +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(info.data), info.size);
> > +    gst_buffer_unmap(buffer, &info);
> > +    gst_buffer_list_remove(buffers, 0, 1);
> 
> Hrm. Is there a way to process the data directly into the AudioBus or is provideInput called after handleAudioBuffer?
> 

provideInput is called periodically, I'm afraid there's no way to avoid this memcpy(), but I'll think again about it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:131
> > +    GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin), "deinterleave"));
> 
> The deinterleave element isn't unreffed automatically by the destuction of m_audioSinkBin? By adopting here and putting it in a GRefPtr you are doing an implicit unref.
> 

Yes but gst_bin_get_by_name refs the element.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:139
> > +    if (m_audioSinkBin) {
> > +        gst_object_unref(m_audioSinkBin);
> > +        m_audioSinkBin = 0;
> > +    }
> 
> There are a few things that I don't understand here:
> 1. m_audioSinkBin seems like it could be a GRefPtr, but it isn't.
> 2. You only unref m_audioSinkBin for older versions of GStreamer.
> 3. You don't actually need to zero out members in the destructor, unless you are about to read it somewhere. The object is about to disappear.
> 

Hum I used the same approach as in the MediaPlayerPrivate. I'll revise this part with a fresh mind :)

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:158
> > +#ifdef GST_API_VERSION_1
> 
> Okay. This is a bit of a nit, but you use  #ifdef GST_API_VERSION_1 some places and #ifndef GST_API_VERSION_1 other places. It's a little clearer to use the same everywhere.
> 

Yes, good point again.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:229
> > +    GstElement* audioQueue2 = gst_element_factory_make("queue", 0);
> 
> There's no audioQueue in this context, so I guess you can just call this audioQueue?
> 

Ah yes forgot to rename it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:236
> > +    int size = 256 * sizeof(float);
> 
> This should probably be static const and maybe named something like chopperDataSize.
> 

Yes I forgot to document this value as well.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:242
> > +    GstCaps* caps = getGStreamerAudioCaps(2, 44100);
> 
> Maybe some global constants like gNumberOfChannels and gBitRate?
> 

Yes!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:306
> > +void AudioSourceProviderGStreamer::cleanElementsAfterDeinterleaveSourcePad(GstPad* pad)
> 
> English nit: clean -> cleanUp
> 

Oh, ok!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:317
> > +    gst_bin_remove_many(GST_BIN(m_audioSinkBin), queue.get(), sink.get(), NULL);
> 
> Will removing the element decrease their reference count? If so, I guess you don't need to also place them in GRefPtrs?
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:26
> > +#include <gst/gst.h>
> 
> Can you avoid using the gstreamer header here?

Hum it can't because of GstFlowReturn which is an enum which can't be forward-declared.

Thanks I'll upload a new patch tomorrow hopefully :)

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