Issue 2770233003: Implemented the GetSources() in native code.

Zhi Huang

Patchset #1 (id:1) has been deleted

3 years, 8 months ago (2017-03-29 17:44:54 UTC) #1

Zhi Huang

Description was changed from ========== Add ContributingSourceInterface BUG=None ========== to ========== Add ContributingSourceInterface Not ready ...

3 years, 8 months ago (2017-03-29 18:00:53 UTC) #2

Zhi Huang

Description was changed from ========== Add ContributingSourceInterface Not ready for review. BUG=None ========== to ========== ...

3 years, 8 months ago (2017-03-30 03:42:43 UTC) #4

Zhi Huang

Description was changed from ========== Added the GetContributingSource() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-03-30 03:43:10 UTC) #5

Zhi Huang

Description was changed from ========== Added the GetContributingSource() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-03-30 03:43:25 UTC) #6

Zhi Huang

Description was changed from ========== Added the GetContributingSource() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-03-30 03:44:40 UTC) #7

Zhi Huang

zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org, solenberg@webrtc.org

3 years, 8 months ago (2017-03-30 03:55:00 UTC) #9

hbos

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h#newcode39 webrtc/api/rtpreceiverinterface.h:39: virtual int8_t audio_level() const = 0; This should be ...

3 years, 8 months ago (2017-03-30 09:51:55 UTC) #12

the sun

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode238 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:238: contributing_sources_.push_back(&contributing_source); IIUC, UpdateContributingSource() will be called on the network ...

3 years, 8 months ago (2017-03-30 11:31:42 UTC) #13

Taylor Brandstetter

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h#newcode39 webrtc/api/rtpreceiverinterface.h:39: virtual int8_t audio_level() const = 0; On 2017/03/30 09:51:54, ...

3 years, 8 months ago (2017-03-30 22:55:38 UTC) #14

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:39: virtual int8_t audio_level() const = 0;
On 2017/03/30 09:51:54, hbos wrote:
> This should be rtc::Optional<...>. 0 is a meaningful value. Otherwise, if a
> negative value is supposed to mean undefined then document that, but I think
> rtc::Optional is self-documenting.
> 
> Even if an implementation supports this field it would only be used if the RTP
> header extension is used and might be undefined, if I understand this
correctly.
> 
> Also: int8_t or uint8_t?

It only ranges from 0 to 127. So might as well make it an int8_t, since we
prefer signed types by default.

I'd also suggest adding a comment making it clear that this isn't implemented
yet, and will always return an empty Optional.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:78: virtual
std::vector<std::unique_ptr<RtpContributingSourceInterface>>
Since this looks so similar to the JS API, but behaves differently, there should
be a comment that documents the differences. Namely:

* Each time the method is called, a vector of new objects is created. If the
application wants to identify them across calls, it should use the "source()"
value, not the value of the pointer.
* The returned RtpContributingSource objects don't get updated automatically
when more packets are received, as will happen with the JS
RTCRtpContributingSource objects. They're just structs, basically.

This confusion may be a good reason to just return a vector of structs, instead
of unique_ptrs wrapping an interface. In short, the issue is that returning a
pointer to an abstract class may make API users think that a "smart" object is
being returned, when really it isn't.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2614: << " which is not exist.";
nit: Minor grammar/spelling issues; I'd suggest changing this to "Attempting to
get contributing sources for SSRC: <ssrc> which does not exist."

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:217:
contributing_sources_.clear();
If contributing_sources_ is only used in this method, it doesn't need to be a
member variable. It can just be returned by value.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:241: // Add the contributing
source using the SSRC.
This doesn't properly handle the corner case of the SSRC changing. I think the
correct behavior, as the spec currently reads, would be to return an object for
the old SSRC for 10 seconds after it switches.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: }
I think your initial idea (std::list of sources sorted by timestamp, and
std::unordered_map from CSRCs to list iterators) is preferable to this
algorithm. If the average number of contributing sources in a 10-second interval
is N, and the average number in an individual packet is M:

* Update time (per RTP packet): O(M), since the map is used for O(1) lookup per
CSRC, and it's O(1) to move each updated element to the front of the list to
keep it sorted.
* "Remove 10-second old things" time: Average O(1), worst case O(N). Since the
list is sorted by timestamp, it's easy to find everything that's older than 10
seconds, and most of the time it will be "nothing".
* Retrieval time: O(N)
* Memory: O(N)

So it has about the same performance characteristics as the current algorithm,
but "getContributingSources" would be more efficient, and we wouldn't have to
choose a hard-coded size for the ring buffer, crossing our fingers that it's
large enough for everyone.

I saw a comment earlier that "map+list will do heap allocs along the way. At
least one per incoming packet...", but this isn't true. It would only need a
heap alloc when a new contributing source shows up. So the main disadvantage I
see is the lack of memory locality of an std::list.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/peerconnection_...
File webrtc/pc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/peerconnection_...
webrtc/pc/peerconnection_unittest.cc:2643: TEST_F(P2PTestConductor,
TestGetContributingSources) {
I recently refactored the tests in this file, FYI. There's no more
"LocalP2PTest", so you'll have to explicitly add an audio track to the caller's
PeerConnection.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc
File webrtc/pc/rtpreceiver.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc#...
webrtc/pc/rtpreceiver.cc:106: auto sources =
channel_->GetContributingSources(ssrc_);
On 2017/03/30 11:31:42, the sun wrote:
> Note, you can add the ssrc entry here and avoid propagating that up the stack
-
> just pick the csrc with the latest time stamp.

But the AudioRtpReceiver only knows about the SSRC it uses as an identifier for
the receive stream. If the actual SSRC of the receive stream changes (handled in
RtpReceiverImpl::CheckSSRCChanged?), it wouldn't know.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.h
File webrtc/pc/rtpreceiver.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.h#n...
webrtc/pc/rtpreceiver.h:43: // TODO(zhihuang): Remove the default implemenation
once the subclasses
nit: Spelling of "implementation"

Also, isn't the default implementation needed on RtpReceiverInterface, not
RtpReceiverInternal? And, assuming the only relevant subclass is
"FakeRtpReceiver" in chromium, it may be helpful to mention it by name, as a
reminder to whoever goes to fix the TODO.

Zhi Huang

Hi, Please take another look. Thanks. https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h#newcode78 webrtc/api/rtpreceiverinterface.h:78: virtual std::vector<std::unique_ptr<RtpContributingSourceInterface>> On ...

3 years, 8 months ago (2017-03-31 06:44:05 UTC) #18

Hi,
Please take another look.
Thanks.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:78: virtual
std::vector<std::unique_ptr<RtpContributingSourceInterface>>
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> Since this looks so similar to the JS API, but behaves differently, there
should
> be a comment that documents the differences. Namely:
> 
> * Each time the method is called, a vector of new objects is created. If the
> application wants to identify them across calls, it should use the "source()"
> value, not the value of the pointer.
> * The returned RtpContributingSource objects don't get updated automatically
> when more packets are received, as will happen with the JS
> RTCRtpContributingSource objects. They're just structs, basically.
> 
> This confusion may be a good reason to just return a vector of structs,
instead
> of unique_ptrs wrapping an interface. In short, the issue is that returning a
> pointer to an abstract class may make API users think that a "smart" object is
> being returned, when really it isn't.

Yeah, I agree that returning plain structs would be more clear.

Done.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2614: << " which is not exist.";
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> nit: Minor grammar/spelling issues; I'd suggest changing this to "Attempting
to
> get contributing sources for SSRC: <ssrc> which does not exist."

Oh, my silly mistake.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_receiver.h:39: int8_t audio_level() const
override { return 0; }
On 2017/03/30 09:51:54, hbos wrote:
> Return a value that doesn't map to a meaningful value, say -1 or
> rtc::Optional<>()

Done.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:30: static const int64_t
kContributingSourcesTimeout = 10000;  // ms
On 2017/03/30 09:51:54, hbos wrote:
> Prefer ms in the name, kContributingSourcesTimeoutMs

Done.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:216:
RtpReceiverImpl::GetContributingSources() {
On 2017/03/30 09:51:54, hbos wrote:
> What's the threading model. Should we either lock or DCHECK that we are on the
> correct thread?

This method should only be called on the worker_thread_. There is a check when
the channel_proxy calls the GetContributingSources().

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:217:
contributing_sources_.clear();
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> If contributing_sources_ is only used in this method, it doesn't need to be a
> member variable. It can just be returned by value.

If it returns the vector by value then I have to change the return type on the
whole objects chain.

Wouldn't the vector be copied everywhere (depending on the compiler's
optimization)?

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:238:
contributing_sources_.push_back(&contributing_source);
On 2017/03/30 11:31:42, the sun wrote:
> IIUC, UpdateContributingSource() will be called on the network thread (which
may
> coincide with the libjingle worker thread right now, but that's not how it
> should be), and GetContributingSources() is called on the worker thread.
> 
> This means that unless the caller guards access to contributing_sources_ with
a
> lock (that's unfeasible), the pointers returned here may point at completely
> different data when accessed.
> 
> I think the solution would be simpler if you just returned a vector of
by-value
> structs, and also if you made RtpContributingSource a plain struct at this
> level. If we want to present it as an interface in the API, we can wrap it
> there.

I think there are two options to solve this:
1) When GetContributingSource() is called, block the network_thread from
updating the buffer and do the scanning and deduplicating stuff.
2) Block the network thread, create another copy of the buffer, unblock the
thread and do scanning and other stuff on the copied buffer without blocking.

Do you think it is worth doing 2) assuming creating another copy would be
faster?

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:241: // Add the contributing
source using the SSRC.
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> This doesn't properly handle the corner case of the SSRC changing. I think the
> correct behavior, as the spec currently reads, would be to return an object
for
> the old SSRC for 10 seconds after it switches.

According to the spec:
"If the RTP packet contains CSRCs, then the RTCRtpContributingSource objects
corresponding to those CSRCs are updated. If the RTP packet contains no CSRCs,
then the RTCRtpContributingSource object corresponding to the SSRC is updated."
This is a little bit confusing.  Does it mean that the timestamp of the SSRC
source is always the last time when a packet without any CSRCs is received? What
if the packets always contain the CSRCs? 

What about updating the SSRC source no matter if the packet contains the CSRCs
or not? 
Then when the SSRC changes, the SSRC source will not be updated for 10 seconds
and when GetContributingSources() is called, just return the old SSRC and old
timestamp.  Does it make sense to you?

By saying "return an object for the old SSRC for 10 seconds after it switches",
do you mean an "additional" object or return old ssrc instead of new one?

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:242: ssrc_source_.reset(new
RtpContributingSource(now, ssrc_));
On 2017/03/30 09:51:54, hbos wrote:
> I thought the SSRC was a special case of CSRC, but that it still corresponded
to
> an RTP packet? If that isn't the case, and it corresponds to "any packet",
> shouldn't it at least be set to the latest RTP packet? This sets it to "now",
> which might not correspond to any packet whatsoever. Is that how it is
supposed
> to work?
> 
> This returns a pointer that is not valid if you call GetContributingSources
> again, even if you are on the network thread and no packets are arriving. The
> lifetime worries me.


I'll just use the plain old structs and return a copy of this.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:506: void
RtpReceiverImpl::UpdateContributingSource() {
On 2017/03/30 09:51:54, hbos wrote:
> Just checking: This is thread safe without holding the lock, yes?
> 
> I see critical_section_rtp_receiver_ is used elsewhere but this is read-only
and
> called from IncomingRtpPacket, the only place that does write, and I assume
> IncomingRtpPacket is always called on the same thread, meaning there cannot be
a
> write happening here if I'm not mistaken.

I added a lock here because the objects need to be copied when extracting the
contributing sources in last 10 seconds.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:510:
contributing_sources_buffer_[current_buffer_index_] = contributing_source;
On 2017/03/30 09:51:54, hbos wrote:
> nit: = RtpContributingSource(... or std::move to move instead of copying.

Done.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: }
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> I think your initial idea (std::list of sources sorted by timestamp, and
> std::unordered_map from CSRCs to list iterators) is preferable to this
> algorithm. If the average number of contributing sources in a 10-second
interval
> is N, and the average number in an individual packet is M:
> 
> * Update time (per RTP packet): O(M), since the map is used for O(1) lookup
per
> CSRC, and it's O(1) to move each updated element to the front of the list to
> keep it sorted.
> * "Remove 10-second old things" time: Average O(1), worst case O(N). Since the
> list is sorted by timestamp, it's easy to find everything that's older than 10
> seconds, and most of the time it will be "nothing".
> * Retrieval time: O(N)
> * Memory: O(N)
> 
> So it has about the same performance characteristics as the current algorithm,
> but "getContributingSources" would be more efficient, and we wouldn't have to
> choose a hard-coded size for the ring buffer, crossing our fingers that it's
> large enough for everyone.
> 
> I saw a comment earlier that "map+list will do heap allocs along the way. At
> least one per incoming packet...", but this isn't true. It would only need a
> heap alloc when a new contributing source shows up. So the main disadvantage I
> see is the lack of memory locality of an std::list.

I can change it once we have an agreement on this. Changing it won't be too hard
since I only have to change this file..

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:26: static const size_t
kContributingSourcesBufferSize = 500;
On 2017/03/30 09:51:54, hbos wrote:
> What is the reason for choosing 500? Is this arbitrary?

I'm not sure what is right size of this. It is expected to be larger than the
packet_per_second * 10 seconds. Then the question is what is the typical packet
rate?

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/t...
File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/t...
webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:30: const int64_t
kContributingSourcesTimeout = 10000;  // ms
On 2017/03/30 09:51:54, hbos wrote:
> + kContributingSourcesTimeoutMs
> - // ms

Done.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1670: Bind(&VoiceMediaChannel::GetContributingSources,
media_channel(), ssrc));
I'll downcast this instead of adding the method to the basic class because there
would be circular dependency between the rtpreceiveritnerface.h and
mediachannel.h.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/peerconnection_...
File webrtc/pc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/peerconnection_...
webrtc/pc/peerconnection_unittest.cc:2643: TEST_F(P2PTestConductor,
TestGetContributingSources) {
On 2017/03/30 22:55:38, Taylor Brandstetter wrote:
> I recently refactored the tests in this file, FYI. There's no more
> "LocalP2PTest", so you'll have to explicitly add an audio track to the
caller's
> PeerConnection.

Acknowledged.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.h
File webrtc/pc/rtpreceiver.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.h#n...
webrtc/pc/rtpreceiver.h:43: // TODO(zhihuang): Remove the default implemenation
once the subclasses
On 2017/03/30 22:55:38, Taylor Brandstetter wrote:
> nit: Spelling of "implementation"
> 
> Also, isn't the default implementation needed on RtpReceiverInterface, not
> RtpReceiverInternal? And, assuming the only relevant subclass is
> "FakeRtpReceiver" in chromium, it may be helpful to mention it by name, as a
> reminder to whoever goes to fix the TODO.

Done.

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (left):

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:70: enum class DegradationPreference {
This is changed because it accidentally conflict with a name in the video
encoder part.

the sun

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h#newcode78 webrtc/api/rtpreceiverinterface.h:78: virtual std::vector<std::unique_ptr<RtpContributingSourceInterface>> On 2017/03/30 22:55:37, Taylor Brandstetter wrote: > ...

3 years, 8 months ago (2017-03-31 07:03:49 UTC) #19

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:78: virtual
std::vector<std::unique_ptr<RtpContributingSourceInterface>>
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> Since this looks so similar to the JS API, but behaves differently, there
should
> be a comment that documents the differences. Namely:
> 
> * Each time the method is called, a vector of new objects is created. If the
> application wants to identify them across calls, it should use the "source()"
> value, not the value of the pointer.
> * The returned RtpContributingSource objects don't get updated automatically
> when more packets are received, as will happen with the JS
> RTCRtpContributingSource objects. They're just structs, basically.
> 
> This confusion may be a good reason to just return a vector of structs,
instead
> of unique_ptrs wrapping an interface. In short, the issue is that returning a
> pointer to an abstract class may make API users think that a "smart" object is
> being returned, when really it isn't.

Perhaps that is the guideline we were looking for? :)

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: }
On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> I think your initial idea (std::list of sources sorted by timestamp, and
> std::unordered_map from CSRCs to list iterators) is preferable to this
> algorithm. If the average number of contributing sources in a 10-second
interval
> is N, and the average number in an individual packet is M:
> 
> * Update time (per RTP packet): O(M), since the map is used for O(1) lookup
per
> CSRC, and it's O(1) to move each updated element to the front of the list to
> keep it sorted.
> * "Remove 10-second old things" time: Average O(1), worst case O(N). Since the
> list is sorted by timestamp, it's easy to find everything that's older than 10
> seconds, and most of the time it will be "nothing".
> * Retrieval time: O(N)
> * Memory: O(N)
> 
> So it has about the same performance characteristics as the current algorithm,
> but "getContributingSources" would be more efficient, and we wouldn't have to
> choose a hard-coded size for the ring buffer, crossing our fingers that it's
> large enough for everyone.

Yeah, I don't particularly like that property either, but felt it an ok
tradeoff. I guess it would be possible to use a growing std::vector instead,
using it as a ring buffer once a 10s interval has been collected. Some kind of
cap would still be needed though.

> I saw a comment earlier that "map+list will do heap allocs along the way. At
> least one per incoming packet...", but this isn't true. It would only need a
> heap alloc when a new contributing source shows up. So the main disadvantage I
> see is the lack of memory locality of an std::list.

What about the remove() then push_back() sequence when updating the timestamp?
Sure, the map isn't really a problem (but note that the 10s limit needs to be
managed in UpdateContributingSource()), and we can get rid of the O(N) remove()
behavior, but there'd still be a heap alloc when appending to
contributing_sources_.

the sun

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc File webrtc/pc/rtpreceiver.cc (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc#newcode106 webrtc/pc/rtpreceiver.cc:106: auto sources = channel_->GetContributingSources(ssrc_); On 2017/03/30 22:55:38, Taylor Brandstetter ...

3 years, 8 months ago (2017-03-31 07:05:42 UTC) #20

Zhi Huang

On 2017/03/31 07:05:42, the sun wrote: > https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc > File webrtc/pc/rtpreceiver.cc (right): > > https://codereview.webrtc.org/2770233003/diff/80001/webrtc/pc/rtpreceiver.cc#newcode106 ...

3 years, 8 months ago (2017-03-31 17:13:53 UTC) #21

Zhi Huang

On 2017/03/31 07:03:49, the sun wrote: > https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h > File webrtc/api/rtpreceiverinterface.h (right): > > https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverinterface.h#newcode78 ...

3 years, 8 months ago (2017-03-31 17:20:42 UTC) #22

On 2017/03/31 07:03:49, the sun wrote:
>
https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
> File webrtc/api/rtpreceiverinterface.h (right):
> 
>
https://codereview.webrtc.org/2770233003/diff/80001/webrtc/api/rtpreceiverint...
> webrtc/api/rtpreceiverinterface.h:78: virtual
> std::vector<std::unique_ptr<RtpContributingSourceInterface>>
> On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> > Since this looks so similar to the JS API, but behaves differently, there
> should
> > be a comment that documents the differences. Namely:
> > 
> > * Each time the method is called, a vector of new objects is created. If the
> > application wants to identify them across calls, it should use the
"source()"
> > value, not the value of the pointer.
> > * The returned RtpContributingSource objects don't get updated automatically
> > when more packets are received, as will happen with the JS
> > RTCRtpContributingSource objects. They're just structs, basically.
> > 
> > This confusion may be a good reason to just return a vector of structs,
> instead
> > of unique_ptrs wrapping an interface. In short, the issue is that returning
a
> > pointer to an abstract class may make API users think that a "smart" object
is
> > being returned, when really it isn't.
> 
> Perhaps that is the guideline we were looking for? :)
> 
>
https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):
> 
>
https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: }
> On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> > I think your initial idea (std::list of sources sorted by timestamp, and
> > std::unordered_map from CSRCs to list iterators) is preferable to this
> > algorithm. If the average number of contributing sources in a 10-second
> interval
> > is N, and the average number in an individual packet is M:
> > 
> > * Update time (per RTP packet): O(M), since the map is used for O(1) lookup
> per
> > CSRC, and it's O(1) to move each updated element to the front of the list to
> > keep it sorted.
> > * "Remove 10-second old things" time: Average O(1), worst case O(N). Since
the
> > list is sorted by timestamp, it's easy to find everything that's older than
10
> > seconds, and most of the time it will be "nothing".
> > * Retrieval time: O(N)
> > * Memory: O(N)
> > 
> > So it has about the same performance characteristics as the current
algorithm,
> > but "getContributingSources" would be more efficient, and we wouldn't have
to
> > choose a hard-coded size for the ring buffer, crossing our fingers that it's
> > large enough for everyone.
> 
> Yeah, I don't particularly like that property either, but felt it an ok
> tradeoff. I guess it would be possible to use a growing std::vector instead,
> using it as a ring buffer once a 10s interval has been collected. Some kind of
> cap would still be needed though.
> 
> > I saw a comment earlier that "map+list will do heap allocs along the way. At
> > least one per incoming packet...", but this isn't true. It would only need a
> > heap alloc when a new contributing source shows up. So the main disadvantage
I
> > see is the lack of memory locality of an std::list.
> 
> What about the remove() then push_back() sequence when updating the timestamp?
> Sure, the map isn't really a problem (but note that the 10s limit needs to be
> managed in UpdateContributingSource()), and we can get rid of the O(N)
remove()
> behavior, but there'd still be a heap alloc when appending to
> contributing_sources_.

Since the map is not a problem, I think it would be better to use it.
Pros:
1. Don't have to store object with same ID, which is very likely to happen in
current approach.
2. When extracting the object for last 10 seconds, it would be faster since we
don't have to deduplicate and the network thread would be blocked for shorter
time.
Cons:
1. The result is not sorted by time (big deal?)
2. More per packet operation (using an unsorted map will be fine, isn't it?)

Taylor Brandstetter

On 2017/03/31 17:13:53, Zhi Huang wrote: > On 2017/03/31 07:05:42, the sun wrote: > > ...

3 years, 8 months ago (2017-03-31 17:23:37 UTC) #23

Taylor Brandstetter

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode217 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:217: contributing_sources_.clear(); On 2017/03/31 06:44:04, Zhi Huang wrote: > On ...

3 years, 8 months ago (2017-03-31 22:10:50 UTC) #24

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:217:
contributing_sources_.clear();
On 2017/03/31 06:44:04, Zhi Huang wrote:
> On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> > If contributing_sources_ is only used in this method, it doesn't need to be
a
> > member variable. It can just be returned by value.
> 
> If it returns the vector by value then I have to change the return type on the
> whole objects chain.
> 
> Wouldn't the vector be copied everywhere (depending on the compiler's
> optimization)?

With C++11, the compiler should optimize away the copy operation, so it's a
pretty cheap move operation at each layer in the stack.

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:241: // Add the contributing
source using the SSRC.
On 2017/03/31 06:44:04, Zhi Huang wrote:
> On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> > This doesn't properly handle the corner case of the SSRC changing. I think
the
> > correct behavior, as the spec currently reads, would be to return an object
> for
> > the old SSRC for 10 seconds after it switches.
> 
> According to the spec:
> "If the RTP packet contains CSRCs, then the RTCRtpContributingSource objects
> corresponding to those CSRCs are updated. If the RTP packet contains no CSRCs,
> then the RTCRtpContributingSource object corresponding to the SSRC is
updated."
> This is a little bit confusing.  Does it mean that the timestamp of the SSRC
> source is always the last time when a packet without any CSRCs is received?
What
> if the packets always contain the CSRCs?
>
> What about updating the SSRC source no matter if the packet contains the CSRCs
> or not? 
> Then when the SSRC changes, the SSRC source will not be updated for 10 seconds
> and when GetContributingSources() is called, just return the old SSRC and old
> timestamp.  Does it make sense to you?

That was the topic of this issue on github:
https://github.com/w3c/webrtc-pc/issues/1091

The direction we're heading in is that the SSRC source is *always* updated. So I
think it would be safe to write this CL assuming that's what happens.

> By saying "return an object for the old SSRC for 10 seconds after it
switches",
> do you mean an "additional" object or return old ssrc instead of new one?

I mean an additional object. Here's an example:

Time 1.0s: Packet received with SSRC X
Time 1.5s: GetContributingSources returns [{source: X, timestamp: 1}]
Time 2.0s: Packet received with SSRC Y
Time 2.5s: GetContributingSources returns
           [{source: X, timestamp: 1},
            {source: Y, timestamp: 2}]

... Some more packets with SSRC Y are received ...

Time 11.5s: GetContributingSources returns [{source: Y, timestamp: 11}]

This is how I would interpret the standard, at least.

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (left):

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:70: enum class DegradationPreference {
On 2017/03/31 06:44:05, Zhi Huang wrote:
> This is changed because it accidentally conflict with a name in the video
> encoder part.

It looks like the two enums are referring to the same thing. Could they just be
unified? May be better in a standalone CL.

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpreceiverin...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:41: rtc::Optional<int8_t> audio_level;
nit: I'd still be in favor of using getters/setters. For example: in the future
we could decide it would be helpful to return both the CSRC and the associated
SSRC in the same object. In which case it may change to:

// Returns the CSRC or SSRC depending on what the object represents.
uint32_t source() {
  return csrc_ ? *csrc_ : ssrc_;
}

uint32_t ssrc() { 
  return ssrc_;
}

private:

// Set if this object represents a CSRC, unset if it
// represents an SSRC.
rtc::Optional<uint32_t> csrc_;
uint32_t ssrc_;

If the data members were private, it would be easy to make this change.

the sun

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode517 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: } On 2017/03/31 07:03:49, the sun wrote: > On ...

3 years, 8 months ago (2017-04-01 12:23:02 UTC) #25

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:517: }
On 2017/03/31 07:03:49, the sun wrote:
> On 2017/03/30 22:55:37, Taylor Brandstetter wrote:
> > I think your initial idea (std::list of sources sorted by timestamp, and
> > std::unordered_map from CSRCs to list iterators) is preferable to this
> > algorithm. If the average number of contributing sources in a 10-second
> interval
> > is N, and the average number in an individual packet is M:
> > 
> > * Update time (per RTP packet): O(M), since the map is used for O(1) lookup
> per
> > CSRC, and it's O(1) to move each updated element to the front of the list to
> > keep it sorted.
> > * "Remove 10-second old things" time: Average O(1), worst case O(N). Since
the
> > list is sorted by timestamp, it's easy to find everything that's older than
10
> > seconds, and most of the time it will be "nothing".
> > * Retrieval time: O(N)
> > * Memory: O(N)
> > 
> > So it has about the same performance characteristics as the current
algorithm,
> > but "getContributingSources" would be more efficient, and we wouldn't have
to
> > choose a hard-coded size for the ring buffer, crossing our fingers that it's
> > large enough for everyone.
> 
> Yeah, I don't particularly like that property either, but felt it an ok
> tradeoff. I guess it would be possible to use a growing std::vector instead,
> using it as a ring buffer once a 10s interval has been collected. Some kind of
> cap would still be needed though.
> 
> > I saw a comment earlier that "map+list will do heap allocs along the way. At
> > least one per incoming packet...", but this isn't true. It would only need a
> > heap alloc when a new contributing source shows up. So the main disadvantage
I
> > see is the lack of memory locality of an std::list.
> 
> What about the remove() then push_back() sequence when updating the timestamp?
> Sure, the map isn't really a problem (but note that the 10s limit needs to be
> managed in UpdateContributingSource()), and we can get rid of the O(N)
remove()
> behavior, but there'd still be a heap alloc when appending to
> contributing_sources_.

Oh, I just realized the remove/push_back can be accomplished with
std::list::splice() instead! With this twist, the map+list solution seems the
most favorable design choice here. If this is what you had in mind all along I'm
sorry I didn't see it.

Zhi Huang

Please take another look. Thanks. https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/160001/webrtc/api/rtpreceiverinterface.h#newcode41 webrtc/api/rtpreceiverinterface.h:41: rtc::Optional<int8_t> audio_level; On 2017/03/31 ...

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

stefan-webrtc

stefan@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org

3 years, 8 months ago (2017-04-04 13:58:13 UTC) #30

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 8 months ago (2017-04-04 18:56:31 UTC) #32

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.webrtc.org/2770233003/260001

3 years, 8 months ago (2017-04-04 18:56:38 UTC) #33

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-04 18:58:29 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/3214) ios_arm64_dbg on ...

3 years, 8 months ago (2017-04-04 18:58:30 UTC) #35

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 8 months ago (2017-04-04 19:52:07 UTC) #36

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.webrtc.org/2770233003/280001

3 years, 8 months ago (2017-04-04 19:52:11 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-04 20:13:35 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/20305)

3 years, 8 months ago (2017-04-04 20:13:36 UTC) #39

the sun

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode232 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:232: std::set<uint32_t> selected_ssrcs; Should the SSRCs go first in the ...

3 years, 8 months ago (2017-04-04 21:10:00 UTC) #41

Zhi Huang

Description was changed from ========== Added the GetContributingSource() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-05 03:33:06 UTC) #42

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

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

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-05 03:35:45 UTC) #44

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-05 03:36:21 UTC) #45

Zhi Huang

Since the working group have achieved some agreements on this, there are some changes on ...

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

Since the working group have achieved some agreements on this, there are some
changes on the interface and struct in this patch.
 
Please take another look.
Thanks.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:232: std::set<uint32_t>
selected_ssrcs;
On 2017/04/04 21:09:58, the sun wrote:
> Should the SSRCs go first in the result, or doesn't that matter?

I don't think this matters but it feels that the latest SSRC comes first makes
more sense.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:512: // If it is a new CSRC,
append a new object to the end of the list.
On 2017/04/04 21:09:58, the sun wrote:
> nit: move comment inside if (..) {

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:515:
contributing_source.set_timestamp(now);
On 2017/04/04 21:09:58, the sun wrote:
> Make these arguments to ctor instead, to avoid the risk of having a
half-inited
> object. It also allows you to use emplace_back() below. See:
> http://en.cppreference.com/w/cpp/container/list/emplace_back

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:526: auto new_list_it =
std::prev(csrc_source_list_.end());
On 2017/04/04 21:09:57, the sun wrote:
> avoid the named temp and make it a single statement

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:530: if
(csrc_source_list_.size() + ssrc_source_list_.size() >
On 2017/04/04 21:09:58, the sun wrote:
> nit: add () around the addition, to make it clear what the intention is

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:539:
ssrc_source_list_[ssrc_source_list_.size() - 1].ssrc() != ssrc_) {
On 2017/04/04 21:09:58, the sun wrote:
> std::prev(ssrc_contributing_sources_.end())->ssrc()

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:543:
ssrc_source_list_.push_back(ssrc_source);
On 2017/04/04 21:09:58, the sun wrote:
> emplace_back

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:553: if (now -
(*it).timestamp() <= kContributingSourcesTimeoutMs) {
On 2017/04/04 21:09:58, the sun wrote:
> Use it->... instead of (*it). (here and elsewhere)

Done.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:565:
ssrc_source_list_.erase(it);
On 2017/04/04 21:09:58, the sun wrote:
> This invalidates 'it' so the ++it in the loop will either fail or cause a
crash.
> Is there a unit test missing?

Thanks for catching this. I'll add a test to cover this case.

 
> Since you know the list elements are sorted, you do not need to erase one at a
> time. You could just search through the container for the first element <=
> kContributingSourcesTimeoutMs and do a single erase of all elements up to that
> point.

Yes, but for the csrc_contributing_sources we may still need to erase it one by
one since we also need to remove the entry from the map.

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:98: // The contributing
sources that uses the CSRC.
On 2017/04/04 21:09:59, the sun wrote:
> I'd prefer to name the member "csrc_contributing_sources_" and remove the
> comment.

We just achieved some agreements on this.
I'll use:
"csrc_sources_" for the "contributing source" 
"ssrc_sources_" for the "synchronization source".

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/api/rtpreceiverin...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:30: struct RtpContributingSource {
On 2017/04/04 21:09:59, the sun wrote:
> I'd suggest instead:
> 
> class RtpContributingSource {
>  public:
>   RtpContributingSource() = delete;
>   RtpContributingSource(int64_t timestamp, uint32_t source)
>       : timestamp_(timestamp), source_(source) {}
> 
>   int64_t timestamp() const { return timestamp_; }
>   void update_timestamp(int64_t timestamp) {
>     RTC_DCHECK_LE(timestamp_, timestamp);
>     timestamp_ = timestamp;
>   }
> 
>   uint32_t source() const { return source_; }
> 
>   // This isn't implemented yet and will always return an empty Optional.
>   // TODO(zhihuang): Implement this to return real audio level.
>   rtc::Optional<int8_t> audio_level() const { return rtc::Optional<int8_t>();
}
> 
>  private:
>   int64_t timestamp_;
>   uint32_t source_;
> };
> 
> 1. We shouldn't lug around the extra weight of two optionals we're not using.
> 2. Adding the ctor arguments allows us to use emplace_back() with the
> containers, and enforces the invariant of the class - you *can't* construct it
> without giving it a timestamp and source.

The working group had some discussions and achieved an agreement on what the api
should look like. I've made some changes based on your suggestion.

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_receiver.h (right):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_receiver.h:96: virtual
std::vector<RtpContributingSource> GetContributingSources() = 0;
On 2017/04/04 21:09:59, the sun wrote:
> Can we 'const' this all the way up?

I think we can do that since they are expected to be read only.

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1671:
Bind(&WebRtcVoiceMediaChannel::GetContributingSources,
On 2017/04/04 21:09:59, the sun wrote:
> Looking at the other methods, the pattern is to call via media_channel() here,
> and add an overridable method on VoiceMediaChannel, instead of casting.
However,
> this is all subject for refactoring and I don't know the code that well, so
> check with deadbeef@ or pthatcher@ if they're ok with the way you're doing it.

I didn't add the method to the base class because there would be a circular
dependency between mediachannel.h and rtpreceiverinterface.h

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (left):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:36: using DegradationPreference =
VideoSendStream::DegradationPreference;
On 2017/04/04 21:09:59, the sun wrote:
> bad merge?

Nope. When introducing api/rtpreceiverinterface.h to the dependency graph, this
DegradationPerference happens to conflict with another enum with the same name
in api/rtpparameters.h. They are actually referring to the same thing, I'll
unify them in another stand alone CL.

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:56: class RtpReceiver;
On 2017/04/04 21:09:59, the sun wrote:
> remove

Done.

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2770233003/diff/320001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:125: virtual
std::vector<webrtc::RtpContributingSource> GetContributingSources();
On 2017/04/04 21:09:59, the sun wrote:
> Add it to the MockChannelProxy too, in mock_voe_channel_proxy.h

Done.

Taylor Brandstetter

lgtm with a couple nits and minor comments about tests. Looks great to me. https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc ...

3 years, 8 months ago (2017-04-05 04:27:56 UTC) #48

stefan-webrtc

stefan@webrtc.org changed reviewers: - stefan@webrtc.org

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

hbos

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h#newcode35 webrtc/api/rtpreceiverinterface.h:35: struct RtpSource { Considering the getters/setters and private members, ...

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

the sun

Looking really good! A few more comments (some of them just echoing hbos'). https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h File ...

3 years, 8 months ago (2017-04-05 14:58:30 UTC) #51

hbos

Yes looking good. Super tiny nit. https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h#newcode30 webrtc/api/rtpreceiverinterface.h:30: enum class RtpSourceType ...

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

danilchap

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h#newcode43 webrtc/api/rtpreceiverinterface.h:43: int64_t timestamp() const { return timestamp_; } can you ...

3 years, 8 months ago (2017-04-05 16:24:22 UTC) #53

Zhi Huang

Sorry about my silly mistakes. Please take another look. Thanks. https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverinterface.h#newcode17 ...

3 years, 8 months ago (2017-04-06 03:09:51 UTC) #58

Sorry about my silly mistakes. Please take another look. 
Thanks.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:17: #include <memory>
On 2017/04/05 14:58:29, the sun wrote:
> Why the need to add this?

Oh, I missed this when removing the unique_ptr.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:30: enum class RtpSourceType {
On 2017/04/05 15:20:16, hbos wrote:
> nit: With enum class you have to qualify the name with "RtpSourceType::" so it
> would be enough to call these "SSRC" and "CSRC".

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:35: struct RtpSource {
On 2017/04/05 14:58:29, the sun wrote:
> On 2017/04/05 11:15:41, hbos wrote:
> > Considering the getters/setters and private members, I'd make this class
> rather
> > than struct.
> 
> +1, make it a class!
> 
> Btw, thanks for not adding any more ways to modify the data than
> update_timestamp() - this looks great!

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:43: int64_t timestamp() const { return
timestamp_; }
On 2017/04/05 16:24:21, danilchap wrote:
> can you add units, is it timestamp_ms?

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/rtpreceiverin...
webrtc/api/rtpreceiverinterface.h:104: virtual std::vector<RtpSource>
GetSources() {
On 2017/04/05 14:58:29, the sun wrote:
> Make this method const:
> virtual std::vector<RtpSource> GetSources() const {
>   ...

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/test/mock_rtp...
File webrtc/api/test/mock_rtpreceiver.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/api/test/mock_rtp...
webrtc/api/test/mock_rtpreceiver.h:14: #include <memory>
On 2017/04/05 14:58:29, the sun wrote:
> Needed why?

I missed this when removing the unique_ptrs.
Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:53: const std::vector<webrtc::RtpSource>
GetSources() override;
On 2017/04/05 14:58:29, the sun wrote:
> On 2017/04/05 11:15:41, hbos wrote:
> > Make the return value non-const since it's returned by value.
> 
> +1 the method should be const, not the return value.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/call/audio_receiv...
File webrtc/call/audio_receive_stream.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/call/audio_receiv...
webrtc/call/audio_receive_stream.h:137: virtual const std::vector<RtpSource>
GetSources() = 0;
On 2017/04/05 11:15:41, hbos wrote:
> Make the return value non-const since it's returned by value.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:30: static const int64_t
kGetSourcesTimeoutMs = 10000;
On 2017/04/05 16:24:21, danilchap wrote:
> constants outside class doesn't have to be marked static.
> +nit: put them right after "using RtpUtility::Payload"

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:32: // The maximum size of
the sources lists.
On 2017/04/05 14:58:30, the sun wrote:
> nit: "The maximum combined size"...

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:222: 
On 2017/04/05 14:58:30, the sun wrote:
> Since we assume that the lists are sorted in the below loops, we should add a
> DCHECK that they are, e.g.:
> 
> RTC_DCHECK(std::is_sorted(ssrc_sources_.begin(), ssrc_sources_.end(),
>     [](const RtpSource& lhs, const RtpSource& rhs) { return lhs.timestamp() <
> rhs.timestamp(); });
> RTC_DCHECK(std::is_sorted(csrc_sources_.begin(), csrc_sources_.end(),
>     [](const RtpSource& lhs, const RtpSource& rhs) { return lhs.timestamp() <
> rhs.timestamp(); });
> 
> http://www.cplusplus.com/reference/algorithm/is_sorted/

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:226: if (now -
rit->timestamp() > kGetSourcesTimeoutMs) {
On 2017/04/05 14:58:30, the sun wrote:
> nit: () around the subtraction, here and below

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:229: if
(selected_ssrcs.find(rit->source_id()) == selected_ssrcs.end()) {
On 2017/04/05 16:24:21, danilchap wrote:
> to avoid double lookup you can use return value of the insert:
> if (selected_ssrcs.insert(rit->source_id()).second) {
>   sources.push_back(*rit);
> }

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:523: // Remove the out of
date objects if the lists are too large.
On 2017/04/05 11:15:41, hbos wrote:
> nit: I think this bit should be moved to the end of UpdateSources(), done once
> and after |ssrc_sources_| is updated. Otherwise we could theoretically do it
> |num_csrcs_| times, and UpdateSources() iterates all of |csrc_sources_|.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:539: // Update the lists and
remove the out of date objects.
On 2017/04/05 14:58:29, the sun wrote:
> With a different function name you wouldn't need the explaining comment, e.g.:
> LimitSourceListSizes()

I'll remove the length limitation and just call this RemoveOutdatedSources().

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:540: void
RtpReceiverImpl::UpdateSourceLists(int64_t now) {
On 2017/04/05 11:15:41, hbos wrote:
> We only throw away outdated objects. Max object count is never enforced. I
think
> this makes kMaxSourceListSize misleading. I think we should always update to
> remove outdates objects. Because the lists are sorted, if the first element is
> not outdated we can stop looking, so it is efficient not to have a
> kMaxSourceListSize.
> 
> If we want to have a max source size for security reasons then we should
really
> enforce the list size, and throw away even up-to-date objects that exceed the
> limit.

I added this because I thought the idea is to make the per packet operation as
simple as possible. But I don't like kMaxSourceListSize either and I can't
justify why it is 100. I'll remove it.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:546: it =
csrc_sources_.erase(it);
On 2017/04/05 14:58:29, the sun wrote:
> On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> > extreme nit: I think this could just do "++it" here, and then outside the
> loop,
> > remove everything up to "it" with:
> > 
> > csrc_sources_.erase(csrc_soruces_.begin(), it);
> > 
> > Which could have a slight performance benefit?
> 
> Possibly, but likely not since it's a std::list. Would be consistent with the
> below loop though so I think it would still make sense.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:553: }
On 2017/04/05 11:15:41, hbos wrote:
> Because of the break this will at most remove one ssrc. If multiple packets
> arrive the same time, ssrc_sources_ could contain outdated objects. It should
> loop for as long as !empty && first element is outdated.
> 
> Since we're doing removal of the first element, how about using list instead
of
> vector?

I don't think it will only remove one ssrc.

The iterator keeps moving until it finds the first one that is not out of date
(goes into the if condition), remove all the objects before it and then break.
There is at least one source that is not outdated, because this method is only
called when a new packet is received.

It seems that the difference is "erase then break" and "break then erase".

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:553: }
On 2017/04/05 14:58:30, the sun wrote:
> On 2017/04/05 11:15:41, hbos wrote:
> > Because of the break this will at most remove one ssrc. If multiple packets
> > arrive the same time, ssrc_sources_ could contain outdated objects. It
should
> > loop for as long as !empty && first element is outdated.
> > 
> > Since we're doing removal of the first element, how about using list instead
> of
> > vector?
> 
> The idea is to remove all old elements in one go. Missing test?

I've added a test for this. 
I agree with Taylor (his comments in the unit tests) that there is no clear way
to do black box test. So I exposed the crsc_sources_ and the ssrc_sources_ in
the RtpReceiverImpl and downcasted the RtpReceiver in the test. Is this
acceptable?

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:554: }
On 2017/04/05 14:58:29, the sun wrote:
> The erase should go here, after searching for the first item to remain in the
> list.

Alright. This looks more clear.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:62: const
std::vector<RtpSource> GetSources() override;
On 2017/04/05 14:58:30, the sun wrote:
> On 2017/04/05 11:15:42, hbos wrote:
> > Make the return value non-const since it's returned by value.
> 
> +1 but make the function const:
> std::vector<RtpSource> GetSources() const override;

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:98: std::list<RtpSource>
csrc_sources_;
On 2017/04/05 11:15:42, hbos wrote:
> Add a comment saying these are sorted chronologically.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:33: void SetUp()
override {
On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> nit: This code can just go in the constructor.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:47:
std::unique_ptr<RTPPayloadRegistry> rtp_payload_registry_;
On 2017/04/05 16:24:21, danilchap wrote:
> nit: doesn't need to wrap it in unique_ptr

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:49: SimulatedClock
fake_clock;
On 2017/04/05 16:24:22, danilchap wrote:
> put fake_clock before rtp_receiver_ to be sure it is destroyed after.
> (also, it will allow to initialize rtp_receiver_ in the initialization list of
> the constructor.)

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:69: EXPECT_EQ(1u,
sources[0].source_id());
On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> nit: We aren't guaranteeing anything about the order, so the test should
> probably be written in an order-agnostic way. For example, using a "find by
> source ID" helper function.

Maybe should use FindSourceByIdAndType since two sources can have same id but
different type.
Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:97:
fake_clock.AdvanceTimeMilliseconds(kGetSourcesTimeoutMs + 1);
On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> To make the test even more thorough, could advance the time to
> kGetSourcesTimeoutMs - 1 and verify that they still are returned.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:170:
TEST_F(RtpReceiverTest, GetSourcesMaxListSize) {
On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> This test would succeed even if the out-of-date objects weren't removed,
because
> they'd be filtered when GetSources is called. I understand this test is meant
to
> exercise the "remove old objects" code, but there isn't really a way to test
> that in a black box fashion.

Agreed. Thanks for catching this.
I plan to expose the source list from the RtpReceiverImpl and downcast the
RtpReceiver to get the list directly. Does it make sense to you? I think it
might be acceptable to have downcasting in the test.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:495: const std::vector<webrtc::RtpSource>
GetSources(uint32_t ssrc);
On 2017/04/05 11:15:42, hbos wrote:
> Make the return value non-const since it's returned by value.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:2767:
kDefaultExpectedAudioFrameCount, kDefaultExpectedVideoFrameCount,
On 2017/04/05 04:27:55, Taylor Brandstetter wrote:
> nit: Only really need one audio track, from caller to callee, and only need to
> wait for one audio frame to be received.

I thought if it works in more complicated cases, it will also works in the one
way audio only case. But I guess the simple case would be enough since we are
only testing the pipeline.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/test/mock_voe_cha...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/test/mock_voe_cha...
webrtc/test/mock_voe_channel_proxy.h:18: #include
"webrtc/voice_engine/channel.h"
On 2017/04/05 14:58:30, the sun wrote:
> No, do not include voice_engine/channel.h here. In fact, since this is mocking
> methods already declared in voice_engine/channel_proxy.h, we should be able to
> rely on transient includes from there.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:155:
VideoSendStream::DegradationPreference::kMaintainResolution),
On 2017/04/05 11:15:42, hbos wrote:
> Shouldn't this be kDegradationDisabled?

Oh yes, this is a bad merge.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:402: const std::vector<RtpSource> GetSources() {
On 2017/04/05 11:15:42, hbos wrote:
> Make the return value non-const since it's returned by value.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:39: struct RtpSource;
On 2017/04/05 14:58:30, the sun wrote:
> Can't forward declare since propagating by-value. Include api/rtpreceiver.h
> instead.

Done.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:125: virtual const
std::vector<webrtc::RtpSource> GetSources();
On 2017/04/05 11:15:42, hbos wrote:
> Make the return value non-const since it's returned by value.

Done.

the sun

A work of art! LGTM % nit https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode30 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:30: static const ...

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

A work of art!

LGTM % nit

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:30: static const int64_t
kGetSourcesTimeoutMs = 10000;
On 2017/04/06 03:09:49, Zhi Huang wrote:
> On 2017/04/05 16:24:21, danilchap wrote:
> > constants outside class doesn't have to be marked static.
> > +nit: put them right after "using RtpUtility::Payload"
> 
> Done.

They should still be made private to the translation unit from the linker's
point of view, otherwise crazy things can happen if a constant/function/class
with the same name is defined elsewhere. Anonymous namespaces are preferred for
this.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:539: // Update the lists and
remove the out of date objects.
On 2017/04/06 03:09:50, Zhi Huang wrote:
> On 2017/04/05 14:58:29, the sun wrote:
> > With a different function name you wouldn't need the explaining comment,
e.g.:
> > LimitSourceListSizes()
> 
> I'll remove the length limitation and just call this RemoveOutdatedSources().

Much better than my suggestion!

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:553: }
On 2017/04/06 03:09:49, Zhi Huang wrote:
> On 2017/04/05 11:15:41, hbos wrote:
> > Because of the break this will at most remove one ssrc. If multiple packets
> > arrive the same time, ssrc_sources_ could contain outdated objects. It
should
> > loop for as long as !empty && first element is outdated.
> > 
> > Since we're doing removal of the first element, how about using list instead
> of
> > vector?
> 
> I don't think it will only remove one ssrc.
> 
> The iterator keeps moving until it finds the first one that is not out of date
> (goes into the if condition), remove all the objects before it and then break.
> There is at least one source that is not outdated, because this method is only
> called when a new packet is received.
> 
> It seems that the difference is "erase then break" and "break then erase".

True, both works.

https://codereview.webrtc.org/2770233003/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:553: }
On 2017/04/06 03:09:50, Zhi Huang wrote:
> On 2017/04/05 14:58:30, the sun wrote:
> > On 2017/04/05 11:15:41, hbos wrote:
> > > Because of the break this will at most remove one ssrc. If multiple
packets
> > > arrive the same time, ssrc_sources_ could contain outdated objects. It
> should
> > > loop for as long as !empty && first element is outdated.
> > > 
> > > Since we're doing removal of the first element, how about using list
instead
> > of
> > > vector?
> > 
> > The idea is to remove all old elements in one go. Missing test?
> 
> I've added a test for this. 
> I agree with Taylor (his comments in the unit tests) that there is no clear
way
> to do black box test. So I exposed the crsc_sources_ and the ssrc_sources_ in
> the RtpReceiverImpl and downcasted the RtpReceiver in the test. Is this
> acceptable?

Yes, it's a good idea though to mark such functions in some way so their purpose
is clear to the casual reader.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:64: const
std::vector<RtpSource>& ssrc_sources() { return ssrc_sources_; }
Is this for testing? Mark that with a comment, or changing the names to e.g.
..._for_testing().

And the methods should be const.

hbos

lgtm https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode548 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:548: ++it; nit: now that every iteration ends with ...

3 years, 8 months ago (2017-04-06 08:17:17 UTC) #60

philipel

lgtm regarding rtp_receiver_impl.{h,cc}, mostly suggestions/nits to make the code cleaner. https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode215 ...

3 years, 8 months ago (2017-04-06 14:33:34 UTC) #61

Zhi Huang

On 2017/04/06 19:01:34, the sun wrote: > We're missing an API level test for this. ...

3 years, 8 months ago (2017-04-06 19:55:18 UTC) #63

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

3 years, 8 months ago (2017-04-06 22:28:28 UTC) #65

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.webrtc.org/2770233003/490001

3 years, 8 months ago (2017-04-06 22:28:31 UTC) #66

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-06 22:30:23 UTC) #67

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/3308) ios_arm64_dbg on ...

3 years, 8 months ago (2017-04-06 22:30:24 UTC) #68

Zhi Huang

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode215 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:215: std::vector<RtpSource> sources; On 2017/04/06 14:33:34, philipel wrote: > I ...

3 years, 8 months ago (2017-04-06 22:30:25 UTC) #69

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:215: std::vector<RtpSource>
sources;
On 2017/04/06 14:33:34, philipel wrote:
> I think it would be cleaner if this was an std::set. You can then remove
> |seleceted_ssrcs| and simply return std::vector<RtpSource>(sources.begin(),
> sources.end());

But then we'll need an additional compare struct for the RtpSource since we'll
remove the objects with same ssrc regardless other attributes.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:512: int64_t now =
clock_->TimeInMilliseconds();
On 2017/04/06 14:33:34, philipel wrote:
> now_ms

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:526:
iterator_by_csrc_[current_remote_csrc_[i]] = std::prev(csrc_sources_.end());
On 2017/04/06 14:33:34, philipel wrote:
> std::prev(csrc_sources_.end()) --> csrc_sources_.rbegin()

The value of the map is not reverse_iterator since I need the forward iterator
for std::list::splice.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:531: if
(ssrc_sources_.size() == 0 ||
On 2017/04/06 14:33:34, philipel wrote:
> ssrc_sources_.empty()

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:532:
std::prev(ssrc_sources_.end())->source_id() != ssrc_) {
On 2017/04/06 14:33:34, philipel wrote:
> std::prev(ssrc_sources_.end()) --> ssrc_sources_.rbegin()

This is a good one!

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:535:
std::prev(ssrc_sources_.end())->update_timestamp_ms(now);
On 2017/04/06 14:33:34, philipel wrote:
> rbegin()

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:548: ++it;
On 2017/04/06 08:17:16, hbos wrote:
> nit: now that every iteration ends with ++it you can move ++it to the end of
the
> for loop head.

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:64: const
std::vector<RtpSource>& ssrc_sources() { return ssrc_sources_; }
On 2017/04/06 06:55:38, the sun wrote:
> Is this for testing? Mark that with a comment, or changing the names to e.g.
> ..._for_testing().
> 
> And the methods should be const.

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:103: std::list<RtpSource>
csrc_sources_;
On 2017/04/06 14:33:34, philipel wrote:
> I think an std::deque might be better in this case since we only insert/remove
> at begining/end of the container.

For csrc_sources, we need to use the std::list::splice operation to maintain the
order.
For ssrc_sources, yes, we can use a deque. But this collection is not expected
to be large, not sure if it will make a big difference.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:67: int64_t timestamp =
fake_clock_.TimeInMilliseconds();
On 2017/04/06 08:17:16, hbos wrote:
> nit/optionally: I'd just remove timestamp and call the fake clock directly
> whenever we want the "now" timestamp.

Done.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:109:
fake_clock_.AdvanceTimeMilliseconds(kGetSourcesTimeoutMs);
On 2017/04/06 08:17:17, hbos wrote:
> This should have the "// Time out."
> You have already done AdvanceTimeMilliseconds(1) so the total is
> kGetSourcesTimeoutMs+1

Not exactly.

After clock is advanced by 1ms (line 95), a new packet comes (line 96) and the
timestamps of the sources are updated. Then the clock is advanced
kGetSourceTimeoutMs and it is not timeout. I'll add a comment to make it more
clear.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:110: ASSERT_EQ(3u,
sources.size());
On 2017/04/06 08:17:17, hbos wrote:
> These asserts/expects are doing the same thing as the above block since
> |sources| have not been updated after timing out. Remove this block, we have
not
> modified |sources| so there is no point to checking its value again.

Oh I meant to test that the sources are still there just before the timeout. I
should call GetSources() again here. Thanks for catching this.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:122: sources =
rtp_receiver_->GetSources();
On 2017/04/06 08:17:16, hbos wrote:
> Move this and the next line to after
> AdvanceTimeMilliseconds(kGetSourcesTimeoutMs).
> Remove the AdvanceTimeMilliseconds(1).

Please see the explanation above.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:2774: }
On 2017/04/06 08:17:17, hbos wrote:
> Should we have the same test but for video too? This should work for both
audio
> and video?

To unblock the Thor team, this only works for audio for now. We'll make it work
for video later.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-06 22:30:40 UTC) #70

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/3308) ios_arm64_dbg on ...

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

Zhi Huang

The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run

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

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.webrtc.org/2770233003/510001

3 years, 8 months ago (2017-04-06 22:33:55 UTC) #73

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-06 23:01:05 UTC) #74

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-06 23:01:06 UTC) #75

hbos

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-07 08:19:29 UTC) #76

hbos

Can we land this? :) My follow-up chromium CL just got LGTM'd: https://codereview.chromium.org/2803693002/ https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc File ...

3 years, 8 months ago (2017-04-07 08:22:21 UTC) #77

philipel

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#newcode215 webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:215: std::vector<RtpSource> sources; On 2017/04/06 22:30:25, Zhi Huang wrote: > ...

3 years, 8 months ago (2017-04-07 08:44:07 UTC) #78

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:215: std::vector<RtpSource>
sources;
On 2017/04/06 22:30:25, Zhi Huang wrote:
> On 2017/04/06 14:33:34, philipel wrote:
> > I think it would be cleaner if this was an std::set. You can then remove
> > |seleceted_ssrcs| and simply return std::vector<RtpSource>(sources.begin(),
> > sources.end());
> 
> But then we'll need an additional compare struct for the RtpSource since we'll
> remove the objects with same ssrc regardless other attributes.

Acknowledged.

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:526:
iterator_by_csrc_[current_remote_csrc_[i]] = std::prev(csrc_sources_.end());
On 2017/04/06 22:30:25, Zhi Huang wrote:
> On 2017/04/06 14:33:34, philipel wrote:
> > std::prev(csrc_sources_.end()) --> csrc_sources_.rbegin()
> 
> The value of the map is not reverse_iterator since I need the forward iterator
> for std::list::splice.

Ah... ofc :)

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h (right):

https://codereview.webrtc.org/2770233003/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.h:103: std::list<RtpSource>
csrc_sources_;
On 2017/04/06 22:30:25, Zhi Huang wrote:
> On 2017/04/06 14:33:34, philipel wrote:
> > I think an std::deque might be better in this case since we only
insert/remove
> > at begining/end of the container.
> 
> For csrc_sources, we need to use the std::list::splice operation to maintain
the
> order.
> For ssrc_sources, yes, we can use a deque. But this collection is not expected
> to be large, not sure if it will make a big difference.

I was thinking instead of splicing, just push_front(back()), update the
timestamp and then pop_back(), but as you said, I don't think it matters much in
this case.

Zhi Huang

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, solenberg@webrtc.org, philipel@webrtc.org Link ...

3 years, 8 months ago (2017-04-07 17:19:45 UTC) #80

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-07 17:19:55 UTC) #81

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-07 17:24:53 UTC) #82

commit-bot: I haz the power

Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15902)

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

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-07 17:43:38 UTC) #84

Zhi Huang

zhihuang@webrtc.org changed reviewers: + stefan@webrtc.org

3 years, 8 months ago (2017-04-07 17:43:38 UTC) #85

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

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

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-07 17:49:10 UTC) #87

Zhi Huang

zhihuang@webrtc.org changed reviewers: + danilchap@webrtc.org

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

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-07 17:54:45 UTC) #90

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 510001, "attempt_start_ts": 1491587672432900, "parent_rev": "bb16a483f2eae187dadd38da4220cb2bba0259d5", "commit_rev": "292084c3765d9f3ee406ca2ec86eae206b540053"}

3 years, 8 months ago (2017-04-07 17:57:24 UTC) #91

commit-bot: I haz the power

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-07 17:57:30 UTC) #92

Message was sent while issue was closed.

Description was changed from

==========
Added the GetSources() to the RtpReceiverInterface and implemented
it for the AudioRtpReceiver.

This method returns a vector of RtpSource(both CSRC source and SSRC 
source) which contains the ID of a source, the timestamp, the source
type (SSRC or CSRC) and the audio level.

The RtpSource objects are buffered and maintained by the
RtpReceiver in webrtc/modules/rtp_rtcp/. When the method is called,
the info of the contributing source will be pulled along the object
chain:
AudioRtpReceiver -> VoiceChannel -> WebRtcVoiceMediaChannel ->
AudioReceiveStream -> voe::Channel -> RtpRtcp module

Spec:https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#widl-RTCRtpReceiver-getContributingSources-sequence-RTCRtpContributingSource

BUG=chromium:703122
TBR=stefan@webrtc.org, danilchap@webrtc.org
==========

to

==========
Added the GetSources() to the RtpReceiverInterface and implemented
it for the AudioRtpReceiver.

This method returns a vector of RtpSource(both CSRC source and SSRC 
source) which contains the ID of a source, the timestamp, the source
type (SSRC or CSRC) and the audio level.

The RtpSource objects are buffered and maintained by the
RtpReceiver in webrtc/modules/rtp_rtcp/. When the method is called,
the info of the contributing source will be pulled along the object
chain:
AudioRtpReceiver -> VoiceChannel -> WebRtcVoiceMediaChannel ->
AudioReceiveStream -> voe::Channel -> RtpRtcp module

Spec:https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#widl-RTCRtpReceiver-getContributingSources-sequence-RTCRtpContributingSource

BUG=chromium:703122
TBR=stefan@webrtc.org, danilchap@webrtc.org

Review-Url: https://codereview.webrtc.org/2770233003
Cr-Commit-Position: refs/heads/master@{#17591}
Committed:
https://chromium.googlesource.com/external/webrtc/+/292084c3765d9f3ee406ca2ec...
==========

commit-bot: I haz the power

Committed patchset #11 (id:510001) as https://chromium.googlesource.com/external/webrtc/+/292084c3765d9f3ee406ca2ec86eae206b540053

3 years, 8 months ago (2017-04-07 17:57:31 UTC) #93

Zhi Huang

On 2017/04/07 08:22:21, hbos wrote: > Can we land this? :) My follow-up chromium CL ...

3 years, 8 months ago (2017-04-07 17:59:03 UTC) #94

Zhi Huang

Description was changed from ========== Added the GetSources() to the RtpReceiverInterface and implemented it for ...

3 years, 8 months ago (2017-04-07 19:46:41 UTC) #95

Message was sent while issue was closed.

Description was changed from

==========
Added the GetSources() to the RtpReceiverInterface and implemented
it for the AudioRtpReceiver.

This method returns a vector of RtpSource(both CSRC source and SSRC 
source) which contains the ID of a source, the timestamp, the source
type (SSRC or CSRC) and the audio level.

The RtpSource objects are buffered and maintained by the
RtpReceiver in webrtc/modules/rtp_rtcp/. When the method is called,
the info of the contributing source will be pulled along the object
chain:
AudioRtpReceiver -> VoiceChannel -> WebRtcVoiceMediaChannel ->
AudioReceiveStream -> voe::Channel -> RtpRtcp module

Spec:https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#widl-RTCRtpReceiver-getContributingSources-sequence-RTCRtpContributingSource

BUG=chromium:703122
TBR=stefan@webrtc.org, danilchap@webrtc.org

Review-Url: https://codereview.webrtc.org/2770233003
Cr-Commit-Position: refs/heads/master@{#17591}
Committed:
https://chromium.googlesource.com/external/webrtc/+/292084c3765d9f3ee406ca2ec...
==========

to

==========
Added the GetSources() to the RtpReceiverInterface and implemented
it for the AudioRtpReceiver.

This method returns a vector of RtpSource(both CSRC source and SSRC 
source) which contains the ID of a source, the timestamp, the source
type (SSRC or CSRC) and the audio level.

The RtpSource objects are buffered and maintained by the
RtpReceiver in webrtc/modules/rtp_rtcp/. When the method is called,
the info of the contributing source will be pulled along the object
chain:
AudioRtpReceiver -> VoiceChannel -> WebRtcVoiceMediaChannel ->
AudioReceiveStream -> voe::Channel -> RtpRtcp module

Spec:https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#widl-RTCRtpReceiver-getContributingSources-sequence-RTCRtpContributingSource

BUG=chromium:703122
TBR=stefan@webrtc.org, danilchap@webrtc.org

Review-Url: https://codereview.webrtc.org/2770233003
Cr-Commit-Position: refs/heads/master@{#17591}
Committed:
https://chromium.googlesource.com/external/webrtc/+/292084c3765d9f3ee406ca2ec...
==========

olka_webrtc

A revert of this CL (patchset #11 id:510001) has been created in https://codereview.webrtc.org/2809613002/ by olka@webrtc.org. ...

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

hbos

On 2017/04/10 10:18:13, olka_webrtc wrote: > A revert of this CL (patchset #11 id:510001) has ...

3 years, 8 months ago (2017-04-10 10:27:50 UTC) #97

danilchap

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

Message was sent while issue was closed.

/rtp_rtcp/ lgtm
with few optional suggestions how tests can look better.

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right):

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:217: {
taking current time and constructing an empty vector and returning vector by
value can be done inside lock too, and this extra indentation can be removed.

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right):

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:22: 
tests, specially with constants, better to place inside unnamed namespace

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:69: header.ssrc = 1;
might be better to name all [cs]src constants, specially when the value (1) look
like it might be a size.
const uint32_t kSsrc = 123;
const uint32_t kCsrc1 = 111;
const uint32_t kCsrc2 = 222;
// or const uint32_t kCsrc[] = { 111, 222 };

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:70: header.timestamp =
fake_clock_.TimeInMilliseconds();
it looks wrong to use current_time as rtp timestamp, specially when you have
kTestRate != 1000
may be make a small helper
static uint32_t rtp_timestamp(int64_t time_ms) {
  return static_cast<uint32_t>(time_ms * kTestRate / 1000);
}
or
uint32_t rtp_timestamp_now() const;
(It doesn't cover large values of time_ms, but should be fine for these tests)

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:75: bool in_order =
false;
may be bool in_order = true; // or better kInOrder = true;
and then, when call IncomingRtpPacket, write !kInOrder

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:78:
EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket(header, kTestPayload, 4,
may be sizeof(kTestPayload) instead of 'magic' value 4

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:82: ASSERT_EQ(3u,
sources.size());
with gmock and RtpSource::operator== or customer matcher you might write
int64_t now_ms = fake_clock_.TimeInMilliseconds();
EXPECT_THAT(sources, UnorderedElementsAre(
RtpSource(now_ms, kSsrc, RtpSourceType::SSRC),
RtpSource(now_ms, kCsrc1, RtpSourceType::CSRC),
RtpSource(now_ms, kCsrc2, RtpSourceType::CSRC)));
instead of 7 ASSERT/EXPECT

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:133: int64_t cur_time =
fake_clock_.TimeInMilliseconds();
now_ms is probably more common name in the codebase.

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:202: for (size_t i = 0;
i < kSourceListsSize; ++i) {
any plan to use this constant in another test? if not - define it inside the
test instead of top of the file. (if yes - still might good to define it inside
each test, like you do with in_order flag)

https://codereview.webrtc.org/2770233003/diff/510001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:204:
header.arrOfCSRCs[0] = (i + 1);
is it intended CSRC is same as SSRC of the next packet?

Issue 2770233003: Implemented the GetSources() in native code. (Closed)

Description

Patch Set 1 : Add the interface #

Patch Set 2 : Merge and add the tests. #

Patch Set 3 : Address the comments related to threading and the special ContributingSource that uses the SSRC. #

Patch Set 4 : Use map+list. #

Patch Set 5 : Merge with origin/master. #

Patch Set 6 : Fix the BUILD file. #

Patch Set 7 : Try to fix the build failure on the bots. #

Patch Set 8 : Renaming. Add a unit test. Resolve the comments. #

Patch Set 9 : Resolve the comments. #

Patch Set 10 : Resolve the comments. Fix the nits. #

Patch Set 11 : Add a direct dependency to the webrtc/voice_engine/BUILD.gn #

Messages