From f70946885102cfe543dc97210ba2053ac17b8bcc Mon Sep 17 00:00:00 2001 From: Randell Jesup Date: Thu, 2 Mar 2017 15:11:22 -0500 Subject: [PATCH] Bug 1337777: if no receive-SSRC was signaled for video, on the first packet reset the VideoReceiveStream r=bwc Note that this stumbles over the use of the PCHandle as a global when initializing the OpenH264 gmp plugin. MozReview-Commit-ID: 7GEvIwwsitk --- dom/media/systemservices/MediaUtils.h | 2 +- .../src/media-conduit/AudioConduit.cpp | 2 +- .../src/media-conduit/AudioConduit.h | 3 +- .../src/media-conduit/MediaConduitInterface.h | 4 +- .../src/media-conduit/VideoConduit.cpp | 79 ++++++++++++++++++- .../src/media-conduit/VideoConduit.h | 20 ++++- .../src/mediapipeline/MediaPipeline.cpp | 3 +- .../peerconnection/MediaPipelineFactory.cpp | 7 +- 8 files changed, 105 insertions(+), 15 deletions(-) diff --git a/dom/media/systemservices/MediaUtils.h b/dom/media/systemservices/MediaUtils.h index 18f7d3e41a5e..c208147aaafc 100644 --- a/dom/media/systemservices/MediaUtils.h +++ b/dom/media/systemservices/MediaUtils.h @@ -173,7 +173,7 @@ private: * RefPtr bar = new Bar(); * NS_DispatchToMainThread(media::NewRunnableFrom([bar]() mutable { * // use bar - * }); + * })); * } * * Capture is by-copy by default, so the nsRefPtr 'bar' is safely copied for diff --git a/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp b/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp index 1ece4c8170f0..d59e9434ced1 100644 --- a/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp +++ b/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ -751,7 +751,7 @@ WebrtcAudioConduit::GetAudioFrame(int16_t speechData[], // Transport Layer Callbacks MediaConduitErrorCode -WebrtcAudioConduit::ReceivedRTPPacket(const void *data, int len) +WebrtcAudioConduit::ReceivedRTPPacket(const void *data, int len, uint32_t ssrc) { CSFLogDebug(logTag, "%s : channel %d", __FUNCTION__, mChannel); diff --git a/media/webrtc/signaling/src/media-conduit/AudioConduit.h b/media/webrtc/signaling/src/media-conduit/AudioConduit.h index 4f435747a84a..06315d4f7b52 100755 --- a/media/webrtc/signaling/src/media-conduit/AudioConduit.h +++ b/media/webrtc/signaling/src/media-conduit/AudioConduit.h @@ -57,7 +57,7 @@ public: * APIs used by the registered external transport to this Conduit to * feed in received RTP Frames to the VoiceEngine for decoding */ - virtual MediaConduitErrorCode ReceivedRTPPacket(const void *data, int len) override; + virtual MediaConduitErrorCode ReceivedRTPPacket(const void *data, int len, uint32_t ssrc) override; /** * APIs used by the registered external transport to this Conduit to @@ -163,6 +163,7 @@ public: size_t len) override; virtual uint64_t CodecPluginID() override { return 0; } + virtual void SetPCHandle(const std::string& aPCHandle) {} explicit WebrtcAudioConduit(): mVoiceEngine(nullptr), diff --git a/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h b/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h index c00ec6644133..b1094f9de9d2 100755 --- a/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h +++ b/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ -196,7 +196,7 @@ public: * Obtained packets are passed to the Media-Engine for further * processing , say, decoding */ - virtual MediaConduitErrorCode ReceivedRTPPacket(const void *data, int len) = 0; + virtual MediaConduitErrorCode ReceivedRTPPacket(const void *data, int len, uint32_t ssrc) = 0; /** * Function triggered on Incoming RTCP packet from the remote @@ -278,6 +278,8 @@ public: virtual uint64_t CodecPluginID() = 0; + virtual void SetPCHandle(const std::string& aPCHandle) = 0; + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaSessionConduit) }; diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp index 69a78e06aa5b..268c00148a35 100755 --- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp +++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ -198,6 +198,8 @@ WebrtcVideoConduit::WebrtcVideoConduit(RefPtr aCall) , mSendStreamConfig(this) // 'this' is stored but not dereferenced in the constructor. , mRecvStream(nullptr) , mRecvStreamConfig(this) // 'this' is stored but not dereferenced in the constructor. + , mRecvSSRCSet(false) + , mRecvSSRCSetInProgress(false) , mSendCodecPlugin(nullptr) , mRecvCodecPlugin(nullptr) , mVideoStatsTimer(do_CreateInstance(NS_TIMER_CONTRACTID)) @@ -682,11 +684,12 @@ bool WebrtcVideoConduit::SetRemoteSSRC(unsigned int ssrc) { mRecvStreamConfig.rtp.remote_ssrc = ssrc; - unsigned int current_ssrc; + unsigned int current_ssrc; if (!GetRemoteSSRC(¤t_ssrc)) { return false; } + mRecvSSRCSet = true; if (current_ssrc == ssrc || !mEngineReceiving) { return true; @@ -1137,6 +1140,20 @@ WebrtcVideoConduit::ConfigureRecvMediaCodecs( mRecvStreamConfig.rtp.fec.red_rtx_payload_type = -1; } + if (!mRecvSSRCSet) { + // Handle un-signalled SSRCs by creating a random one and then when it actually gets set, + // we'll destroy and recreate. Simpler than trying to unwind all the logic that assumes + // the receive stream is created and started when we ConfigureRecvMediaCodecs() + unsigned int ssrc; + do { + SECStatus rv = PK11_GenerateRandom(reinterpret_cast(&ssrc), sizeof(ssrc)); + if (rv != SECSuccess) { + return kMediaConduitUnknownError; + } + } while (ssrc == 0); // webrtc.org code has fits if you select an SSRC of 0 + + mRecvStreamConfig.rtp.remote_ssrc = ssrc; + } // FIXME(jesup) - Bug 1325447 -- SSRCs configured here are a problem. // 0 isn't allowed. Would be best to ask for a random SSRC from the RTP code. // Would need to call rtp_sender.cc -- GenerateSSRC(), which isn't exposed. It's called on @@ -1149,7 +1166,8 @@ WebrtcVideoConduit::ConfigureRecvMediaCodecs( if (rv != SECSuccess) { return kMediaConduitUnknownError; } - } while (ssrc == mRecvStreamConfig.rtp.remote_ssrc); + } while (ssrc == mRecvStreamConfig.rtp.remote_ssrc || ssrc == 0); + // webrtc.org code has fits if you select an SSRC of 0 mRecvStreamConfig.rtp.local_ssrc = ssrc; @@ -1157,8 +1175,8 @@ WebrtcVideoConduit::ConfigureRecvMediaCodecs( mRecvCodecList.SwapElements(recv_codecs); recv_codecs.Clear(); mRecvStreamConfig.rtp.rtx.clear(); - // Rebuilds mRecvStream from mRecvStreamConfig DeleteRecvStream(); + // Rebuilds mRecvStream from mRecvStreamConfig MediaConduitErrorCode rval = CreateRecvStream(); if (rval != kMediaConduitNoError) { CSFLogError(logTag, "%s Start Receive Error %d ", __FUNCTION__, rval); @@ -1734,8 +1752,61 @@ WebrtcVideoConduit::DeliverPacket(const void* data, int len) } MediaConduitErrorCode -WebrtcVideoConduit::ReceivedRTPPacket(const void* data, int len) +WebrtcVideoConduit::ReceivedRTPPacket(const void* data, int len, uint32_t ssrc) { + bool queue = mRecvSSRCSetInProgress; + if (!mRecvSSRCSet && !mRecvSSRCSetInProgress) { + mRecvSSRCSetInProgress = true; + queue = true; + // Handle the ssrc-not-signaled case; lock onto first ssrc + // We can't just do this here; it has to happen on MainThread :-( + // We also don't want to drop the packet, nor stall this thread, so we hold + // the packet (and any following) for inserting once the SSRC is set. + + // Ensure lamba captures refs + RefPtr self = this; + nsCOMPtr thread; + if (NS_WARN_IF(NS_FAILED(NS_GetCurrentThread(getter_AddRefs(thread))))) { + return kMediaConduitRTPProcessingFailed; + } + NS_DispatchToMainThread(media::NewRunnableFrom([self, thread, ssrc]() mutable { + // Normally this is done in CreateOrUpdateMediaPipeline() for + // initial creation and renegotiation, but here we're rebuilding the + // Receive channel at a lower level. This is needed whenever we're + // creating a GMPVideoCodec (in particular, H264) so it can communicate + // errors to the PC. + WebrtcGmpPCHandleSetter setter(self->mPCHandle); + self->SetRemoteSSRC(ssrc); // this will likely re-create the VideoReceiveStream + // We want to unblock the queued packets on the original thread + thread->Dispatch(media::NewRunnableFrom([self]() mutable { + self->mRecvSSRCSetInProgress = false; + // SSRC is set; insert queued packets + for (auto& packet : self->mQueuedPackets) { + CSFLogDebug(logTag, "%s: seq# %u, Len %d ", __FUNCTION__, + (uint16_t)ntohs(((uint16_t*) packet->mData)[1]), packet->mLen); + + if (self->DeliverPacket(packet->mData, packet->mLen) != kMediaConduitNoError) { + CSFLogError(logTag, "%s RTP Processing Failed", __FUNCTION__); + // Keep delivering and then clear the queue + } + } + self->mQueuedPackets.Clear(); + + return NS_OK; + }), NS_DISPATCH_NORMAL); + return NS_OK; + })); + // we'll return after queuing + } + if (queue) { + // capture packet for insertion after ssrc is set + UniquePtr packet((QueuedPacket*) malloc(sizeof(QueuedPacket) + len-1)); + packet->mLen = len; + memcpy(packet->mData, data, len); + mQueuedPackets.AppendElement(Move(packet)); + return kMediaConduitNoError; + } + CSFLogDebug(logTag, "%s: seq# %u, Len %d ", __FUNCTION__, (uint16_t)ntohs(((uint16_t*) data)[1]), len); diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.h b/media/webrtc/signaling/src/media-conduit/VideoConduit.h index be61a560942d..ee00ba29ff06 100755 --- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h +++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h @@ -103,7 +103,7 @@ public: * APIs used by the registered external transport to this Conduit to * feed in received RTP Frames to the VideoEngine for decoding */ - virtual MediaConduitErrorCode ReceivedRTPPacket(const void* data, int len) override; + virtual MediaConduitErrorCode ReceivedRTPPacket(const void* data, int len, uint32_t ssrc) override; /** * APIs used by the registered external transport to this Conduit to @@ -262,6 +262,10 @@ public: virtual uint64_t CodecPluginID() override; + virtual void SetPCHandle(const std::string& aPCHandle) override { + mPCHandle = aPCHandle; + } + unsigned short SendingWidth() override { return mSendingWidth; } @@ -469,6 +473,7 @@ private: unsigned short mNumReceivingStreams; bool mVideoLatencyTestEnable; uint64_t mVideoLatencyAvg; + // all in bps! int mMinBitrate; int mStartBitrate; int mPrefMaxBitrate; @@ -496,6 +501,17 @@ private: webrtc::VideoReceiveStream* mRecvStream; // Must call webrtc::Call::DestroyVideoReceiveStream to delete webrtc::VideoReceiveStream::Config mRecvStreamConfig; + // We can't create mRecvStream without knowing the remote SSRC + // Atomic since we key off this on packet insertion, which happens + // on a different thread. + Atomic mRecvSSRCSet; + // The runnable to set the SSRC is in-flight; queue packets until it's done. + bool mRecvSSRCSetInProgress; + struct QueuedPacket { + int mLen; + uint8_t mData[1]; + }; + nsTArray> mQueuedPackets; // The lifetime of these codecs are maintained by the VideoConduit instance. // They are passed to the webrtc::VideoSendStream or VideoReceiveStream, @@ -508,6 +524,8 @@ private: nsCOMPtr mVideoStatsTimer; SendStreamStatistics mSendStreamStats; ReceiveStreamStatistics mRecvStreamStats; + + std::string mPCHandle; }; } // end namespace diff --git a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp index a34124ea88e6..2834516915c1 100644 --- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp +++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ -1071,13 +1071,12 @@ void MediaPipeline::RtpPacketReceived(TransportLayer *layer, MOZ_MTLOG(ML_NOTICE, "Error unprotecting RTP in " << description_ << "len= " << len << "[" << tmp << "...]"); - return; } MOZ_MTLOG(ML_DEBUG, description_ << " received RTP packet."); increment_rtp_packets_received(out_len); - (void)conduit_->ReceivedRTPPacket(inner_data.get(), out_len); // Ignore error codes + (void)conduit_->ReceivedRTPPacket(inner_data.get(), out_len, header.ssrc); // Ignore error codes } void MediaPipeline::RtcpPacketReceived(TransportLayer *layer, diff --git a/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp b/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp index 819ccdd3ddce..a7654a1b9e03 100644 --- a/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp +++ b/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ -446,6 +446,7 @@ MediaPipelineFactory::CreateOrUpdateMediaPipeline( if (NS_FAILED(rv)) { return rv; } + conduit->SetPCHandle(mPC->GetHandle()); } else { // We've created the TransportFlow, nothing else to do here. return NS_OK; @@ -839,11 +840,9 @@ MediaPipelineFactory::GetOrCreateVideoConduit( // NOTE(pkerr) - this is new behavior. Needed because the CreateVideoReceiveStream // method of the Call API will assert (in debug) and fail if a value is not provided // for the remote_ssrc that will be used by the far-end sender. - if (ssrcs->empty()) { - MOZ_MTLOG(ML_ERROR, "No SSRC set for receive track"); - return NS_ERROR_FAILURE; + if (!ssrcs->empty()) { + conduit->SetRemoteSSRC(ssrcs->front()); } - conduit->SetRemoteSSRC(ssrcs->front()); if (!extmaps.empty()) { conduit->AddLocalRTPExtensions(false, extmaps);