[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 #49 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Comment on attachment 242818
  --> https://bugs.webkit.org/attachment.cgi?id=242818
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242818&action=review

Looks good, just some comments

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:54
> +static void copyGstreamerBuffersToAudioChannel(GstAdapter* adapter, AudioBus* bus , int channelNumber, size_t framesToProcess)

GStreamer with a capital S :)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:63
> +    if (gst_adapter_available(adapter) >= bytes) {

What if there are >= bytes available after you took bytes out of the adapter? Should there be a "while (avail >= bytes)" here?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:66
> +            memcpy(destination, data, bytes);

gst_adapter_copy() would be more efficient in theory and does not require mapping first (which is why it could be more efficient)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:69
> +            gst_adapter_flush(adapter, bytes);

I think you should gst_adapter_clear() when receiving a FLUSH_STOP event on the appsink (needs a pad probe, appsink does not tell you unfortunately)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:179
> +    case GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT:

You would get POSITION_MONO here when having mono input

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:249
> +

Maybe check if you get more than 2 pads here, just in case

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:315
> +    GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin.get()), "deinterleave"));

And unmute the volume element maybe?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:322
> +    gst_iterator_foreach(iter, cleanUpElementsAfterDeinterleaveSourcePadCallback, this);

You could cause not-linked errors if the pipeline is still running after all deinterleave srcpads are unlinked. I guess in practice not because of the tee before this, but it's not clear to me to which state reset() should reset.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:323
> +    // GValue item = G_VALUE_INIT;

Remove the commented out code :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1891
>          gst_object_unref(audioSinkBin);

Maybe create the bin only when needed

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141209/7018ac3c/attachment-0002.html>