GNOME Bugzilla – Bug 796794
flowcombiner: update_pad_flow should check that the pad is inside the combiner
Last modified: 2018-11-03 12:47:26 UTC
Created attachment 373000 [details] [review] patch Currently it is not doing that and that can create issues
Comment on attachment 373000 [details] [review] patch Looks good but I would prefer a g_critical() (or a g_return_if_fail()) here. It's a clear programming error, this should never happen :)
I wonder if we can do something smarter here? Iterating the list twice for each flow combination just for a sanity check seems potentially expensive and unnecessary to me. Might not matter if it's just a few pads of course, but..
Created attachment 373001 [details] [review] patch (In reply to Sebastian Dröge (slomo) from comment #1) > Looks good but I would prefer a g_critical() (or a g_return_if_fail()) here. > It's a clear programming error, this should never happen :) Done.
(In reply to Tim-Philipp Müller from comment #2) > I wonder if we can do something smarter here? Iterating the list twice for > each flow combination just for a sanity check seems potentially expensive > and unnecessary to me. Might not matter if it's just a few pads of course, > but.. I don't know how we could do this. Maybe passing the pad thru down to gst_flow_combiner_get_flow but we might be running some other code in-between and bail out before we reach that check.
Created attachment 373002 [details] [review] patch 2/2 (In reply to Xabier Rodríguez Calvar from comment #4) > (In reply to Tim-Philipp Müller from comment #2) > > I wonder if we can do something smarter here? Iterating the list twice for > > each flow combination just for a sanity check seems potentially expensive > > and unnecessary to me. Might not matter if it's just a few pads of course, > > but.. > > I don't know how we could do this. Maybe passing the pad thru down to > gst_flow_combiner_get_flow but we might be running some other code > in-between and bail out before we reach that check. In my second patch (that would apply on top of the first) I create the unsafe version that avoids the check.
Created attachment 373003 [details] [review] patch 2/2 Rebased against master
We could store the pads in a GHashTable ;)
(In reply to Sebastian Dröge (slomo) from comment #7) > We could store the pads in a GHashTable ;) We could! Maybe some other day! :-p PS: It's a pity there is not GHashSet, cause in this case we would just need they keys without the values.
(In reply to Xabier Rodríguez Calvar from comment #8) > PS: It's a pity there is not GHashSet, cause in this case we would just need > they keys without the values. There is `g_hash_table_add` which aims at offering a "set like" API on top of GHashTable.
If the order of the pads does not matter, we could store the pads in a sorted array instead of a list and bisect over it with gst_util_array_binary_search() ;)
Created attachment 373134 [details] [review] 1/2 migrate flow combiner pads set from GQueue to a sorted GArray
Created attachment 373135 [details] [review] 2/2 add check for existing pad in update_pad_flow This opens a huge can of worms, at least in qtdemux. I think this should be fixed but I don't think I have the time to address all implications.
(In reply to Xabier Rodríguez Calvar from comment #12) > This opens a huge can of worms, at least in qtdemux. I think this should be > fixed but I don't think I have the time to address all implications. Or maybe my patches are buggy...
Can't help but wonder if all of this is really worth it? :)
(In reply to Tim-Philipp Müller from comment #14) > Can't help but wonder if all of this is really worth it? :) Maybe not. We can just let people make programming mistakes. I had submitted a couple of patches you didn't like and recommended to use gst_util_array_binary_search() which is exactly what I did.
Created attachment 373140 [details] [review] 1/2 migrate flow combiner pads set from GQueue to a sorted GPtrArray
Created attachment 373141 [details] [review] 2/2 add check for existing pad in update_pad_flow
These two patches should work, please review if you have a moment.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/303.