Full impl of NnChannel::SetSendParameters and NnChannel::SetRecvParameters

Full use of NnChannel::SetSendParameters and NnChannel::SetRecvParameters.

SetOptions(), SetMaxBandwidth(), Set[Send|Recv]RtpHeaderExtensions(), Set[Send|Recv]Codecs() are now private.

BUG=webrtc:4690
R=pbos@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/b071a19019a0a2173cc139c960d6ef6946a1c581
Cr-Commit-Position: refs/heads/master@{#9973}

[the sun, 2015-09-03 09:46:05]

solenberg@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[the sun, 2015-09-03 09:48:02]

Peter: FYI: I'm giving it a shot to get rid of the individual setters on the
media channels but this is nowhere near finished yet. I'll ping when it is in a
reasonable state to look at.

[pthatcher1, 2015-09-04 23:21:16]

This is looking great already.  Thanks for doing this!

https://codereview.webrtc.org/1327933002/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1175: if (!voice_channel_->MuteStream(ssrc,
!enable, &options)) {
I like this.  We should take in one step further rename this to something like
Send(ssrc, active, options).  Because RtpSender::Send(...) is what it will be
called later.

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:821: // ch->SetOptions(options);
The options don't matter until the other parameters are available as well.  So
the only thing going on here is that the parameters are remembered until later.

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:184: bool Init(const
std::vector<VideoCodec>& recv_codecs);
Why are recv codecs part of Init?

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

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:592: // }
Same here: they just need to be remembered for later.  I'm guessing it would
make sense to pass them into the WebRtcVoiceMediaChannel constructor.  Either
that or you'll need to go refactor WebRtcSession to keep them around and then
pass them in along with SetRemote/SetLocalDescription.  I'm guessing the first
option will be easier.

https://codereview.webrtc.org/1327933002/diff/40001/talk/session/media/channe...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/session/media/channe...
talk/session/media/channel_unittest.cc:186: //     this,
&ChannelTest<T>::OnMediaMuted);
This is only used by TypingMonitor, which isn't used by anything.  So we could
remove TypingMonitor, and then remove this.

[the sun, 2015-09-08 11:31:12]

https://codereview.webrtc.org/1327933002/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1175: if (!voice_channel_->MuteStream(ssrc,
!enable, &options)) {
On 2015/09/04 23:21:16, pthatcher1 wrote:
> I like this.  We should take in one step further rename this to something like
> Send(ssrc, active, options).  Because RtpSender::Send(...) is what it will be
> called later.

I decided to take it even further and collapse SetLocalRenderer() into the same
function. Turns out to be a change big enough to warrant a separate CL:
https://codereview.chromium.org/1311533009/

https://codereview.webrtc.org/1327933002/diff/40001/talk/session/media/channe...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/session/media/channe...
talk/session/media/channel_unittest.cc:186: //     this,
&ChannelTest<T>::OnMediaMuted);
On 2015/09/04 23:21:16, pthatcher1 wrote:
> This is only used by TypingMonitor, which isn't used by anything.  So we could
> remove TypingMonitor, and then remove this.

Pooff: https://codereview.chromium.org/1327033002/

I actually checked before whether it was used, but must have misread the code.

[the sun, 2015-09-11 15:03:03]

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:821: // ch->SetOptions(options);
On 2015/09/04 23:21:16, pthatcher1 wrote:
> The options don't matter until the other parameters are available as well.  So
> the only thing going on here is that the parameters are remembered until
later. 
> 

Done.

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

https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:592: // }
On 2015/09/04 23:21:16, pthatcher1 wrote:
> Same here: they just need to be remembered for later.  I'm guessing it would
> make sense to pass them into the WebRtcVoiceMediaChannel constructor.  Either
> that or you'll need to go refactor WebRtcSession to keep them around and then
> pass them in along with SetRemote/SetLocalDescription.  I'm guessing the first
> option will be easier.

Done.

[the sun, 2015-09-14 15:19:15]

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

[the sun, 2015-09-14 15:24:57]

On 2015/09/11 15:03:03, the sun wrote:
>
https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
> File talk/media/base/fakemediaengine.h (right):
> 
>
https://codereview.webrtc.org/1327933002/diff/40001/talk/media/base/fakemedia...
> talk/media/base/fakemediaengine.h:821: // ch->SetOptions(options);
> On 2015/09/04 23:21:16, pthatcher1 wrote:
> > The options don't matter until the other parameters are available as well. 
So
> > the only thing going on here is that the parameters are remembered until
> later. 
> > 
> 
> Done.
> 
>
https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1327933002/diff/40001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:592: // }
> On 2015/09/04 23:21:16, pthatcher1 wrote:
> > Same here: they just need to be remembered for later.  I'm guessing it would
> > make sense to pass them into the WebRtcVoiceMediaChannel constructor. 
Either
> > that or you'll need to go refactor WebRtcSession to keep them around and
then
> > pass them in along with SetRemote/SetLocalDescription.  I'm guessing the
first
> > option will be easier.
> 
> Done.

All tests updated except the WebRtcVideoEngine tests that somehow touch
NnParameters.max_bandwidth_bps.

It turns out max_bandwidth_bps == -1 means "no limit" in WViE, not "unset". If
you have any ideas on how to best deal with that, they're welcome. I'm pondering
the problem until tomorrow...

[pthatcher1, 2015-09-14 19:42:02]

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1074: AudioRenderer* renderer) = 0;
Instead of "bool mute", can we have it be "bool send" (the opposite of mute)?

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1153: virtual bool SetVideoSend(uint32 ssrc, bool
mute,
Same here.

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
File talk/media/base/videoengine_unittest.h (left):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
talk/media/base/videoengine_unittest.h:1785: // Tests that we cannot receive key
frames with unsignalled recv disabled.
We are you removing the tests for unsignalled_recv_stream_limit?

We shouldn't do that unless we remove unsignalled_recv_stream_limit, which I'd
be OK with since it doesn't do anything any more.  But that should be in a
separate CL, I think.

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:814: SetRecvCodecs(recv_codecs);
What if SetRecvCodecs fails?

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:940: }
I think this should be handled by the caller of SetRecvCodecs, not inside of
SetRecvCodecs.  Same with SetSendCodecs.

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

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2098: }
Same here

[the sun, 2015-09-14 19:58:00]

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
File talk/media/base/videoengine_unittest.h (left):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
talk/media/base/videoengine_unittest.h:1785: // Tests that we cannot receive key
frames with unsignalled recv disabled.
On 2015/09/14 19:42:02, pthatcher1 wrote:
> We are you removing the tests for unsignalled_recv_stream_limit?
> 
> We shouldn't do that unless we remove unsignalled_recv_stream_limit, which I'd
> be OK with since it doesn't do anything any more.  But that should be in a
> separate CL, I think.

These are helper functions which turned out to be unused.

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:940: }
On 2015/09/14 19:42:02, pthatcher1 wrote:
> I think this should be handled by the caller of SetRecvCodecs, not inside of
> SetRecvCodecs.  Same with SetSendCodecs.

I was wondering about wether it should be at all legal to give an empty list of
codecs to Set[Send|Recv]Parameters(). Before this CL, none of the other state
will be changed if setting codecs fail, which it will in case of an empty list.

This was only changed once tests relied on it (the ones that do nothing but
SetOptions, etc). Another option would be to always set up a default codec in
the tests.

WDYT?

[the sun, 2015-09-15 15:54:07]

On 2015/09/14 19:58:00, the sun wrote:
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
> File talk/media/base/videoengine_unittest.h (left):
> 
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
> talk/media/base/videoengine_unittest.h:1785: // Tests that we cannot receive
key
> frames with unsignalled recv disabled.
> On 2015/09/14 19:42:02, pthatcher1 wrote:
> > We are you removing the tests for unsignalled_recv_stream_limit?
> > 
> > We shouldn't do that unless we remove unsignalled_recv_stream_limit, which
I'd
> > be OK with since it doesn't do anything any more.  But that should be in a
> > separate CL, I think.
> 
> These are helper functions which turned out to be unused.
> 
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvideoengine2.cc:940: }
> On 2015/09/14 19:42:02, pthatcher1 wrote:
> > I think this should be handled by the caller of SetRecvCodecs, not inside of
> > SetRecvCodecs.  Same with SetSendCodecs.
> 
> I was wondering about wether it should be at all legal to give an empty list
of
> codecs to Set[Send|Recv]Parameters(). Before this CL, none of the other state
> will be changed if setting codecs fail, which it will in case of an empty
list.
> 
> This was only changed once tests relied on it (the ones that do nothing but
> SetOptions, etc). Another option would be to always set up a default codec in
> the tests.
> 
> WDYT?

Funny enough, it turns out WebRtcVoiceMediaChannel accepts an empty list for
SetRecvCodecs() but none of the other codec setters do. It claims this is ok
because "Already configured codecs are never removed"
(webrtcvoiceengine.cc:1864). Why does video work differently?

[pthatcher1, 2015-09-16 03:45:23]

On 2015/09/15 15:54:07, the sun wrote:
> On 2015/09/14 19:58:00, the sun wrote:
> >
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
> > File talk/media/base/videoengine_unittest.h (left):
> > 
> >
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/videoeng...
> > talk/media/base/videoengine_unittest.h:1785: // Tests that we cannot receive
> key
> > frames with unsignalled recv disabled.
> > On 2015/09/14 19:42:02, pthatcher1 wrote:
> > > We are you removing the tests for unsignalled_recv_stream_limit?
> > > 
> > > We shouldn't do that unless we remove unsignalled_recv_stream_limit, which
> I'd
> > > be OK with since it doesn't do anything any more.  But that should be in a
> > > separate CL, I think.
> > 
> > These are helper functions which turned out to be unused.
> > 
> >
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> > File talk/media/webrtc/webrtcvideoengine2.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> > talk/media/webrtc/webrtcvideoengine2.cc:940: }
> > On 2015/09/14 19:42:02, pthatcher1 wrote:
> > > I think this should be handled by the caller of SetRecvCodecs, not inside
of
> > > SetRecvCodecs.  Same with SetSendCodecs.
> > 
> > I was wondering about wether it should be at all legal to give an empty list
> of
> > codecs to Set[Send|Recv]Parameters(). Before this CL, none of the other
state
> > will be changed if setting codecs fail, which it will in case of an empty
> list.
> > 
> > This was only changed once tests relied on it (the ones that do nothing but
> > SetOptions, etc). Another option would be to always set up a default codec
in
> > the tests.
> > 
> > WDYT?
> 
> Funny enough, it turns out WebRtcVoiceMediaChannel accepts an empty list for
> SetRecvCodecs() but none of the other codec setters do. It claims this is ok
> because "Already configured codecs are never removed"
> (webrtcvoiceengine.cc:1864). Why does video work differently?

"Already configured codecs are never removed" is not true.  That's a bug. 
However, removing *all* codecs should either be an error or mean "stop sending".
 Removing nothing is wrong.

[pthatcher1, 2015-09-16 03:45:30]

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:940: }
On 2015/09/14 19:58:00, the sun wrote:
> On 2015/09/14 19:42:02, pthatcher1 wrote:
> > I think this should be handled by the caller of SetRecvCodecs, not inside of
> > SetRecvCodecs.  Same with SetSendCodecs.
> 
> I was wondering about wether it should be at all legal to give an empty list
of
> codecs to Set[Send|Recv]Parameters(). Before this CL, none of the other state
> will be changed if setting codecs fail, which it will in case of an empty
list.
> 
> This was only changed once tests relied on it (the ones that do nothing but
> SetOptions, etc). Another option would be to always set up a default codec in
> the tests.
> 
> WDYT?

I think leaving this here as-is just for unit tests is too risky.  I think we
have two options:

A.  The new contract is "you have to provide everything at once, including at
least one codec", then tests need to provide at least one codec.  

B.  The new contract is "you have to provide everything at once, but if there
are no codecs, we won't send anything".  

I could see a case for either.  I'm guessing it's easiest right now to do A and
update the tests to have at least one codec.

[the sun, 2015-09-16 13:01:17]

On 2015/09/16 03:45:30, pthatcher1 wrote:
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/1327933002/diff/220001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvideoengine2.cc:940: }
> On 2015/09/14 19:58:00, the sun wrote:
> > On 2015/09/14 19:42:02, pthatcher1 wrote:
> > > I think this should be handled by the caller of SetRecvCodecs, not inside
of
> > > SetRecvCodecs.  Same with SetSendCodecs.
> > 
> > I was wondering about wether it should be at all legal to give an empty list
> of
> > codecs to Set[Send|Recv]Parameters(). Before this CL, none of the other
state
> > will be changed if setting codecs fail, which it will in case of an empty
> list.
> > 
> > This was only changed once tests relied on it (the ones that do nothing but
> > SetOptions, etc). Another option would be to always set up a default codec
in
> > the tests.
> > 
> > WDYT?
> 
> I think leaving this here as-is just for unit tests is too risky.  I think we
> have two options:
> 
> A.  The new contract is "you have to provide everything at once, including at
> least one codec", then tests need to provide at least one codec.  
> 
> B.  The new contract is "you have to provide everything at once, but if there
> are no codecs, we won't send anything".  
> 
> I could see a case for either.  I'm guessing it's easiest right now to do A
and
> update the tests to have at least one codec.

Fixed.

In order to make the tests work I had to slightly change the meaning of
VideoSendParameters::max_bandwidth_bps.
Previously, anything <= 0 would be treated as infinite bandwidth. Since we're
always setting the max bandwidth, that would cause
WebRtcVideoChannel2::SetMaxSendBandwidth() to override any codec specific
settings. As a workaround, I've let <0 mean "leave as is" and ==0 mean "set to
inifinite".

[the sun, 2015-09-16 13:01:50]

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1074: AudioRenderer* renderer) = 0;
On 2015/09/14 19:42:02, pthatcher1 wrote:
> Instead of "bool mute", can we have it be "bool send" (the opposite of mute)?

I agree, but it is hardly related to this CL.

https://codereview.webrtc.org/1327933002/diff/220001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1153: virtual bool SetVideoSend(uint32 ssrc, bool
mute,
On 2015/09/14 19:42:02, pthatcher1 wrote:
> Same here.

Ditto.

[pthatcher1, 2015-09-17 06:02:26]

lgtm

[pbos-webrtc, 2015-09-17 12:13:53]

lgtm, consider mah nits (I found this confusing while reading)

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.h:164: const std::vector<VideoCodec>&
recv_codecs,
rename initial_recv_codecs please, to me they look like they'd be const right
now

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:286: const AudioOptions& options,
if these are initial as well, please initial_options as they can be changed in
runtime

[the sun, 2015-09-17 13:37:48]

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.h:164: const std::vector<VideoCodec>&
recv_codecs,
On 2015/09/17 12:13:53, pbos-webrtc wrote:
> rename initial_recv_codecs please, to me they look like they'd be const right
> now

I don't think that is necessary. As long as we have some APIs to change
codecs/options, then it can't be assumed recv_codecs are constant.

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1327933002/diff/280001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:286: const AudioOptions& options,
On 2015/09/17 12:13:53, pbos-webrtc wrote:
> if these are initial as well, please initial_options as they can be changed in
> runtime

Here also, I don't think that is necessary.

[the sun, 2015-09-17 13:38:53]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-09-17 13:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1327933002/#ps300001 (title:
"rebase+fix")

[commit-bot: I haz the power, 2015-09-17 13:39:07]

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

[the sun, 2015-09-17 14:43:12]

Committed patchset #17 (id:320001) manually as
b071a19019a0a2173cc139c960d6ef6946a1c581 (presubmit successful).

[commit-bot: I haz the power, 2015-09-17 14:43:14]

Patchset 17 (id:??) landed as
https://crrev.com/b071a19019a0a2173cc139c960d6ef6946a1c581
Cr-Commit-Position: refs/heads/master@{#9973}