[Live-devel] Live555 RTSP Client never sees RTCP BYE message from Live555 Server

[Matt Schuckmann]

I understand that it's hard to test bugs on modified code, I'd submit my 
modifications to the project but you've already told me that you won't 
accept some of them (I understand your reasons), and I'm not ready to 
submit the others. I'm only trying to help you out by reporting what I 
see, I haven't changed any of the code in the areas I'm referring to here.

I'm pretty sure that this would happen with unmodified code, just see if 
your bye handler is called in OpenRTSP after sending or a teardown 
message while the source is still playing. Or better yet stick a network 
sniffer on a test setup and you'll see the server never sends the bye 
message after receiving a teardown from the client.

Also note the call sequence when the server is tearing things down:
The server deletes the clientSession which causes reclaimStreamStates to 
be called which causes OnDemandServerMediaSubsession::deleteStream().
The first thing OnDemandServerMediaSubsession::deleteStream() does is 
call StreamState::endPlaying() on the streamState with destination 
addresses to stop playing for.
End playing removes the destinations from the groupSock associated with 
the RTCPInstance object.
OnDemandServerMediaSubsession::deleteStream() goes through and 
decrements the refcounts for all the streamStates and deletes all the 
streamState's with a reference count of 0 (which a unicast session is 
all of them). The destructor for the streamState objects calls 
StreamState::reclaim which calls Medium::close() for the RTCPinstance , 
which deletes the RTCPInstance. The destructor for RTCPInstance calls 
SendBYE() which on the surface appears to work but in fact doesn't do 
anything at all, because all the destinations in the groupsock have been 
removed already so the BYE message never gets sent to the server.
Make sense?

Ross Finlayson wrote:
> In general, it's hard to respond to alleged bug reports on modified 
> code.  The best bug reports are those that apply to the original, 
> unmodified code, so we can (hopefully) reproduce the problem (if any) 
> ourselves.
>
>
>> PS. You also should note that the BYE handler code in OpenRTSP causes 
>> all the streams to be deleted and the RTCPInstance objects with them, 
>> the problem is the RTCPInstance object is in the process of handling 
>> a packet.
>
> Remember that this is all single-threaded code.  If an "RTCPInstance" 
> object is deleted, then it's not also 'in the process' of doing 
> anything else.  The only way it could also be involved in 'handling a 
> packet' would be if this packet handling happened later, as a result 
> of an 'incoming packet' event in the event loop.  But that should 
> never happen, because - as a result of deleting the "RTCPInstance" 
> object - "TaskScheduler::turnOffBackgroundReadHandling()" gets called.

I'm very aware that it's single threaded and I've made sure to work 
within that context. The fact that it's a single thread library doesn't 
preclude one from calling delete from within one of that objects 
methods, or worse yet while such a method is on the call stack, which is 
the case here.

If you look at the BYE case in RTCPInstance::incomingReportHandler1() 
you see that the byeHandler is called on or about line 522 (I've changed 
all the fprintf's to calls to envir() << to improve debugging in a non 
command line app, so my line numbers might not match yours, that's the 
only change I've made to this code).
If you follow the call sequence for OpenRTSP you'll see that this 
becomes a call to subsessionByeHandler in PlayCommon.cpp,  If all the 
subsessions have been stopped subsesionByeHandler() then calls 
sessionAfterPlaying() (also in PlayCommon.cpp).  The 
sessionAfterPlaying() function then calls Shutdown() which calls 
closeMediaSinks() and tearDownStreams() , and closes the session before 
calling exit(). It think it's closeMediaSink() or tearDownStreams() that 
causes all the streams and there respective RTCPInstance's to be 
deleted. Now if you remove the call to exit() you'd see that eventually 
the call stack will unwind all the way back up to 
RTCPInstance::incomingReportHandler1() which oops the object associated 
with this call has been deleted so if any of the execution after the 
call the ByeHandler tries to use the any of that objects state you'll be 
in trouble. That's what I mean by the RTCPInstance is deleted while it's 
in the process of handling the BYE message. I generally consider it bad 
form to call delete on a object while one of it's methods are on the 
call stack.  In this particular case I don't think that 
RTCPInstance::incomingReportHandler1() should be changed it's, it can't 
know that a client is going to want to delete it, in fact it should 
assume that it won't. More likely OpenRTSP should be changed to clean 
things up outside of the call sequence origonating from it's byeHandler.
My case I just scheduled a delayed task with the taskScheduler to do the 
clean up.

I discovered this when:
1. I got the server to send BYE messages by adding a call to SendBYE as 
I described before.
2. I changed all the fprintf's in RTCPInstance to envir() << and my app 
would crash when RTCPInstance::incomingReportHandler1() would try to log 
something after the call to the byeHandler.

I hope this all makes sense and it helps you and other users of this 
very useful library out.

Matt S.