Created attachment 130436 [details] [review]
Patch for _CUSTOM_CAST_CTOR and _CUSTOM_CONSTRUCT_CTOR

Created attachment 130437 [details] [review]
Patch with changelog entry

I don't understand what _CUSTOM_CONSTRUCT_CTOR is for? What constructor is it for? The default one? Or is it used with arguments instead of _WRAP_CTOR()?

It does the same thing for the 'const Glib::ConstructParams&' constructor as what _CUSTOM_CAST_CTOR does for the cast constructor.  Right now, in gstreamermm, both the cast and the Glib::ConstructParam& constructor of Gst::Object look like:


Object::Object(const Glib::ConstructParams& construct_params)
:
  Glib::Object(construct_params)
{
  //The floating reference is convenience for C,
  //but is useless and difficult for us:
  gst_object_ref(gobj());
  gst_object_sink(gobj());
}

Object::Object(GstObject* castitem)
:
  Glib::Object((GObject*)(castitem))
{
  if(G_LIKELY(GST_OBJECT_IS_FLOATING(castitem)))
  {
    //The floating reference is convenience for C,
    //but is useless and difficult for us:
    gst_object_ref(gobj());
    gst_object_sink(gobj());
  }
}

_CUSTOM_CAST_CTOR allows Gst::Object to be wrapped by gmmproc, but also to keep the cast constructor code as above.  In the same way, _CUSTOM_CONSTRUCT_CTOR allows the code of the Glib::ConstructParams& to also be kept.

The code needs to be kept for both constructors because otherwise when, for example, plug-ins are created (with the Glib::ConstructParams& constructor), warnings are caused without the floating reference code in the Glib::ConstructParams& constructor.

To test, simply remove the custom code in the Glib::ConstructParams& constructor, startup and exit the media player example in gstreamermm.  With the code in the Glib::ConstructParams& constructor, there are no warnings.  Without the code, warnings related to reference counts are issued.

_CUSTOM_CONSTRUCT_CTOR is just so that a custom Glib::ConstructParams& constructor can be included and custom code can be included in the constructor that way.

(In reply to comment #4)
> To test, simply remove the custom code in the Glib::ConstructParams&
> constructor, startup and exit the media player example in gstreamermm.  With
> the code in the Glib::ConstructParams& constructor, there are no warnings. 
> Without the code, warnings related to reference counts are issued.

The warnings would not be generated if the Gst::XImageSink (video_sink) is added to the Gst::PlayBin2 (pipeline) in the media player example as is done with plug-ins in the ogg_player_gtkmm example (I think that adding elements to bins/pipelines corrects floating reference problem -- see gst_bin_add[1] and gst_object_set_parent[2]).

[1] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-add
[2] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstObject.html#gst-object-set-parent

The problem is that if the Gst::XImageSink is added to the Gst::PlayBin2 in the media player example, it can't be used when setting the Gst::PlayBin2's property_video_sink() so that the element can be used for video display (I tested this in a regular environment using the previous to the last revision, 2112, because something about how GStreamer is configured in the jhbuild environment doesn't let the media player example run successfully).

(In reply to comment #4)
> _CUSTOM_CONSTRUCT_CTOR is just so that a custom Glib::ConstructParams&
> constructor can be included and custom code can be included in the constructor
> that way.
> 

Maybe it ought to be called _CUSTOM_CONSTRUCT_PARAMS_CTOR?

(In reply to comment #4)
> It does the same thing for the 'const Glib::ConstructParams&' constructor as
> what _CUSTOM_CAST_CTOR does for the cast constructor.

But surely no such constructor is generated normally. It's only in Glib::ObjectBase isn't it?

I'm sorry.  I was getting the following error using the patch in bug #575136:

object.cc:161: error: redefinition of ‘Gst::Object::Object(const Glib::ConstructParams&)’
object.cc:32: error: ‘Gst::Object::Object(const Glib::ConstructParams&)’ previously defined here

And thought that it was because one was generated when in fact the constructor is duplicated in that patch in object.ccg.  Apologies.

Created attachment 130985 [details] [review]
_CUSTOM_CAST_CTOR patch

Would this patch be acceptable?

So you need it to allow you to do this:

Object::Object(GstObject* castitem)
:
  Glib::Object((GObject*)(castitem))
{
  if(G_LIKELY(GST_OBJECT_IS_FLOATING(castitem)))
  {
    //The floating reference is convenience for C,
    //but is useless and difficult for us:
    gst_object_ref(gobj());
    gst_object_sink(gobj());
  }
}

Wouldn't that only be called when you call wrap()? I guess that it's unusual for a wrap() to change the floating ref. Does Gtk::Object do that? Can you give an example of code that needs this?

I guess I didn't think about this enough.  The code was there a while so I thought it was necessary and operated under that assumption.  Right now my thoughts are that if someone already had a GstObject and wanted to wrap() it, they should worry about floating references and not the cast constructor because it was probably obtained using C code.  In essence (and in light of when the cast constructor is used), I think that custom code would not be needed in the cast constructor so I'll be closing this bug.

Now I remember(In reply to comment #7)
> (In reply to comment #4)
> > It does the same thing for the 'const Glib::ConstructParams&' constructor as
> > what _CUSTOM_CAST_CTOR does for the cast constructor.
> 
> But surely no such constructor is generated normally. It's only in
> Glib::ObjectBase isn't it?

Aparently, the ConstructParams constructor is generated.  I removed the duplicate constructor so that there is only one and I still get a duplicate error definition error:

object.cc:138: error: redefinition of ‘Gst::Object::Object(const Glib::ConstructParams&)’
object.cc:32: error: ‘Gst::Object::Object(const Glib::ConstructParams&)’ previously defined here

As I mentioned, I think the custom code is needed so that the floating references of plug-ins that are created in gstreamermm are handled correctly.  If you think this should be handled in glibmm, then I'd re-open this report and submit a patch for a custom ConstructParams constructor.  If not, gstreamermm would probably have to have its own _CLASS_GSTOBJECT macro that could allow a custom ConstructParams constructor.

Sorry, I was wrong. The (construct_params) constructor is indeed generated for each class, and that is correct and necessary. However Gtk::Object manages to implement it's own code for this in its object.ccg file. I wonder how. Could you try to figure that out?

I also notice that Gtk::Object does correct the floating reference in its case constructor, by using  a _CUSTOM_CTOR_CAST macro (probably in gtkobject.m4). I was wrong about that too. So please do add something like that to gobject.m4, but use that _CUSTOM_CTOR_CAST name instead of _CUSTOM_CAST_CTOR, for consistency with gtkobject.m4.

Created attachment 131049 [details] [review]
Correct patch (one macro and both constructors not generated)

(In reply to comment #13)
> Sorry, I was wrong. The (construct_params) constructor is indeed generated for
> each class, and that is correct and necessary. However Gtk::Object manages to
> implement it's own code for this in its object.ccg file. I wonder how. Could
> you try to figure that out?

The piece that was missing is that _CLASS_GTKOBJECT's _CUSTOM_CTOR_CAST makes both the cast and the contruct_params constructors not be generated (I was using two macros instead of the one -- sorry).  I committed the updated patch to glibmm branches 2-14, 2-16, 2-18 and to trunk.  Thanks for your patience.