Split out dscp option from VideoOptions to new struct MediaChannelOptions.

Split out dscp option from VideoOptions to new struct MediaChannelOptions. 

The idea is to separate on-construction options from more mutable options. Additional video options will follow later.

BUG=webrtc:5438

[nisse-webrtc, 2016-01-29 14:01:08]

nisse@webrtc.org changed reviewers:
+ pbos@webrtc.org, perkj@webrtc.org

[nisse-webrtc, 2016-01-29 14:01:25]

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

[commit-bot: I haz the power, 2016-01-29 14:01:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646253004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646253004/1

[commit-bot: I haz the power, 2016-01-29 14:11:08]

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

[commit-bot: I haz the power, 2016-01-29 14:11:09]

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

[nisse-webrtc, 2016-01-29 15:07:20]

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

[commit-bot: I haz the power, 2016-01-29 15:07:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646253004/10018
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646253004/10018

[commit-bot: I haz the power, 2016-01-29 16:00:53]

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

[commit-bot: I haz the power, 2016-01-29 16:00:54]

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

[pbos-webrtc, 2016-01-29 16:22:56]

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:4095: // TODO(nisse): To keep this
test, we need a method to return the
Add that method then, don't drop test coverage. :)

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

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:98: MediaChannelOptions() {}
Drop the ctor unless you want to move it to a .cc file.

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:101: bool dscp;  // TODO(nisse): More descriptive
name?
enable_dscp? or enable_dscp_41 or whatever it was called? 'bool enable_dscp =
false;'

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:1119: DataMediaChannel() {}
Do you need/want this ctor?

[pbos-webrtc, 2016-01-29 16:23:47]

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:4095: // TODO(nisse): To keep this
test, we need a method to return the
On 2016/01/29 16:22:56, pbos-webrtc wrote:
> Add that method then, don't drop test coverage. :)

Anything external you can observe by using a fake network interface or so to
make sure it's actually configured and not just a getter?

[nisse-webrtc, 2016-02-01 08:29:49]

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1646253004/diff/10018/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:4095: // TODO(nisse): To keep this
test, we need a method to return the
On 2016/01/29 16:23:47, pbos-webrtc wrote:
> On 2016/01/29 16:22:56, pbos-webrtc wrote:
> > Add that method then, don't drop test coverage. :)
> 
> Anything external you can observe by using a fake network interface or so to
> make sure it's actually configured and not just a getter?

I think I've seen some other tests using FakeNetworkInterface, so that's
probably the way to go.

I think in general getter methods also for audio and video options are
questionable.

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

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:98: MediaChannelOptions() {}
On 2016/01/29 16:22:56, pbos-webrtc wrote:
> Drop the ctor unless you want to move it to a .cc file.

I think I'm showing C++ ignorance here. The thing is, I want a default
constructor setting the options to false. If I define a plain struct in C++, do
I get a default constructor initializing all members properly? Or will new
instances get uninitialized values?

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:101: bool dscp;  // TODO(nisse): More descriptive
name?
On 2016/01/29 16:22:56, pbos-webrtc wrote:
> enable_dscp? or enable_dscp_41 or whatever it was called? 'bool enable_dscp =
> false;'

enable_dcsp sounds good. And the actual codepoint used when enabled is different
for audio and video channels. 

That's why I had to add the DscpValue method. I don't quite like the way it's
used, I would prefer to have a member of type rtc::DiffServCodePoint, and set it
on construction depending on the value of the enable_dscp option and the
subclass being constructed. But I haven't found any nice and easy way to do that
(the "obvious" way, to let the constructor call the virtual method DscpValue,
doesn't work).

https://codereview.webrtc.org/1646253004/diff/10018/talk/media/base/mediachan...
talk/media/base/mediachannel.h:1119: DataMediaChannel() {}
On 2016/01/29 16:22:56, pbos-webrtc wrote:
> Do you need/want this ctor?

Probably not, it's a leftover from an earlier version where MediaChannel lacked
a default constructor. I'll delete it.

[nisse-webrtc, 2016-02-01 09:03:04]

Description was changed from

==========
Split out dscp option from VideoOptions to new struct MediaChannelOptions.

BUG=webrtc:5438
==========

to

==========
Split out dscp option from VideoOptions to new struct MediaChannelOptions. 

The idea is to separate on-construction options from more mutable options.
Additional video options will follow later.

BUG=webrtc:5438
==========

[nisse-webrtc, 2016-02-01 09:03:04]

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

[nisse-webrtc, 2016-02-01 09:43:15]

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

[commit-bot: I haz the power, 2016-02-01 09:43:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646253004/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646253004/50001

[nisse-webrtc, 2016-02-01 09:53:29]

Updated the testcase now, to create a new channel for each setting of the
enable_dscp flag. 

With the current implementation, enable_dscp == false means that the diffserv
codepoint is explicitly set to DSCP_DEFAULT. And this happens on SetInterface.

It's possible to change it so that enable_dscp == false means that we don't set
anything on the interface, like the behavior for an unset option before this
change. No idea if that makes sense. 

Also not entirely sure about the protection status of the MediaChannel::SetDscp
method. I changed it to private, because it's intended to be used from the
constructor only. But maybe that makes little sense since the SetOption method
isn't private (apparently called only from WebRtcVideoChannel2::SetInterface, to
tune socket buffers).

BTW, what is the NetworkInterface class representing? I guess it's an open
socket, not a network interface like, e.g, eth0?

[commit-bot: I haz the power, 2016-02-01 10:43:19]

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

[commit-bot: I haz the power, 2016-02-01 10:43:19]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2016-02-01 10:57:54]

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

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:100: bool enable_dscp = false;
To me it sounds like DSCP_DEFAULT is also some form of DSCP? If so this flag
name doesn't make sense (but it didn't before either). pthatcher@ can you help
name this?

If DSCP_DEFAULT means that DSCP is off, please comment that in the DscpValue()
method below, or rename to DSCP_OFF if possible.

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:390: virtual rtc::DiffServCodePoint DscpValue()
const { return rtc::DSCP_DEFAULT; }
This method name confuses me, should this be DscpValueForMediaTypeWhenEnabled()
or something? pthatcher@ how do we make this more readable?

[nisse-webrtc, 2016-02-01 11:26:34]

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

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:100: bool enable_dscp = false;
On 2016/02/01 10:57:54, pbos-webrtc wrote:
> To me it sounds like DSCP_DEFAULT is also some form of DSCP?

We're talking about is the diffserv bits in the IPv4 header, right? If so, it's
not optional, they're present in every packet, with some default if you don't
tell the IP stack otherwise.

In this piece of the code, my intention is to just mirror the constraint on the
wire. I hope we don't need three states, i.e., that a missing "googDscp"
constraint should be in all respects equivalent to a "googDscp" constraint with
the value false.

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:390: virtual rtc::DiffServCodePoint DscpValue()
const { return rtc::DSCP_DEFAULT; }
On 2016/02/01 10:57:54, pbos-webrtc wrote:
> This method name confuses me, should this be
DscpValueForMediaTypeWhenEnabled()
> or something? pthatcher@ how do we make this more readable?

The intention is to return the diffserve codepoint appropriate for this media
type. It's very static. The videoengine and voiceengine report different values,


For video AF41, "assured forwarding", see
https://en.wikipedia.org/wiki/Differentiated_services#Assured_Forwarding_.28A...

For audio, it's EF, high priority expedited forwarding.

For data, I just return DSCP_DEFAULT, which means that enable_dscp has no
effect.

Personally, I think diffserv is pretty useless.

[pbos-webrtc, 2016-02-01 11:27:21]

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

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:390: virtual rtc::DiffServCodePoint DscpValue()
const { return rtc::DSCP_DEFAULT; }
On 2016/02/01 11:26:34, nisse-webrtc wrote:
> On 2016/02/01 10:57:54, pbos-webrtc wrote:
> > This method name confuses me, should this be
> DscpValueForMediaTypeWhenEnabled()
> > or something? pthatcher@ how do we make this more readable?
> 
> The intention is to return the diffserve codepoint appropriate for this media
> type. It's very static. The videoengine and voiceengine report different
values,
> 
> 
> For video AF41, "assured forwarding", see
>
https://en.wikipedia.org/wiki/Differentiated_services#Assured_Forwarding_.28A...
> 
> For audio, it's EF, high priority expedited forwarding.
> 
> For data, I just return DSCP_DEFAULT, which means that enable_dscp has no
> effect.
> 
> Personally, I think diffserv is pretty useless.

Name this MediaTypeDscpValue() then perhaps?

[nisse-webrtc, 2016-02-01 12:09:46]

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

https://codereview.webrtc.org/1646253004/diff/50001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:390: virtual rtc::DiffServCodePoint DscpValue()
const { return rtc::DSCP_DEFAULT; }
On 2016/02/01 11:27:21, pbos-webrtc wrote:
> On 2016/02/01 11:26:34, nisse-webrtc wrote:
> > On 2016/02/01 10:57:54, pbos-webrtc wrote:
> > > This method name confuses me, should this be
> > DscpValueForMediaTypeWhenEnabled()
> > > or something? pthatcher@ how do we make this more readable?
> > 
> > The intention is to return the diffserve codepoint appropriate for this
media
> > type. It's very static. The videoengine and voiceengine report different
> values,
> > 
> > 
> > For video AF41, "assured forwarding", see
> >
>
https://en.wikipedia.org/wiki/Differentiated_services#Assured_Forwarding_.28A...
> > 
> > For audio, it's EF, high priority expedited forwarding.
> > 
> > For data, I just return DSCP_DEFAULT, which means that enable_dscp has no
> > effect.
> > 
> > Personally, I think diffserv is pretty useless.
> 
> Name this MediaTypeDscpValue() then perhaps?

Done.

[pbos-webrtc, 2016-02-01 14:44:39]

Thanks, this looks a lot better so far.

https://codereview.webrtc.org/1646253004/diff/70001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1646253004/diff/70001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:4092: #if 0
Assuming fixing this test is still pending?

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:97: struct MediaChannelOptions {
Can we call this Config? this may be fuzzy, but the current options are
reconfigured on the fly, so maybe config could be construction-time?

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:98: // Set DSCP value for packet sent from media
channel. This flag
packets, also 80 chars per line.

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:381: MediaChannel() : options_(),
network_interface_(NULL) {}
Can you remove the default constructor?

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2386: // Current implementation
always sets the DiffServCodePoint.
// Default value when DSCP is disabled should be DSCP_DEFAULT.

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:2838:
TEST_F(WebRtcVoiceEngineTestFake, TestSetDscpOptions) {
Pref ConfiguresDscpInNetworkInterface or something like that. Also test the
default options like in video, preferably.

[pthatcher1, 2016-02-01 18:20:18]

I think we need to rethink this a little bit.  Eventually, we are going to get
rid of channels and media channels, so calling these things "channel options" is
just something we'll have to refactor soon anyway.  Further, the implementation
is mixing up splitting dynamic vs. static configuration with audio vs. video vs.
shared.

To explain in more detail what I am thinking: the future of this part of the
WebRTC API is going to be an RtpSender object which has a .send or
.setParameters method which provides all the parameters for sending audio or
video, via one big RtpParameters struct.  We've been headed in that direction
with the creation of the RtpSender object and the VideoSendParameters (a
subclass of RtpSendParamters).  

So if you want to remove something from VideoOptions or AudioOptions, it ought
to go in there.  For example, max_bandwidth_bps is there and not in options.   I
think an RtpParameters.dscp would make sense.  I would much prefer that over yet
another options struct.

But now we have a fundamental API question to answer:  can the user of the API
change the dscp value of an RtpSender dynamically?  If the answer is yes, that
RtpParameters is the right place for it.  If the answer is no, then we need a
new set of parameters that get passed into the RtpSender constructor, perhaps
something like RtpConstructorParameters or RtpInitParameters.  We haven't found
anything yet like that (which we can't change dynamically), and so we haven't
had the need for such a constructor argument.  Even the output transport and the
input track can be changed dynamically.

As for DSCP specifically: I don't see a reason why we can't change it
dynamically.  Taylor, a member of our team, is currently working on making DSCP
markings per-packet, which would make having it be dynamic trivial to implement.
 


So, if anything, I would recommend we move dscp out of VideoOptions and
AudioOptions and into RtpParameters and leave it as a dynamic parameter that the
API user can change any time.  And as Taylor implements per-packet DSCP, it will
fall right into place.

[nisse-webrtc, 2016-02-02 08:11:20]

On 2016/02/01 18:20:18, pthatcher1 wrote:
> To explain in more detail what I am thinking: the future of this part of the
> WebRTC API is going to be an RtpSender object which has a .send or
> .setParameters method which provides all the parameters for sending audio or
> video, via one big RtpParameters struct.

We'll see how that works out. My gut feeling is that "one big struct" makes
sense if it 
represents something like the contents of user config file, but for things set
via program logic, it's clearer to keep things separate. But I don't really have
an informed opinion right now.

> We haven't found
> anything yet like that (which we can't change dynamically), and so we haven't
> had the need for such a constructor argument.  Even the output transport and
the
> input track can be changed dynamically.

My context is cl https://codereview.webrtc.org/1608793004/, where neither you
nor pbos liked the SetSharedOprions method. So the options handled there are the
ones I'd like to remove from VideoOptions: dscp, cpu_overuse_detection, and
disable_prerenderer_smoothing. My understanding is that the latter two make
little sense to change dynamically. As for dscp, do we really want to spend a
lot of effort on that? (If any of you wants to know how I think about QoS, see
chapter 2 of https://www.lysator.liu.se/~nisse/network-book/network-book.pdf).

> So, if anything, I would recommend we move dscp out of VideoOptions and
> AudioOptions and into RtpParameters and leave it as a dynamic parameter that
the
> API user can change any time.  And as Taylor implements per-packet DSCP, it
will
> fall right into place.

I could try that. But what about the other two "shared" options? And what I
really want is to make progress on https://codereview.webrtc.org/1608793004/, so
how to move that forward?

[nisse-webrtc, 2016-02-02 11:32:50]

https://codereview.webrtc.org/1646253004/diff/70001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1646253004/diff/70001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:4092: #if 0
On 2016/02/01 14:44:38, pbos-webrtc wrote:
> Assuming fixing this test is still pending?

Done (but not yet uploaded). Was harder than expected, since it needed some
hacking of FakeMediaEngine.

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:97: struct MediaChannelOptions {
On 2016/02/01 14:44:38, pbos-webrtc wrote:
> Can we call this Config? this may be fuzzy, but the current options are
> reconfigured on the fly, so maybe config could be construction-time?

Makes sense, but not changing it now, we need to decide if it should be moved
elsewhere and if it should be static or not.

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:381: MediaChannel() : options_(),
network_interface_(NULL) {}
On 2016/02/01 14:44:38, pbos-webrtc wrote:
> Can you remove the default constructor?

I need either a default constructor here, or a default constructor on
DataMediaChannel which passes default options, or require options also when
constructing a DataMediaChannel, even though they're currently not effective.

Hmm, maybe the last option is the best.

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2386: // Current implementation
always sets the DiffServCodePoint.
On 2016/02/01 14:44:38, pbos-webrtc wrote:
> // Default value when DSCP is disabled should be DSCP_DEFAULT.

Done.

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

https://codereview.webrtc.org/1646253004/diff/70001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:2838:
TEST_F(WebRtcVoiceEngineTestFake, TestSetDscpOptions) {
On 2016/02/01 14:44:38, pbos-webrtc wrote:
> Pref ConfiguresDscpInNetworkInterface or something like that. Also test the
> default options like in video, preferably.

Makes sense. But I'm not doing that right away.

[pthatcher1, 2016-02-02 19:50:30]

On 2016/02/02 08:11:20, nisse-webrtc wrote:
> On 2016/02/01 18:20:18, pthatcher1 wrote:
> > To explain in more detail what I am thinking: the future of this part of the
> > WebRTC API is going to be an RtpSender object which has a .send or
> > .setParameters method which provides all the parameters for sending audio or
> > video, via one big RtpParameters struct.
> 
> We'll see how that works out. My gut feeling is that "one big struct" makes
> sense if it 
> represents something like the contents of user config file, but for things set
> via program logic, it's clearer to keep things separate. But I don't really
have
> an informed opinion right now.

Another option I just thought of is to make it a configuration of the
MediaController.  Then it would be same for all RtpSenders (streams), not just
one.  Which would make sense in this context.

> 
> > We haven't found
> > anything yet like that (which we can't change dynamically), and so we
haven't
> > had the need for such a constructor argument.  Even the output transport and
> the
> > input track can be changed dynamically.
> 
> My context is cl https://codereview.webrtc.org/1608793004/, where neither you
> nor pbos liked the SetSharedOprions method. So the options handled there are
the
> ones I'd like to remove from VideoOptions: dscp, cpu_overuse_detection, and
> disable_prerenderer_smoothing. My understanding is that the latter two make
> little sense to change dynamically. As for dscp, do we really want to spend a
> lot of effort on that? (If any of you wants to know how I think about QoS, see
> chapter 2 of https://www.lysator.liu.se/~nisse/network-book/network-book.pdf).
> 
> > So, if anything, I would recommend we move dscp out of VideoOptions and
> > AudioOptions and into RtpParameters and leave it as a dynamic parameter that
> the
> > API user can change any time.  And as Taylor implements per-packet DSCP, it
> will
> > fall right into place.
> 
> I could try that. But what about the other two "shared" options? And what I
> really want is to make progress on https://codereview.webrtc.org/1608793004/,
so
> how to move that forward?

If they are options that apply to all media streams together, we might consider
making it an option of MediaController:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

At that level, a MediaOptions or MediaControllerConfig might make sense. 
Unfortunately, the MediaController is currently just a thin wrapper around
ChannelManager and webrtc::Call.  Eventually, we'd like it to replace
ChannelManager instead.  Perhaps it would just pass this thing down to
ChannelManager, which would pass it down to BaseChannels, which would pass them
down to MediaChannels.  MediaOptions might be a good name after all, except that
it doesn't make it clear that these are static options, which needs to be clear.

Perhaps we should call them StaticMediaOptions.

[nisse-webrtc, 2016-02-03 12:34:01]

On 2016/02/02 19:50:30, pthatcher1 wrote:

> Another option I just thought of is to make it a configuration of the
> MediaController.  Then it would be same for all RtpSenders (streams), not just
> one.  Which would make sense in this context.

This adds to my confusion... MediaController seems to own an webrtc::Call (it
also gets a channel_manager, but that's seems to be mostly unused). webrtc::Call
gets some construction time settings, and it seems to own all streams between
two particular end points, in some sense. But I don't see any obvious connection
to RtpSender and RtpReceiver.

To be concrete, consider the cpu_overuse_detection flag, what should the path be
from WebRtcSession::Initialize to WebRtcVideoChannel2?

> Perhaps it would just pass this thing down to
> ChannelManager, which would pass it down to BaseChannels, which would pass
them
> down to MediaChannels.

Is there a one-to-one correspondance between channel managers and media
controllers? The construction seems a bit convoluted, involving
PeerConnectionFactory in some way. 

> Perhaps we should call them StaticMediaOptions.

I think we should try to adhere to the convention that FooConfig are
construction time settings.

[pthatcher1, 2016-02-03 18:31:12]

On 2016/02/03 12:34:01, nisse-webrtc wrote:
> On 2016/02/02 19:50:30, pthatcher1 wrote:
> 
> > Another option I just thought of is to make it a configuration of the
> > MediaController.  Then it would be same for all RtpSenders (streams), not
just
> > one.  Which would make sense in this context.
> 
> This adds to my confusion... MediaController seems to own an webrtc::Call (it
> also gets a channel_manager, but that's seems to be mostly unused).
webrtc::Call
> gets some construction time settings, and it seems to own all streams between
> two particular end points, in some sense. But I don't see any obvious
connection
> to RtpSender and RtpReceiver.

Someday, RtpSender and RtpReceiver will be created by MediaController, and
control will be passed down via RtpSender::SetParameters (see the comment above
MediaControllerInterface).

For now, RtpSender and RtpReceiver don't pass down control via SetParameters. 
They are passed down via BaseChannel::SetRemoteDescription and
SetLocalDescription, and BaseChannels are owned by ChannelManager.   Further
RtpSender works on top of a "provider" which is a WebRtcSession, which passes
the work down to a BaseChannel.  

TL;DR: The connection is RtpSender -> provider/WebRtcSession -> VideoChannel and
MediaController -> ChannelManager -> VideoChannel.

It's indirect, but it's there.  We hope to fix this someday to be more
direct/clear.

> 
> To be concrete, consider the cpu_overuse_detection flag, what should the path
be
> from WebRtcSession::Initialize to WebRtcVideoChannel2?


WebRtcSession -> VideoChannel -> WebRtcMediaVideoChannel2  at the time when
VideoChannel is created.


> 
> > Perhaps it would just pass this thing down to
> > ChannelManager, which would pass it down to BaseChannels, which would pass
> them
> > down to MediaChannels.
> 
> Is there a one-to-one correspondance between channel managers and media
> controllers? The construction seems a bit convoluted, involving
> PeerConnectionFactory in some way. 

Yes, it's convoluted.

A ChannelManager is across PeerConnections.  A MediaController is
per-PeerConnection.  In other words, many MediaControllers share a
ChannelManager. 

We intend to remove ChannelManager eventually and just have MediaController. 
And we intend to remove BaseChannel and just have RtpSender/RtpReceiver.


> 
> > Perhaps we should call them StaticMediaOptions.
> 
> I think we should try to adhere to the convention that FooConfig are
> construction time settings.

If that's the convention, then sure.  I'd be happy with MediaConfig passed into
the MediaController constructor, and then passed into the VideoChannel and
VoiceChannel constructors.

[nisse-webrtc, 2016-02-04 09:21:02]

On 2016/02/03 18:31:12, pthatcher1 wrote:
> On 2016/02/03 12:34:01, nisse-webrtc wrote:
> > To be concrete, consider the cpu_overuse_detection flag, what should the
path
> be
> > from WebRtcSession::Initialize to WebRtcVideoChannel2?
>  
> WebRtcSession -> VideoChannel -> WebRtcMediaVideoChannel2  at the time when
> VideoChannel is created.

So if I move cpu_overuse_detection and disable_prerenderer_smoothing from
VideoOptions into a new struct VideoChannelConfig (construction time settings)
and pass along to the VideoMediaChannel constructor (similarly to how
enable_dscp is handled in this cl), is that ok? What should I do with dscp,
leave it for now and do a new cl for move the two video-specific
construction-time settings?

[pthatcher1, 2016-02-04 17:21:31]

On 2016/02/04 09:21:02, nisse-webrtc wrote:
> On 2016/02/03 18:31:12, pthatcher1 wrote:
> > On 2016/02/03 12:34:01, nisse-webrtc wrote:
> > > To be concrete, consider the cpu_overuse_detection flag, what should the
> path
> > be
> > > from WebRtcSession::Initialize to WebRtcVideoChannel2?
> >  
> > WebRtcSession -> VideoChannel -> WebRtcMediaVideoChannel2  at the time when
> > VideoChannel is created.
> 
> So if I move cpu_overuse_detection and disable_prerenderer_smoothing from
> VideoOptions into a new struct VideoChannelConfig (construction time settings)
> and pass along to the VideoMediaChannel constructor (similarly to how
> enable_dscp is handled in this cl), is that ok? What should I do with dscp,
> leave it for now and do a new cl for move the two video-specific
> construction-time settings.

VideoChannelConfig is an awkward name for something that is passed into the
VideoMediaChannel (VideoChannel != VideoMediaChannel).  But
VideoMediaChannelConfig is also an awkward name because this thing will be set 
in the PeerConnection, which doesn't know anything about VideoMediaChannels. 
Finally, it's our intention to get rid of both VideoChannels and
VideoMediaChannels in the future (skip directly from RtpSender to the streams
below WebRtcVideoMediaChannel).  So, it needs to be a name that makes sense up
at PeerConnection and down at WebRtcVideoMediaChannel and doesn't have the name
"channel" se it.

How about "MediaConfig" or "MediaControllerConfig"?  Pass it into
MediaController's constructor.  Then add MediaController::config(), and call it
in WebRtcSession::CreateVideoChannel and pass it into the VideoChannel
constructor and so on down?  I'm more in favor of "MediaConfig", but
"MediaControllerConfig" would be OK as well.

The values in it could be:

cpu_overuse_detection
disable_video_prerender_smoothing
dscp


And you could pass the same thing to VoiceChannel.  Some things won't make sense
for audio (video smoothing), but many things will (dscp, maybe
cpu_overuse_detection).

[nisse-webrtc, 2016-02-05 10:45:19]

On 2016/02/04 17:21:31, pthatcher1 wrote:
> How about "MediaConfig" or "MediaControllerConfig"? 

MediaConfig sounds ok to me.

> Pass it into
> MediaController's constructor.  Then add MediaController::config(), and call
it
> in WebRtcSession::CreateVideoChannel and pass it into the VideoChannel
> constructor and so on down?

I'll try this approach. I've made a not very careful rebase of the current cl,
but I'm going to start on a new one for the new approach.
 
> The values in it could be:
> 
> cpu_overuse_detection
> disable_video_prerender_smoothing
> dscp

And for now, enable_dscp will be a construction time config?

> And you could pass the same thing to VoiceChannel.  Some things won't make
sense
> for audio (video smoothing), but many things will (dscp, maybe
> cpu_overuse_detection).

I think I would prefer to structure MediaConfig to keep audio and video
separate, and only pass the video-relevant config down to the video channels.
We'll see if that's easy or not, it's not essential.