Repository navigation
Implement sending RTCP BYE when a media track is closed - #1624
aronrosenberg wants to merge 1 commit into
Conversation
20e8a96 to
a5c5da2
Compare
| bool disableAutoGathering = false; | ||
| bool forceMediaTransport = false; | ||
| bool disableFingerprintVerification = false; | ||
| bool sendRtcpByeOnTrackClose = false; |
There was a problem hiding this comment.
I think there should not be an opt-in per-connection config flag for such a behavior. It should be enabled by default, with optionally a compile flag to disable if you think regressions are possible. If you really want to make it optional at runtime it should be implemented in a media handler.
There was a problem hiding this comment.
I redid the design and removed the opt-in config option and made it always be sent. I left out a compile time removal option.
| // Only audio and video tracks carry RTP/RTCP traffic — application (data channel) and | ||
| // other non-media m= sections never have an SSRC stream and must not emit BYE. | ||
| auto mediaType = mMediaDescription.type(); | ||
| if (mediaType != "audio" && mediaType != "video") | ||
| return; |
There was a problem hiding this comment.
Tracks are guaranteed to be media, there is no need for this redundant test, and it's misleading because it hints that media type could be application or something else here. You should remove it.
There was a problem hiding this comment.
Removed and used a new integration model inside a MediaHandler. I did this by creating a new close() virtual that a handler can implement and is called sync when the track is torn down before the transports might get closed.
| // RFC 3550 section 6.3.7: a participant which never sent RTP/RTCP MUST NOT send BYE. | ||
| // Only send for tracks we were transmitting on. | ||
| if (dir != Description::Direction::SendOnly && dir != Description::Direction::SendRecv) | ||
| return; |
There was a problem hiding this comment.
This check doesn't do what the comment says. First, the direction doesn't say if an RTP/RTCP packet was actually sent on the track. Then, RTCP packets are also sent on RecvOnly tracks.
There was a problem hiding this comment.
Fixed and updated the logic in the new handler implementation.
The Track class now keeps a mSentPacket which is set by the transport send if any outbound packet has been sent. This is used in the new close() model, along with the direction in the handler so it can make an intelligent choice about if the send side SR handler needs to send the BYE under the send side SSRC, or in a recv-only case if the recv side SSRC is used to send the BYE.
| // One compound RR + SDES + BYE per SSRC | ||
| for (size_t i = 0; i < ssrcs.size(); ++i) { | ||
| SSRC ssrc = ssrcs[i]; | ||
|
|
||
| size_t rrSize = RtcpRr::SizeWithReportBlocks(0); | ||
| size_t sdesSize = RtcpSdes::Size({{uint8_t(cnames[i].size())}}); | ||
| size_t byeSize = RtcpBye::SizeWithSsrcs(1); | ||
|
|
||
| auto msg = make_message(rrSize + sdesSize + byeSize, Message::Control); | ||
|
|
||
| auto rr = reinterpret_cast<RtcpRr *>(msg->data()); | ||
| rr->preparePacket(ssrc, 0); | ||
|
|
||
| auto sdes = reinterpret_cast<RtcpSdes *>(msg->data() + rrSize); | ||
| auto chunk = sdes->getChunk(0); | ||
| chunk->setSSRC(ssrc); | ||
| auto item = chunk->getItem(0); | ||
| item->type = 1; // CNAME | ||
| item->setText(cnames[i]); | ||
| sdes->preparePacket(1); | ||
|
|
||
| auto bye = reinterpret_cast<RtcpBye *>(msg->data() + rrSize + sdesSize); | ||
| bye->preparePacket(1); | ||
| bye->setSsrc(0, ssrc); | ||
|
|
||
| try { | ||
| transport->sendMedia(msg); | ||
| } catch (const std::exception &e) { | ||
| PLOG_WARNING << "Failed to send RTCP BYE for SSRC " << ssrc << ": " << e.what(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This should definitely be done in RtcpReceivingSession instead of generating a dummy RR. It would also make it easier to arm the sending of RTCP BYE as soon as an RTP/RTCP packet has been sent out.
There was a problem hiding this comment.
This has all been moved into both RtcpReceivingSession and RtcpSrReporter and goes into the appropriate SR or RR report depending on the Direction of the track.
a5c5da2 to
adfc964
Compare
adfc964 to
fc09953
Compare
| if (auto track = it->second.lock()) | ||
| track->incoming(message); | ||
| if (delivered.insert(track.get()).second) | ||
| track->incoming(message); |
There was a problem hiding this comment.
@paullouisageneau I want to call out this change very closely. This was another pre-existing bug I found and fixed. If a compound RTCP packet had multiple SSRCs belonging to the same track, it was previously being delivered N times, once per unique SSRC. The new BYE's when RTX is active were triggering this because the SSRC for the main media and the SSRC for RTX were in the same BYE payload.
It was also possible if multiple SSRCs were used for main media that this would spawn duplicate RR's being sent as when a SR with multiple SSRC's was received, it would deliver the SR twice to the handler which would then cause two RR's to be sent skewing the transmission states. I don't think would actually happen since the library doesn't implement simulcast support using multiple SSRC's, only RID based.
228eed6 to
02c2e7c
Compare
| mProcessor.enqueue(&PeerConnection::detachTransports, shared_from_this()); | ||
| } | ||
|
|
||
| void PeerConnection::detachTransports() { |
There was a problem hiding this comment.
@paullouisageneau I want to call out this change very closely. I had to switch the closeTransports() call to be a mix of sync and async since otherwise the closeTracks would often lose the race to being able to put packets on the wire from the close() handler as the transport itself would be closed by the time the async invoke occurred if invoked from the destructor. I added unit tests which proved this behavior, but it is a structural change.
If you have other ideas on a better to solve this, let me know and I can change it.
Sends an RTCP BYE when a media track is closed, so remote peers learn a stream has ended rather than having to time it out. * Create new RtcpBye packet type * Add a MediaHandler::close() hook with a closeChain() walk, so handlers are told the media is closing and can send final packets while the transports are still up * Send the BYE from RtcpSrReporter (its sending SSRC plus the RTX SSRC) and from RtcpReceivingSession (receive-only media) * Detach the transports after the queued close tasks so the BYE is not raced by transport teardown, without delaying the Closed state or the callback reset * Extract SSRCs from inbound BYE when demultiplexing compound RTCP * Deliver compound RTCP packets to each track only once, not once per SSRC per track in a payload * Add unit and integration tests for the packet, both handlers, every media direction, multiple tracks, RTX, and the close ordering
02c2e7c to
86b82ee
Compare
Sends an RTCP BYE when a media track is closed, so remote peers learn a stream has ended rather than having to time it out.
MediaHandler::close()hook with acloseChain()walk, so handlers are told the media is closing and can send final packets while the transports are still upRtcpSrReporter(its sending SSRC plus the RTX SSRC) and fromRtcpReceivingSession(receive-only media)AI Assisted