GNOME Bugzilla – Bug 779055
basetransform: improve handling of empty peercaps when intersecting
Last modified: 2017-02-22 09:57:49 UTC
in gstbasetranform.c around line 674 check for empty peercaps is not present should we also check it along with peercaps not NULL check? Is this scenario possible or is it always expected that element should return either NULL caps or valid? Previous discussion histroy: ----------------------------- [reply] [−] Comment 23 Sebastian Dröge (slomo) [GStreamer developer] 2017-02-09 11:53:49 UTC If they're empty, they stay empty, no? Might just be a fast-path to error out faster here. If you want to provide a patch, just do so :) [reply] [−] Comment 24 abhimanyu.v@imgtec.com 2017-02-09 12:30:41 UTC Created attachment 345310 [details] [review] [review] 0001-dont-intersect-if-peercaps-is-empty_new Check of empty peercaps as it should be similar to null peercaps. [reply] [−] Comment 25 Sebastian Dröge (slomo) [GStreamer developer] 2017-02-10 11:11:28 UTC Comment on attachment 345310 [details] [review] [review] 0001-dont-intersect-if-peercaps-is-empty_new This does not apply to latest GIT master, and would also cause the empty peercaps to not be unreffed and leaked. [reply] [−] Comment 26 abhimanyu.v@imgtec.com 2017-02-14 09:59:08 UTC Created attachment 345712 [details] [review] [review] new patch for fixing empty peercaps usecase New patch created from master [reply] [−] Comment 27 abhimanyu.v@imgtec.com 2017-02-14 10:00:48 UTC >and would also cause the empty peercaps to not be unreffed and leaked. This seems not the case on master as at line 724, peercaps is being freed. All path seems to go through it. [reply] [−] Comment 28 Sebastian Dröge (slomo) [GStreamer developer] 2017-02-14 10:29:29 UTC Review of attachment 345712 [details] [review] [review]: ::: libs/gst/base/gstbasetransform.c @@ +672,3 @@ gst_caps_unref (peerfilter); + if (peercaps && !gst_caps_is_empty (peercaps)) { This is not equivalent though. If the caps are empty, now we would work with the template caps. Before we would work with the empty caps and error out. I think you want to add a "if (gst_caps_is_empty(peercaps)) goto done;" above instead [reply] [−] Comment 29 abhimanyu.v@imgtec.com 2017-02-14 13:03:43 UTC >I think you want to add a "if (gst_caps_is_empty(peercaps)) goto done;" above instead This will cause caps to return NULL which will still fail linking. we would need to return some caps either it could be peerfilter as in case of peerfilter empty or we return template as it is now. I think empty caps is kind of similar to NULL caps practically?
Created attachment 346420 [details] [review] new patch for fixing empty peercaps usecase
(In reply to abhimanyu.v from comment #0) > > This will cause caps to return NULL which will still fail linking. we would > need to return some caps either it could be peerfilter as in case of > peerfilter empty or we return template as it is now. I think empty caps is > kind of similar to NULL caps practically? Looking at the code of gst_pad_peer_query_caps() again, there's no way how this can return NULL at all. So the NULL-case is never called currently, and it's not clear why it exists. The intention behind the NULL-case seems to be "there is no peer", in which case just the template is taken. "there is no peer (yet)" is very different from empty caps (aka "no supported common formats) though, the former is more like any caps. Is there any specific problem your patch is solving? How can it be reproduced?
>Is there any specific problem your patch is solving? How can it be reproduced? I got this problem (on gst 1.8 branch) when deinterleave code was returning empty caps in case iterator got updated in between the iteration. After applying you fix to not return empty, this problem is no more. The purpose of this patch was mostly to see if there could be case where element can return empty (in case of no bug). In case where they are allowed to return empty caps then it shouldnt fail negotiation?
If empty caps are returned (depending on the details but usually) this means that negotiation failed. It means that there are no possible caps to negotiate to that downstream would be able to handle.
okay, if that is case i am happy to close this bug. Thanks!
Ok, thanks for looking into this nonetheless :)