Closed Bug 1363667 Opened 7 years ago Closed 6 years ago

Add getContributingSources and getSynchronizationSources to RTCRtpReceiver

Categories

(Core :: WebRTC, enhancement, P2)

Product:
Core
Component:
WebRTC
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
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)

Bug 1363667 - P1 - Add RTP Sources WebIDL
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P2 - Add RTP Sources RTP header extensions
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P3 - RTP Source Observer
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P4 - RTP Source Observer unit tests
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P5 - RTP Source PeerConnection JS IF
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P6 - RTP Source PeerConnection JS impl
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P7 - RTP Source mochitests
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P7.1 - await on sync sources in mochitest
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P2.1 - Fix jsep extmap unit test
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1363667 - P6.1 - disable broken web platform tests
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
Rank: 25
Priority: -- → P2
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 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+
Attachment #8899916 - Flags: review?(jib) → review?(mfroman)
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+
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
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 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?
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 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 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 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]
Attachment #8897185 - Attachment is obsolete: true
Attachment #8899916 - Attachment is obsolete: true
Attachment #8911004 - Attachment is obsolete: true
Attachment #8913868 - Attachment is obsolete: true
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 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 on attachment 8928239 [details]
Bug 1363667 - P2 - Add RTP Sources RTP header extensions

https://reviewboard.mozilla.org/r/199460/#review204576

r+ w/ nits.
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 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 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 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 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 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 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+
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 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 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 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!
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
Attachment #8929667 - Flags: review?(mfroman)
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 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 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+
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 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+
Attachment #8930722 - Flags: review?(mfroman)
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+
Attachment #8931104 - Flags: review?(jib)
Attachment #8931104 - Flags: review?(jib) → review?(drno)
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+
Attachment #8931104 - Flags: review?(drno)
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 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 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?
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
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)
Jorg, yes, it looks like it. I am looking into this now.
Flags: needinfo?(na-g)
Flags: needinfo?(mfroman)
Flags: needinfo?(jib)
Looks like a const mismatch on this line:   void GetRtpSources(const int64_t aTimeNow,
Jib, yes. I am about to push to try.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
No longer blocks: 1422987
Depends on: 1422987
Blocks: meet
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
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
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.
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.