Issue 2803693002: RTCRtpReceiver.getContributingSources() added.

hbos_chromium

Patchset #1 (id:1) has been deleted

3 years, 8 months ago (2017-04-05 16:23:41 UTC) #1

hbos_chromium

Description was changed from ========== RTCRtpReceiver.getContributingSources() added. BUG=703122 ========== to ========== RTCRtpReceiver.getContributingSources() added. Based on ...

3 years, 8 months ago (2017-04-05 16:34:37 UTC) #2

hbos_chromium

Description was changed from ========== RTCRtpReceiver.getContributingSources() added. Based on CL https://codereview.webrtc.org/2770233003/, need to wait for ...

3 years, 8 months ago (2017-04-05 16:35:15 UTC) #3

Description was changed from

==========
RTCRtpReceiver.getContributingSources() added.

Based on CL https://codereview.webrtc.org/2770233003/, need to wait
for that to land and roll into chromium before we can run bots.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

BUG=703122
==========

to

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

  Based on CL https://codereview.webrtc.org/2770233003/, need to wait
for that to land and roll into chromium before we can run bots.

BUG=703122
==========

hbos_chromium

Description was changed from ========== RTCRtpReceiver.getContributingSources() added. Adds RTCRtpReceiver.getContributingSources(), https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources, and RTCRtpContributingSource, https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource, behind flag ...

3 years, 8 months ago (2017-04-05 16:36:23 UTC) #4

Description was changed from

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

  Based on CL https://codereview.webrtc.org/2770233003/, need to wait
for that to land and roll into chromium before we can run bots.

BUG=703122
==========

to

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources. This uses
mocking to update contributing sources in a predictable way.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

  Based on CL https://codereview.webrtc.org/2770233003/, need to wait
for that to land and roll into chromium before we can run bots.

BUG=703122
==========

hbos_chromium

hbos@chromium.org changed reviewers: + foolip@chromium.org, guidou@chromium.org

3 years, 8 months ago (2017-04-05 16:44:43 UTC) #5

Taylor_Brandstetter

deadbeef@chromium.org changed reviewers: + deadbeef@chromium.org

3 years, 8 months ago (2017-04-05 19:28:28 UTC) #7

Taylor_Brandstetter

lgtm; this is exactly how I imagined it working. I like your approach with the ...

3 years, 8 months ago (2017-04-05 19:28:29 UTC) #8

hbos_chromium

PTAL guidou and foolip. foolip, the most interesting bits are the changes to: RTCRtpReceiver.[idl/h/cpp] RTCRtpContributingSource.[idl/h/cpp] ...

3 years, 8 months ago (2017-04-06 08:59:43 UTC) #9

foolip

https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runner/mock_webrtc_peer_connection_handler.cc File content/shell/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runner/mock_webrtc_peer_connection_handler.cc#newcode241 content/shell/test_runner/mock_webrtc_peer_connection_handler.cc:241: class MockWebRTCRtpContributingSource Didn't look closely at this. Would this ...

3 years, 8 months ago (2017-04-06 10:07:24 UTC) #10

hbos_chromium

PTAL foolip and guidou. https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runner/mock_webrtc_peer_connection_handler.cc File content/shell/test_runner/mock_webrtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runner/mock_webrtc_peer_connection_handler.cc#newcode241 content/shell/test_runner/mock_webrtc_peer_connection_handler.cc:241: class MockWebRTCRtpContributingSource On 2017/04/06 10:07:23, ...

3 years, 8 months ago (2017-04-06 11:10:15 UTC) #11

PTAL foolip and guidou.

https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runn...
File content/shell/test_runner/mock_webrtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2803693002/diff/60001/content/shell/test_runn...
content/shell/test_runner/mock_webrtc_peer_connection_handler.cc:241: class
MockWebRTCRtpContributingSource
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> Didn't look closely at this. Would this mock get created when running
> LayoutTests? (web-platform-tests)

Yes, all LayoutTests use mock_webrtc_peer_connection_handler.cc.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html:202:
* is invoked in the next iteration of the event loop.
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> Promise.resolve().then(callback) is invoked at the next microtask checkpoint,
> and setTimeout(fn, 0) runs when all other tasks on the queue have been run, so
> combining them doesn't result in quite what is described here.
> 
> To test that the timing of the Microtask::enqueueTask bits of the
implementation
> are correct, just Promise.resolve() should suffice.

Done.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29:
m_receiver->updateSourcesIfNeeded();
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> This is a bit odd, and suggests that it's possible for the source.timestamp to
> change if one keeps the object alive and checks back later. In the spec, it
> looks like these objects are basically PODs, and could in fact be dictionaries
> although I assume that's too late and not worth pursuing.
> 
> The spec doesn't seem to say if getContributingSources() should return the
same
> RTCRtpContributingSource instances every time, what do other implementation
do?

The spec says its an interface, not a dictionary, and explicitly says that when
an RTP packet is received the RTCRtpContributingSource objects corresponding to
those SSRCs/CSRCs are updated. This means we have to updateSourcesIfNeeded, even
when examining existing objects.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:32: if
(m_contributingSourcesNeedsUpdating) {
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> if (!m_contributingSourcesNeedsUpdating) return.

Done.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:34: for
(const auto& webContributingSource : m_receiver->getSources()) {
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> 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.

Done.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the
cache valid until the next iteration of the event loop. As such,
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> s/iteration of the event loop/microtask checkpoint/. Does the spec say
something
> about holding the results stable as well?

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.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.h (right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.h:30:
HeapVector<Member<RTCRtpContributingSource>> getContributingSources();
On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> Perhaps return it by const reference to avoid a copy? (The bindings code just
> passes the value along to ToV8, so there's a good chance a copy is avoided for
> realz.)

Done. Nice catch!

Guido Urdaneta

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-06 11:43:04 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803693002/80001

3 years, 8 months ago (2017-04-06 11:43:17 UTC) #13

hbos_chromium

(The dependent CL has not landed and rolled into chromium yet, the bots will fail.)

3 years, 8 months ago (2017-04-06 11:50:31 UTC) #15

Guido Urdaneta

https://codereview.chromium.org/2803693002/diff/80001/content/renderer/media/webrtc/rtc_rtp_contributing_source.h File content/renderer/media/webrtc/rtc_rtp_contributing_source.h (right): https://codereview.chromium.org/2803693002/diff/80001/content/renderer/media/webrtc/rtc_rtp_contributing_source.h#newcode19 content/renderer/media/webrtc/rtc_rtp_contributing_source.h:19: RTCRtpContributingSource(const webrtc::RtpSource& source); explicit https://codereview.chromium.org/2803693002/diff/80001/content/renderer/media/webrtc/rtc_rtp_receiver.cc File content/renderer/media/webrtc/rtc_rtp_receiver.cc (right): https://codereview.chromium.org/2803693002/diff/80001/content/renderer/media/webrtc/rtc_rtp_receiver.cc#newcode53 ...

3 years, 8 months ago (2017-04-06 12:54:57 UTC) #16

hbos_chromium

hbos@chromium.org changed reviewers: + jochen@chromium.org

3 years, 8 months ago (2017-04-06 14:49:36 UTC) #17

hbos_chromium

PTAL guidou, foolip. jochen please take a look at content/renderer/BUILD.gn content/shell/test_runner/mock_webrtc_peer_connection_handler.cc https://codereview.chromium.org/2803693002/diff/80001/content/renderer/media/webrtc/rtc_rtp_contributing_source.h File content/renderer/media/webrtc/rtc_rtp_contributing_source.h (right): ...

3 years, 8 months ago (2017-04-06 14:49:38 UTC) #18

Guido Urdaneta

On 2017/04/06 15:51:20, Guido Urdaneta wrote: > lgtm lgtm after discussing offline

3 years, 8 months ago (2017-04-06 15:57:48 UTC) #20

Guido Urdaneta

Sorry, that last comment was for another CL, but this lgtm anyway.

3 years, 8 months ago (2017-04-06 15:58:47 UTC) #21

Taylor_Brandstetter

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp File third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp (right): https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp#newcode29 third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29: m_receiver->updateSourcesIfNeeded(); On 2017/04/06 11:10:15, hbos_chromium wrote: > On 2017/04/06 ...

3 years, 8 months ago (2017-04-06 20:34:41 UTC) #22

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29:
m_receiver->updateSourcesIfNeeded();
On 2017/04/06 11:10:15, hbos_chromium wrote:
> On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > This is a bit odd, and suggests that it's possible for the source.timestamp
to
> > change if one keeps the object alive and checks back later. In the spec, it
> > looks like these objects are basically PODs, and could in fact be
dictionaries
> > although I assume that's too late and not worth pursuing.
> > 
> > The spec doesn't seem to say if getContributingSources() should return the
> same
> > RTCRtpContributingSource instances every time, what do other implementation
> do?
> 
> The spec says its an interface, not a dictionary, and explicitly says that
when
> an RTP packet is received the RTCRtpContributingSource objects corresponding
to
> those SSRCs/CSRCs are updated. This means we have to updateSourcesIfNeeded,
even
> when examining existing objects.

I interpreted the spec the same way. It may not be stated explicitly, but it's
heavily implied.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the
cache valid until the next iteration of the event loop. As such,
On 2017/04/06 11:10:15, hbos_chromium wrote:
> On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > s/iteration of the event loop/microtask checkpoint/. Does the spec say
> something
> > about holding the results stable as well?
> 
> 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.

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

lgtm https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp File third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp (right): https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp#newcode29 third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29: m_receiver->updateSourcesIfNeeded(); On 2017/04/06 20:34:41, Taylor_Brandstetter wrote: > On ...

3 years, 8 months ago (2017-04-07 06:49:51 UTC) #23

lgtm

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29:
m_receiver->updateSourcesIfNeeded();
On 2017/04/06 20:34:41, Taylor_Brandstetter wrote:
> On 2017/04/06 11:10:15, hbos_chromium wrote:
> > On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > > This is a bit odd, and suggests that it's possible for the
source.timestamp
> to
> > > change if one keeps the object alive and checks back later. In the spec,
it
> > > looks like these objects are basically PODs, and could in fact be
> dictionaries
> > > although I assume that's too late and not worth pursuing.
> > > 
> > > The spec doesn't seem to say if getContributingSources() should return the
> > same
> > > RTCRtpContributingSource instances every time, what do other
implementation
> > do?
> > 
> > The spec says its an interface, not a dictionary, and explicitly says that
> when
> > an RTP packet is received the RTCRtpContributingSource objects corresponding
> to
> > those SSRCs/CSRCs are updated. This means we have to updateSourcesIfNeeded,
> even
> > when examining existing objects.
> 
> I interpreted the spec the same way. It may not be stated explicitly, but it's
> heavily implied.

OK, so the spec says "Each time an RTP packet is received, the
RTCRtpContributingSource objects are updated" but doesn't do anything to try to
avoid the values changing at random times if the networking is on another
thread.

Also, if it's necessary to have a link back to the RTCRtpReceiver to implement
this, should the spec just expose that as well?

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the
cache valid until the next iteration of the event loop. As such,
On 2017/04/06 20:34:41, Taylor_Brandstetter wrote:
> On 2017/04/06 11:10:15, hbos_chromium wrote:
> > On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > > s/iteration of the event loop/microtask checkpoint/. Does the spec say
> > something
> > > about holding the results stable as well?
> > 
> > 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.
> 
> 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. 

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.)

jochen (gone - plz use gerrit)

lgtm with nit https://codereview.chromium.org/2803693002/diff/120001/content/renderer/media/webrtc/rtc_rtp_contributing_source.h File content/renderer/media/webrtc/rtc_rtp_contributing_source.h (right): https://codereview.chromium.org/2803693002/diff/120001/content/renderer/media/webrtc/rtc_rtp_contributing_source.h#newcode28 content/renderer/media/webrtc/rtc_rtp_contributing_source.h:28: }; disallow copy & assign

3 years, 8 months ago (2017-04-07 07:06:55 UTC) #24

hbos_chromium

Sweet, I'll land this as soon as https://codereview.webrtc.org/2770233003/ has landed and rolled into chromium. https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp ...

3 years, 8 months ago (2017-04-07 08:18:52 UTC) #25

Sweet, I'll land this as soon as https://codereview.webrtc.org/2770233003/ has
landed and rolled into chromium.

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpContributingSource.cpp:29:
m_receiver->updateSourcesIfNeeded();
On 2017/04/07 06:49:51, foolip_UTC7 wrote:
> On 2017/04/06 20:34:41, Taylor_Brandstetter wrote:
> > On 2017/04/06 11:10:15, hbos_chromium wrote:
> > > On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > > > This is a bit odd, and suggests that it's possible for the
> source.timestamp
> > to
> > > > change if one keeps the object alive and checks back later. In the spec,
> it
> > > > looks like these objects are basically PODs, and could in fact be
> > dictionaries
> > > > although I assume that's too late and not worth pursuing.
> > > > 
> > > > The spec doesn't seem to say if getContributingSources() should return
the
> > > same
> > > > RTCRtpContributingSource instances every time, what do other
> implementation
> > > do?
> > > 
> > > The spec says its an interface, not a dictionary, and explicitly says that
> > when
> > > an RTP packet is received the RTCRtpContributingSource objects
corresponding
> > to
> > > those SSRCs/CSRCs are updated. This means we have to
updateSourcesIfNeeded,
> > even
> > > when examining existing objects.
> > 
> > I interpreted the spec the same way. It may not be stated explicitly, but
it's
> > heavily implied.
> 
> OK, so the spec says "Each time an RTP packet is received, the
> RTCRtpContributingSource objects are updated" but doesn't do anything to try
to
> avoid the values changing at random times if the networking is on another
> thread.
> 
> Also, if it's necessary to have a link back to the RTCRtpReceiver to implement
> this, should the spec just expose that as well?

Good point, filed https://github.com/w3c/webrtc-pc/issues/1117

https://codereview.chromium.org/2803693002/diff/120001/content/renderer/media...
File content/renderer/media/webrtc/rtc_rtp_contributing_source.h (right):

https://codereview.chromium.org/2803693002/diff/120001/content/renderer/media...
content/renderer/media/webrtc/rtc_rtp_contributing_source.h:28: };
On 2017/04/07 07:06:55, jochen wrote:
> disallow copy & assign

Done.

Taylor_Brandstetter

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp (right): https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp#newcode57 third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the cache valid until the next iteration of ...

3 years, 8 months ago (2017-04-07 17:07:46 UTC) #26

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the
cache valid until the next iteration of the event loop. As such,
On 2017/04/07 06:49:51, foolip_UTC7 wrote:
> On 2017/04/06 20:34:41, Taylor_Brandstetter wrote:
> > On 2017/04/06 11:10:15, hbos_chromium wrote:
> > > On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > > > s/iteration of the event loop/microtask checkpoint/. Does the spec say
> > > something
> > > > about holding the results stable as well?
> > > 
> > > 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.
> > 
> > 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. 
> 
> 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.)

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.

hbos_chromium

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-10 07:11:57 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803693002/160001

3 years, 8 months ago (2017-04-10 07:12:07 UTC) #28

hbos_chromium

Description was changed from ========== RTCRtpReceiver.getContributingSources() added. Adds RTCRtpReceiver.getContributingSources(), https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources, and RTCRtpContributingSource, https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource, behind flag ...

3 years, 8 months ago (2017-04-10 07:34:35 UTC) #29

Description was changed from

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources. This uses
mocking to update contributing sources in a predictable way.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

  Based on CL https://codereview.webrtc.org/2770233003/, need to wait
for that to land and roll into chromium before we can run bots.

BUG=703122
==========

to

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources. This uses
mocking to update contributing sources in a predictable way.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

BUG=703122
==========

foolip

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp (right): https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp#newcode57 third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the cache valid until the next iteration of ...

3 years, 8 months ago (2017-04-10 07:42:34 UTC) #30

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
(right):

https://codereview.chromium.org/2803693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp:57: // the
cache valid until the next iteration of the event loop. As such,
On 2017/04/07 17:07:46, Taylor_Brandstetter wrote:
> On 2017/04/07 06:49:51, foolip_UTC7 wrote:
> > On 2017/04/06 20:34:41, Taylor_Brandstetter wrote:
> > > On 2017/04/06 11:10:15, hbos_chromium wrote:
> > > > On 2017/04/06 10:07:23, foolip_UTC7 wrote:
> > > > > s/iteration of the event loop/microtask checkpoint/. Does the spec say
> > > > something
> > > > > about holding the results stable as well?
> > > > 
> > > > 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.
> > > 
> > > 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. 
> > 
> > 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.)
> 
> 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.

Great, thanks!

hbos_chromium

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@chromium.org, guidou@chromium.org, jochen@chromium.org, foolip@chromium.org ...

3 years, 8 months ago (2017-04-10 08:06:47 UTC) #33

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803693002/160001

3 years, 8 months ago (2017-04-10 08:07:00 UTC) #34

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491811606870620, "parent_rev": "7d23dcb9307f8fd2204c18e33877747e86ecd230", "commit_rev": "7a16f508ad712bc38ff72e979dd5155a4015c320"}

3 years, 8 months ago (2017-04-10 09:17:25 UTC) #35

commit-bot: I haz the power

Description was changed from ========== RTCRtpReceiver.getContributingSources() added. Adds RTCRtpReceiver.getContributingSources(), https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources, and RTCRtpContributingSource, https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource, behind flag ...

3 years, 8 months ago (2017-04-10 09:18:16 UTC) #36

Message was sent while issue was closed.

Description was changed from

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources. This uses
mocking to update contributing sources in a predictable way.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

BUG=703122
==========

to

==========
RTCRtpReceiver.getContributingSources() added.

Adds RTCRtpReceiver.getContributingSources(),
https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getcontributingsources,
and RTCRtpContributingSource,
https://w3c.github.io/webrtc-pc/#dom-rtcrtpcontributingsource,
behind flag "RuntimeEnabled=RTCRtpReceiver".

Contributing sources come in two flavors, SSRCs and CSRCs. This
interface represents both. For now we ignore SSRCs and only return
CSRCs. It was recently decided that SSRCs would be returned in a
separate method to distinguish between the two cases.

A LayoutTest is used to thoroughly test all blink functionality with
regards to CSRCs and the caching of contributing sources. This uses
mocking to update contributing sources in a predictable way.

The browsertest verifies that no contributing sources are used in a
normal peerconnection call. The multiple CSRCs case is currently not
possible to test in a browsertest so we rely on the LayoutTest (and
webrtc layer testing of contributing sources).

BUG=703122

Review-Url: https://codereview.chromium.org/2803693002
Cr-Commit-Position: refs/heads/master@{#463199}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a16f508ad712bc38ff72e979dd5...
==========

commit-bot: I haz the power

Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7a16f508ad712bc38ff72e979dd5155a4015c320

3 years, 8 months ago (2017-04-10 09:18:18 UTC) #37

hbos_chromium

3 years, 8 months ago (2017-04-10 12:01:52 UTC) #38

Message was sent while issue was closed.

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/2813443003/ by hbos@chromium.org.

The reason for reverting is: The dependent CL was reverted due to suspected of:
https://bugs.chromium.org/p/webrtc/issues/detail?id=7465.

Issue 2803693002: RTCRtpReceiver.getContributingSources() added. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed deadbeef's comments #

Patch Set 3 : Rebase with dependent CL, timestamp in ms, not s #

Patch Set 4 : Addressed foolip's comments #

Patch Set 5 : global-interface-listing-expected.txt updated #

Patch Set 6 : Addressed guidou's comments #

Patch Set 7 : DISALLOW_COPY_AND_ASSIGN #

Patch Set 8 : Rebase with master after dep CL landed #

Messages