Refactor the relationship between BaseChannel and MediaChannel so that we send over all the paramet…

Refactor the relationship between BaseChannel and MediaChannel so that we send over all the parameters in one method call rather then having them broken up into multiple method calls.  This should allow future refactorings of the WebRtcVideoEngine2 to not recreate configurations so many times, and have more simple code as well.

R=deadbeef@webrtc.org, pbos@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/c2ee2c86f905991a8cd05ee1f35bea105b41e4e0

[pthatcher1, 2015-07-10 21:38:19]

pthatcher@webrtc.org changed reviewers:
+ guoweis@webrtc.org, juberti@webrtc.org, pbos@webrtc.org

[pbos-webrtc, 2015-07-11 08:46:47]

On 2015/07/10 21:38:20, pthatcher1 wrote:

Haven't started looking proper yet, but do we need SetSendCodecs etc. to be
public after this? Or is that for unittests at the moment?

[pbos-webrtc, 2015-07-11 08:48:16]

https://codereview.webrtc.org/1229283003/diff/60001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1229283003/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:988: struct AudioSendParameters {
Can I has ToString() methods on all of these for logging?

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

Change looks good, but it feels like I had to read the same diff 4 times, can
you get some content into base classes or shared functions?

https://codereview.webrtc.org/1229283003/diff/60001/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (left):

https://codereview.webrtc.org/1229283003/diff/60001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:167: static const char
kAttributeXGoogleBufferLatency[] =
Part of different CL right?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1453: // Just maintain the last set of header
extensions.
Should this be stateful? Shouldn't all extensions always be in the content here?
If no extensions are negotiated we shouldn't be using the previously set ones
right?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1495: // CA_UPDATE is only used by the Harmony
library to update send
This why we need to maintain last set of parameters? Can we add a TODO to move
the state to Harmony and remove CA_UPDATE?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1530: send_params.extensions =
last_send_params_.extensions;
Is this tied to CA_UPDATE? Otherwise no set extensions should mean no
extensions, correct?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1853: recv_params.extensions =
video->rtp_header_extensions();
Feels like I've read this code 4x times now, can some of this be refactored out?
GetExtensions(const MediaContentDescription&, const std::vector<RtpExtension>&
last_extensions, std::vector<RtpExtension>*), or something?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1898: // CA_UPDATE is only used by the Harmony
library to update send
Can some of this go in the base class?

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:2268: if (action == CA_UPDATE &&
Same things here, did I read this 4x times? Anything that we can do?

[pthatcher1, 2015-07-13 22:58:28]

https://codereview.webrtc.org/1229283003/diff/60001/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (left):

https://codereview.webrtc.org/1229283003/diff/60001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:167: static const char
kAttributeXGoogleBufferLatency[] =
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Part of different CL right?

Yeah, I merged that CL into this one.  It will be submitted first, though.

https://codereview.webrtc.org/1229283003/diff/60001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1229283003/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:988: struct AudioSendParameters {
On 2015/07/11 08:48:16, pbos-webrtc wrote:
> Can I has ToString() methods on all of these for logging?

Done.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1453: // Just maintain the last set of header
extensions.
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Should this be stateful? Shouldn't all extensions always be in the content
here?
> If no extensions are negotiated we shouldn't be using the previously set ones
> right?

The existing behavior is that if !rtp_header_extensions_set(), the header
extensions are not changed.  So, if they aren't set, we should be using the
previously set ones.  I'd like to fix the code up the stack to not rely on that
and pass down a full list every time, but that will have to be in later CLs.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1495: // CA_UPDATE is only used by the Harmony
library to update send
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> This why we need to maintain last set of parameters? Can we add a TODO to move
> the state to Harmony and remove CA_UPDATE?

No, this is a separate thing, not related to the header extensions.  But I did
add the TODO.  That makes sense.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1530: send_params.extensions =
last_send_params_.extensions;
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Is this tied to CA_UPDATE? Otherwise no set extensions should mean no
> extensions, correct?

No, this has nothing to do with CA_UPDATE.  Honestly, I'm not entirely sure
what's relying on this.  But I'm taking enough risk changing as much code as I
am, and I don't want to change any more behavior than I have to.  But I've added
a TODO to remove rtp_header_extensions_set() if we can.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1853: recv_params.extensions =
video->rtp_header_extensions();
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Feels like I've read this code 4x times now, can some of this be refactored
out?
> GetExtensions(const MediaContentDescription&, const std::vector<RtpExtension>&
> last_extensions, std::vector<RtpExtension>*), or something?

Maybe.  The code before was structured a lot to share code, but that ended up
being a big problem: it's what encouraged the "break up content into pieces"
behavior that I'm trying to remove.  Plus, it just made the code much less
readable.  I'm in favor of a little duplication for more readability.  

That said, I think there is some duplication I can remove around
set_secure_required/SetSrtp_w/SetRtcpMux_w.  I'll try to improve that area a
bit.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:1898: // CA_UPDATE is only used by the Harmony
library to update send
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Can some of this go in the base class?

SetSendCodecs can't very easily (which is why it wasn't before). And
updateRemoteStreams_w already is.  But I'll try SetRtcpMux_w/SetSrtp_w, like I
mentioned.

https://codereview.webrtc.org/1229283003/diff/60001/talk/session/media/channe...
talk/session/media/channel.cc:2268: if (action == CA_UPDATE &&
On 2015/07/12 15:56:08, pbos-webrtc wrote:
> Same things here, did I read this 4x times? Anything that we can do?

There's nothing to be done between local and remote.  So you're going to be
reading it at last 2 times.  

Then it's a matter of re-reading it between audio and video (2 vs 4 times).  For
some parts (UpdateLocalStream_w), there isn't much more to move.  It's already
all there.  But perhaps for SetSrtp_w and SetRtcpMux_w, as I mentioned.  I'll
give that a shot.

[pthatcher1, 2015-07-14 07:04:44]

I factor some common code into SetRtpTransportParameters_w, which helped a
little.  I'll see if I can do more tomorrow.

[pbos-webrtc, 2015-07-14 08:25:01]

Looks better!

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

https://codereview.webrtc.org/1229283003/diff/100001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:146: ost << val.ToString() << ",";
Don't add "," at the end.

https://codereview.webrtc.org/1229283003/diff/100001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/100001/talk/session/media/chann...
talk/session/media/channel.cc:910: return false;
Don't you need SafeSetError anymore? (I never understood what that one did.)

[pthatcher1, 2015-07-16 10:47:11]

OK, I removed much more duplication.  It's getting about as small as it can be.

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

https://codereview.webrtc.org/1229283003/diff/100001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:146: ost << val.ToString() << ",";
On 2015/07/14 08:25:01, pbos-webrtc wrote:
> Don't add "," at the end.

Done.

https://codereview.webrtc.org/1229283003/diff/100001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/100001/talk/session/media/chann...
talk/session/media/channel.cc:910: return false;
On 2015/07/14 08:25:01, pbos-webrtc wrote:
> Don't you need SafeSetError anymore? (I never understood what that one did.)

SetSrtp_w call SafeSetError.

[pbos-webrtc, 2015-07-16 14:39:17]

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

https://codereview.webrtc.org/1229283003/diff/120001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1010: struct RtpParameters {
This is part of another CL and shouldn't be landed with this CL, correct? Can
you rebase?

[pbos-webrtc, 2015-07-16 14:39:52]

https://codereview.webrtc.org/1229283003/diff/120001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/120001/talk/session/media/chann...
talk/session/media/channel.cc:153: // ***
Remove this comment (regardless of which CL it belongs to). :)

[pbos-webrtc, 2015-07-16 14:40:39]

never mind, got confused over which CL this one was referring to

[pbos-webrtc, 2015-07-16 14:46:47]

lgtm, but I'd really like someone else to take a look. Especially at channel.cc.

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

https://codereview.webrtc.org/1229283003/diff/120001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:146: ost << val.ToString() << ",";
Don't add "," to end of string. Also should this be declared more locally
(further down) ?

[pthatcher1, 2015-07-16 22:47:59]

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

[pthatcher1, 2015-07-16 22:48:00]

Taylor has agreed to review the CL as well, especially to make sure I kept
behavior in channel.cc the same and didn't break anything.

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

https://codereview.webrtc.org/1229283003/diff/120001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:146: ost << val.ToString() << ",";
On 2015/07/16 14:46:47, pbos-webrtc wrote:
> Don't add "," to end of string. Also should this be declared more locally
> (further down) ?

Done.

https://codereview.webrtc.org/1229283003/diff/120001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:1010: struct RtpParameters {
On 2015/07/16 14:39:17, pbos-webrtc wrote:
> This is part of another CL and shouldn't be landed with this CL, correct? Can
> you rebase?

Acknowledged.

[Taylor Brandstetter, 2015-07-18 00:16:31]

Here are a few minor things I found. Aside from these, the major changes are
just the orders in which methods are called, which doesn't make a difference for
the most part.

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:160: if (desc->has_codecs()) {
Are you sure only a CA_UPDATE will give a description without codecs? There was
this type of condition before: "if (action != CA_UPDATE ||
audio->has_codecs())". Which means it *would* set an empty list of codecs if the
action wasn't CA_UPDATE.

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:938:
set_secure_required(content->crypto_required() != CT_NONE);
Should set_secure_required only be called here if src == CS_LOCAL? That's how it
worked before.

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:1474: if (!SetRtpTransportParameters_w(content,
action, CS_LOCAL, error_desc)) {
If SetRtpTransportParameters_w fails, this method will just return false,
whereas previously it would log "Failed to set [local|remote] [voice|video]
description". Should we set an error here, or at least log a warning message?

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:1542: LOG(LS_WARNING) << "Failed to set remote
audio description receive streams";
I notice that SafeSetError is used if UpdateLocalStreams_w fails, but
LOG(LS_WARNING) is used if UpdateRemoteStreams_w fails; is this intentional?

[pthatcher1, 2015-07-31 10:59:20]

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:160: if (desc->has_codecs()) {
On 2015/07/18 00:16:31, Taylor Brandstetter wrote:
> Are you sure only a CA_UPDATE will give a description without codecs? There
was
> this type of condition before: "if (action != CA_UPDATE ||
> audio->has_codecs())". Which means it *would* set an empty list of codecs if
the
> action wasn't CA_UPDATE.

If anything other than a CA_UPDATE has an empty list of codecs, that's
equivalent to not being able to send or receive.  It may be possible to provide
such a description, but it's probably a programmer error if they do so.

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:938:
set_secure_required(content->crypto_required() != CT_NONE);
On 2015/07/18 00:16:31, Taylor Brandstetter wrote:
> Should set_secure_required only be called here if src == CS_LOCAL? That's how
it
> worked before.

Good catch.  I've fixed it.

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:1474: if (!SetRtpTransportParameters_w(content,
action, CS_LOCAL, error_desc)) {
On 2015/07/18 00:16:31, Taylor Brandstetter wrote:
> If SetRtpTransportParameters_w fails, this method will just return false,
> whereas previously it would log "Failed to set [local|remote] [voice|video]
> description". Should we set an error here, or at least log a warning message?

SetRtpTransportParameters_w will set the error like it did before, but further
down (in SetSrtp_w, etc).

https://codereview.webrtc.org/1229283003/diff/140001/talk/session/media/chann...
talk/session/media/channel.cc:1542: LOG(LS_WARNING) << "Failed to set remote
audio description receive streams";
On 2015/07/18 00:16:31, Taylor Brandstetter wrote:
> I notice that SafeSetError is used if UpdateLocalStreams_w fails, but
> LOG(LS_WARNING) is used if UpdateRemoteStreams_w fails; is this intentional?

Good catch. 

Fixed.

[Taylor Brandstetter, 2015-07-31 17:49:08]

lgtm

[the sun, 2015-08-04 14:32:17]

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

[the sun, 2015-08-04 14:32:18]

https://codereview.webrtc.org/1229283003/diff/160001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1229283003/diff/160001/talk/session/media/chann...
talk/session/media/channel.cc:1484: SafeSetError("Failed to set local video
description recv parameters.",
s/video/audio

https://codereview.webrtc.org/1229283003/diff/160001/talk/session/media/chann...
talk/session/media/channel.cc:1892:
MaybeCacheRtpAbsSendTimeHeaderExtension(video->rtp_header_extensions());
Looks like this must be done for audio too.

[pthatcher1, 2015-08-07 23:05:48]

Committed patchset #10 (id:180001) manually as
c2ee2c86f905991a8cd05ee1f35bea105b41e4e0 (presubmit successful).