GNOME Bugzilla – Bug 731716
funnel: fix eos handling and unit test case
Last modified: 2014-06-19 14:48:59 UTC
Refine eos handling in funnel. Also funnel unit test cases are incorrect related to EOS handling.
I am working on it and soon provide a patch related to it.
Created attachment 278537 [details] [review] Fix for funnel unittestcase
It would be nice if your commit message actually explained what you fixed/what changed, or how the current behaviour wasn't right (easier for reviewers to match that to what the patch does). It seems to me one might want different modes for funnel: one where it forwards the first EOS, and one where it only forwards EOS once it's see EOS on all input pads. Either seems equally plausible IMHO, unless you envision the element is only used on once specific use case.
The funnel implementation as such was implemented to forward EOS when all the sinkpad receives EOS on all input pad. And also in the unit test if you see the comment, it was clearly mentioned it should fail (but the condition check was done not to fail). The related issue I worked on was https://bugzilla.gnome.org/show_bug.cgi?id=727945 After that I found the testcase is wrong and modified it. Also change in funnel code is related to eos. Suppose one of the funnel is forwarding eos and no data has come to either of the sink pad, then also it was forwarding the EOS. because the previous code was like if ((GST_EVENT_TYPE (event) == GST_EVENT_EOS) && (!gst_funnel_all_sinkpads_eos_unlocked (funnel, pad))) { forward = FALSE; } else if (funnel->last_sinkpad && (pad != funnel->last_sinkpad)) { .... } So it was faling first condition and also second because (funnel->last_sinkpad == NULL). And it was forwarding the EOS. Please let me know if you need any further clarification on it. (In reply to comment #3) > It would be nice if your commit message actually explained what you fixed/what > changed, or how the current behaviour wasn't right (easier for reviewers to > match that to what the patch does). It seems to me one might want different > modes for funnel: one where it forwards the first EOS, and one where it only > forwards EOS once it's see EOS on all input pads. Either seems equally > plausible IMHO, unless you envision the element is only used on once specific > use case.
Comment on attachment 278537 [details] [review] Fix for funnel unittestcase Need to refine the git comment.
Created attachment 278766 [details] [review] Patch for fix funnel EOS handling and wrong unittest When no data is coming from sinkpads and eos events arrived at one of the sinkpad, funnel forwards the EOS event to downstream. It forwards the EOS because lastsink pad is NULL. Also the unit testcase of the funnel is not checking the correct behavior as it should. The unit test case should fail if one of the sink pad has already EOS present on it and we are trying to push one more EOS.
Pushed commit 5b3ee70ea953227439820a38f2fc87842c3a5025 Author: Srimanta Panda <srimanta.panda@axis.com> Date: Mon Jun 16 13:47:55 2014 +0200 Fix funnel EOS handling and wrong unittest When no data is coming from sinkpads and eos events arrived at one of the sinkpad, funnel forwards the EOS event to downstream. It forwards the EOS because lastsink pad is NULL. Also the unit testcase of the funnel is not checking the correct behavior as it should. The unit test case should fail if one of the sink pad has already EOS present on it and we are trying to push one more EOS. https://bugzilla.gnome.org/show_bug.cgi?id=731716