RTCRtpReceiver.getContributingSources() added.

third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp

Index: third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
diff --git a/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp b/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
index e929cb6b097b02b5cd35cc05d06d702514c9511e..edf164f4c9c2b7553fa247282204b121e267b139 100644
--- a/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
+++ b/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
@@ -4,6 +4,8 @@
 
 #include "modules/peerconnection/RTCRtpReceiver.h"
 
+#include "bindings/core/v8/Microtask.h"
+#include "public/platform/WebRTCRtpContributingSource.h"
 #include "wtf/text/WTFString.h"
 
 namespace blink {
@@ -20,8 +22,57 @@ MediaStreamTrack* RTCRtpReceiver::track() const {
   return m_track;
 }
 
+HeapVector<Member<RTCRtpContributingSource>>
+RTCRtpReceiver::getContributingSources() {
+  updateSourcesIfNeeded();
+  return m_contributingSources;
+}
+
+void RTCRtpReceiver::updateSourcesIfNeeded() {
+  if (m_contributingSourcesNeedsUpdating) {

[foolip, 2017/04/06 10:07:23]

if (!m_contributingSourcesNeedsUpdating) return.

[hbos_chromium, 2017/04/06 11:10:15]

Done.

+    m_contributingSources.clear();
+    for (const auto& webContributingSource : m_receiver->getSources()) {

[foolip, 2017/04/06 10:07:23]

Can you spell out the type here, unless it's some complex template type? At
`*it->value = *webContributingSource` below I have few clues about what it means
without code search.

[hbos_chromium, 2017/04/06 11:10:15]

Done.

+      if (webContributingSource->sourceType() ==
+          WebRTCRtpContributingSourceType::SSRC) {
+        // TODO(hbos): When |getSynchronizationSources| is added to get SSRC
+        // sources don't ignore SSRCs here.
+        continue;
+      }
+      DCHECK_EQ(webContributingSource->sourceType(),
+                WebRTCRtpContributingSourceType::CSRC);
+      auto it =
+          m_contributingSourcesBySourceId.find(webContributingSource->source());
+      if (it == m_contributingSourcesBySourceId.end()) {
+        RTCRtpContributingSource* contributingSource =
+            new RTCRtpContributingSource(this, *webContributingSource);
+        m_contributingSourcesBySourceId.insert(contributingSource->source(),
+                                               contributingSource);
+        m_contributingSources.push_back(contributingSource);
+      } else {
+        *it->value = *webContributingSource;
+        m_contributingSources.push_back(it->value);
+      }
+    }
+    // Clear the flag and schedule a microtask to reset it to true. This makes
+    // the cache valid until the next iteration of the event loop. As such,

[foolip, 2017/04/06 10:07:23]

s/iteration of the event loop/microtask checkpoint/. Does the spec say something
about holding the results stable as well?

[hbos_chromium, 2017/04/06 11:10:15]

Done.

The spec says objects are to be updated when an RTP packet is received. But
javascript and webrtc runs on different threads - it is my, hta's and deadbeef's
interpretation that this should not happen multiple times in the same event loop
execution. If it did, you wouldn't be able to compare sources (like sort) or do
much anything because the values could change unexpectedly, breaking the
illusion of javascript being single-threaded.

Either the editors thought this was implicit (this holds true for other
counters) or they should clarify the spec to make it explicit. I'll file a bug.

[Taylor_Brandstetter, 2017/04/06 20:34:41]

The webrtc editors ended up filing this exact bug in a meeting this morning:
https://github.com/w3c/webrtc-pc/issues/1111

This topic also came up in the working group meeting on Tuesday, and there seems
to be consensus about it. 

[foolip, 2017/04/07 06:49:51]

OK. The situation is very similar to media elements. I think keeping all state
fixed between microtask checkpoints is the right thing here.

However, a very tricky thing is to update all state at once and not when each
individual bit of state is read. In this CL, all of the contributing sources are
updated at the same time, but is that all of the related state? (This is
actually totally broken for media elements, so take no inspiration there.)

[Taylor_Brandstetter, 2017/04/07 17:07:46]

As far as I know, this is all the related state. For all the use cases of
getContributingSources we've been considering, the application will only be
inspecting the contributing sources of a single RTCRtpReceiver, so as long as
they're consistent with each other, we should be good.

The spec should address this point as well, though. I'll leave a comment.

[foolip, 2017/04/10 07:42:33]

Great, thanks!

+    // sources represent a snapshot and can be compared reliably in .js code,
+    // no risk of being updated due to an RTP packet arriving. E.g.
+    // "source.timestamp == source.timestamp" will always be true.
+    m_contributingSourcesNeedsUpdating = false;
+    Microtask::enqueueMicrotask(
+        WTF::bind(&RTCRtpReceiver::setContributingSourcesNeedsUpdating,
+                  wrapWeakPersistent(this)));
+  }
+}
+
+void RTCRtpReceiver::setContributingSourcesNeedsUpdating() {
+  m_contributingSourcesNeedsUpdating = true;
+}
+
 DEFINE_TRACE(RTCRtpReceiver) {
   visitor->trace(m_track);
+  visitor->trace(m_contributingSourcesBySourceId);
+  visitor->trace(m_contributingSources);
 }
 
 }  // namespace blink