I'm not sure it's a bug, but to confirm that GstQuery cannot be referenced more than once before being processed, run the attached ogg player program (taken from Chapter 10 of the GStreamer Application Development Manual) where the GstQuery is referenced after being created and then sent to the pipeline in the cb_print_position() function.  The warnings are generated each time the program attempts to query the stream position for output.

The reason I'm reporting this is because I foresee possible bug reports in the C++ bindings when people begin to use it.  As I explained, the Queries in C++ will need to be cast for parsing, and the order of the casting (whether before or after being sent) cannot be predicted.  If the cast occurs before, warnings will be generated (unless we warn users of bindings not do this :-)).

We've written the same program I've attached to this report using the new bindings and it produces the same results as the attached program.  The C++ version is found in the examples/ogg_player directory of the binding's trunk (revision 1296) and may be tested by building the bindings and executing the examples/ogg_player/oggplayer executable.

I'd like to explain my concern:  The relevant C++ code snippet is the following:

bool print_stream_position(void)
{
 Gst::Format fmt = Gst::FORMAT_TIME;
 gint64 pos = 0;
 gint64 len = 0;

 Glib::RefPtr<Gst::Query> query = Gst::QueryPosition::create(fmt);

   Glib::RefPtr<Gst::QueryPosition> posQuery =
        Glib::RefPtr<Gst::QueryPosition>::cast_dynamic(query);

 if (pipeline->query(query)
   && pipeline->query_duration(fmt, len)) {

   ...
  }
  return true;
}

A Query is created by calling the Gst::QueryPosition:create() method (which is the same as calling gst_query_position_new).

As I explained in the report, the Glib::RefPtr<> is an "auto pointer" class that keeps automatic glib reference counting for the class between the <> by unreferencing the object if it goes out of scope).  However, the casting (the second Glib::RefPtr<...>(...) statement) also references the Query and the procesing of the query (in the if statement condition) produces the same warnings as the C code I've attached.  If the cast statement is placed inside the if block, however, the warnings go away (because the Query is not referenced a second time before being processed).

I'm reporting this in case this might be something that might also affect gstreamer because a GstQuery might be referenced more than once in C code also.  If it does affect gstreamer and it is a bug, fixing it for gstreamer would fix it for the C++ bindings we're developing also.  If not, we will have to deal with the possible reports or try to warn developers not to cast Queries until after they've been processed.  Thanks for your attention.

Created attachment 103384 [details]
Ogg player that references GstQuery before sending

This is intended behaviour, it's not possible to modify the query results when multiple refs are taken (because this would suggest there are multiple writers). Also the query remains valid after a call to gst_pad/element_query() and needs to be unreffed after usage. 

This is a really weird hack. I can't yet think of a way to get around it while still using our RefPtr in C++.

This is frustrating me more and more. This disturbs language bindings, which will tend to take references more often than C coders do explicitly, to avoid reference-counting problems. That shouldn't be considered bad thing, and it's not something you should be interfering with. Even if it "suggests" that there could be multiple writers, it doesn't meant that there are.

Specifically, it is stopping us from doing "chained" code like this (though I think we have an example where it is more useful):
caps->set_simple("width", width)->set_simple("height", height);

I agree - there are a couple of the structures in GStreamer that use refcnts to indicate writability (GstBuffers, queries too, maybe others?) and I've always thought it a little awkward.

I don't know any backward-compatible way to change it though other than trying to add some bizarre 'read-only ref' concept alongside the gobject refcnt.

Is it possible to include a field in the GstStructure that determines immutability and use that instead of the refcount? 

(pasted from mailing list discussion)

One possible workaround for this is to add a wrapper_refcount field
that is only used for language wrappers.  Language wrappers would
increment the wrapper_refcount field to indicate that the refcount
is due to the bindings only, and not to be used as in determining
writablity.  (This is only a suggestion.  Don't know if it would
work, or even wise.  Definitely something we need to address in 0.11.
Actually, we needed to address it in 0.7, but it got dropped because
of more pressing issues.)

The suggestion sounds okay.  Could the mini object be kept from destruction if there were binding references?  Could some functions such as gst_mini_object_bindings_reference() and gst_mini_object_bindings_unreference() be included?  A solution to this bug would certainly help the functionality of gstreamermm.  Thanks for your considerations.

BTW, I'm more than happy to provide a patch if there is a feasible solution even if it's accepted for a version later than 0.11.

If used in bindings, how do you ensure that there are no two "owners" of the miniobject that try to change it, leading to unexpected behaviour or even crashes?

I guess there would have to be a way to ensure that no two owners could write to the mini object.  I still think that some other way other than referencing should be used to ensure that no two owners write to one mini object though I may not know enough of the internals to think of an alternative right now.

That's exactly the problem we're facing right now :) And you could say that you claim ownership of an object by having a reference to it, thus if there are two references or two owners the object is not writable. (Yes, no need to tell me that this is a problem for bindings, I had the problems with the C# and Vala bindings too)

Wouldn't it be possible to take advantage of the 1.0 API break to resolve this by including a GMutex, or maybe even a GRWLock, in the relevant structs to represent a write-lock instead of hijacking the reference-counting functionality?

(In reply to comment #14)
> Wouldn't it be possible to take advantage of the 1.0 API break to resolve this
> by including a GMutex, or maybe even a GRWLock, in the relevant structs to
> represent a write-lock instead of hijacking the reference-counting
> functionality?

No, the problem is not about locking. It is about making sure that a thread that wants to write makes a copy of the object first to ensure the modifications are not seem by other threads.

I think this is basically implemented now in functionality, but not used in all mini objects yet. Downgrading severity, since even if it wasn't implemented it wouldn't happen any more now ;)

Probably still a fair few open issues for bindings though, but that might best be dealt with in another bug.

Wim, is there anything left to do here?

I think the implementation is complete, just need to make all miniobjects use it where it makes sense. But that would be for 2.0.

> I think the implementation is complete, just need to make all
> miniobjects use it where it makes sense. But that would be
> for 2.0.

Ok, let's close this then. I don't know if we want to clone a new bug for making all miniobjects use it. As far as I'm concerned 2.0 will handle this stuff at the language level ;)