Closed
Bug 1363667
Opened 7 years ago
Closed 6 years ago
Add getContributingSources and getSynchronizationSources to RTCRtpReceiver
Categories
(Core :: WebRTC, enhancement, P2)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: ng, Assigned: ng)
References
(Blocks 5 open bugs)
Details
(Keywords: dev-doc-needed)
Attachments
(10 files, 4 obsolete files)
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mjf
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
There are new methods related to the sources and contributing sources on RTCRtpReceiver and we should implement them and their associated objects. Implementing new methods on RTCRtpReceiver: * getContributingSources [1] * getSynchronizationSources [2] These new methods are expected to be called at a high frequency and special care should be taken to insure that they are performant. [3] Implementing new objects: * RTCRtpContributingSource [4] * RTCRtpSynchronizationSource [5] [1] https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources [2] https://w3c.github.io/webrtc-pc/#dom-rtcrtpsynchronizationsource [3] https://github.com/w3c/webrtc-pc/pull/1149#issuecomment-29 [4] https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource [5] https://w3c.github.io/webrtc-pc/#dom-rtcrtpsynchronizationsource
Updated•7 years ago
|
Rank: 25
Priority: -- → P2
Comment 1•6 years ago
|
||
This has become high priority, as a major meeting site can use this feature to improve performance with 20+ participants.
Rank: 25 → 14
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
See also https://github.com/w3c/webrtc-pc/issues/1533.
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8897185 [details] Bug 1363667 - P1 - Add Contrib/Sync sources webidl; https://reviewboard.mozilla.org/r/168478/#review173730 Lgtm, but make sure to get a DOM reviewer as well.
Attachment #8897185 -
Flags: review?(jib) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8899916 -
Flags: review?(jib) → review?(mfroman)
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8899916 [details] Bug 1363667 - P2 - adding csrc-audio-level rtp header ext; https://reviewboard.mozilla.org/r/171238/#review176926 r+ after fixing nits. ::: media/webrtc/trunk/webrtc/common_types.h:903 (Diff revision 1) > // TODO(danilchap): Update url from draft to release version. > StreamId rtpStreamId; > StreamId repairedRtpStreamId; > > StreamId mId; > + CsrcAudioLevelList csrcAudioLevels; Probably need to initialize numAudioLevels to zero, or provide a default constructor to initialize to zero. ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:159 (Diff revision 1) > > +class CsrcAudioLevel { > + public: > + static constexpr RTPExtensionType kId = kRtpExtensionCsrcAudioLevel; > + static constexpr const char* kUri = > + "urn:ietf:params:rtp-hdrext:Csrc-audio-level"; This doesn't quite match ("Csrc" vs "csrc") the uri defined in config.cc ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:352 (Diff revision 1) > +bool CsrcAudioLevel::Write(uint8_t* data, > + const CsrcAudioLevelList& csrcAudioLevels) { > + for(uint8_t i = 0; i < csrcAudioLevels.numAudioLevels; i++) { > + data[i] = csrcAudioLevels.arrOfAudioLevels[i] & 0x7f; > + } > + return csrcAudioLevels.numAudioLevels; A comment here explaining that parsing an empty CsrcAudioLevel isn't allowed, so writing an empty CsrcAudioLevel should fail would be nice. Providing I'm interpretting this correctly. ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:486 (Diff revision 1) > 0x00, 0x00}; // Extension padding. > + const uint8_t kPacket4[] = { > + 0x90, kPayloadType, kSeqNumFirstByte, kSeqNumSecondByte, > + 0x65, 0x43, 0x12, 0x78, // Timestamp. > + 0x12, 0x34, 0x56, 0x78, // Ssrc. > + 0xbe, 0xde, 0x00, 0x05, // Extensions block of size 6x32bit words. Shouldn't the comment read 5x32bit? ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:491 (Diff revision 1) > + 0xbe, 0xde, 0x00, 0x05, // Extensions block of size 6x32bit words. > + 0x21, 'H', 'D', // Extension with id = 2, size = (1+1). > + 0x12, 'r', 't', 'x', // Extension with id = 1, size = (2+1). > + 0x35, 'm', 'i', 'd', 'v', 'a', 'l', // Extension with id = 3, size = (5+1). > + 0x43, 0x01, 0x10, 0x00, // Extenstionwith id = 4, size = (3+1) > + 0x7f, 0x00}; // Extension 4 cont. 2 bytes of padding. 1 byte of padding ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility_unittest.cc:151 (Diff revision 1) > // Parse should ignore extension. > EXPECT_FALSE(header.extension.hasTransmissionTimeOffset); > EXPECT_EQ(sizeof(kPacket), header.headerLength); > } > > TEST(RtpHeaderParser, ParseAll9Extensions) { ParseAllTenExtensions? ParseAll10Extensions really, but that seems difficult to read.
Attachment #8899916 -
Flags: review?(mfroman) → review+
Comment 8•6 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8911004 [details] Bug 1363667 - P3 - JS impl of getContributingSources; https://reviewboard.mozilla.org/r/182474/#review188040 Looks good after fixing nits. ::: dom/media/PeerConnection.js:1699 (Diff revision 1) > contractID: PC_DTMF_SENDER_CONTRACT, > QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports]) > }); > > +class RTCRtpContributingSource { > + // TODO construct from RTCRtpContributingSourceEntry Still TODO? ::: dom/media/PeerConnection.js:1701 (Diff revision 1) > }); > > +class RTCRtpContributingSource { > + // TODO construct from RTCRtpContributingSourceEntry > + constructor(pc, entry, receiver) { > + dump("@@NG constructor called \n"); This looks like debug logging. Did you mean to leave this? ::: dom/media/PeerConnection.js:1717 (Diff revision 1) > + > + get source() { > + this._entry.source(); > + } > + > + get timestamp () { It may be worth a comment here to note that calls to _fetch() here are for keeping a live object updated (vs lazily populating the fields). ::: dom/media/PeerConnection.js:1798 (Diff revision 1) > async () => this._pc.getStats(this.track)); > } > + > + getContributingSources() { > + this._fetchContributingSources(); > + let cutoffTime = new Date().getTime() - 10 * 1000; Is it worth mentioning where the 10 seconds comes from? Or that the spec requires keeping things in the cache for that length of time?
Attachment #8911004 -
Flags: review?(mfroman) → review+
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8913868 [details] Bug 1363667 - P4 - adding rtp audio level observer; https://reviewboard.mozilla.org/r/185268/#review191056 ::: media/webrtc/signaling/src/media-conduit/AudioLevelObserver.cpp:17 (Diff revision 1) > +AudioLevelObserver::OnRtpCsrcAudioLevels(const webrtc::WebRtcRTPHeader* aHeader) > +{ > + MutexAutoLock lock(mLevelGuard); > + auto& header = aHeader->header; > + auto& list = header.extension.csrcAudioLevels; > + uint8_t num = std::min(header.numCSRCs, list.numAudioLevels); When these numbers are different, how are the CSRCs and audio levels matched up with each other? ::: media/webrtc/signaling/src/media-conduit/AudioLevelObserver.cpp:20 (Diff revision 1) > + auto& header = aHeader->header; > + auto& list = header.extension.csrcAudioLevels; > + uint8_t num = std::min(header.numCSRCs, list.numAudioLevels); > + for (uint8_t i = 0; i < num; num++) { > + uint32_t csrc = header.arrOfCSRCs[i]; > + mCsrcAudioLevels[csrc].Update(csrc, Is there a possibility for the array of CSRCs to change size between calls? If so, can there be fewer, or changed CSRCs such that the "old" CSRCs would remain in the mCsrcAudioLevels? ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:762 (Diff revision 1) > if (error) { > MOZ_MTLOG(ML_ERROR, "EnableAudioLevelExtension failed: " << error); > return NS_ERROR_FAILURE; > } > } > + // TODO @@NG Ref conduit->GetAudioLevelObserver(); Did you mean to leave this TODO? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:638 (Diff revision 1) > static std::string GetStreamId(const DOMMediaStream& aStream); > static std::string GetTrackId(const dom::MediaStreamTrack& track); > > void OnMediaError(const std::string& aError); > > + void RegisterAudioLevelObserver(const JsepTrack& aTrack, When/who calls this? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:825 (Diff revision 1) > > static void > DTMFSendTimerCallback_m(nsITimer* timer, void*); > > nsTArray<DTMFState> mDTMFStates; > + Extra spaces. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2607 (Diff revision 1) > + { > + MutexAutoLock lock(mContributingSourcesGuard); > + const auto& it = mAudioLevelObservers.find(trackId); > + if (it == mAudioLevelObservers.end()) { > + NS_WARNING("No audio level observer registered for track!"); > + return NS_OK; Should outContributingSources be cleared even if no observer is found? In the case of not finding an observer is NS_OK the right thing to return?
Assignee | ||
Comment 17•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8913868 [details] Bug 1363667 - P4 - adding rtp audio level observer; https://reviewboard.mozilla.org/r/185268/#review191056 > When these numbers are different, how are the CSRCs and audio levels matched up with each other? The lists are in the same order. It is expected that the number of reported audio levels can be less than the number of CSRCs, but the inverse should never be true. Adding a sanity check. > Is there a possibility for the array of CSRCs to change size between calls? If so, can there be fewer, or changed CSRCs such that the "old" CSRCs would remain in the mCsrcAudioLevels? Yes, we need to keep a history of the last values associated with eash CSRC within a 10 second window. That window was enforced in JS, but I am going to do that here as well. > When/who calls this? Fixed. > Should outContributingSources be cleared even if no observer is found? In the case of not finding an observer is NS_OK the right thing to return? If no observer is found, it is because the track is a video track, in that case the proper thing to do is return an empty list.
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8913868 [details] Bug 1363667 - P4 - adding rtp audio level observer; https://reviewboard.mozilla.org/r/185268/#review191614 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2586 (Diff revision 2) > + const JsepTrack& aTrack, > + RefPtr<AudioLevelObserver> aObserver) > +{ > + { > + MutexAutoLock lock(mAudioLevelGuard); > + mAudioLevelObservers[aTrack.GetTrackId()] = aObserver; Warning: Parameter 'aobserver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8913868 [details] Bug 1363667 - P4 - adding rtp audio level observer; https://reviewboard.mozilla.org/r/185268/#review191620 r+ after fixing nits. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.h:329 (Diff revision 2) > uint32_t mLastTimestamp; > > uint32_t mSamples; > uint32_t mLastSyncLog; > + > + RefPtr<AudioLevelObserver> mAudioLevelObserver; I don't see this being assigned anywhere. ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:726 (Diff revision 2) > if (!conduit->SetLocalSSRCs(std::vector<unsigned int>(1,aTrackPair.mRecvonlySsrc))) { > MOZ_MTLOG(ML_ERROR, "SetLocalSSRC failed"); > return NS_ERROR_FAILURE; > } > } > + conduit->GetAudioLevelObserver(); This statement isn't doing anything at the moment.
Attachment #8913868 -
Flags: review?(mfroman) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8913868 [details] Bug 1363667 - P4 - adding rtp audio level observer; https://reviewboard.mozilla.org/r/185268/#review191688 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2697 (Diff revision 3) > + const JsepTrack& aTrack, > + RefPtr<AudioLevelObserver> aObserver) > +{ > + { > + MutexAutoLock lock(mAudioLevelGuard); > + mAudioLevelObservers[aTrack.GetTrackId()] = aObserver; Warning: Parameter 'aobserver' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54915d931fdf7cae518a315b8218cf6008688b42
Assignee | ||
Comment 30•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=762bb2ab863081390031003592bd7a081715fc0d
Assignee | ||
Comment 31•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a0d1080a24b93499f8d389652e399154074d18
Assignee | ||
Comment 32•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1480b98707fd2d24617443be4b8178780b9a17be
Assignee | ||
Comment 33•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=156fb505efacf12bc57539ccd7468d1064fe720a
Assignee | ||
Comment 34•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23d836d2337849b1ca74f86d0aee99a44f7f1bb
Assignee | ||
Comment 35•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fb26c3048210df020a3c260802021b39fcef04e
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8897185 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8899916 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8911004 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8913868 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8928238 -
Flags: review?(mfroman)
Attachment #8928239 -
Flags: review?(mfroman)
Attachment #8928240 -
Flags: review?(mfroman)
Attachment #8928241 -
Flags: review?(mfroman)
Attachment #8928242 -
Flags: review?(mfroman)
Attachment #8928243 -
Flags: review?(mfroman)
Attachment #8928244 -
Flags: review?(mfroman)
Comment 43•6 years ago
|
||
mozreview-review |
Comment on attachment 8928239 [details] Bug 1363667 - P2 - Add RTP Sources RTP header extensions https://reviewboard.mozilla.org/r/199460/#review204566 ::: media/webrtc/signaling/src/media-conduit/AudioConduit.h:96 (Diff revision 1) > * transmission sub-system on the engine. > */ > virtual MediaConduitErrorCode ConfigureRecvMediaCodecs( > const std::vector<AudioCodecConfig* >& codecConfigList) override; > - /** > - * Function to enable the audio level extension > + > + MediaConduitErrorCode Did you purposely remove the comment here? ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:724 (Diff revision 1) > + const SdpExtmapAttributeList::Extmap* audioLevelExt = > + aTrack.GetNegotiatedDetails()->GetExt( > + webrtc::RtpExtension::kAudioLevelUri); > + if (audioLevelExt && (audioLevelExt->direction & sdp::kSend)) { > + MOZ_MTLOG(ML_DEBUG, "Calling EnableAudioLevelExtension dir=send"); > + error = conduit->EnableAudioLevelExtension(true, audioLevelExt->entry, true); > + > + if (error) { > + MOZ_MTLOG(ML_ERROR, "EnableAudioLevelExtension failed: " << error); > + return NS_ERROR_FAILURE; > + } > + } > + > + if (audioLevelExt && (audioLevelExt->direction & sdp::kRecv)) { > + MOZ_MTLOG(ML_DEBUG, "Calling EnableAudioLevelExtension dir=recv"); > + error = conduit->EnableAudioLevelExtension(true, > + audioLevelExt->entry, > + false); > + if (error) { > + MOZ_MTLOG(ML_ERROR, "EnableAudioLevelExtension failed: " << error); > + return NS_ERROR_FAILURE; > + } > + } Now that you've changed RtpExtension::kAudioLevelUri to sendrecv, I think this section and the below in the else clause can be moved out and done in one location instead of the same code in the "if (receiving)" clause and the else clause.
Attachment #8928239 -
Flags: review?(mfroman) → review+
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8928239 [details] Bug 1363667 - P2 - Add RTP Sources RTP header extensions https://reviewboard.mozilla.org/r/199460/#review204576 r+ w/ nits.
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8928238 [details] Bug 1363667 - P1 - Add RTP Sources WebIDL https://reviewboard.mozilla.org/r/199458/#review204578 Looks good to me, but I think you'll need a dom peer review here.
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8928240 [details] Bug 1363667 - P3 - RTP Source Observer https://reviewboard.mozilla.org/r/199462/#review204580 r+ w/ nits ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h:544 (Diff revision 1) > virtual bool SetDtmfPayloadType(unsigned char type, int freq) = 0; > > virtual bool InsertDTMFTone(int channel, int eventCode, bool outOfBand, > int lengthMs, int attenuationDb) = 0; > + /** > + * Returns a reference to an AudioLevelObserver which observes Did you mean s/AudioLevelObserver/RtpSourceObserver/ ? ::: media/webrtc/signaling/src/media-conduit/RtpSourceObserver.cpp:72 (Diff revision 1) > + > +const RtpSourceObserver::RtpSourceEntry* > +RtpSourceObserver::RtpSourceHistory::FindClosestNotAfter(int64_t aTime) const { > + auto lastFound = mDetailedHistory.cbegin(); > + bool found = false; > + for (const auto& it : mDetailedHistory) { Would you mind adding a comment here to give a little detail on what is really happening here?
Attachment #8928240 -
Flags: review?(mfroman) → review+
Comment 47•6 years ago
|
||
mozreview-review |
Comment on attachment 8928241 [details] Bug 1363667 - P4 - RTP Source Observer unit tests https://reviewboard.mozilla.org/r/199464/#review204582 Looks good to me.
Attachment #8928241 -
Flags: review?(mfroman) → review+
Comment 48•6 years ago
|
||
mozreview-review |
Comment on attachment 8928242 [details] Bug 1363667 - P5 - RTP Source PeerConnection JS IF https://reviewboard.mozilla.org/r/199466/#review204586 ::: dom/webidl/PeerConnectionImpl.webidl:54 (Diff revision 1) > optional unsigned long duration = 100, > optional unsigned long interToneGap = 70); > [Throws] > DOMString getDTMFToneBuffer(RTCRtpSender sender); > + sequence<RTCRtpSourceEntry> > + getRtpSources(MediaStreamTrack track, DOMHighResTimeStamp rtpSourceNow); Weird indenting thing or a rendering issue in mozreview?
Comment 49•6 years ago
|
||
mozreview-review |
Comment on attachment 8928242 [details] Bug 1363667 - P5 - RTP Source PeerConnection JS IF https://reviewboard.mozilla.org/r/199466/#review204588 1 possible nit, but otherwise fine excepting the dom peer review.
Comment 50•6 years ago
|
||
mozreview-review |
Comment on attachment 8928243 [details] Bug 1363667 - P6 - RTP Source PeerConnection JS impl https://reviewboard.mozilla.org/r/199468/#review204592 r+ w/ nits ::: dom/media/PeerConnection.js:1816 (Diff revision 1) > + this._fetchRtpSources(); > + return this._rtpSources.get(type + source).entry; Leading whitespace has a tab. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:469 (Diff revision 1) > > nsresult > GetParameters(dom::MediaStreamTrack& aTrack, > std::vector<JsepTrack::JsConstraints>* aOutConstraints); > > + // test-onlg: called from contributing sources mochitests. s/test-onlg/test-only/
Attachment #8928243 -
Flags: review?(mfroman) → review+
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8928244 [details] Bug 1363667 - P7 - RTP Source mochitests https://reviewboard.mozilla.org/r/199470/#review204598 Looks good to me.
Attachment #8928244 -
Flags: review?(mfroman) → review+
Assignee | ||
Comment 52•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8928240 [details] Bug 1363667 - P3 - RTP Source Observer https://reviewboard.mozilla.org/r/199462/#review204580 > Would you mind adding a comment here to give a little detail on what is really happening here? This method scans the history for the entry whose timestamp is closest too a given timestamp but no greater. Because it is scanning forward, it keeps track of the closest entry it has found so far in case it overshoots. There is no before map.begin() which complicates things, and I have to track if something was really found. If that didn't answer the question, let me know.
Comment 53•6 years ago
|
||
mozreview-review |
Comment on attachment 8928238 [details] Bug 1363667 - P1 - Add RTP Sources WebIDL https://reviewboard.mozilla.org/r/199458/#review204632
Attachment #8928238 -
Flags: review?(mfroman) → review+
Comment 54•6 years ago
|
||
mozreview-review |
Comment on attachment 8928242 [details] Bug 1363667 - P5 - RTP Source PeerConnection JS IF https://reviewboard.mozilla.org/r/199466/#review204634
Attachment #8928242 -
Flags: review?(mfroman) → review+
Comment 55•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8928240 [details] Bug 1363667 - P3 - RTP Source Observer https://reviewboard.mozilla.org/r/199462/#review204580 > This method scans the history for the entry whose timestamp is closest too a given timestamp but no greater. Because it is scanning forward, it keeps track of the closest entry it has found so far in case it overshoots. There is no before map.begin() which complicates things, and I have to track if something was really found. If that didn't answer the question, let me know. This bit from above is exactly what I would have wished to see as the comment: Because it is scanning forward, it keeps track of the closest entry it has found so far in case it overshoots. There is no before map.begin() which complicates things, and I have to track if something was really found. Makes perfect sense!
Assignee | ||
Comment 56•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab369d23d2fc8c724da9d3ff1f779142b0652324
Assignee | ||
Comment 57•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e0e4f2c613488c2aa7c495a08b530bc8a0a7b9
Assignee | ||
Comment 58•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fcfded0298449fe3b77676dcbf54c9928dadb3d
Assignee | ||
Comment 59•6 years ago
|
||
WebIDL review information: These patches add the methods getContributingSources[0] and getSynchronizationSources[1] to RTCRtpReceiver[2] and associated RTCRtpContributingSource[3] and RTCRtpSynchronizationSources[4] dictionaries. Note: that a change was approved at the latest TPAC though a PR has not yet been submitted. This change proposes to make RTCRtpContributingSource and RTCRtpSynchronizationSources dictionaries instead of interfaces. We do not anticipate any changes to the fields. Note: we do not currently support the voiceActivityFlag. Note: there is an error in the spec WebIDL for RTCRtpSynchronizationSources where the audioLevel is not annotated as mutable. [0] https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources [1] https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getsynchronizationsources [2] https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver [3] https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource [4] https://w3c.github.io/webrtc-pc/#dom-rtcrtpsynchronizationsource
Assignee | ||
Comment 60•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd359462f35e81a7e3ed0dea27d778b139fade4f
Assignee | ||
Comment 61•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=270b7284ecabd7aa31e4d6ca49429316fc39ae98
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8929667 -
Flags: review?(mfroman)
Comment 69•6 years ago
|
||
mozreview-review |
Comment on attachment 8928238 [details] Bug 1363667 - P1 - Add RTP Sources WebIDL https://reviewboard.mozilla.org/r/199458/#review206396 ::: dom/webidl/RTCRtpReceiver.webidl:7 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. > * > * The origin of this IDL file is > * http://lists.w3.org/Archives/Public/public-webrtc/2014May/0067.html Please update this to the current draft spec: https://w3c.github.io/webrtc-pc/#rtcrtpreceiver-interface ::: dom/webidl/RTCRtpSources.webidl:7 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.html This also doesn't seem like the right spec URL. ::: dom/webidl/RTCRtpSources.webidl:10 (Diff revision 1) > + * > + * The origin of this IDL file is > + * https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.html > + */ > + > +dictionary RTCRtpContributingSource { The spec says these should be interfaces and not dictionaries. I believe this is an observable difference. https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource ::: dom/webidl/RTCRtpSources.webidl:16 (Diff revision 1) > + DOMHighResTimeStamp timestamp; > + unsigned long source; > + byte? audioLevel; > +}; > + > +dictionary RTCRtpSynchronizationSource { Same here, the spec says this should be an interface. https://w3c.github.io/webrtc-pc/#dom-rtcrtpsynchronizationsource
Comment 70•6 years ago
|
||
mozreview-review |
Comment on attachment 8928242 [details] Bug 1363667 - P5 - RTP Source PeerConnection JS IF https://reviewboard.mozilla.org/r/199466/#review206398 Looks like chrome-only webidl changes. r=me for DOM webidl review.
Attachment #8928242 -
Flags: review+
Comment 71•6 years ago
|
||
mozreview-review |
Comment on attachment 8928238 [details] Bug 1363667 - P1 - Add RTP Sources WebIDL https://reviewboard.mozilla.org/r/199458/#review206400 I've been informed that the interfaces were changed to dictionaries at TPAC. Please link to the spec issue in a webidl comment. r=me with comments addressed. Thanks.
Attachment #8928238 -
Flags: review+
Assignee | ||
Comment 72•6 years ago
|
||
mozreview-review |
Comment on attachment 8928238 [details] Bug 1363667 - P1 - Add RTP Sources WebIDL https://reviewboard.mozilla.org/r/199458/#review206406 ::: dom/webidl/RTCRtpSources.webidl:16 (Diff revision 1) > + DOMHighResTimeStamp timestamp; > + unsigned long source; > + byte? audioLevel; > +}; > + > +dictionary RTCRtpSynchronizationSource { A comment has been added addressing this.
Comment 73•6 years ago
|
||
mozreview-review |
Comment on attachment 8929667 [details] Bug 1363667 - P7.1 - await on sync sources in mochitest https://reviewboard.mozilla.org/r/200944/#review206444 r+ w/ nit. ::: dom/media/tests/mochitest/test_peerConnection_audioSynchronizationSources.html:25 (Diff revision 1) > + receivers[1].getSynchronizationSources().length > 0) { > + break; > + } > + await wait(250); > + } > + } Blank line to separate functions?
Attachment #8929667 -
Flags: review?(mfroman) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 82•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53fb1cb8d6ede3e6f7602f2a2d15beee61c8d2d7
Assignee | ||
Comment 83•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dde1c814c13ecfc9cab9d7164fa8af2eab548b5
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 86•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1345b16b1ba682942859d5a82f164ac536fa3a01
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 95•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c80d4aeb5bbd7caac0f97944ca0028bce62b26e
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8930722 -
Flags: review?(mfroman)
Comment 97•6 years ago
|
||
mozreview-review |
Comment on attachment 8930722 [details] Bug 1363667 - P2.1 - Fix jsep extmap unit test https://reviewboard.mozilla.org/r/201838/#review207130 Looks good to me.
Attachment #8930722 -
Flags: review?(mfroman) → review+
Assignee | ||
Comment 98•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18e105fd411592ecaedbfb6cb54b17a53ef20af4
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8931104 -
Flags: review?(jib)
Assignee | ||
Updated•6 years ago
|
Attachment #8931104 -
Flags: review?(jib) → review?(drno)
Comment 100•6 years ago
|
||
mozreview-review |
Comment on attachment 8931104 [details] Bug 1363667 - P6.1 - disable broken web platform tests https://reviewboard.mozilla.org/r/202192/#review207550
Attachment #8931104 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Attachment #8931104 -
Flags: review?(drno)
Assignee | ||
Comment 101•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d7e18889fc632cb4600e4a3c0f927d2bcca09d0
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 112•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3757f3fae08913f539a0ef0b105b0ac4d165c031
Comment hidden (mozreview-request) |
Assignee | ||
Comment 114•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d4a2e6e38f7f51d2f7e7a4fb636d0da7132dbc
Assignee | ||
Comment 115•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb745d92a0bb5c2b296a6af2e9e615c9fe016531
Comment hidden (mozreview-request) |
Assignee | ||
Comment 117•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1452361f70be288ec92297bdd2d33cb314e82d
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 118•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=041bea4d2d076ab4c43c3f695c78d7b2dc9ed51a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 129•6 years ago
|
||
mozreview-review |
Comment on attachment 8928240 [details] Bug 1363667 - P3 - RTP Source Observer https://reviewboard.mozilla.org/r/199462/#review209988 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2453 (Diff revision 6) > } > > +void > +PeerConnectionImpl::RegisterRtpSourceObserver( > + const JsepTrack& aTrack, > + RefPtr<RtpSourceObserver> aObserver) Warning: The parameter 'aobserver' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param] RefPtr<RtpSourceObserver> aObserver) ^ const &
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 138•6 years ago
|
||
mozreview-review |
Comment on attachment 8928240 [details] Bug 1363667 - P3 - RTP Source Observer https://reviewboard.mozilla.org/r/199462/#review210264 r+ w/ nits. ::: media/webrtc/signaling/src/media-conduit/RtpSourceObserver.h:51 (Diff revisions 4 - 7) > * Note: this takes jitter into account when calculating the window so > * the window is actually [time - jitter - 10 sec .. time - jitter] > */ > void > - GetRtpSources(int64_t aTimeNow, > + GetRtpSources(const int64_t aTimeNow, > nsTArray<dom::RTCRtpSourceEntry>& outLevels) const; Renamed outLevels to outSources in the cpp file, but not here.
Comment 139•6 years ago
|
||
mozreview-review |
Comment on attachment 8928243 [details] Bug 1363667 - P6 - RTP Source PeerConnection JS impl https://reviewboard.mozilla.org/r/199468/#review210266 r+ /w nits. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2251 (Diff revisions 4 - 7) > PeerConnectionImpl::GetNowInRtpSourceReferenceTime() > { > return RtpSourceObserver::NowInReportClockTime(); > } > > nsresult There was a comment here in a previous revision that called out the test-only nature of the method. Would you mind putting that back in here?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 158•6 years ago
|
||
Pushed by na-g@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/2339bb528429 P1 - Add RTP Sources WebIDL r=bkelly,mjf https://hg.mozilla.org/integration/autoland/rev/215b7b587368 P2 - Add RTP Sources RTP header extensions r=mjf https://hg.mozilla.org/integration/autoland/rev/8a7257832c9c P3 - RTP Source Observer r=mjf https://hg.mozilla.org/integration/autoland/rev/9a3a17011b69 P4 - RTP Source Observer unit tests r=mjf https://hg.mozilla.org/integration/autoland/rev/eba72c63dc51 P5 - RTP Source PeerConnection JS IF r=bkelly,mjf https://hg.mozilla.org/integration/autoland/rev/d491c0d62d49 P6 - RTP Source PeerConnection JS impl r=mjf https://hg.mozilla.org/integration/autoland/rev/2d9f44dbb9b7 P7 - RTP Source mochitests r=mjf https://hg.mozilla.org/integration/autoland/rev/aeff64058cbc P7.1 - await on sync sources in mochitest r=mjf https://hg.mozilla.org/integration/autoland/rev/4c370f0933af P2.1 - Fix jsep extmap unit test r=mjf https://hg.mozilla.org/integration/autoland/rev/e9a9d920b679 P6.1 - disable broken web platform tests r=jib
Comment 159•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2339bb528429 https://hg.mozilla.org/mozilla-central/rev/215b7b587368 https://hg.mozilla.org/mozilla-central/rev/8a7257832c9c https://hg.mozilla.org/mozilla-central/rev/9a3a17011b69 https://hg.mozilla.org/mozilla-central/rev/eba72c63dc51 https://hg.mozilla.org/mozilla-central/rev/d491c0d62d49 https://hg.mozilla.org/mozilla-central/rev/2d9f44dbb9b7 https://hg.mozilla.org/mozilla-central/rev/aeff64058cbc https://hg.mozilla.org/mozilla-central/rev/4c370f0933af https://hg.mozilla.org/mozilla-central/rev/e9a9d920b679
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 160•6 years ago
|
||
Hi, the Thunderbird sheriff here. Is it possible that this busted our Windows builds? https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=ede07b1d8a69a49eb1dca01c22c042951b666142 c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\mozilla\media\webrtc\signaling\src\media-conduit\AudioConduit.h(256): error C2220: warning treated as error - no 'object' file generated I saw the failure on try-comm-central before so I did a clobber before triggering a build on comm-central, but it didn't help.
Flags: needinfo?(na-g)
Flags: needinfo?(mfroman)
Flags: needinfo?(jib)
Assignee | ||
Comment 161•6 years ago
|
||
Jorg, yes, it looks like it. I am looking into this now.
Flags: needinfo?(na-g)
Flags: needinfo?(mfroman)
Flags: needinfo?(jib)
Comment 162•6 years ago
|
||
Looks like a const mismatch on this line: void GetRtpSources(const int64_t aTimeNow,
Assignee | ||
Comment 163•6 years ago
|
||
Jib, yes. I am about to push to try.
Assignee | ||
Comment 164•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14b3bf69cd7d88d4fbff561e34ceffe6e4c39b5d
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 165•6 years ago
|
||
Whoops, I shouldn't have reopened unless it has been backed out apparently.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Comment 166•6 years ago
|
||
Documentation work should be done, although I would appreciate a review (for which I'm ni'ing Nico). In the absence of review comments or changes, I will assume the documentation listed below is done for the purposes of this bug. Please let me know if you see any issues (or feel free to tweak things yourself!). The changes made follow: Pages created: https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpReceiver/getSynchronizationSources https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSynchronizationSource https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSynchronizationSource/voiceActivityFlag https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver (a very early draft) https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Intro_to_RTP (early draft) (the draft pages are incomplete but a review of the current content would be helpful since it's relevant) Pages updated: https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpReceiver/getContributingSources https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpContributingSource https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpContributingSource/source https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpContributingSource/timestamp https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpContributingSource/audioLevel (Updates to existing pages generally include both info about Firefox compatibility and improvements to overall quality, content, and so on) Pull Requests submitted: https://github.com/mdn/kumascript/pull/575 (adds missing interfaces to the database for WebRTC) And, finally, added a note to Firefox 59 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/59#Media_and_WebRTC
Flags: needinfo?(na-g)
Keywords: dev-doc-needed → dev-doc-complete
Comment 167•6 years ago
|
||
I'm reopening this from the doc's perspective. Sheppy, these are new pages, you should make a PR to add the compat data to the BCD repository. Thank you.
Keywords: dev-doc-complete → dev-doc-needed
Comment 168•6 years ago
|
||
Commit pushed to master at https://github.com/mdn/kumascript https://github.com/mdn/kumascript/commit/22e22756efebc52b0a3eec411be38ed9f0182fac Bug 1363667: add RTCRtpTransceiver, RTCRtpContributingSource, and RTCRtpSynchronizationSourc to WebRTC Group (#575) Adds three interfaces to the WebRTC API: RTCRtpTransceiver, RTCRtpContributingSource, and RTCRtpSynchronizationSource.
Assignee | ||
Comment 169•6 years ago
|
||
Thanks, Sheppy. It all looks good to me with two caveats. First, our implementation of RTCRtpSynchronizationSource has a limitation where it does not yet contain an audioLevel in all cases. That field is only present when the audio level is provided in the RTP header. Second, the the Transceiver work was mainly done in bug 1290948, by Byron. There is also the open bug 1258409.
Flags: needinfo?(na-g)
You need to log in
before you can comment on or make changes to this bug.
Description
•