Base A/V synchronization on sync_labels.

Base A/V synchronization on sync_labels.

Groups of streams that should be synchronized are signalled through
SDP. These should be used rather than synchronizing the first-added
video stream to the first-added audio stream implicitly.

BUG=webrtc:4667
R=hta@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org
TBR=mflodman@webrtc.org

Committed: https://crrev.com/8fc7fa798f7a36955f1b933980401afad2aff592
Cr-Commit-Position: refs/heads/master@{#9586}

[pbos-webrtc, 2015-06-11 10:38:56]

add missing tests + fix bug

[pbos-webrtc, 2015-06-11 10:40:40]

PTAL, this might require a couple of bounces back and forth.

[pbos-webrtc, 2015-06-11 10:41:58]

rebase

[pbos-webrtc, 2015-06-11 10:46:24]

fix win compile error, bah

[the sun, 2015-06-11 11:52:04]

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1690: const std::string& sync_label()
const { return sync_label_; }
Adding the sync label here feels backwards to me. I think going forward we will
be better served with a map ssrc->StreamParams, e.g. receive_stream_params_. and
to look there for the sync label.

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2487: new
WebRtcVoiceChannelRenderer(channel, sp.sync_label, audio_transport)));
This will never be used in practise?

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:3630: WebRtcVoiceChannelRenderer*
channel) {
No need to take the ..Renderer* as argument. We can look it up from the
receive_channels_ map. Yes, I know you will say it requires an unnecessary map
lookup, to which I will answer it doesn't matter here (1. few objects in the
map, 2. code due for refactoring) - keep the interface simple instead!

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:3634: if (!call_)
{} please.

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:48: int channel_id = -1;
voe_channel_id

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:51: // TODO(pbos): Synchronize streams in a sync
group, not just video streams
Is this TODO necessary? Will it be addressed in the foreseeable future? Better
add an issue for it instead?

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv...
File webrtc/video/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv...
webrtc/video/audio_receive_stream.cc:51: CHECK(config.channel_id != -1);
Why not DCHECK()? If the channel doesn't exist, subsequent calls to VoE will
fail anyway.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.cc:247: DCHECK(config.channel_id != -1);
Validate config in object ctor instead.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:962: // Test that changing the
combined_audio_video_bwe option results in the
c/p error

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:450: std::map<int, std::string>
sync_labels_;
leftovers?

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: // Test that changing the
combined_audio_video_bwe option results in the
c/p error

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3404:
EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1)
Is this really style guide compliant formatting?

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc#newc...
webrtc/video/call.cc:68: class SyncGroup {
Simplify as discussed offline.

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te...
File webrtc/video/call_perf_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te...
webrtc/video/call_perf_tests.cc:279: 
dd

[pbos-webrtc, 2015-06-11 11:57:16]

failing tests

[pbos-webrtc, 2015-06-11 14:34:25]

remove sync groups + feedback

[pbos-webrtc, 2015-06-11 14:48:42]

more feedback

[pbos-webrtc, 2015-06-11 14:48:53]

phew, PTAL

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1690: const std::string& sync_label()
const { return sync_label_; }
On 2015/06/11 11:52:03, the sun wrote:
> Adding the sync label here feels backwards to me. I think going forward we
will
> be better served with a map ssrc->StreamParams, e.g. receive_stream_params_.
and
> to look there for the sync label.

Done.

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2487: new
WebRtcVoiceChannelRenderer(channel, sp.sync_label, audio_transport)));
On 2015/06/11 11:52:03, the sun wrote:
> This will never be used in practise?

Correct.

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:3630: WebRtcVoiceChannelRenderer*
channel) {
On 2015/06/11 11:52:03, the sun wrote:
> No need to take the ..Renderer* as argument. We can look it up from the
> receive_channels_ map. Yes, I know you will say it requires an unnecessary map
> lookup, to which I will answer it doesn't matter here (1. few objects in the
> map, 2. code due for refactoring) - keep the interface simple instead!

Done.

https://codereview.webrtc.org/1181653002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:3634: if (!call_)
On 2015/06/11 11:52:03, the sun wrote:
> {} please.

Gah, done.

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:48: int channel_id = -1;
On 2015/06/11 11:52:03, the sun wrote:
> voe_channel_id

Done.

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/audio_receive_stre...
webrtc/audio_receive_stream.h:51: // TODO(pbos): Synchronize streams in a sync
group, not just video streams
On 2015/06/11 11:52:03, the sun wrote:
> Is this TODO necessary? Will it be addressed in the foreseeable future? Better
> add an issue for it instead?

Added an issue, and referenced it here. I think it's good to have it explicit in
the header though, since it looks like it should work.

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv...
File webrtc/video/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1181653002/diff/20001/webrtc/video/audio_receiv...
webrtc/video/audio_receive_stream.cc:51: CHECK(config.channel_id != -1);
On 2015/06/11 11:52:03, the sun wrote:
> Why not DCHECK()? If the channel doesn't exist, subsequent calls to VoE will
> fail anyway.

Done.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.cc:247: DCHECK(config.channel_id != -1);
On 2015/06/11 at 11:52:04, the sun wrote:
> Validate config in object ctor instead.

Done.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:962: // Test that changing the
combined_audio_video_bwe option results in the
On 2015/06/11 at 11:52:04, the sun wrote:
> c/p error

Removed, done.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:450: std::map<int, std::string>
sync_labels_;
On 2015/06/11 11:52:04, the sun wrote:
> leftovers?

Done.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: // Test that changing the
combined_audio_video_bwe option results in the
On 2015/06/11 11:52:04, the sun wrote:
> c/p error

Done.

https://codereview.webrtc.org/1181653002/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3404:
EXPECT_TRUE(call.GetAudioReceiveStream(kAudioSsrc1)
On 2015/06/11 11:52:04, the sun wrote:
> Is this really style guide compliant formatting?

Yep, where style == clang-format with Chromium settings. I'm not going to do
manual code formatting.

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call.cc#newc...
webrtc/video/call.cc:68: class SyncGroup {
On 2015/06/11 11:52:04, the sun wrote:
> Simplify as discussed offline.

Done.

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te...
File webrtc/video/call_perf_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te...
webrtc/video/call_perf_tests.cc:279: 
On 2015/06/11 11:52:04, the sun wrote:
> dd

ddone

https://codereview.webrtc.org/1181653002/diff/60001/webrtc/video/call_perf_te...
webrtc/video/call_perf_tests.cc:336: 
Done.

[the sun, 2015-06-11 15:38:48]

Getting there...

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576: receive_stream_params_[ssrc] = sp;
DCHECK that there is no entry in map for this ssrc.

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3639: config.rtp.extensions =
recv_rtp_extensions_;
{}

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str...
webrtc/audio_receive_stream.h:46: // Handle to underlying VoiceEngine handle,
used to map AudioReceiveStream
Handle to a handle? Rephrase, also include something about it being temporary.

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc#new...
webrtc/video/call.cc:448: void
Call::AddAudioReceiveStreamSync(AudioReceiveStream* stream) {
I'd like you to spend a few minutes to see if this can be simplified into 1 or 2
functions instead: UpdateAVSync(sync_group), or
UpdateAudioAVSync(..)+UpdateVideoAVSync(..). Some work can be done in situ at
the call spot instead.

If that's 100% impossible/unsuitable, at the very least make the code more
readable by pulling stream->config().sync_group into a local const &.

[pbos-webrtc, 2015-06-12 15:52:59]

pull sync into one method

[pbos-webrtc, 2015-06-12 15:53:14]

PTAL

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576: receive_stream_params_[ssrc] = sp;
On 2015/06/11 15:38:48, the sun wrote:
> DCHECK that there is no entry in map for this ssrc.

Done.

https://codereview.webrtc.org/1181653002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3639: config.rtp.extensions =
recv_rtp_extensions_;
On 2015/06/11 15:38:48, the sun wrote:
> {}

:Done

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/audio_receive_str...
webrtc/audio_receive_stream.h:46: // Handle to underlying VoiceEngine handle,
used to map AudioReceiveStream
On 2015/06/11 15:38:48, the sun wrote:
> Handle to a handle? Rephrase, also include something about it being temporary.

Done.

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/120001/webrtc/video/call.cc#new...
webrtc/video/call.cc:448: void
Call::AddAudioReceiveStreamSync(AudioReceiveStream* stream) {
On 2015/06/11 15:38:48, the sun wrote:
> I'd like you to spend a few minutes to see if this can be simplified into 1 or
2
> functions instead: UpdateAVSync(sync_group), or
> UpdateAudioAVSync(..)+UpdateVideoAVSync(..). Some work can be done in situ at
> the call spot instead.
> 
> If that's 100% impossible/unsuitable, at the very least make the code more
> readable by pulling stream->config().sync_group into a local const &.

Done.

[the sun, 2015-06-15 10:28:50]

LGTM

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new...
webrtc/video/call.cc:457: if (sync_audio_stream == nullptr) {
else {

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new...
webrtc/video/call.cc:465: if (sync_audio_stream != nullptr)
if (sync_audio_stream)

[pbos-webrtc, 2015-06-15 12:05:21]

feedback

[pbos-webrtc, 2015-06-15 12:05:33]

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new...
webrtc/video/call.cc:457: if (sync_audio_stream == nullptr) {
On 2015/06/15 10:28:50, the sun wrote:
> else {

Done.

https://codereview.webrtc.org/1181653002/diff/140001/webrtc/video/call.cc#new...
webrtc/video/call.cc:465: if (sync_audio_stream != nullptr)
On 2015/06/15 10:28:50, the sun wrote:
> if (sync_audio_stream)

Done.

[stefan-webrtc, 2015-06-16 15:33:40]

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576:
DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());
Can someone give us a fake sdp and cause a crash here, or is this ssrc always
generated locally?

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3640: // Only add RTP extensions if we
support combined AV BWE.
A/V BWE

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group =
receive_stream_params_[ssrc].sync_label;
Does this mean A/V BWE depends on sync labels? I would like some explanations on
this code.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:455: sync_audio_stream = it->second;
Shouldn't we fail with an error or warning if two video streams are synced to
one audio stream? Or in general if more than 2 streams are synced together? We
don't really support that.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream :
video_receive_streams_) {
Is this syncing all video receive streams to the audio stream if one of them had
a matching group? Doesn't necessarily have to be all that should be synced,
right?

[the sun, 2015-06-18 08:38:37]

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576:
DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> Can someone give us a fake sdp and cause a crash here, or is this ssrc always
> generated locally?

Notice that we look for the ssrc in receive_channels_ a few lines up, and return
gracefully if we find it. This assert is just to make sure receive_channels_ and
receive_sgtream_params_ are in sync.

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group =
receive_stream_params_[ssrc].sync_label;
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> Does this mean A/V BWE depends on sync labels? I would like some explanations
on
> this code.

No, we create the ARS regardless, but only set rtp extensions on it if we have
combined BWE enabled.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:455: sync_audio_stream = it->second;
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> Shouldn't we fail with an error or warning if two video streams are synced to
> one audio stream? Or in general if more than 2 streams are synced together? We
> don't really support that.

Since we're literally using the SDP sync group we should fire errors based on
what is supported in the relevant spec (https://tools.ietf.org/html/rfc5888 ?).
Whenever we can't support the requested configuration, we should log warnings.

[mflodman, 2015-06-22 04:53:28]

A few initial questions, think I need some more background how this should work.
I guess I'm misunderstanding the RFC at some point.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group)
Would prefer to include AV in this name.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:348: void Call::DestroyVideoReceiveStream(
Shouldn't we call ConfigureSync from here too?

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:447: // Set sync only if there was no previous one.
Meaning we can only sync one audio stream with one video stream in a call, not
multiple pairs?

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream :
video_receive_streams_) {
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> Is this syncing all video receive streams to the audio stream if one of them
had
> a matching group? Doesn't necessarily have to be all that should be synced,
> right?

Agree.

[hta-webrtc, 2015-06-22 11:05:37]

Just to quote the specs a bit:

getusermedia spec:

http://w3c.github.io/mediacapture-main/getusermedia.html#stream-api

Each MediaStream can contain zero or more MediaStreamTrack objects. All tracks
in a MediaStream are intended to be synchronized when rendered. This is not a
hard requirement, since it might not be possible to synchronize tracks from
sources that have different clocks. Different MediaStream objects do not need to
be synchronized.

draft-ietf-mmusic-msid:

https://tools.ietf.org/html/draft-ietf-mmusic-msid-10#section-3

specifies that MSID is used to convey the association of a MediaStreamTrack to a
MediaStream.

[mflodman, 2015-06-22 11:08:21]

Thanks Harald, I was looking at RFC 5888 and the LS semantic.

[stefan-webrtc, 2015-06-22 14:19:42]

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576:
DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());
On 2015/06/18 08:38:37, the sun wrote:
> On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> > Can someone give us a fake sdp and cause a crash here, or is this ssrc
always
> > generated locally?
> 
> Notice that we look for the ssrc in receive_channels_ a few lines up, and
return
> gracefully if we find it. This assert is just to make sure receive_channels_
and
> receive_sgtream_params_ are in sync.

Acknowledged.

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3645: config.sync_group =
receive_stream_params_[ssrc].sync_label;
On 2015/06/18 08:38:37, the sun wrote:
> On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> > Does this mean A/V BWE depends on sync labels? I would like some
explanations
> on
> > this code.
> 
> No, we create the ARS regardless, but only set rtp extensions on it if we have
> combined BWE enabled.

Yes, I have no clue how I read this code. :)

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str...
webrtc/audio_receive_stream.h:53: // to one of the audio streams. Tracked by
issue webrtc:4762.
"...not just one video stream to one of the audio streams.", right? We can't
sync two video streams to one audio stream AFAIK.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group)
On 2015/06/22 04:53:28, mflodman wrote:
> Would prefer to include AV in this name.

I'm not sure I agree. We may want to sync video with video as well, right?

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:455: sync_audio_stream = it->second;
On 2015/06/18 08:38:37, the sun wrote:
> On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> > Shouldn't we fail with an error or warning if two video streams are synced
to
> > one audio stream? Or in general if more than 2 streams are synced together?
We
> > don't really support that.
> 
> Since we're literally using the SDP sync group we should fire errors based on
> what is supported in the relevant spec (https://tools.ietf.org/html/rfc5888
?).

I'm not sure what you mean with this. Log errors when an SDP configures
something we don't support?

> Whenever we can't support the requested configuration, we should log warnings.

Agreed. And maybe return an error? Not sure what is expected by a javascript
user in this case, but trying to sync 2 or more video streams to a single audio
stream will break a/v sync for all.

[pbos-webrtc, 2015-06-24 09:45:58]

feedback

[pbos-webrtc, 2015-06-24 09:46:09]

PTAL

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2576:
DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());
On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote:
> On 2015/06/18 08:38:37, the sun wrote:
> > On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> > > Can someone give us a fake sdp and cause a crash here, or is this ssrc
> always
> > > generated locally?
> > 
> > Notice that we look for the ssrc in receive_channels_ a few lines up, and
> return
> > gracefully if we find it. This assert is just to make sure receive_channels_
> and
> > receive_sgtream_params_ are in sync.
> 
> Acknowledged.

I moved the storing of the StreamParams to prevent having to erase them, now
they are stored at the same place as receive_channels_.

https://codereview.webrtc.org/1181653002/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3640: // Only add RTP extensions if we
support combined AV BWE.
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> A/V BWE

Done.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str...
File webrtc/audio_receive_stream.h (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/audio_receive_str...
webrtc/audio_receive_stream.h:53: // to one of the audio streams. Tracked by
issue webrtc:4762.
On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote:
> "...not just one video stream to one of the audio streams.", right? We can't
> sync two video streams to one audio stream AFAIK.

Correct, done.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:112: void ConfigureSync(const std::string& sync_group)
On 2015/06/22 14:19:42, stefan-webrtc (holmer) wrote:
> On 2015/06/22 04:53:28, mflodman wrote:
> > Would prefer to include AV in this name.
> 
> I'm not sure I agree. We may want to sync video with video as well, right?

Yes, we want this parameter to be for syncing streams in general, limiting this
to a pair of a/v channels is an implementation detail.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:348: void Call::DestroyVideoReceiveStream(
On 2015/06/22 04:53:28, mflodman wrote:
> Shouldn't we call ConfigureSync from here too?

Yes, so that the audio stream can be synced to something else, thanks.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:447: // Set sync only if there was no previous one.
On 2015/06/22 04:53:27, mflodman wrote:
> Meaning we can only sync one audio stream with one video stream in a call, not
> multiple pairs?

Acknowledged.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:455: sync_audio_stream = it->second;
On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> Shouldn't we fail with an error or warning if two video streams are synced to
> one audio stream? Or in general if more than 2 streams are synced together? We
> don't really support that.

Logging a warning should be appropriate. A SDP is correct in requesting more
than two streams to be synchronized, we just don't support it.

https://codereview.webrtc.org/1181653002/diff/160001/webrtc/video/call.cc#new...
webrtc/video/call.cc:467: for (VideoReceiveStream* video_stream :
video_receive_streams_) {
On 2015/06/22 04:53:27, mflodman wrote:
> On 2015/06/16 15:33:39, stefan-webrtc (holmer) wrote:
> > Is this syncing all video receive streams to the audio stream if one of them
> had
> > a matching group? Doesn't necessarily have to be all that should be synced,
> > right?
> 
> Agree.

Yes, huge bug here. Thanks.

I changed it so that it'll only sync if it's the same group, and it will only
sync one pair. The rest will get the channel id -1, and remove sync.

[pbos-webrtc, 2015-06-24 09:46:28]

rebase

[stefan-webrtc, 2015-06-24 11:57:35]

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:369:
ConfigureSync(receive_stream_impl->config().sync_group);
Can't this crash if video_receive_ssrcs_ is empty? Probably shouldn't happen.
Maybe move the check from 371 to 369?

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:479: LOG(LS_WARNING) << "Attempting to sync more than one
audio/video pair. "
Isn't it? The following configuration is ok right?
{audio1, video1}
{audio2, video2} 

While the following is not:
{audio1, audio2, video1}
{audio3, video2, video3}

I don't think the comment reflects that.

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:485: : -1);
I'd prefer to move this complex ?: statement out from the function call. Make it
a regular if...

[pbos-webrtc, 2015-06-24 14:15:22]

feedback

[pbos-webrtc, 2015-06-24 14:16:20]

updated log message

[pbos-webrtc, 2015-06-24 14:17:21]

PTAL

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:369:
ConfigureSync(receive_stream_impl->config().sync_group);
On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote:
> Can't this crash if video_receive_ssrcs_ is empty? Probably shouldn't happen.
> Maybe move the check from 371 to 369?

Done.

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:479: LOG(LS_WARNING) << "Attempting to sync more than one
audio/video pair. "
On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote:
> Isn't it? The following configuration is ok right?
> {audio1, video1}
> {audio2, video2} 
> 
> While the following is not:
> {audio1, audio2, video1}
> {audio3, video2, video3}
> 
> I don't think the comment reflects that.

Agreed, comment updated.

https://codereview.webrtc.org/1181653002/diff/180001/webrtc/video/call.cc#new...
webrtc/video/call.cc:485: : -1);
On 2015/06/24 11:57:35, stefan-webrtc (holmer) wrote:
> I'd prefer to move this complex ?: statement out from the function call. Make
it
> a regular if...

Done.

[stefan-webrtc, 2015-06-25 15:29:00]

lgtm

[pbos-webrtc, 2015-06-25 19:23:30]

pbos@webrtc.org changed reviewers:
+ hta@webrtc.org

[pbos-webrtc, 2015-06-25 19:23:31]

+R hta@

mflodman@ PTAL

[pbos-webrtc, 2015-06-25 19:23:44]

On 2015/06/25 19:23:31, pbos-webrtc wrote:
> +R hta@
> 
> mflodman@ PTAL

(hta@ PTAL as well)

[hta-webrtc, 2015-07-06 19:04:00]

LGTM

with comments. These are nits.

https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3432: .rtp.extensions.empty());
Drive-by comment: This test doesn't seem very future-proof. The combined BWE
option may be the only RTP extension we use *today*, but what happens the day
the "mid" RTP option goes live?

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est...
File webrtc/video/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est...
webrtc/video/bitrate_estimator_tests.cc:208: // underlying channel.
This comment is not clear. Under what conditions is the voe_channel_id bogus,
and is zero always a bogus value?

Possible rewording:

"We set voe_channel_id to 0, which is bogus but legal. In production, the
voe_channel_id is always set to the id of an underlying channel."

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc#new...
webrtc/video/call.cc:481: "the current implementation.";
Should there be a bug number filed to track this restriction?

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t...
File webrtc/video/call_perf_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t...
webrtc/video/call_perf_tests.cc:286: CreateStreams();
This seems unclean somehow - can you rewrite this as

if (create_audio_first)
  create audio
  CreateStreams()
else
  CreateStreams()
  create_audio

?

[pbos-webrtc, 2015-07-09 10:14:51]

rebase

[pbos-webrtc, 2015-07-09 10:21:12]

feedback

[pbos-webrtc, 2015-07-09 10:21:49]

Won't push this before M45 cuts, it feels risky.

https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3432: .rtp.extensions.empty());
On 2015/07/06 19:04:00, hta wrote:
> Drive-by comment: This test doesn't seem very future-proof. The combined BWE
> option may be the only RTP extension we use *today*, but what happens the day
> the "mid" RTP option goes live?

Yes, this isn't meant to be future proof, it's supposed to be non-optional.
Before this CL we never created audio receive streams if there wasn't combined
BWE, so this was a way to get stream-based A/V sync in at all.

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est...
File webrtc/video/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/bitrate_est...
webrtc/video/bitrate_estimator_tests.cc:208: // underlying channel.
On 2015/07/06 19:04:00, hta wrote:
> This comment is not clear. Under what conditions is the voe_channel_id bogus,
> and is zero always a bogus value?
> 
> Possible rewording:
> 
> "We set voe_channel_id to 0, which is bogus but legal. In production, the
> voe_channel_id is always set to the id of an underlying channel."
> 

Rephrased as:

// Bogus non-default id to prevent hitting a DCHECK when creating the
// AudioReceiveStream. Every receive stream has to correspond to an  
// underlying channel id.

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc
File webrtc/video/call.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call.cc#new...
webrtc/video/call.cc:481: "the current implementation.";
On 2015/07/06 19:04:00, hta wrote:
> Should there be a bug number filed to track this restriction?

https://code.google.com/p/webrtc/issues/detail?id=4762

Put it in a comment as well.

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t...
File webrtc/video/call_perf_tests.cc (right):

https://codereview.webrtc.org/1181653002/diff/240001/webrtc/video/call_perf_t...
webrtc/video/call_perf_tests.cc:286: CreateStreams();
On 2015/07/06 19:04:00, hta wrote:
> This seems unclean somehow - can you rewrite this as
> 
> if (create_audio_first)
>   create audio
>   CreateStreams()
> else
>   CreateStreams()
>   create_audio
> 
> ?

Agreed, done.

[pbos-webrtc, 2015-07-15 08:30:38]

rebase

[pbos-webrtc, 2015-07-15 12:29:41]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-07-15 12:29:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, hta@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1181653002/#ps300001 (title:
"rebase")

[commit-bot: I haz the power, 2015-07-15 12:29:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181653002/300001

[commit-bot: I haz the power, 2015-07-15 12:44:06]

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

[commit-bot: I haz the power, 2015-07-15 12:44:07]

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

[pbos-webrtc, 2015-07-15 12:56:08]

TBR=mflodman@ for:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    webrtc/audio_receive_stream.h
    webrtc/video_receive_stream.h

[pbos-webrtc, 2015-07-15 12:56:35]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2015-07-15 12:56:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181653002/300001

[commit-bot: I haz the power, 2015-07-15 14:30:07]

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

[commit-bot: I haz the power, 2015-07-15 14:30:09]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pbos-webrtc, 2015-07-15 14:34:03]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2015-07-15 14:34:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181653002/300001

[commit-bot: I haz the power, 2015-07-15 15:03:04]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-07-15 15:03:11]

Patchset 16 (id:??) landed as
https://crrev.com/8fc7fa798f7a36955f1b933980401afad2aff592
Cr-Commit-Position: refs/heads/master@{#9586}