[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 #24 from Martin Robinson <mrobinson at webkit.org>  2013-01-18 09:36:28 PST ---
(From update of attachment 183025)
View in context: https://bugs.webkit.org/attachment.cgi?id=183025&action=review

Looking a bit better. I still have some questions though...

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:21
> +#include "config.h"
> +
> +#include "AudioSourceProviderGStreamer.h"

Extra newline here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:37
> +#include <wtf/UnusedParam.h>

This should go in the first block of includes.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:91
> +#ifdef GST_API_VERSION_1
> +    if (!gst_buffer_list_length(buffers)) {
> +        bus->zero();
> +        return;
> +    }
> +
> +    GstBuffer* buffer = gst_buffer_list_get(buffers, 0);
> +    GstMapInfo info;
> +    gst_buffer_map(buffer, &info, GST_MAP_READ);
> +    memcpy(bus->channel(channelNumber)->mutableData(), info.data, info.size);
> +    gst_buffer_unmap(buffer, &info);
> +    gst_buffer_list_remove(buffers, 0, 1);
> +#else
> +    GstBufferListIterator* iter = gst_buffer_list_iterate(buffers);
> +    gst_buffer_list_iterator_next_group(iter);
> +    GstBuffer* buffer = gst_buffer_list_iterator_next(iter);
> +    if (!buffer) {
> +        bus->zero();
> +        gst_buffer_list_iterator_free(iter);
> +        return;
> +    }
> +
> +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(GST_BUFFER_DATA(buffer)), GST_BUFFER_SIZE(buffer));
> +    gst_buffer_list_iterator_remove(iter);
> +    gst_buffer_list_iterator_free(iter);
> +#endif

Here's on block where the #ifs are still opposite of the rest of the file.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:167
> +#ifndef GST_API_VERSION_1
> +    if (m_audioSinkBin) {
> +        gst_object_unref(m_audioSinkBin);
> +        m_audioSinkBin = 0;
> +    }
> +    gst_buffer_list_iterator_free(m_frontLeftBuffersIterator);
> +    gst_buffer_list_iterator_free(m_frontRightBuffersIterator);
> +#endif
> +    gst_buffer_list_unref(m_frontLeftBuffers);
> +    gst_buffer_list_unref(m_frontRightBuffers);

I still don't really understand why m_audioSinkBin isn't a RefPtr or unreffed for Gstreamer 0.1. I think that the not-unreffing thing deserves at least a comment explaining why.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:188
> +    GstCaps* caps = gst_buffer_get_caps(buffer);

You can use a GRefPtr here, right?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:193
> +    GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);

You could use a GOwnPtr here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:209
> +    GstSample* sample = gst_app_sink_pull_sample(sink);

Why not add GstSample support to GRefPtrGStreamer?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:409
> +    bool done = false;
> +    while (!done) {
> +#ifndef GST_API_VERSION_1
> +        gpointer data;
> +
> +        switch (gst_iterator_next(iter, &data)) {
> +        case GST_ITERATOR_OK: {
> +            GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
> +            cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> +            break;
> +        }
> +#else
> +        GValue item = G_VALUE_INIT;
> +        switch (gst_iterator_next(iter, &item)) {
> +        case GST_ITERATOR_OK: {
> +            GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
> +            cleanUpElementsAfterDeinterleaveSourcePad(pad);
> +            break;
> +        }
> +#endif
> +        case GST_ITERATOR_RESYNC:
> +            gst_iterator_resync(iter);
> +            break;
> +        case GST_ITERATOR_ERROR:
> +            done = true;
> +            break;
> +        case GST_ITERATOR_DONE:
> +            done = true;
> +            break;
> +        }
> +#ifdef GST_API_VERSION_1

I think maybe you can simplify this a bit:

#ifndef GST_API_VERSION_1
gpointer item;
#else
GValue item = G_VALUE_INIT;
#endif


GstIteratorResult result = gst_iterator_next(iter, &item)
while (result != GST_ITERATOR_ERROR && result != GST_ITERATOR_DONE) {
    if (result == GST_ITERATOR_OK) {
#ifndef GST_API_VERSION_1
    GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
     cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
#else
    GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
    cleanUpElementsAfterDeinterleaveSourcePad(pad);
#endif
    } elseif (result == GST_ITERATOR_RESYNC
        gst_iterator_resync(iter);
   else
       ASSERT(result == GST_ITERATOR_DONE || result == GST_ITERATOR_ERROR);
}

It seems like you are also missing some error handling here?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:410
> +        g_value_unset(&item);

Do you only need to unset the value at the end, only when result == GST_ITERATOR_OK, or every iteration?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:43
> +        static PassOwnPtr<AudioSourceProviderGStreamer> create() { return adoptPtr(new AudioSourceProviderGStreamer()); }
> +        AudioSourceProviderGStreamer();
> +        ~AudioSourceProviderGStreamer();
> +
> +        void provideInput(AudioBus*, size_t framesToProcess);
> +        void setClient(AudioSourceProviderClient*);

"public" and "private" should be at the first level of indentation.

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