GNOME Bugzilla – Bug 704760
opencv: disparity-map calculation element
Last modified: 2013-08-23 10:02:13 UTC
Created attachment 249926 [details] [review] 1st full patch GstDisparity "This element computes a disparity map from two stereo images, meaning each one coming from a different camera, both looking at the same scene and relatively close to each other - more on this below. The disparity map is a proxy of the depth of a scene as seen from the camera." This element has an interesting twist: same as videomeasure, it is a muxer that normally needs to wait until both frames are present on its input pads before carrying out the stereo mapping. This is, strongly inspired by videomeasure, achieved via a mutex + condition variable + a buffer being NULL or not.
Review of attachment 249926 [details] [review]: ::: ext/opencv/gstdisparity.cpp @@ +161,3 @@ + {METHOD_SGBM, "Semi-global block matching algorithm", "sgbm"}, + {METHOD_VAR, "Variational matching algorithm", "svar"}, + {METHOD_GC, "Graph=cut based matching algorithm", "sgc"}, Should this be Graph=cut or Graph-cut? @@ +262,3 @@ + gst_pad_set_chain_function (filter->sinkpad_right, + GST_DEBUG_FUNCPTR (gst_disparity_chain_right)); + GST_PAD_SET_PROXY_CAPS (filter->sinkpad_right); I don't think this makes negotiation happen correctly. At the moment when one of the pads has caps, the other one has to return exactly these caps as a result of the CAPS query and can't accept anything else anymore. Apart from that it is PROXY_CAPS, yes. So implement a query function on both pads that does this and does not call gst_pad_query_default() for the CAPS query. @@ +318,3 @@ + + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_CAPS: All this needs some blocking, the CAPS events will arrive from two different threads and can happen at the same time @@ +324,3 @@ + gst_video_info_from_caps (&info, caps); + + GST_WARNING (" Negotiating caps via event (%s) %s", Not a warning, and should be GST_WARNING_OBJECT(pad, ...) if anything :) Same for everything else, GST_DEBUG_OBJECT(), ... @@ +330,3 @@ + fs->width = info.width; + fs->height = info.height; + fs->actualChannels = info.finfo->n_components; Maybe just store the GstVideoInfo in the instance struct instead of copying some of its part there @@ +348,3 @@ + + GST_DEBUG (" Negotiated caps via event (%s) %s", + gst_pad_get_name (pad), gst_caps_to_string (caps)); GST_DEBUG_OBJECT (pad, "Negotiated caps %" GST_PTR_FORMAT, caps) Otherwise you leak the object name and the caps string (alternatively "%s:%s", GST_DEBUG_PAD_NAME(pad)) @@ +403,3 @@ + if (fs->buffer_left) { + GST_INFO (" right is busy, wait and hold"); + g_cond_wait (fs->cond, fs->lock); Also here GST_DEBUG_OBJECT(pad, ...) like everywhere else :) So for the GCond handling there are two things missing. This can easily deadlock right now. First of all you should always put the waiting in a loop, i.e. while (fs->buffer_left) g_cond_wait(). And then you need to unblock the waiting in GstElement::change_state() when going from PAUSED to READY before chaining up to the parent class' change_state implementation. You would for that introduce a new instance variable "flushing" that is set to FALSE in READY->PAUSED and to TRUE in PAUSED->READY (and afterwards you signal the GCond). If any of the chain functions sees that flushing is TRUE, it should return GST_FLOW_FLUSHING immediately. If after g_cond_wait() it is TRUE, return GST_FLOW_FLUSHING immediately too. @@ +493,3 @@ + cvCvtColor (fs->cvRGB_right, fs->cvGray_right, CV_RGB2GRAY); + run_svar_iteration (fs); + //cvNormalize(fs->cvGray_depth_map2, fs->cvGray_depth_map2, 0, 255, CV_MINMAX, NULL); C99 comment @@ +516,3 @@ + + ret = gst_pad_push (fs->srcpad, buffer); + //gst_object_unref (fs); C99 comment, and the unref is not necessary ::: ext/opencv/gstdisparity.h @@ +50,3 @@ +#include <highgui.h> // includes highGUI definitions +#endif +#ifdef HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H Don't you want configure checks for these two? And #include config.h?
(In reply to comment #1) > Review of attachment 249926 [details] [review]: > > ::: ext/opencv/gstdisparity.cpp > @@ +161,3 @@ > + {METHOD_SGBM, "Semi-global block matching algorithm", "sgbm"}, > + {METHOD_VAR, "Variational matching algorithm", "svar"}, > + {METHOD_GC, "Graph=cut based matching algorithm", "sgc"}, > > Should this be Graph=cut or Graph-cut? > Graph-cut :) > @@ +262,3 @@ > + gst_pad_set_chain_function (filter->sinkpad_right, > + GST_DEBUG_FUNCPTR (gst_disparity_chain_right)); > + GST_PAD_SET_PROXY_CAPS (filter->sinkpad_right); > > I don't think this makes negotiation happen correctly. At the moment when one > of the pads has caps, the other one has to return exactly these caps as a > result of the CAPS query and can't accept anything else anymore. Apart from > that it is PROXY_CAPS, yes. So implement a query function on both pads that > does this and does not call gst_pad_query_default() for the CAPS query. > Hmm I don't fully understand, I thought at the moment the negotiation would happen via sink_event, so perhaps I should modify that function so on second call it returns the format accepted in the first call, and keep the caps proxied? > @@ +318,3 @@ > + > + switch (GST_EVENT_TYPE (event)) { > + case GST_EVENT_CAPS: > > All this needs some blocking, the CAPS events will arrive from two different > threads and can happen at the same time > Added a mutex on both sides to make this a critical section. I guess gst_disparity_init() is called before any event handling (rtfm for me?) ;) > @@ +324,3 @@ > + gst_video_info_from_caps (&info, caps); > + > + GST_WARNING (" Negotiating caps via event (%s) %s", > > Not a warning, and should be GST_WARNING_OBJECT(pad, ...) if anything :) Same > for everything else, GST_DEBUG_OBJECT(), ... > Corrected/ cleaned up. > @@ +330,3 @@ > + fs->width = info.width; > + fs->height = info.height; > + fs->actualChannels = info.finfo->n_components; > > Maybe just store the GstVideoInfo in the instance struct instead of copying > some of its part there > GstVideoInfo has loads of fields including pointers to other structs, and since I just need 3 fields instead I thought better to get them copied. > @@ +348,3 @@ > + > + GST_DEBUG (" Negotiated caps via event (%s) %s", > + gst_pad_get_name (pad), gst_caps_to_string (caps)); > > GST_DEBUG_OBJECT (pad, "Negotiated caps %" GST_PTR_FORMAT, caps) > > Otherwise you leak the object name and the caps string (alternatively "%s:%s", > GST_DEBUG_PAD_NAME(pad)) Done. > > @@ +403,3 @@ > + if (fs->buffer_left) { > + GST_INFO (" right is busy, wait and hold"); > + g_cond_wait (fs->cond, fs->lock); > > Also here GST_DEBUG_OBJECT(pad, ...) like everywhere else :) > Done. > > So for the GCond handling there are two things missing. This can easily > deadlock right now. First of all you should always put the waiting in a loop, > i.e. while (fs->buffer_left) g_cond_wait(). The cond should keep the chain_{left,right} fully blocked until the other chain arrives at the rendezvous. What would be the situation in which a chain function blocked at the cond variable would wake up _not_ being signalled by the other chain function? > And then you need to unblock the > waiting in GstElement::change_state() when going from PAUSED to READY before > chaining up to the parent class' change_state implementation. You would for > that introduce a new instance variable "flushing" that is set to FALSE in > READY->PAUSED and to TRUE in PAUSED->READY (and afterwards you signal the > GCond). If any of the chain functions sees that flushing is TRUE, it should > return GST_FLOW_FLUSHING immediately. If after g_cond_wait() it is TRUE, return > GST_FLOW_FLUSHING immediately too. Ok, done this part. > > @@ +493,3 @@ > + cvCvtColor (fs->cvRGB_right, fs->cvGray_right, CV_RGB2GRAY); > + run_svar_iteration (fs); > + //cvNormalize(fs->cvGray_depth_map2, fs->cvGray_depth_map2, 0, 255, > CV_MINMAX, NULL); > > C99 comment > Done. > @@ +516,3 @@ > + > + ret = gst_pad_push (fs->srcpad, buffer); > + //gst_object_unref (fs); > > C99 comment, and the unref is not necessary > Done. Is the forgotten remaining of some debugging. > ::: ext/opencv/gstdisparity.h > @@ +50,3 @@ > +#include <highgui.h> // includes highGUI definitions > +#endif > +#ifdef HAVE_OPENCV2_HIGHGUI_HIGHGUI_C_H > > Don't you want configure checks for these two? And #include config.h? Actually they are not needed :) The highgui and their checks are in place from gstmotioncells.{h,cpp} times, so they can be -and have been- removed.
Created attachment 250212 [details] [review] 1st delta over 1st patch, Sebastian's comments addressed. git patch-format over the first patch.
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 249926 [details] [review] [details]: > > > > ::: ext/opencv/gstdisparity.cpp > > @@ +161,3 @@ > > + {METHOD_SGBM, "Semi-global block matching algorithm", "sgbm"}, > > + {METHOD_VAR, "Variational matching algorithm", "svar"}, > > + {METHOD_GC, "Graph=cut based matching algorithm", "sgc"}, > > > > Should this be Graph=cut or Graph-cut? > > > Graph-cut :) > > > @@ +262,3 @@ > > + gst_pad_set_chain_function (filter->sinkpad_right, > > + GST_DEBUG_FUNCPTR (gst_disparity_chain_right)); > > + GST_PAD_SET_PROXY_CAPS (filter->sinkpad_right); > > > > I don't think this makes negotiation happen correctly. At the moment when one > > of the pads has caps, the other one has to return exactly these caps as a > > result of the CAPS query and can't accept anything else anymore. Apart from > > that it is PROXY_CAPS, yes. So implement a query function on both pads that > > does this and does not call gst_pad_query_default() for the CAPS query. > > > > Hmm I don't fully understand, I thought at the moment the negotiation would > happen via sink_event, so perhaps I should modify that function so on second > call > it returns the format accepted in the first call, and keep the caps proxied? Yes, that's what videomixer does for example. Proxy the CAPS query always, unless a CAPS event arrived already. In that case return the caps that were set previously. > > @@ +318,3 @@ > > + > > + switch (GST_EVENT_TYPE (event)) { > > + case GST_EVENT_CAPS: > > > > All this needs some blocking, the CAPS events will arrive from two different > > threads and can happen at the same time > > > > Added a mutex on both sides to make this a critical section. I guess > gst_disparity_init() is called before any event handling (rtfm for me?) ;) Yes, that's called before anybody even has a pointer to the instance :) > > So for the GCond handling there are two things missing. This can easily > > deadlock right now. First of all you should always put the waiting in a loop, > > i.e. while (fs->buffer_left) g_cond_wait(). > > The cond should keep the chain_{left,right} fully blocked until the other chain > arrives at > the rendezvous. What would be the situation in which a chain function blocked > at the cond > variable would wake up _not_ being signalled by the other chain function? Flushes and the pipeline being shut down.
Review of attachment 250212 [details] [review]: You'll have to implement a query function on the sinkpads, and there do the CAPS query handling as described above (proxy if no caps set, set caps if caps are set) ::: ext/opencv/gstdisparity.cpp @@ +185,2 @@ G_DEFINE_TYPE (GstDisparity, gst_disparity, GST_TYPE_ELEMENT); +static GstElementClass *parent_class = NULL; Not needed, G_DEFINE_TYPE() creates and initializes a gst_disparity_parent_class variable for you @@ +231,3 @@ + element_class->change_state = gst_disparity_change_state; + parent_class = (GstElementClass *) g_type_class_peek_parent (klass); It also does the g_type_class_peek_parent() for you @@ +321,3 @@ + case GST_STATE_CHANGE_PAUSED_TO_READY: + fs->flushing = true; + g_cond_signal (fs->cond); You have to lock the mutex when signalling a GCond. On some weird platforms this is racy otherwise. Also broadcast just in case :) @@ +440,3 @@ fs = GST_DISPARITY (parent); + GST_DEBUG_OBJECT (pad, "processing frame from left"); + if (fs->flushing) Protect all accesses to flushing with the mutex @@ +448,3 @@ + GST_DEBUG_OBJECT (pad, " right is free, continuing"); + if (fs->flushing) + return GST_FLOW_FLUSHING; while (fs->buffer_left) { g_cond_wait(); if (flushing) return GST_FLOW_FLUSHING; } Also unlock before return @@ +483,3 @@ + GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing"); + if (fs->flushing) + return GST_FLOW_FLUSHING; Same as above here
Created attachment 250380 [details] [review] 2nd full patch GstDisparity Addressed all issues regarding locking and usage of g_cond_lock. Also removed the not needed parent_class. BUT: Still don't see the caps negotiation clearly :( I tried forcing the caps on both src and the other sink when there is a sink_event, but then the negotiation doesn't work out. Help! :'( Uploaded a new patch since the code has changed a bit.
Comment on attachment 250380 [details] [review] 2nd full patch GstDisparity This doesn't look like the patch you wanted to attach :)
(In reply to comment #6) > BUT: Still don't see the caps negotiation clearly :( > I tried forcing the caps on both src and the other sink when there is a > sink_event, but then the negotiation doesn't work out. Help! :'( What you would do is something like the following: sink_query(CAPS): - If !gst_pad_has_current_caps(srcpad): return gst_pad_get_template_caps(pad); - else: return gst_pad_get_current_caps(srcpad); sink_event(CAPS): - if !gst_pad_has_current_caps(srcpad): gst_pad_set_caps(srcpad, caps); - else if !gst_caps_is_equal(gst_pad_get_current_caps(srcpad), caps): fail(); - else: succeed();
Created attachment 250442 [details] [review] 3rd full patch GstDisparity Addressed from before all the comments on adding critical section to manipulate state changes etc. And also now adapted the caps negotiation following Sebastian's comments: added sink_query. Pls note that I moved the OpenCV-specific allocations into another function initialise_disparity, so the handle_sink_event and (new) handle_query functions are easy to spot. (Sorry about the messed up previous patch :( )
ping :)
ping2?
Review of attachment 250442 [details] [review]: ::: ext/opencv/gstdisparity.cpp @@ +324,3 @@ + GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS; + GstDisparity *fs = GST_DISPARITY (element); + g_mutex_lock (fs->lock); Better take the lock just around the lines that need it, someone might add something else to the switch statement later @@ +342,3 @@ + transition); + + g_mutex_lock (fs->lock); Same here @@ +373,3 @@ + /* Critical section since both pads handle event sinking simultaneously */ + g_mutex_lock (fs->lock); + GstVideoInfo info; Put all variable declarations at the beginning of a block @@ +387,3 @@ + "format", G_TYPE_STRING, "RGB", + "width", G_TYPE_INT, fs->width, + "height", G_TYPE_INT, fs->height, NULL); Use gst_video_info_to_caps() to create raw video caps @@ +396,3 @@ + + GST_INFO_OBJECT (pad, " Negotiated caps via event, %" GST_PTR_FORMAT, + caps); Put ret in the debug output too @@ +402,3 @@ + break; + } + ret = gst_pad_event_default (pad, parent, event); You probably don't want to forward the event if above the caps equality check has failed @@ +418,3 @@ + else + gst_query_set_caps_result (query, + gst_pad_get_current_caps (fs->srcpad)); This is leaking the return values of gst_pad_get_pad_template_caps() and gst_pad_get_current_caps(), also needs locking @@ +448,3 @@ + filter = GST_DISPARITY (object); + gst_disparity_release_all_pointers (filter); + filter->width = filter->height = filter->actualChannels = 0; Not necessary to reset these to 0 in finalize @@ +469,3 @@ + GST_DEBUG_OBJECT (pad, "processing frame from left"); + if (fs->flushing) + return GST_FLOW_FLUSHING; Check flushing after taking the mutex @@ +476,3 @@ + GST_DEBUG_OBJECT (pad, " right is free, continuing"); + if (fs->flushing) + return GST_FLOW_FLUSHING; Returning without unlocking the mutex @@ +481,3 @@ + + if (!gst_buffer_map (buffer, &info, (GstMapFlags) GST_MAP_READWRITE)) { + return GST_FLOW_ERROR; Returning without unlocking the mutex @@ +504,3 @@ + GST_DEBUG_OBJECT (pad, "processing frame from right"); + if (fs->flushing) + return GST_FLOW_FLUSHING; Check flushing after taking the mutex @@ +511,3 @@ + GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing"); + if (fs->flushing) + return GST_FLOW_FLUSHING; Returning without unlocking the mutex @@ +587,3 @@ + + GST_DEBUG_OBJECT (pad, " right has finished"); + fs->buffer_left = NULL; Unmap and unref the left buffer @@ +591,3 @@ + g_mutex_unlock (fs->lock); + + ret = gst_pad_push (fs->srcpad, buffer); As the right buffer is always the one that is forwarded it would make sense to proxy the allocation query from the right sinkpad to the srcpad
(In reply to comment #12) > Review of attachment 250442 [details] [review]: > > ::: ext/opencv/gstdisparity.cpp > @@ +324,3 @@ > + GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS; > + GstDisparity *fs = GST_DISPARITY (element); > + g_mutex_lock (fs->lock); > > Better take the lock just around the lines that need it, someone might add > something else to the switch statement later Done. > > @@ +342,3 @@ > + transition); > + > + g_mutex_lock (fs->lock); > > Same here Done. > > @@ +373,3 @@ > + /* Critical section since both pads handle event sinking simultaneously > */ > + g_mutex_lock (fs->lock); > + GstVideoInfo info; > > Put all variable declarations at the beginning of a block Done. > > @@ +387,3 @@ > + "format", G_TYPE_STRING, "RGB", > + "width", G_TYPE_INT, fs->width, > + "height", G_TYPE_INT, fs->height, NULL); > > Use gst_video_info_to_caps() to create raw video caps > Done. > @@ +396,3 @@ > + > + GST_INFO_OBJECT (pad, " Negotiated caps via event, %" GST_PTR_FORMAT, > + caps); > > Put ret in the debug output too > Done. > @@ +402,3 @@ > + break; > + } > + ret = gst_pad_event_default (pad, parent, event); > > You probably don't want to forward the event if above the caps equality check > has failed > Done. > @@ +418,3 @@ > + else > + gst_query_set_caps_result (query, > + gst_pad_get_current_caps (fs->srcpad)); > > This is leaking the return values of gst_pad_get_pad_template_caps() and > gst_pad_get_current_caps(), also needs locking > Hopefully done :) > @@ +448,3 @@ > + filter = GST_DISPARITY (object); > + gst_disparity_release_all_pointers (filter); > + filter->width = filter->height = filter->actualChannels = 0; > > Not necessary to reset these to 0 in finalize Done. > > @@ +469,3 @@ > + GST_DEBUG_OBJECT (pad, "processing frame from left"); > + if (fs->flushing) > + return GST_FLOW_FLUSHING; > > Check flushing after taking the mutex > Done. > @@ +476,3 @@ > + GST_DEBUG_OBJECT (pad, " right is free, continuing"); > + if (fs->flushing) > + return GST_FLOW_FLUSHING; > > Returning without unlocking the mutex Done. > > @@ +481,3 @@ > + > + if (!gst_buffer_map (buffer, &info, (GstMapFlags) GST_MAP_READWRITE)) { > + return GST_FLOW_ERROR; > > Returning without unlocking the mutex Done. > > @@ +504,3 @@ > + GST_DEBUG_OBJECT (pad, "processing frame from right"); > + if (fs->flushing) > + return GST_FLOW_FLUSHING; > > Check flushing after taking the mutex Done. > > @@ +511,3 @@ > + GST_DEBUG_OBJECT (pad, " left has just provided a frame, continuing"); > + if (fs->flushing) > + return GST_FLOW_FLUSHING; > > Returning without unlocking the mutex Done. > > @@ +587,3 @@ > + > + GST_DEBUG_OBJECT (pad, " right has finished"); > + fs->buffer_left = NULL; > > Unmap and unref the left buffer Done unmapping but not unrefing: fs->buffer_left is just a mapping of an already unreferenced incoming buffer (in chain_left), after the unmap, both will get automatically unreferenced and destroyed. > > @@ +591,3 @@ > + g_mutex_unlock (fs->lock); > + > + ret = gst_pad_push (fs->srcpad, buffer); > > As the right buffer is always the one that is forwarded it would make sense to > proxy the allocation query from the right sinkpad to the srcpad I don't really understand that :)
Created attachment 252234 [details] [review] Addressing comments over 3rd full patch. Incremental patch addressing (most of) Sebastian's comments.
Review of attachment 252234 [details] [review]: ::: ext/opencv/gstdisparity.cpp @@ +480,2 @@ if (fs->flushing) return GST_FLOW_FLUSHING; Return without unlocking the mutex @@ +517,2 @@ if (fs->flushing) return GST_FLOW_FLUSHING; Return without unlocking the mutex @@ +601,2 @@ GST_DEBUG_OBJECT (pad, " right has finished"); + gst_buffer_unmap (fs->buffer_left, &info); You must not unref the buffer in chain_left, but unref it here after unmapping
(In reply to comment #13) > > @@ +591,3 @@ > > + g_mutex_unlock (fs->lock); > > + > > + ret = gst_pad_push (fs->srcpad, buffer); > > > > As the right buffer is always the one that is forwarded it would make sense to > > proxy the allocation query from the right sinkpad to the srcpad > > I don't really understand that :) In the query function of the right sinkpad, when you receive an ALLOCATION query, you should forward this query to the srcpad. But only for the right sinkpad (as that is the one that gets the buffers which are forwarded downstream). That way upstream of the right sinkpad will ideally allocate buffers via a buffer pool that is provided by the sink.
(In reply to comment #15) > Review of attachment 252234 [details] [review]: > > ::: ext/opencv/gstdisparity.cpp > @@ +480,2 @@ > if (fs->flushing) > return GST_FLOW_FLUSHING; > > Return without unlocking the mutex > > @@ +517,2 @@ > if (fs->flushing) > return GST_FLOW_FLUSHING; > > Return without unlocking the mutex > > @@ +601,2 @@ > GST_DEBUG_OBJECT (pad, " right has finished"); > + gst_buffer_unmap (fs->buffer_left, &info); > > You must not unref the buffer in chain_left, but unref it here after unmapping Got it! - done those 3
(In reply to comment #16) > (In reply to comment #13) > > > > @@ +591,3 @@ > > > + g_mutex_unlock (fs->lock); > > > + > > > + ret = gst_pad_push (fs->srcpad, buffer); > > > > > > As the right buffer is always the one that is forwarded it would make sense to > > > proxy the allocation query from the right sinkpad to the srcpad > > > > I don't really understand that :) > > In the query function of the right sinkpad, when you receive an ALLOCATION > query, you should forward this query to the srcpad. But only for the right > sinkpad (as that is the one that gets the buffers which are forwarded > downstream). That way upstream of the right sinkpad will ideally allocate > buffers via a buffer pool that is provided by the sink. Hopefully done as well :)
Created attachment 252725 [details] [review] More addressing comments over 3rd full patch Addressed some missing return-without-releasing mutex, corrected buffer unref position, and hopefully added GST_QUERY_ALLOCATION correctly.
Review of attachment 252725 [details] [review]: ::: ext/opencv/gstdisparity.cpp @@ +433,3 @@ + case GST_QUERY_ALLOCATION: + if (pad == fs->sinkpad_right) + ret = gst_pad_query_default (fs->srcpad, parent, query); Almost, gst_pad_peer_query(srcpad, ...)
Created attachment 252731 [details] [review] 4th full patch GstDisparity Using gst_pad_peer_query(...) (line 435) Merged into one big patch since it seems that is converging.
Does not compile here after applying all patches (also can you make a combined patch with "git format-patch -1" after committing everything locally?): CXX libgstopencv_la-MotionCells.lo In file included from gstdisparity.cpp:125:0: gstdisparity.h:96:3: error: 'CvStereoGCState' does not name a type CvStereoGCState *sgc; /* This is a C implementation */ ^ gstdisparity.cpp: In function 'void gst_disparity_init(GstDisparity*)': gstdisparity.cpp:283:31: error: 'g_mutex_new' was not declared in this scope filter->lock = g_mutex_new (); ^ gstdisparity.cpp:284:30: error: 'g_cond_new' was not declared in this scope filter->cond = g_cond_new (); ^ gstdisparity.cpp: In function 'void gst_disparity_finalize(GObject*)': gstdisparity.cpp:470:28: error: 'g_cond_free' was not declared in this scope g_cond_free (filter->cond); ^ gstdisparity.cpp:471:29: error: 'g_mutex_free' was not declared in this scope g_mutex_free (filter->lock); ^ gstdisparity.cpp: In function 'int initialise_sbm(GstDisparity*)': gstdisparity.cpp:687:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc = cvCreateStereoGCState (16, 2); ^ gstdisparity.cpp:687:45: error: 'cvCreateStereoGCState' was not declared in this scope filter->sgc = cvCreateStereoGCState (16, 2); ^ gstdisparity.cpp:730:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->Ithreshold = 5; ^ gstdisparity.cpp:731:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->interactionRadius = 1; ^ gstdisparity.cpp:732:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->occlusionCost = 10000; ^ gstdisparity.cpp:733:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->minDisparity = 0; ^ gstdisparity.cpp:734:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->numberOfDisparities = 16; /* Coming from constructor too */ ^ gstdisparity.cpp:735:11: error: 'GstDisparity' has no member named 'sgc' filter->sgc->maxIters = 1; /* Coming from constructor too */ ^ gstdisparity.cpp: In function 'int run_sgc_iteration(GstDisparity*)': gstdisparity.cpp:775:44: error: 'GstDisparity' has no member named 'sgc' filter->cvGray_depth_map1_2, filter->sgc, 0); ^ gstdisparity.cpp:775:50: error: 'cvFindStereoCorrespondenceGC' was not declared in this scope filter->cvGray_depth_map1_2, filter->sgc, 0); ^
(In reply to comment #22) > Does not compile here after applying all patches (also can you make a combined > patch with "git format-patch -1" after committing everything locally?): > > > CXX libgstopencv_la-MotionCells.lo > In file included from gstdisparity.cpp:125:0: > gstdisparity.h:96:3: error: 'CvStereoGCState' does not name a type > CvStereoGCState *sgc; /* This is a C implementation */ > ^ > gstdisparity.cpp: In function 'void gst_disparity_init(GstDisparity*)': > gstdisparity.cpp:283:31: error: 'g_mutex_new' was not declared in this scope > filter->lock = g_mutex_new (); > ^ > gstdisparity.cpp:284:30: error: 'g_cond_new' was not declared in this scope > filter->cond = g_cond_new (); > ^ > gstdisparity.cpp: In function 'void gst_disparity_finalize(GObject*)': > gstdisparity.cpp:470:28: error: 'g_cond_free' was not declared in this scope > g_cond_free (filter->cond); > ^ > gstdisparity.cpp:471:29: error: 'g_mutex_free' was not declared in this scope > g_mutex_free (filter->lock); > ^ > gstdisparity.cpp: In function 'int initialise_sbm(GstDisparity*)': > gstdisparity.cpp:687:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc = cvCreateStereoGCState (16, 2); > ^ > gstdisparity.cpp:687:45: error: 'cvCreateStereoGCState' was not declared in > this scope > filter->sgc = cvCreateStereoGCState (16, 2); > ^ > gstdisparity.cpp:730:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->Ithreshold = 5; > ^ > gstdisparity.cpp:731:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->interactionRadius = 1; > ^ > gstdisparity.cpp:732:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->occlusionCost = 10000; > ^ > gstdisparity.cpp:733:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->minDisparity = 0; > ^ > gstdisparity.cpp:734:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->numberOfDisparities = 16; /* Coming from constructor too > */ > ^ > gstdisparity.cpp:735:11: error: 'GstDisparity' has no member named 'sgc' > filter->sgc->maxIters = 1; /* Coming from constructor too */ > ^ > gstdisparity.cpp: In function 'int run_sgc_iteration(GstDisparity*)': > gstdisparity.cpp:775:44: error: 'GstDisparity' has no member named 'sgc' > filter->cvGray_depth_map1_2, filter->sgc, 0); > ^ > gstdisparity.cpp:775:50: error: 'cvFindStereoCorrespondenceGC' was not declared > in this scope > filter->cvGray_depth_map1_2, filter->sgc, 0); > ^ Perhaps OpenCV has no legacy enabled and then it fails to find CvStereoGCState. Puzzles me that it complains about g_mutex_new() etc ??
Created attachment 252747 [details] [review] Squashed all commits in git format-patch mode
Perhaps package libopencv-legacy-dev should be enabled? The configure.ac asks for opencv "generic" but then checks for highgui.h which is part of another package (libopencv-highgui-dev). Perhaps I could add a legacy check to the global configure.ac?
I have libopencv-legacy-dev installed, the missing type is defined in opencv2/legacy/legacy.hpp
Review of attachment 252747 [details] [review]: Please choose a better commit message :) ::: ext/opencv/gstdisparity.cpp @@ +433,3 @@ + case GST_QUERY_ALLOCATION: + if (pad == fs->sinkpad_right) + ret = gst_pad_peer_query (fs->srcpad, query); else ret = FALSE;
Created attachment 252758 [details] [review] Squashed all commits in git format-patch mode With changed comment ;)
Created attachment 252759 [details] [review] Added return false when alloc query on the left pad.
(In reply to comment #26) > I have libopencv-legacy-dev installed, the missing type is defined in > opencv2/legacy/legacy.hpp Yes and legacy.hpp is included via cvaux.h (or similar) which comes from cv.h. OpenCV packaging is cumbersome, it goes partially into /usr/include/opencv and part under /usr/include/opencv2 and not all files (specially .h ones) are packaged, hence the mistery inclusions...
Hmm, so how do we make this work properly now? :) Is there a non-legacy replacement for this type?
I have a bit of a mess on my side, but perhaps the key would be to add to gstdisparity.h, after the include <cv.h> : #include <legacy/legacy.hpp> cv.h comes from /usr/include/opencv and legacy/legacy from /usr/include/opencv2 so who knows how the coexistence would be right now ... I can/will try cleaning up my system to work from packages but will take me a while - just saying ;)
Actually I meant #include <opencv2/legacy/legacy.hpp>
Hum, I removed my OpenCV from source and installed from precise packages. I had installed: sudo aptitude install libopencv-dev libopencv-legacy-dev libcv-dev libcvaux-dev libhighgui-dev and I compiled ok without any added legacy.hpp inclusions. CvStereoGCState is defined in opencv2/calib3d/calib3d.hpp which is pulled in directly from cv.h , so legacy is actually not needed, my mistake before, sorry, calib3d.hpp is owned by package libopencv-calib3d-dev .
I'm using opencv 2.4.5 and there it is defined in opencv2/legacy/legacy.hpp only. opencv2/calib3d/calib3d.hpp defines CvStereoBMState though which looks like it is doing more or less the same
Created attachment 252825 [details] [review] Opencv disparity element in single patch compiling in opencv 2.4.3 Added #include <opencv2/legacy/legacy.hpp> which seems to solve the OpenCV 2.4.3+ problem with CvStereoGCState and does not have troubles with older OpenCV versions.
commit 5e25d41b84b61d78e6e48b4a78a253f3ec6ab5ed Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Fri Aug 23 12:01:07 2013 +0200 opencv: Port to non-deprecated GMutex/GCond API commit fbb5dd38c7d454fec3ece29493ac38b812e9106d Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com> Date: Fri Aug 23 11:38:04 2013 +0200 opencv: Add disparity-map calculation element https://bugzilla.gnome.org/show_bug.cgi?id=704760