Move some send stream configuration into webrtc::AudioSendStream.

Move some send stream configuration into webrtc::AudioSendStream.

BUG=webrtc:4690
Committed: https://crrev.com/3a94154035fa16e4efd91125311f076b547c38b9
Cr-Commit-Position: refs/heads/master@{#10652}

[the sun, 2015-11-11 12:48:42]

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

[the sun, 2015-11-12 13:07:25]

On 2015/11/11 12:48:43, the sun wrote:

btw, not submitting this before the M48 roll, should you find time to review
before then...

[tommi, 2015-11-12 13:44:57]

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

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1383: config_.rtp.extensions =
extensions;
possibly silly question - does the caller need the vector after constructing the
audio send stream or will it be thrown away?  I'm basically wondering if it
would make sense to pass by ptr and do a swap().  Maybe this happens so rarely
and contains so few elements that it's not worth it, but figured I'd ask anyway.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1384: stream_ =
call_->CreateAudioSendStream(config_);
should we first DCHECK that stream_ is nullptr?

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1390: return stream_->GetStats();
any chance that stream_ might be null?

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1459: webrtc::AudioSendStream* stream_ =
nullptr;
nit: if you could add a note about ownership or lifetime, that would be nice.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1524: if (send_rtp_extensions_ !=
send_rtp_extensions) {
does order matter or is it guaranteed that a vector of RtpExtension objects is
always sorted? (would it make sense to use a set?)

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1525: send_rtp_extensions_ =
send_rtp_extensions;
do the send streams have a pointer back to the channel or some other way to
access it?  I'm wondering if that should be used to access the extension list or
if it's better to copy the list into every stream. (if better to copy, is it a
performance optimization to avoid locks or something like that?)

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1534: if (!SetOptions(params.options)) {
nit:

return SetOptions(params.options);

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:67: RTC_CHECK_EQ(0,
rtp->SetRTCP_CNAME(channel_id, config.rtp.c_name.c_str()));
Are all these CHECKs reasonable?  I think checks are good but I think CHECKs
should be there for a reason (usually there would be a security issue if  CHECK
hits), but if the implementation is simple enough that it shouldn't fail, DCHECK
might be the way to go.  CHECK isn't free (as opposed to DCHECK), so my concern
is code bloat.

[the sun, 2015-11-12 15:16:44]

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

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1383: config_.rtp.extensions =
extensions;
On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> possibly silly question - does the caller need the vector after constructing
the
> audio send stream or will it be thrown away?  I'm basically wondering if it
> would make sense to pass by ptr and do a swap().  Maybe this happens so rarely
> and contains so few elements that it's not worth it, but figured I'd ask
anyway.

Yes, the caller needs it. The same extensions are set for multiple streams, and
I'd rather keep this interface analogous with the c-tor. OTOH, I can use a swap
in SetSendParameters(); thanks.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1384: stream_ =
call_->CreateAudioSendStream(config_);
On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> should we first DCHECK that stream_ is nullptr?

Done.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1390: return stream_->GetStats();
On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> any chance that stream_ might be null?

Yes, stream_ is mutable so a check is in place.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1459: webrtc::AudioSendStream* stream_ =
nullptr;
On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> nit: if you could add a note about ownership or lifetime, that would be nice.

Done.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1524: if (send_rtp_extensions_ !=
send_rtp_extensions) {
On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> does order matter or is it guaranteed that a vector of RtpExtension objects is
> always sorted? (would it make sense to use a set?)

Order does not matter.

I don't think a set makes a lot of sense here; if we want to express the rules
surrounding rtp header extensions, there are other properties except for
uniqueness (id range, some are mutually exclusive, etc). Rather than using a
data structure that will communicate one such aspect, we could have a
specialized collection which verifies all. 

Such logic really belongs in code handling the SDP. Down here, we might just as
well pass them in a vector and DCHECK that things are sane...

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1525: send_rtp_extensions_ =
send_rtp_extensions;
On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> do the send streams have a pointer back to the channel or some other way to
> access it?  I'm wondering if that should be used to access the extension list
or
> if it's better to copy the list into every stream. (if better to copy, is it a
> performance optimization to avoid locks or something like that?)

There is some pessimisation here, yes. Keep in mind though that we are talking
about ~124 bytes to be copied for header extensions when streams are
created/reconfigured (which is quite rare). The extensions are stored in the
AudioSendStream::Config POD, and actually copied once more into the
internal::AudioSendStream.

Adding a pointer from WebRtcAudioSendStream back to WVoMC would make that
relationship circular, which IMO is tantamount to building technical debt. I
don't think any conceivable gain is warranted in this case.

Instead, if copying these bytes needs to be avoided at all costs, I would
suggest making each RtpExtension object ref counted, although that will slightly
increase the static memory load (by some 2x16 bytes, which is 26% of the object
sizes in this case). WViE should be changed to work the same too. Also, say
after me please; shared_ptr...

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1534: if (!SetOptions(params.options)) {
On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> nit:
> 
> return SetOptions(params.options);

Done.

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:67: RTC_CHECK_EQ(0,
rtp->SetRTCP_CNAME(channel_id, config.rtp.c_name.c_str()));
On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> Are all these CHECKs reasonable?  I think checks are good but I think CHECKs
> should be there for a reason (usually there would be a security issue if 
CHECK
> hits), but if the implementation is simple enough that it shouldn't fail,
DCHECK
> might be the way to go.  CHECK isn't free (as opposed to DCHECK), so my
concern
> is code bloat.

DCHECK is stripped out in non-debug builds, right?

Here's the condition in checks.h:
#if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))

The code here is required in all builds, hence the CHECK. Going forward, the
methods on the underlying Channel object will be called directly and changed as
much as possible to not return any error codes unless necessary.

[tommi, 2015-11-12 16:36:18]

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

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1524: if (send_rtp_extensions_ !=
send_rtp_extensions) {
On 2015/11/12 15:16:44, the sun wrote:
> On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> > does order matter or is it guaranteed that a vector of RtpExtension objects
is
> > always sorted? (would it make sense to use a set?)
> 
> Order does not matter.
> 
> I don't think a set makes a lot of sense here; if we want to express the rules
> surrounding rtp header extensions, there are other properties except for
> uniqueness (id range, some are mutually exclusive, etc). Rather than using a
> data structure that will communicate one such aspect, we could have a
> specialized collection which verifies all. 
> 
> Such logic really belongs in code handling the SDP. Down here, we might just
as
> well pass them in a vector and DCHECK that things are sane...

Just in case we're talking about different things, my concern was about whether
the != operator would incorrectly handle a case like below or if a case like
below is not possible:

{1,2,3} != {3,2,1}

They're not equal only due to the order.
I might be misunderstanding things.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1525: send_rtp_extensions_ =
send_rtp_extensions;
On 2015/11/12 15:16:44, the sun wrote:
> On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> > do the send streams have a pointer back to the channel or some other way to
> > access it?  I'm wondering if that should be used to access the extension
list
> or
> > if it's better to copy the list into every stream. (if better to copy, is it
a
> > performance optimization to avoid locks or something like that?)
> 
> There is some pessimisation here, yes. Keep in mind though that we are talking
> about ~124 bytes to be copied for header extensions when streams are
> created/reconfigured (which is quite rare). The extensions are stored in the
> AudioSendStream::Config POD, and actually copied once more into the
> internal::AudioSendStream.
> 
> Adding a pointer from WebRtcAudioSendStream back to WVoMC would make that
> relationship circular, which IMO is tantamount to building technical debt. I
> don't think any conceivable gain is warranted in this case.
> 
> Instead, if copying these bytes needs to be avoided at all costs, I would
> suggest making each RtpExtension object ref counted, although that will
slightly
> increase the static memory load (by some 2x16 bytes, which is 26% of the
object
> sizes in this case). WViE should be changed to work the same too. Also, say
> after me please; shared_ptr...

Agreed.  I don't want to add a circular relationship but I was wondering if
there already is one (and one might exist for the purposes of making state
accessible).

OK as is.

oh, shared_ptr!

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:67: RTC_CHECK_EQ(0,
rtp->SetRTCP_CNAME(channel_id, config.rtp.c_name.c_str()));
On 2015/11/12 15:16:44, the sun wrote:
> On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> > Are all these CHECKs reasonable?  I think checks are good but I think CHECKs
> > should be there for a reason (usually there would be a security issue if 
> CHECK
> > hits), but if the implementation is simple enough that it shouldn't fail,
> DCHECK
> > might be the way to go.  CHECK isn't free (as opposed to DCHECK), so my
> concern
> > is code bloat.
> 
> DCHECK is stripped out in non-debug builds, right?
> 
> Here's the condition in checks.h:
> #if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))
> 
> The code here is required in all builds, hence the CHECK. Going forward, the
> methods on the underlying Channel object will be called directly and changed
as
> much as possible to not return any error codes unless necessary.

Yes, DCHECK is stripped out.  You don't have to put the expressions themselves
into the DCHECK though.

[the sun, 2015-11-16 13:49:52]

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

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1524: if (send_rtp_extensions_ !=
send_rtp_extensions) {
On 2015/11/12 16:36:18, tommi (webrtc) wrote:
> On 2015/11/12 15:16:44, the sun wrote:
> > On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> > > does order matter or is it guaranteed that a vector of RtpExtension
objects
> is
> > > always sorted? (would it make sense to use a set?)
> > 
> > Order does not matter.
> > 
> > I don't think a set makes a lot of sense here; if we want to express the
rules
> > surrounding rtp header extensions, there are other properties except for
> > uniqueness (id range, some are mutually exclusive, etc). Rather than using a
> > data structure that will communicate one such aspect, we could have a
> > specialized collection which verifies all. 
> > 
> > Such logic really belongs in code handling the SDP. Down here, we might just
> as
> > well pass them in a vector and DCHECK that things are sane...
> 
> Just in case we're talking about different things, my concern was about
whether
> the != operator would incorrectly handle a case like below or if a case like
> below is not possible:
> 
> {1,2,3} != {3,2,1}
> 
> They're not equal only due to the order.
> I might be misunderstanding things.

No, you're quite right. Sorry for being daft.

On closer inspection it turns out no validation of the header extension is
provided by layers higher up. The SDP is simply parsed and pushed down here. So,
order will matter, and in other ways other things (ID range, actual supported
extensions, etc) will also matter.

The video engine does a better job validating the supplied extensions, but the
code is contained to WViMC. I started out refactoring that but it shoots off in
a different direction from the changes here. Would you be ok if I do that in a
separate CL (this CL doesn't change the behavior we currently have)?

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1525: send_rtp_extensions_ =
send_rtp_extensions;
On 2015/11/12 16:36:18, tommi (webrtc) wrote:
> On 2015/11/12 15:16:44, the sun wrote:
> > On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> > > do the send streams have a pointer back to the channel or some other way
to
> > > access it?  I'm wondering if that should be used to access the extension
> list
> > or
> > > if it's better to copy the list into every stream. (if better to copy, is
it
> a
> > > performance optimization to avoid locks or something like that?)
> > 
> > There is some pessimisation here, yes. Keep in mind though that we are
talking
> > about ~124 bytes to be copied for header extensions when streams are
> > created/reconfigured (which is quite rare). The extensions are stored in the
> > AudioSendStream::Config POD, and actually copied once more into the
> > internal::AudioSendStream.
> > 
> > Adding a pointer from WebRtcAudioSendStream back to WVoMC would make that
> > relationship circular, which IMO is tantamount to building technical debt. I
> > don't think any conceivable gain is warranted in this case.
> > 
> > Instead, if copying these bytes needs to be avoided at all costs, I would
> > suggest making each RtpExtension object ref counted, although that will
> slightly
> > increase the static memory load (by some 2x16 bytes, which is 26% of the
> object
> > sizes in this case). WViE should be changed to work the same too. Also, say
> > after me please; shared_ptr...
> 
> Agreed.  I don't want to add a circular relationship but I was wondering if
> there already is one (and one might exist for the purposes of making state
> accessible).
> 
> OK as is.
> 
> oh, shared_ptr!

Well, strictly speaking there is an implicit circular relationship to WVoMC
because the latter owns the VoE channel used here, but that will be going away.

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1418503010/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:67: RTC_CHECK_EQ(0,
rtp->SetRTCP_CNAME(channel_id, config.rtp.c_name.c_str()));
On 2015/11/12 16:36:18, tommi (webrtc) wrote:
> On 2015/11/12 15:16:44, the sun wrote:
> > On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> > > Are all these CHECKs reasonable?  I think checks are good but I think
CHECKs
> > > should be there for a reason (usually there would be a security issue if 
> > CHECK
> > > hits), but if the implementation is simple enough that it shouldn't fail,
> > DCHECK
> > > might be the way to go.  CHECK isn't free (as opposed to DCHECK), so my
> > concern
> > > is code bloat.
> > 
> > DCHECK is stripped out in non-debug builds, right?
> > 
> > Here's the condition in checks.h:
> > #if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))
> > 
> > The code here is required in all builds, hence the CHECK. Going forward, the
> > methods on the underlying Channel object will be called directly and changed
> as
> > much as possible to not return any error codes unless necessary.
> 
> Yes, DCHECK is stripped out.  You don't have to put the expressions themselves
> into the DCHECK though.

You're right. There will be some code bloat because of the CHECK. Also, since
I've actually verified that these calls will never fail in our case, DCHECKs
should be used (it's a logic assumption, not a potential runtime issue).

[tommi, 2015-11-16 14:05:27]

lgtm

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

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1524: if (send_rtp_extensions_ !=
send_rtp_extensions) {
On 2015/11/16 13:49:51, the sun wrote:
> On 2015/11/12 16:36:18, tommi (webrtc) wrote:
> > On 2015/11/12 15:16:44, the sun wrote:
> > > On 2015/11/12 13:44:57, tommi (webrtc) wrote:
> > > > does order matter or is it guaranteed that a vector of RtpExtension
> objects
> > is
> > > > always sorted? (would it make sense to use a set?)
> > > 
> > > Order does not matter.
> > > 
> > > I don't think a set makes a lot of sense here; if we want to express the
> rules
> > > surrounding rtp header extensions, there are other properties except for
> > > uniqueness (id range, some are mutually exclusive, etc). Rather than using
a
> > > data structure that will communicate one such aspect, we could have a
> > > specialized collection which verifies all. 
> > > 
> > > Such logic really belongs in code handling the SDP. Down here, we might
just
> > as
> > > well pass them in a vector and DCHECK that things are sane...
> > 
> > Just in case we're talking about different things, my concern was about
> whether
> > the != operator would incorrectly handle a case like below or if a case like
> > below is not possible:
> > 
> > {1,2,3} != {3,2,1}
> > 
> > They're not equal only due to the order.
> > I might be misunderstanding things.
> 
> No, you're quite right. Sorry for being daft.
> 
> On closer inspection it turns out no validation of the header extension is
> provided by layers higher up. The SDP is simply parsed and pushed down here.
So,
> order will matter, and in other ways other things (ID range, actual supported
> extensions, etc) will also matter.
> 
> The video engine does a better job validating the supplied extensions, but the
> code is contained to WViMC. I started out refactoring that but it shoots off
in
> a different direction from the changes here. Would you be ok if I do that in a
> separate CL (this CL doesn't change the behavior we currently have)? 

Yes, another CL is fine.

https://codereview.webrtc.org/1418503010/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1525: send_rtp_extensions_ =
send_rtp_extensions;
On 2015/11/16 13:49:51, the sun wrote:
> On 2015/11/12 16:36:18, tommi (webrtc) wrote:
> > On 2015/11/12 15:16:44, the sun wrote:
> > > On 2015/11/12 13:44:56, tommi (webrtc) wrote:
> > > > do the send streams have a pointer back to the channel or some other way
> to
> > > > access it?  I'm wondering if that should be used to access the extension
> > list
> > > or
> > > > if it's better to copy the list into every stream. (if better to copy,
is
> it
> > a
> > > > performance optimization to avoid locks or something like that?)
> > > 
> > > There is some pessimisation here, yes. Keep in mind though that we are
> talking
> > > about ~124 bytes to be copied for header extensions when streams are
> > > created/reconfigured (which is quite rare). The extensions are stored in
the
> > > AudioSendStream::Config POD, and actually copied once more into the
> > > internal::AudioSendStream.
> > > 
> > > Adding a pointer from WebRtcAudioSendStream back to WVoMC would make that
> > > relationship circular, which IMO is tantamount to building technical debt.
I
> > > don't think any conceivable gain is warranted in this case.
> > > 
> > > Instead, if copying these bytes needs to be avoided at all costs, I would
> > > suggest making each RtpExtension object ref counted, although that will
> > slightly
> > > increase the static memory load (by some 2x16 bytes, which is 26% of the
> > object
> > > sizes in this case). WViE should be changed to work the same too. Also,
say
> > > after me please; shared_ptr...
> > 
> > Agreed.  I don't want to add a circular relationship but I was wondering if
> > there already is one (and one might exist for the purposes of making state
> > accessible).
> > 
> > OK as is.
> > 
> > oh, shared_ptr!
> 
> Well, strictly speaking there is an implicit circular relationship to WVoMC
> because the latter owns the VoE channel used here, but that will be going
away.

Acknowledged.

[the sun, 2015-11-16 14:14:36]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-11-16 14:14:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1418503010/#ps180001 (title:
"rebase")

[commit-bot: I haz the power, 2015-11-16 14:14:44]

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

[commit-bot: I haz the power, 2015-11-16 15:34:54]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-16 15:35:08]

Patchset 10 (id:??) landed as
https://crrev.com/3a94154035fa16e4efd91125311f076b547c38b9
Cr-Commit-Position: refs/heads/master@{#10652}