[gstreamermm] MiniObject: fixed gst_mini_object_make_writable wrapper

From: Marcin Kolny <mkolny src gnome org>
To: commits-list gnome org
Cc:
Subject: [gstreamermm] MiniObject: fixed gst_mini_object_make_writable wrapper
Date: Tue, 19 Aug 2014 22:46:55 +0000 (UTC)
commit 101ab099ba37cfab6dfd367647e1438c12d8c3f9
Author: Marcin Kolny <marcin kolny gmail com>
Date:   Wed Aug 20 00:09:26 2014 +0200

    MiniObject: fixed gst_mini_object_make_writable wrapper
    
    gst_mini_object_make_writable unrefs object, but in gstreamermm,
    Glib::RefPtr class is used, so we have to make an extra reference
    for Glib::RefPtr destructor (where unreference method is called).
    
        * .gitignore: added miniobject test script to ignored list
        * gstreamer/src/buffer.ccg:
        * gstreamer/src/bufferlist.ccg
        * gstreamer/src/caps.ccg:
        * gstreamer/src/event.ccg:
        * gstreamer/src/message.ccg:
        * gstreamer/src/query.ccg: used MiniObject::create_writable method
          in create_writable implementation of  derived classes
        * gstreamer/src/caps.hg: Gst::MiniObject as a base class for
          Gst::Caps, updated comment
        * gstreamer/src/bufferlist.hg:
        * gstreamer/src/event.hg:
        * gstreamer/src/query.hg: updated comment
        * gstreamer/src/miniobject.ccg: fixed create_writable
          implementation
        * gstreamer/src/miniobject.hg: create_writable as hand-written
          method
        * tests/Makefile.am:
        * tests/test-miniobject.cc: added Gst::MiniObject test suite

 .gitignore                   |    1 +
 gstreamer/src/buffer.ccg     |   34 ++++------------------------------
 gstreamer/src/bufferlist.ccg |    2 +-
 gstreamer/src/bufferlist.hg  |    7 +++----
 gstreamer/src/caps.ccg       |   30 ++----------------------------
 gstreamer/src/caps.hg        |   12 +++++++++++-
 gstreamer/src/event.ccg      |    3 +--
 gstreamer/src/event.hg       |    5 +++--
 gstreamer/src/message.ccg    |    2 +-
 gstreamer/src/miniobject.ccg |   31 +++++++++++++++++++++++++++++++
 gstreamer/src/miniobject.hg  |   12 +++++++++++-
 gstreamer/src/query.ccg      |    4 ++--
 gstreamer/src/query.hg       |    3 +--
 tests/Makefile.am            |    3 ++-
 tests/test-miniobject.cc     |   36 ++++++++++++++++++++++++++++++++++++
 15 files changed, 110 insertions(+), 75 deletions(-)
---
diff --git a/.gitignore b/.gitignore
index 870ae81..a6a385b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -484,6 +484,7 @@ tests/test-bus
 tests/test-caps
 tests/test-ghostpad
 tests/test-init
+tests/test-miniobject
 tests/test-pad
 tests/test-query
 tests/test-structure
diff --git a/gstreamer/src/buffer.ccg b/gstreamer/src/buffer.ccg
index 369005c..88dc7e2 100644
--- a/gstreamer/src/buffer.ccg
+++ b/gstreamer/src/buffer.ccg
@@ -30,40 +30,14 @@ Glib::RefPtr<Gst::Buffer> Buffer::copy() const
   return Glib::wrap(gst_buffer_copy(gobj()), true);
 }
 
-Glib::RefPtr<Gst::Buffer> Buffer::create_writable()
+Glib::RefPtr<Gst::Buffer> Buffer::create(guint size)
 {
-  /*
-   * This function is generally used in the following pattern:
-   * RefPtr<Buffer> p = (...);
-   * p = p->create_writable();
-   *
-   * There are two cases:
-   * 1. object is not writable, therefore:
-   *   - somebody else may have another reference to the object (but it this might change in the meantime)
-   *   - gst_buffer_make_writable may return a new copy of object
-   *   - we have to make additional ref, that will be unreffed in gst_buffer_make_writable
-   * 2. object is_writable, then:
-   *   - our caller has the only one reference to object (therefore nobody can increase refcount in the 
meantime)
-   *   - gst_buffer_make_writable will return the same object and will not do any reffing/unreffing
-   *   - we cannot make any additional refs before calling gst_buffer_make_writable, since it would enforce 
unnecessary
-   *     copying of the object
-   *   - we are supposed to create a new RefPtr with is own reference to the object, therefore we need to 
"take copy"
-   *   - however when caller release the pointer (implicitly, during assignment) the refcount will be 1 again
-   */
-  if(is_writable())
-  {
-    return Glib::wrap(gst_buffer_make_writable(gobj()), true); // take copy so original object is left to 
the current owner (and hopefully will be released soon)
-  }
-  else
-  {
-    reference(); // gst_buffer_make_writable(buf) will unref the old buffer, but our caller is still holding 
RefPtr to it
-    return Glib::wrap(gst_buffer_make_writable(gobj()));
-  }
+  return Glib::wrap(gst_buffer_new_allocate(NULL, size, NULL));
 }
 
-Glib::RefPtr<Gst::Buffer> Buffer::create(guint size)
+Glib::RefPtr<Buffer> Buffer::create_writable()
 {
-  return Glib::wrap(gst_buffer_new_allocate(NULL, size, NULL));
+  return Glib::RefPtr<Buffer>::cast_static(MiniObject::create_writable());
 }
 
 } // namespace Gst
diff --git a/gstreamer/src/bufferlist.ccg b/gstreamer/src/bufferlist.ccg
index 4c9096c..63aada9 100644
--- a/gstreamer/src/bufferlist.ccg
+++ b/gstreamer/src/bufferlist.ccg
@@ -77,7 +77,7 @@ bool BufferList::is_writable() const
 
 Glib::RefPtr<BufferList> BufferList::create_writable()
 {
-  return Glib::wrap(gst_buffer_list_make_writable(gobj()));
+  return Glib::RefPtr<BufferList>::cast_static(MiniObject::create_writable());
 }
 
 void BufferList::foreach(const SlotForeach& slot)
diff --git a/gstreamer/src/bufferlist.hg b/gstreamer/src/bufferlist.hg
index bef2350..33f7356 100644
--- a/gstreamer/src/bufferlist.hg
+++ b/gstreamer/src/bufferlist.hg
@@ -83,12 +83,11 @@ public:
    */
   bool is_writable() const;
 
-  /** Makes a writable buffer list from the buffer list. If the buffer list is
-   * already writable, this will simply return the same buffer list. A copy
+  /** Makes a writable buffer list from the given buffer list. If the source buffer
+   * list is already writable, this will simply return the same buffer list. A copy
    * will otherwise be made.
-   * @return The same buffer list if it is writable, otherwise a new copy.
    *
-   * Since 0.10.24
+   * @return A buffer list (possibly the same pointer) that is writable.
    */
   Glib::RefPtr<BufferList> create_writable();
 
diff --git a/gstreamer/src/caps.ccg b/gstreamer/src/caps.ccg
index 94c1cdc..f50cbc7 100644
--- a/gstreamer/src/caps.ccg
+++ b/gstreamer/src/caps.ccg
@@ -52,35 +52,9 @@ Glib::RefPtr<Gst::Caps> Caps::create_simple(const Glib::ustring& media_type)
   return result;
 }
 
-Glib::RefPtr<Gst::Caps> Caps::create_writable()
+Glib::RefPtr<Caps> Caps::create_writable()
 {
-  /*
-   * This function is generally used in the following pattern:
-   * RefPtr<Caps> p = (...);
-   * p = p->create_writable();
-   *
-   * There are two cases:
-   * 1. object is not writable, therefore:
-   *   - somebody else may have another reference to the object (but it this might change in the meantime)
-   *   - gst_caps_make_writable may return a new copy of object
-   *   - we have to make additional ref, that will be unreffed in gst_caps_make_writable
-   * 2. object is_writable, then:
-   *   - our caller has the only one reference to object (therefore nobody can increase refcount in the 
meantime)
-   *   - gst_caps_make_writable will return the same object and will not do any reffing/unreffing
-   *   - we cannot make any additional refs before calling gst_caps_make_writable, since it would enforce 
unnecessary
-   *     copying of the object
-   *   - we are supposed to create a new RefPtr with is own reference to the object, therefore we need to 
"take copy"
-   *   - however when caller release the pointer (implicitly, during assignment) the refcount will be 1 again
-   */
-  if(gst_caps_is_writable(gobj()))
-  {
-    return Glib::wrap(gst_caps_make_writable(gobj()), true); // take copy so original object is left to the 
current owner (and hopefully will be released soon)
-  }
-  else
-  {
-    reference(); // gst_caps_make_writable(buf) will unref the old caps, but our caller is still holding 
RefPtr to it
-    return Glib::wrap(gst_caps_make_writable(gobj()));
-  }
+  return Glib::RefPtr<Caps>::cast_static(MiniObject::create_writable());
 }
 
 Glib::RefPtr<Gst::Caps> Caps::create(const Structure& structure)
diff --git a/gstreamer/src/caps.hg b/gstreamer/src/caps.hg
index 9036503..e3e72c1 100644
--- a/gstreamer/src/caps.hg
+++ b/gstreamer/src/caps.hg
@@ -18,6 +18,7 @@
  */
 
 #include <gst/gst.h>
+#include <gstreamermm/miniobject.h>
 #include <gstreamermm/structure.h>
 
 _DEFS(gstreamermm,gst)
@@ -56,7 +57,7 @@ struct Structure;
  * caps->set_simple("height", 240);
  * @endcode
  */
-class Caps 
+class Caps : public MiniObject
 {
   _CLASS_OPAQUE_REFCOUNTED(Caps, GstCaps, gst_caps_new_empty, gst_caps_ref, gst_caps_unref)
   _IGNORE(gst_caps_ref, gst_caps_unref)
@@ -197,6 +198,15 @@ public:
   //This is const (returns a non const) because it always creates a new instance:
   _WRAP_METHOD(Glib::RefPtr<Gst::Caps> get_difference(const Glib::RefPtr<const Gst::Caps>& subtrahend_caps) 
const, gst_caps_subtract)
 
+  /**
+   * Returns a writable copy of @caps.
+   *
+   * If there is only one reference count on Gst::Caps object , the caller must be
+   * the owner, and so this function will return the caps object unchanged. If on
+   * the other hand there is more than one reference on the object, a new caps object
+   * will be returned.
+   * @return A buffer (possibly the same pointer) that is writable.
+   */
   Glib::RefPtr<Gst::Caps> create_writable();
   _WRAP_METHOD(Glib::RefPtr<Gst::Caps> truncate(), gst_caps_truncate)
 };
diff --git a/gstreamer/src/event.ccg b/gstreamer/src/event.ccg
index 97ee3c7..6087209 100644
--- a/gstreamer/src/event.ccg
+++ b/gstreamer/src/event.ccg
@@ -49,8 +49,7 @@ EventTypeFlags get_flags(EventType t)
 
 Glib::RefPtr<Gst::Event> Event::create_writable()
 {
-  return
-   Glib::wrap(GST_EVENT(gst_mini_object_make_writable(GST_MINI_OBJECT(gobj()))));
+  return Glib::RefPtr<Event>::cast_static(MiniObject::create_writable());
 }
 
 bool Event::is_downstream() const
diff --git a/gstreamer/src/event.hg b/gstreamer/src/event.hg
index c388e61..9f3851e 100644
--- a/gstreamer/src/event.hg
+++ b/gstreamer/src/event.hg
@@ -127,8 +127,9 @@ public:
   _WRAP_METHOD(guint32 get_seqnum() const, gst_event_get_seqnum)
   _WRAP_METHOD(void set_seqnum(guint32 seqnum), gst_event_set_seqnum)
 
-  /** Checks if an event is writable.  If not, a writable copy is made and
-   * returned.
+  /**
+   * Makes a writable event from the given event. If the source event is
+   * already writable, this will simply return the same event.
    * @return A Gst::Event (possibly the same reference) that is writable.
    */
   Glib::RefPtr<Gst::Event> create_writable();
diff --git a/gstreamer/src/message.ccg b/gstreamer/src/message.ccg
index 9e09ede..aea1599 100644
--- a/gstreamer/src/message.ccg
+++ b/gstreamer/src/message.ccg
@@ -45,7 +45,7 @@ Glib::QueryQuark get_quark(MessageType t)
 
 Glib::RefPtr<Gst::Message> Message::create_writable()
 {
-  return Glib::wrap(gst_message_make_writable(gobj()));
+  return Glib::RefPtr<Message>::cast_static(MiniObject::create_writable());
 }
 
 Glib::RefPtr<Gst::MessageEos>
diff --git a/gstreamer/src/miniobject.ccg b/gstreamer/src/miniobject.ccg
index 137dc42..5692502 100644
--- a/gstreamer/src/miniobject.ccg
+++ b/gstreamer/src/miniobject.ccg
@@ -34,4 +34,35 @@ MiniObject::~MiniObject()
     gst_mini_object_unref(gobject_);
 }
 
+Glib::RefPtr<Gst::MiniObject> MiniObject::create_writable()
+{
+  /*
+   * This function is generally used in the following pattern:
+   * RefPtr<Buffer> p = (...);
+   * p = p->create_writable();
+   *
+   * There are two cases:
+   * 1. object is not writable, therefore:
+   *   - somebody else may have another reference to the object (but it this might change in the meantime)
+   *   - gst_mini_object_make_writable may return a new copy of object
+   *   - we have to make additional ref, that will be unreffed in gst_mini_object_make_writable
+   * 2. object is_writable, then:
+   *   - our caller has the only one reference to object (therefore nobody can increase refcount in the 
meantime)
+   *   - gst_mini_object_make_writable will return the same object and will not do any reffing/unreffing
+   *   - we cannot make any additional refs before calling gst_mini_object_make_writable, since it would 
enforce unnecessary
+   *     copying of the object
+   *   - we are supposed to create a new RefPtr with is own reference to the object, therefore we need to 
"take copy"
+   *   - however when caller release the pointer (implicitly, during assignment) the refcount will be 1 again
+   */
+  if(is_writable())
+  {
+    return Glib::wrap(gst_mini_object_make_writable(gobj()), true); // take copy so original object is left 
to the current owner (and hopefully will be released soon)
+  }
+  else
+  {
+    reference(); // gst_mini_object_make_writable(buf) will unref the old buffer, but our caller is still 
holding RefPtr to it
+    return Glib::wrap(gst_mini_object_make_writable(gobj()));
+  }
+}
+
 } //namespace Gst
diff --git a/gstreamer/src/miniobject.hg b/gstreamer/src/miniobject.hg
index 978a5d1..b10bab4 100644
--- a/gstreamer/src/miniobject.hg
+++ b/gstreamer/src/miniobject.hg
@@ -44,7 +44,17 @@ public:
   _WRAP_METHOD(bool lock(LockFlags flags), gst_mini_object_lock)
   _WRAP_METHOD(void unlock(LockFlags flags), gst_mini_object_unlock)
   _WRAP_METHOD(bool is_writable() const, gst_mini_object_is_writable)
-  _WRAP_METHOD(Glib::RefPtr<MiniObject> create_writable(), gst_mini_object_make_writable)
+
+  _IGNORE(gst_mini_object_make_writable)
+  /** Checks if a mini-object is writable.  If not, a writable copy is made and
+   * returned.  This gives away the reference to the original mini object,
+   * and returns a reference to the new object.
+   *
+   * MT safe
+   * @return A mini-object (possibly the same pointer) that
+   * is writable.
+   */
+  Glib::RefPtr<MiniObject> create_writable();
 
   // Copying a mini object can be achieved by assignment.
   _IGNORE(gst_mini_object_copy)
diff --git a/gstreamer/src/query.ccg b/gstreamer/src/query.ccg
index 5b1f07f..d720234 100644
--- a/gstreamer/src/query.ccg
+++ b/gstreamer/src/query.ccg
@@ -41,9 +41,9 @@ Glib::QueryQuark get_quark(QueryType t)
 
 } //namespace Enums
 
-Glib::RefPtr<Gst::Query> Query::create_writable()
+Glib::RefPtr<Query> Query::create_writable()
 {
-  return Glib::wrap(gst_query_make_writable(gobj()));
+  return Glib::RefPtr<Query>::cast_static(MiniObject::create_writable());
 }
 
 Glib::RefPtr<Gst::QueryApplication>
diff --git a/gstreamer/src/query.hg b/gstreamer/src/query.hg
index d8803fd..7948ac9 100644
--- a/gstreamer/src/query.hg
+++ b/gstreamer/src/query.hg
@@ -100,8 +100,7 @@ class Query : public MiniObject
   _IGNORE(gst_query_ref, gst_query_unref)
 
 public:
-   /** Makes a writable query from the given query.  Does exactly what
-    * Gst::MiniObject::create_writable() does for the Gst::Query.
+   /** Makes a writable query from the given query.
     * @return A Gst::Query (possibly the same pointer) that is writable.
     */
    Glib::RefPtr<Gst::Query> create_writable();
diff --git a/tests/Makefile.am b/tests/Makefile.am
index e1e7fdf..25fdef5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -22,7 +22,7 @@ LDADD = $(GSTREAMERMM_LIBS) $(local_libgstreamermm) -lgtest -lpthread
 
 check_PROGRAMS = test-caps test-buffer test-bus test-caps test-pad \
                  test-allocator test-atomicqueue test-bin \
-                 test-urihandler test-ghostpad test-init \
+                 test-urihandler test-ghostpad test-init test-miniobject \
                  test-query test-structure test-taglist test-plugin-appsink \
                  test-plugin-appsrc test-plugin-register test-plugin-pushsrc \
                  test-regression-bininpipeline test-regression-binplugin \
@@ -42,6 +42,7 @@ test_buffer_SOURCES                   = test-buffer.cc $(TEST_MAIN_SOURCE)
 test_bus_SOURCES                       = test-bus.cc $(TEST_MAIN_SOURCE)
 test_ghostpad_SOURCES          = test-ghostpad.cc $(TEST_MAIN_SOURCE)
 test_init_SOURCES                      = test-init.cc $(TEST_MAIN_SOURCE)
+test_miniobject_SOURCES                = test-miniobject.cc $(TEST_MAIN_SOURCE)
 test_pad_SOURCES                       = test-pad.cc $(TEST_MAIN_SOURCE)
 test_query_SOURCES                     = test-query.cc $(TEST_MAIN_SOURCE)
 test_structure_SOURCES         = test-structure.cc $(TEST_MAIN_SOURCE)
diff --git a/tests/test-miniobject.cc b/tests/test-miniobject.cc
new file mode 100644
index 0000000..dca5adb
--- /dev/null
+++ b/tests/test-miniobject.cc
@@ -0,0 +1,36 @@
+/*
+ * test-miniobject.cc
+ *
+ *  Created on: Aug 19, 2014
+ *      Author: m.kolny
+ */
+
+#include <gtest/gtest.h>
+#include <gstreamermm.h>
+
+using namespace Gst;
+using Glib::RefPtr;
+
+TEST(MiniObjectTest, ShouldCorrectCreateWritableObjects)
+{
+       // we cannot create Gst::MiniObject object, so I used Gst::Buffer::create
+       // method to make an object.
+       RefPtr<MiniObject> obj = Buffer::create();
+
+       ASSERT_EQ(1, obj->gobj()->refcount);
+
+       obj = obj->create_writable();
+
+       ASSERT_EQ(1, obj->gobj()->refcount);
+
+       RefPtr<MiniObject> obj2 = obj;
+
+       ASSERT_EQ(2, obj->gobj()->refcount);
+       ASSERT_EQ(2, obj2->gobj()->refcount);
+
+       obj2 = obj->create_writable();
+
+       ASSERT_EQ(1, obj->gobj()->refcount);
+       ASSERT_EQ(1, obj2->gobj()->refcount);
+}
+
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]