[GStreamer] Use smart pointers and modernize code in WebKitWebAudioSourceGStreamer
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Jan 2017 11:27:38 +0000 (11:27 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Jan 2017 11:27:38 +0000 (11:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=166886

Reviewed by Xabier Rodriguez-Calvar.

This patch doesn't change the behavior, so it's covered by existing Web Audio tests. It replaces pointers with
smart pointers, uses WTF::Vector instead of GSList and simplifies the code to map/unmap GstBuffers.

* platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:
(webKitWebAudioSrcConstructed):
(webKitWebAudioSrcFinalize):
(webKitWebAudioSrcLoop):
(webKitWebAudioSrcChangeState):
* platform/graphics/gstreamer/GRefPtrGStreamer.cpp:
(WTF::derefGPtr<GstBufferList>):
(WTF::adoptGRef):
(WTF::refGPtr<GstBufferPool>):
(WTF::derefGPtr<GstBufferPool>):
* platform/graphics/gstreamer/GRefPtrGStreamer.h:
* platform/graphics/gstreamer/GStreamerUtilities.cpp:
(WebCore::mapGstBuffer):
* platform/graphics/gstreamer/GStreamerUtilities.h:
* platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
(StreamingClient::createReadBuffer):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@210584 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp
Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp
Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h
Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp
Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h
Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp

index ba648ca..f1509c0 100644 (file)
@@ -1,3 +1,30 @@
+2017-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GStreamer] Use smart pointers and modernize code in WebKitWebAudioSourceGStreamer
+        https://bugs.webkit.org/show_bug.cgi?id=166886
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        This patch doesn't change the behavior, so it's covered by existing Web Audio tests. It replaces pointers with
+        smart pointers, uses WTF::Vector instead of GSList and simplifies the code to map/unmap GstBuffers.
+
+        * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:
+        (webKitWebAudioSrcConstructed):
+        (webKitWebAudioSrcFinalize):
+        (webKitWebAudioSrcLoop):
+        (webKitWebAudioSrcChangeState):
+        * platform/graphics/gstreamer/GRefPtrGStreamer.cpp:
+        (WTF::derefGPtr<GstBufferList>):
+        (WTF::adoptGRef):
+        (WTF::refGPtr<GstBufferPool>):
+        (WTF::derefGPtr<GstBufferPool>):
+        * platform/graphics/gstreamer/GRefPtrGStreamer.h:
+        * platform/graphics/gstreamer/GStreamerUtilities.cpp:
+        (WebCore::mapGstBuffer):
+        * platform/graphics/gstreamer/GStreamerUtilities.h:
+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
+        (StreamingClient::createReadBuffer):
+
 2017-01-11  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r182947.
index aca6b5c..995ef97 100644 (file)
@@ -60,12 +60,15 @@ struct _WebKitWebAudioSourcePrivate {
     GRefPtr<GstTask> task;
     GRecMutex mutex;
 
-    GSList* sources; // List of appsrc. One appsrc for each planar audio channel.
-    GstPad* sourcePad; // src pad of the element, interleaved wav data is pushed to it.
+    // List of appsrc. One appsrc for each planar audio channel.
+    Vector<GRefPtr<GstElement>> sources;
+
+    // src pad of the element, interleaved wav data is pushed to it.
+    GstPad* sourcePad;
 
     guint64 numberOfSamples;
 
-    GstBufferPool* pool;
+    GRefPtr<GstBufferPool> pool;
 };
 
 enum {
@@ -75,11 +78,6 @@ enum {
     PROP_FRAMES
 };
 
-typedef struct {
-    GstBuffer* buffer;
-    GstMapInfo info;
-} AudioSrcBuffer;
-
 static GstStaticPadTemplate srcTemplate = GST_STATIC_PAD_TEMPLATE("src",
     GST_PAD_SRC,
     GST_PAD_ALWAYS,
@@ -220,7 +218,7 @@ static void webKitWebAudioSrcConstructed(GObject* object)
     // appsrc ! . which is plugged to a new interleave request sinkpad.
     for (unsigned channelIndex = 0; channelIndex < priv->bus->numberOfChannels(); channelIndex++) {
         GUniquePtr<gchar> appsrcName(g_strdup_printf("webaudioSrc%u", channelIndex));
-        GstElement* appsrc = gst_element_factory_make("appsrc", appsrcName.get());
+        GRefPtr<GstElement> appsrc = gst_element_factory_make("appsrc", appsrcName.get());
         GRefPtr<GstCaps> monoCaps = adoptGRef(getGStreamerMonoAudioCaps(priv->sampleRate));
 
         GstAudioInfo info;
@@ -229,16 +227,15 @@ static void webKitWebAudioSrcConstructed(GObject* object)
         GRefPtr<GstCaps> caps = adoptGRef(gst_audio_info_to_caps(&info));
 
         // Configure the appsrc for minimal latency.
-        g_object_set(appsrc, "max-bytes", static_cast<guint64>(2 * priv->bufferSize), "block", TRUE,
+        g_object_set(appsrc.get(), "max-bytes", static_cast<guint64>(2 * priv->bufferSize), "block", TRUE,
             "blocksize", priv->bufferSize,
             "format", GST_FORMAT_TIME, "caps", caps.get(), nullptr);
 
-        priv->sources = g_slist_prepend(priv->sources, gst_object_ref(appsrc));
+        priv->sources.append(appsrc);
 
-        gst_bin_add(GST_BIN(src), appsrc);
-        gst_element_link_pads_full(appsrc, "src", priv->interleave.get(), "sink_%u", GST_PAD_LINK_CHECK_NOTHING);
+        gst_bin_add(GST_BIN(src), appsrc.get());
+        gst_element_link_pads_full(appsrc.get(), "src", priv->interleave.get(), "sink_%u", GST_PAD_LINK_CHECK_NOTHING);
     }
-    priv->sources = g_slist_reverse(priv->sources);
 
     // interleave's src pad is the only visible pad of our element.
     GRefPtr<GstPad> targetPad = adoptGRef(gst_element_get_static_pad(priv->interleave.get(), "src"));
@@ -252,8 +249,6 @@ static void webKitWebAudioSrcFinalize(GObject* object)
 
     g_rec_mutex_clear(&priv->mutex);
 
-    g_slist_free_full(priv->sources, reinterpret_cast<GDestroyNotify>(gst_object_unref));
-
     priv->~WebKitWebAudioSourcePrivate();
     GST_CALL_PARENT(G_OBJECT_CLASS, finalize, ((GObject* )(src)));
 }
@@ -319,26 +314,19 @@ static void webKitWebAudioSrcLoop(WebKitWebAudioSrc* src)
         return;
     }
 
+    ASSERT(priv->pool);
     GstClockTime timestamp = gst_util_uint64_scale(priv->numberOfSamples, GST_SECOND, priv->sampleRate);
     priv->numberOfSamples += priv->framesToPull;
     GstClockTime duration = gst_util_uint64_scale(priv->numberOfSamples, GST_SECOND, priv->sampleRate) - timestamp;
 
-    GSList* channelBufferList = 0;
-    for (int i = g_slist_length(priv->sources) - 1; i >= 0; i--) {
-        AudioSrcBuffer* buffer = g_new(AudioSrcBuffer, 1);
-        GstBuffer* channelBuffer;
-
-        GstFlowReturn ret = gst_buffer_pool_acquire_buffer(priv->pool, &channelBuffer, nullptr);
-
+    Vector<GRefPtr<GstBuffer>> channelBufferList;
+    channelBufferList.reserveInitialCapacity(priv->sources.size());
+    for (unsigned i = 0; i < priv->sources.size(); ++i) {
+        GRefPtr<GstBuffer> buffer;
+        GstFlowReturn ret = gst_buffer_pool_acquire_buffer(priv->pool.get(), &buffer.outPtr(), nullptr);
         if (ret != GST_FLOW_OK) {
-            g_free(buffer);
-            while (channelBufferList) {
-                buffer = static_cast<AudioSrcBuffer*>(channelBufferList->data);
-                gst_buffer_unmap(buffer->buffer, &buffer->info);
-                gst_buffer_unref(buffer->buffer);
-                g_free(buffer);
-                channelBufferList = g_slist_delete_link(channelBufferList, channelBufferList);
-            }
+            for (auto& buffer : channelBufferList)
+                unmapGstBuffer(buffer.get());
 
             // FLUSHING and EOS are not errors.
             if (ret < GST_FLOW_EOS || ret == GST_FLOW_NOT_LINKED)
@@ -347,44 +335,38 @@ static void webKitWebAudioSrcLoop(WebKitWebAudioSrc* src)
             return;
         }
 
-        ASSERT(channelBuffer);
-        buffer->buffer = channelBuffer;
-        GST_BUFFER_TIMESTAMP(channelBuffer) = timestamp;
-        GST_BUFFER_DURATION(channelBuffer) = duration;
-        gst_buffer_map(channelBuffer, &buffer->info, (GstMapFlags) GST_MAP_READWRITE);
-        priv->bus->setChannelMemory(i, reinterpret_cast<float*>(buffer->info.data), priv->framesToPull);
-        channelBufferList = g_slist_prepend(channelBufferList, buffer);
+        ASSERT(buffer);
+        GST_BUFFER_TIMESTAMP(buffer.get()) = timestamp;
+        GST_BUFFER_DURATION(buffer.get()) = duration;
+        mapGstBuffer(buffer.get(), GST_MAP_READWRITE);
+        priv->bus->setChannelMemory(i, reinterpret_cast<float*>(getGstBufferDataPointer(buffer.get())), priv->framesToPull);
+        channelBufferList.uncheckedAppend(WTFMove(buffer));
     }
 
     // FIXME: Add support for local/live audio input.
     priv->provider->render(0, priv->bus, priv->framesToPull);
 
-    GSList* sourcesIt = priv->sources;
-    GSList* buffersIt = channelBufferList;
+    ASSERT(channelBufferList.size() == priv->sources.size());
+    bool failed = false;
+    for (unsigned i = 0; i < priv->sources.size(); ++i) {
+        // Unmap before passing on the buffer.
+        auto& buffer = channelBufferList[i];
+        unmapGstBuffer(buffer.get());
 
-    GstFlowReturn ret = GST_FLOW_OK;
-    for (int i = 0; sourcesIt && buffersIt; sourcesIt = g_slist_next(sourcesIt), buffersIt = g_slist_next(buffersIt), ++i) {
-        GstElement* appsrc = static_cast<GstElement*>(sourcesIt->data);
-        AudioSrcBuffer* buffer = static_cast<AudioSrcBuffer*>(buffersIt->data);
-        GstBuffer* channelBuffer = buffer->buffer;
+        if (failed)
+            continue;
 
-        // Unmap before passing on the buffer.
-        gst_buffer_unmap(channelBuffer, &buffer->info);
-        g_free(buffer);
-
-        if (ret == GST_FLOW_OK) {
-            ret = gst_app_src_push_buffer(GST_APP_SRC(appsrc), channelBuffer);
-            if (ret != GST_FLOW_OK) {
-                // FLUSHING and EOS are not errors.
-                if (ret < GST_FLOW_EOS || ret == GST_FLOW_NOT_LINKED)
-                    GST_ELEMENT_ERROR(src, CORE, PAD, ("Internal WebAudioSrc error"), ("Failed to push buffer on %s flow: %s", GST_OBJECT_NAME(appsrc), gst_flow_get_name(ret)));
-                gst_task_stop(src->priv->task.get());
-            }
-        } else
-            gst_buffer_unref(channelBuffer);
+        auto& appsrc = priv->sources[i];
+        // Leak the buffer ref, because gst_app_src_push_buffer steals it.
+        GstFlowReturn ret = gst_app_src_push_buffer(GST_APP_SRC(appsrc.get()), buffer.leakRef());
+        if (ret != GST_FLOW_OK) {
+            // FLUSHING and EOS are not errors.
+            if (ret < GST_FLOW_EOS || ret == GST_FLOW_NOT_LINKED)
+                GST_ELEMENT_ERROR(src, CORE, PAD, ("Internal WebAudioSrc error"), ("Failed to push buffer on %s flow: %s", GST_OBJECT_NAME(appsrc.get()), gst_flow_get_name(ret)));
+            gst_task_stop(src->priv->task.get());
+            failed = true;
+        }
     }
-
-    g_slist_free(channelBufferList);
 }
 
 static GstStateChangeReturn webKitWebAudioSrcChangeState(GstElement* element, GstStateChange transition)
@@ -414,11 +396,12 @@ static GstStateChangeReturn webKitWebAudioSrcChangeState(GstElement* element, Gs
     switch (transition) {
     case GST_STATE_CHANGE_READY_TO_PAUSED: {
         GST_DEBUG_OBJECT(src, "READY->PAUSED");
+
         src->priv->pool = gst_buffer_pool_new();
-        GstStructure* config = gst_buffer_pool_get_config(src->priv->pool);
+        GstStructure* config = gst_buffer_pool_get_config(src->priv->pool.get());
         gst_buffer_pool_config_set_params(config, nullptr, src->priv->bufferSize, 0, 0);
-        gst_buffer_pool_set_config(src->priv->pool, config);
-        if (!gst_buffer_pool_set_active(src->priv->pool, TRUE))
+        gst_buffer_pool_set_config(src->priv->pool.get(), config);
+        if (!gst_buffer_pool_set_active(src->priv->pool.get(), TRUE))
             returnValue = GST_STATE_CHANGE_FAILURE;
         else if (!gst_task_start(src->priv->task.get()))
             returnValue = GST_STATE_CHANGE_FAILURE;
@@ -426,13 +409,13 @@ static GstStateChangeReturn webKitWebAudioSrcChangeState(GstElement* element, Gs
     }
     case GST_STATE_CHANGE_PAUSED_TO_READY:
         GST_DEBUG_OBJECT(src, "PAUSED->READY");
+
 #if GST_CHECK_VERSION(1, 4, 0)
-        gst_buffer_pool_set_flushing(src->priv->pool, TRUE);
+        gst_buffer_pool_set_flushing(src->priv->pool.get(), TRUE);
 #endif
         if (!gst_task_join(src->priv->task.get()))
             returnValue = GST_STATE_CHANGE_FAILURE;
-        gst_buffer_pool_set_active(src->priv->pool, FALSE);
-        gst_object_unref(src->priv->pool);
+        gst_buffer_pool_set_active(src->priv->pool.get(), FALSE);
         src->priv->pool = nullptr;
         break;
     default:
index 2372243..23190f0 100644 (file)
@@ -221,6 +221,26 @@ template<> void derefGPtr<GstBufferList>(GstBufferList* ptr)
         gst_buffer_list_unref(ptr);
 }
 
+template<> GRefPtr<GstBufferPool> adoptGRef(GstBufferPool* ptr)
+{
+    ASSERT(!ptr || !g_object_is_floating(ptr));
+    return GRefPtr<GstBufferPool>(ptr, GRefPtrAdopt);
+}
+
+template<> GstBufferPool* refGPtr<GstBufferPool>(GstBufferPool* ptr)
+{
+    if (ptr)
+        gst_object_ref_sink(GST_OBJECT(ptr));
+
+    return ptr;
+}
+
+template<> void derefGPtr<GstBufferPool>(GstBufferPool* ptr)
+{
+    if (ptr)
+        gst_object_unref(ptr);
+}
+
 template<> GRefPtr<GstSample> adoptGRef(GstSample* ptr)
 {
     return GRefPtr<GstSample>(ptr, GRefPtrAdopt);
index 1a336cc..9f9bb6d 100644 (file)
@@ -33,6 +33,7 @@ typedef struct _GstBus GstBus;
 typedef struct _GstElementFactory GstElementFactory;
 typedef struct _GstBuffer GstBuffer;
 typedef struct _GstBufferList GstBufferList;
+typedef struct _GstBufferPool GstBufferPool;
 typedef struct _GstSample GstSample;
 typedef struct _GstTagList GstTagList;
 typedef struct _GstEvent GstEvent;
@@ -83,6 +84,10 @@ template<> GRefPtr<GstBufferList> adoptGRef(GstBufferList*);
 template<> GstBufferList* refGPtr<GstBufferList>(GstBufferList*);
 template<> void derefGPtr<GstBufferList>(GstBufferList*);
 
+template<> GRefPtr<GstBufferPool> adoptGRef(GstBufferPool*);
+template<> GstBufferPool* refGPtr<GstBufferPool>(GstBufferPool*);
+template<> void derefGPtr<GstBufferPool>(GstBufferPool*);
+
 template<> GRefPtr<GstSample> adoptGRef(GstSample* ptr);
 template<> GstSample* refGPtr<GstSample>(GstSample* ptr);
 template<> void derefGPtr<GstSample>(GstSample* ptr);
index f71123b..9c49fd8 100644 (file)
@@ -120,17 +120,17 @@ char* getGstBufferDataPointer(GstBuffer* buffer)
     return reinterpret_cast<char*>(mapInfo->data);
 }
 
-void mapGstBuffer(GstBuffer* buffer)
+void mapGstBuffer(GstBuffer* buffer, uint32_t flags)
 {
     GstMapInfo* mapInfo = static_cast<GstMapInfo*>(fastMalloc(sizeof(GstMapInfo)));
-    if (!gst_buffer_map(buffer, mapInfo, GST_MAP_WRITE)) {
+    if (!gst_buffer_map(buffer, mapInfo, static_cast<GstMapFlags>(flags))) {
         fastFree(mapInfo);
         gst_buffer_unref(buffer);
         return;
     }
 
     GstMiniObject* miniObject = reinterpret_cast<GstMiniObject*>(buffer);
-    gst_mini_object_set_qdata(miniObject, g_quark_from_static_string(webkitGstMapInfoQuarkString), mapInfo, 0);
+    gst_mini_object_set_qdata(miniObject, g_quark_from_static_string(webkitGstMapInfoQuarkString), mapInfo, nullptr);
 }
 
 void unmapGstBuffer(GstBuffer* buffer)
index eb2f77c..f79a8cf 100644 (file)
@@ -58,7 +58,7 @@ bool getSampleVideoInfo(GstSample*, GstVideoInfo&);
 GstBuffer* createGstBuffer(GstBuffer*);
 GstBuffer* createGstBufferForData(const char* data, int length);
 char* getGstBufferDataPointer(GstBuffer*);
-void mapGstBuffer(GstBuffer*);
+void mapGstBuffer(GstBuffer*, uint32_t);
 void unmapGstBuffer(GstBuffer*);
 bool initializeGStreamer();
 unsigned getGstPlayFlag(const char* nick);
index 0da4a88..2a7f89e 100644 (file)
@@ -865,7 +865,7 @@ char* StreamingClient::createReadBuffer(size_t requestedSize, size_t& actualSize
 
     GstBuffer* buffer = gst_buffer_new_and_alloc(requestedSize);
 
-    mapGstBuffer(buffer);
+    mapGstBuffer(buffer, GST_MAP_WRITE);
 
     WTF::GMutexLocker<GMutex> locker(*GST_OBJECT_GET_LOCK(src));
     priv->buffer = adoptGRef(buffer);