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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2012-12-27 09:58:51 PST ---
(From update of attachment 180805)
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.

> 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.

> 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.

> 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.

> 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?

> 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?

> 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.

> 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.

> 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.

> 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?

> 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.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:242
> +    GstCaps* caps = getGStreamerAudioCaps(2, 44100);

Maybe some global constants like gNumberOfChannels and gBitRate?

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

English nit: clean -> cleanUp

> 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?

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