GNOME Bugzilla – Bug 773218
rtpbin: pipeline gets an EOS when any rtpsources byes
Last modified: 2017-07-05 01:24:32 UTC
When we have multiple sender sources we can not send the EOS to the source pad until every rtpsource is done.
Created attachment 338011 [details] [review] Patch
We encountered this issue while checking the retransmission support in OpenWebRTC (TEST_RTX define in the transport agent).
See also bug #768725. I think it should only EOS per session, and only per session if all streams belonging to a session are EOS
Created attachment 338178 [details] [review] Patch
This last patch handles the EOS in the rtcp loop checking the amount of sources in the session and removes the option the sources can produce the EOS when sending a BYE message.
Created attachment 338180 [details] [review] Patch
Review of attachment 338180 [details] [review]: Not sure I understand. What should happen here AFAIU is that the whole session goes EOS if all sources are BYE. How is your patch doing that? ::: gst/rtpmanager/gstrtpsession.c @@ +1158,3 @@ GST_RTP_SESSION_LOCK (rtpsession); + + if (!rtp_session_get_num_sources (session)) { Is the source removed from the session whenever it goes BYE? And that here then makes sure to EOS if no sources are left? Not sure if we should distinguish between internal sources or not (i.e. should we care that a remote source did not BYE yet?), and if this session is sending or only receiving.
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 338180 [details] [review] [review]: > > Not sure I understand. What should happen here AFAIU is that the whole > session goes EOS if all sources are BYE. How is your patch doing that? > The rtpsession keeps the count of the amount of sources and everytime it runs the timeout it checks if it can remove the sources in the cleanup method. We could do it returning a value to the rtp_session_on_timeout even, instead of asking for the number. When a source goes BYE it is removed and rtpsession keeps the count of them. > ::: gst/rtpmanager/gstrtpsession.c > @@ +1158,3 @@ > GST_RTP_SESSION_LOCK (rtpsession); > + > + if (!rtp_session_get_num_sources (session)) { > > Is the source removed from the session whenever it goes BYE? And that here > then makes sure to EOS if no sources are left? Not sure if we should > distinguish between internal sources or not (i.e. should we care that a > remote source did not BYE yet?), and if this session is sending or only > receiving. Yep, rtpsession basically does that, it removes the sources when they go BYE considering all the complex situations. I was considering also what kind of sources we should consider and I think the safest solution is the total amount because it includes everything. We could try to make it sooner using active sources maybe but it seems we are getting small benefit of dealing with a more complex situation to send the EOS before.
Can you explain that a bit more in the commit message then (and fix the typo), and add a comment before the EOS sending to explain why? Seems like a good approach then For the RTP we already forward EOS events already on all the related pads, right?
(In reply to Sebastian Dröge (slomo) from comment #9) > Can you explain that a bit more in the commit message then (and fix the > typo), and add a comment before the EOS sending to explain why? Seems like a > good approach then > Right. > For the RTP we already forward EOS events already on all the related pads, > right? I'm not sure about this, I did not check the code for other pads. I'll check it, but it seems this is the only eos we were generating.
(In reply to Alejandro G. Castro from comment #10) > > For the RTP we already forward EOS events already on all the related pads, > > right? > > I'm not sure about this, I did not check the code for other pads. I'll check > it, but it seems this is the only eos we were generating. Basically check what the sink pad event functions are doing with EOS. If they do nothing (gst_pad_event_default()), then it should be fine. If they are doing something explicit, it might not be ok
(In reply to Sebastian Dröge (slomo) from comment #11) > (In reply to Alejandro G. Castro from comment #10) > > > > For the RTP we already forward EOS events already on all the related pads, > > > right? > > > > I'm not sure about this, I did not check the code for other pads. I'll check > > it, but it seems this is the only eos we were generating. > > Basically check what the sink pad event functions are doing with EOS. If > they do nothing (gst_pad_event_default()), then it should be fine. If they > are doing something explicit, it might not be ok I checked this situation, the EOS event for the receive sink pad sends the event also fo the rtcp_src, here I wonder if we should also do the same for the send source because without the rtcp connection not sure if the send if going to behave. For the send sink pad it basically schedules a bye message in all the rtpsources, there is a fixme though because it is not clear we could be still receiving and we do not want to stop that even if the send closes. I guess it is good solution for the moment but when checking different use cases we could have to review that part also in the near future. Going back to the case we are checking, we can not send EOS to the rtcp pad when one of the sources byes because we could have still other sources alive in the session, for instance when rtx source is closed, or when the initial dummy source is destroyed. Trying to make sure we send the EOS in the right moment we added it when there is no more rtpsources in the session, it is safe because we are sure no source is open or has any pending events.
Created attachment 338501 [details] [review] Patch
(In reply to Alejandro G. Castro from comment #12) > (In reply to Sebastian Dröge (slomo) from comment #11) > > (In reply to Alejandro G. Castro from comment #10) > > > > > > For the RTP we already forward EOS events already on all the related pads, > > > > right? > > > > > > I'm not sure about this, I did not check the code for other pads. I'll check > > > it, but it seems this is the only eos we were generating. > > > > Basically check what the sink pad event functions are doing with EOS. If > > they do nothing (gst_pad_event_default()), then it should be fine. If they > > are doing something explicit, it might not be ok > > I checked this situation, the EOS event for the receive sink pad sends the > event also fo the rtcp_src, here I wonder if we should also do the same for > the send source because without the rtcp connection not sure if the send if > going to behave. For the send sink pad it basically schedules a bye message > in all the rtpsources, there is a fixme though because it is not clear we > could be still receiving and we do not want to stop that even if the send > closes. I guess it is good solution for the moment but when checking > different use cases we could have to review that part also in the near > future. Can you open a new bug about the EOS events that come from the outside? Let's keep this bug only about the BYE handling then :) > Going back to the case we are checking, we can not send EOS to the rtcp pad > when one of the sources byes because we could have still other sources alive > in the session, for instance when rtx source is closed, or when the initial > dummy source is destroyed. Trying to make sure we send the EOS in the right > moment we added it when there is no more rtpsources in the session, it is > safe because we are sure no source is open or has any pending events. ack
Review of attachment 338501 [details] [review]: Seems almost good to go ::: gst/rtpmanager/gstrtpsession.c @@ +1167,3 @@ + gst_object_ref (rtcp_src); + GST_LOG_OBJECT (rtpsession, "sending EOS"); + gst_pad_push_event (rtpsession->send_rtcp_src, gst_event_new_eos ()); Unlock before pushing the event, lock afterwards
(In reply to Sebastian Dröge (slomo) from comment #15) > Review of attachment 338501 [details] [review] [review]: > > Seems almost good to go > > ::: gst/rtpmanager/gstrtpsession.c > @@ +1167,3 @@ > + gst_object_ref (rtcp_src); > + GST_LOG_OBJECT (rtpsession, "sending EOS"); > + gst_pad_push_event (rtpsession->send_rtcp_src, gst_event_new_eos > ()); > > Unlock before pushing the event, lock afterwards (In reply to Sebastian Dröge (slomo) from comment #14) > (In reply to Alejandro G. Castro from comment #12) > > (In reply to Sebastian Dröge (slomo) from comment #11) > > > (In reply to Alejandro G. Castro from comment #10) > > > > > > > > For the RTP we already forward EOS events already on all the related pads, > > > > > right? > > > > > > > > I'm not sure about this, I did not check the code for other pads. I'll check > > > > it, but it seems this is the only eos we were generating. > > > > > > Basically check what the sink pad event functions are doing with EOS. If > > > they do nothing (gst_pad_event_default()), then it should be fine. If they > > > are doing something explicit, it might not be ok > > > > I checked this situation, the EOS event for the receive sink pad sends the > > event also fo the rtcp_src, here I wonder if we should also do the same for > > the send source because without the rtcp connection not sure if the send if > > going to behave. For the send sink pad it basically schedules a bye message > > in all the rtpsources, there is a fixme though because it is not clear we > > could be still receiving and we do not want to stop that even if the send > > closes. I guess it is good solution for the moment but when checking > > different use cases we could have to review that part also in the near > > future. > > Can you open a new bug about the EOS events that come from the outside? > Let's keep this bug only about the BYE handling then :) > Done: bufg #773574 Thanks very much for your review.
Created attachment 338587 [details] [review] Patch
Review of attachment 338587 [details] [review]: Thanks :)
commit eeea2a7fe88a17b15318d5b6ae6e190b2f777030 Author: Alejandro G. Castro <alex@igalia.com> Date: Wed Oct 26 13:21:29 2016 +0200 rtpbin: pipeline gets an EOS when any rtpsources byes Instead of sending EOS when a source byes we have to wait for all the sources to be gone, which means they already sent BYE and were removed from the session. We now handle the EOS in the rtcp loop checking the amount of sources in the session. https://bugzilla.gnome.org/show_bug.cgi?id=773218
Sebastian asked me to re-open this. This patch appears to cause a regression for the following scenario: 1) gst-rtsp-server/examples$ ./test-record '( queue name=depay0 ! fakesink dump=true )' 2) gst-launch-1.0 audiotestsrc num-buffers=10 samplesperbuffer=8000 ! audio/x-raw,rate=8000 ! alawenc ! rtspclientsink location=rtsp://127.0.0.1:8554/test rtx-time=0 The elaborate pipeline is just to make sure we send exactly 10 secs of data. With this patch, this pipeline hangs after ~10 seconds, instead of EOS-ing and finishing properly. If you pick e.g. num-buffers=1 (for one second of data), it always works. With the patch reverted, it also finishes reliably.
Should revert that for 1.10, and for 1.12 find a solution here. It was apparently not safe enough for a stable release. commit ae62fcb7933ef7728e45eadf0b3e7c26ee4b243c Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Mar 17 12:19:53 2017 +0200 Revert "rtpbin: pipeline gets an EOS when any rtpsources byes" This reverts commit eeea2a7fe88a17b15318d5b6ae6e190b2f777030. It breaks EOS in some sender pipelines, see https://bugzilla.gnome.org/show_bug.cgi?id=773218#c20
Alejandro, can you provide some kind of testcase for your scenario so we can make sure it also still works after the new fix?
Sorry about this, we do not have a simple testcase right now. It is a problem for when bundling the media for the openwebrtc, which means you have to use webkit and chrome with an app that requests the bundle policy. We are currently using it so we will keep an eye on this and come back when this hits our repositories or we have time to check it before. Thanks for the heads up.
Let's revert it for now for the release then. It looks like it needs a bit more fine-tuning. commit 50a4b5bc0d46f82ad0d61878f84d39b53b87b82b Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Apr 19 12:28:12 2017 +0100 Revert "rtpbin: pipeline gets an EOS when any rtpsources byes" This reverts commit eeea2a7fe88a17b15318d5b6ae6e190b2f777030. It breaks EOS in some sender pipelines, see https://bugzilla.gnome.org/show_bug.cgi?id=773218#c20
Alejandro, what should happen about this bug?
I'll close it with NEEDINFO until we come to the point we can test if it is still there in the new versions. Thanks for your help.
Well, your patch was reverted so the bug will be there again in newer versions :)
Yeah :-), that is why I thought NEEDINFO is the right choice, when someone finds the issue again and has the time to check it can use all this information. Best option is to create a small test case, I'll keep this in my TODO, so that I can do it when I find some spare time, not sure when though ;-).
We stumbled onto this indepdendantly, here is a series of two patches that fixes this, including a unit test. The problem with the RTSP sink was that the EOS was not sent when it should have been in that case with the other patch. commit 7e7e52caa0c0d8e64735c1939bfc070a1148c942 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue Jul 4 12:24:41 2017 -0400 rtpsession: Only send EOS if all sources have been marked bye Now that multiple sender RTPSource can share the same RTPSession, we must not send an EOS unless they're all marked bye. commit 96e71b02867319dd11efd838394e4a8ab459d6c2 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue Jul 4 17:42:25 2017 -0400 rtpsession: Send EOS if all internal sources sent bye The ones which are not internal should not matter, and we should wait for all sources to have sent their BYEs. And add unit test https://bugzilla.gnome.org/show_bug.cgi?id=773218