Stop caching supported codecs in WebRtcVideoEngine2

Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337
Committed: https://crrev.com/9f71ec5a3e3175751f4475b126cfda89767363f2
Cr-Commit-Position: refs/heads/master@{#15007}

[magjed_webrtc, 2016-11-09 14:51:43]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-09 15:16:35]

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

[commit-bot: I haz the power, 2016-11-09 15:16:36]

Dry run: This issue passed the CQ dry run.

[magjed1, 2016-11-09 15:25:12]

Description was changed from

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=None
==========

to

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337
==========

[magjed1, 2016-11-09 15:25:12]

magjed@google.com changed reviewers:
+ tommi@webrtc.org

[magjed1, 2016-11-09 15:25:26]

magjed@google.com changed reviewers:
+ magjed@google.com

[magjed1, 2016-11-09 15:25:27]

tommi - please take a look.

[tommi, 2016-11-09 16:24:31]

lgtm.  Minor thing below that might be good to get right and consistent, but not
blocking this CL.

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc#n...
webrtc/pc/channelmanager.cc:33: return _stricmp(kRtxCodecName,
codec.name.c_str()) == 0;
in tests, we have case sensitive comparisons for this codec name. Outside of
tests we have stricmp in a couple of places (looks like you're copying one
here), but in SdpAudioFormat we actually have a case sensitive comparison when
we do lookups.

Can you check with Harald to see what is the right thing to be doing?  I suspect
case sensitive is what we want.  If so, you could replace IsRtxCodec with simply
codec.name.compare(kRtxCodecName) == 0

Btw, there are also macros for doing case sensitive and case insensitive string
compare.  See STR_NCASE_CMP.

[magjed_webrtc, 2016-11-09 20:37:30]

magjed@webrtc.org changed reviewers:
- magjed@google.com

[magjed_webrtc, 2016-11-09 20:37:53]

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc#n...
webrtc/pc/channelmanager.cc:33: return _stricmp(kRtxCodecName,
codec.name.c_str()) == 0;
On 2016/11/09 16:24:30, tommi (webrtc) wrote:
> in tests, we have case sensitive comparisons for this codec name. Outside of
> tests we have stricmp in a couple of places (looks like you're copying one
> here), but in SdpAudioFormat we actually have a case sensitive comparison when
> we do lookups.
> 
> Can you check with Harald to see what is the right thing to be doing?  I
suspect
> case sensitive is what we want.  If so, you could replace IsRtxCodec with
simply
> codec.name.compare(kRtxCodecName) == 0
> 
> Btw, there are also macros for doing case sensitive and case insensitive
string
> compare.  See STR_NCASE_CMP.

Ok, I will ask hta@ for advice. The code here is just moved from the existing
code at line 147, but it might be wrong.

Regarding std::string::compare, won't that create a temporary std::string out of
the kRtxCodecName (char*), which will hurt performance?

[tommi, 2016-11-09 20:59:50]

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc#n...
webrtc/pc/channelmanager.cc:33: return _stricmp(kRtxCodecName,
codec.name.c_str()) == 0;
On 2016/11/09 20:37:53, magjed_webrtc wrote:
> On 2016/11/09 16:24:30, tommi (webrtc) wrote:
> > in tests, we have case sensitive comparisons for this codec name. Outside of
> > tests we have stricmp in a couple of places (looks like you're copying one
> > here), but in SdpAudioFormat we actually have a case sensitive comparison
when
> > we do lookups.
> > 
> > Can you check with Harald to see what is the right thing to be doing?  I
> suspect
> > case sensitive is what we want.  If so, you could replace IsRtxCodec with
> simply
> > codec.name.compare(kRtxCodecName) == 0
> > 
> > Btw, there are also macros for doing case sensitive and case insensitive
> string
> > compare.  See STR_NCASE_CMP.
> 
> Ok, I will ask hta@ for advice. The code here is just moved from the existing
> code at line 147, but it might be wrong.
> 
> Regarding std::string::compare, won't that create a temporary std::string out
of
> the kRtxCodecName (char*), which will hurt performance?

There's a version of compare() that takes a const char*, so the performance
should be the same as with strcmp.

[magjed_webrtc, 2016-11-09 21:15:10]

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2492473002/diff/1/webrtc/pc/channelmanager.cc#n...
webrtc/pc/channelmanager.cc:33: return _stricmp(kRtxCodecName,
codec.name.c_str()) == 0;
On 2016/11/09 20:59:50, tommi (webrtc) wrote:
> On 2016/11/09 20:37:53, magjed_webrtc wrote:
> > On 2016/11/09 16:24:30, tommi (webrtc) wrote:
> > > in tests, we have case sensitive comparisons for this codec name. Outside
of
> > > tests we have stricmp in a couple of places (looks like you're copying one
> > > here), but in SdpAudioFormat we actually have a case sensitive comparison
> when
> > > we do lookups.
> > > 
> > > Can you check with Harald to see what is the right thing to be doing?  I
> > suspect
> > > case sensitive is what we want.  If so, you could replace IsRtxCodec with
> > simply
> > > codec.name.compare(kRtxCodecName) == 0
> > > 
> > > Btw, there are also macros for doing case sensitive and case insensitive
> > string
> > > compare.  See STR_NCASE_CMP.
> > 
> > Ok, I will ask hta@ for advice. The code here is just moved from the
existing
> > code at line 147, but it might be wrong.
> > 
> > Regarding std::string::compare, won't that create a temporary std::string
out
> of
> > the kRtxCodecName (char*), which will hurt performance?
> 
> There's a version of compare() that takes a const char*, so the performance
> should be the same as with strcmp.

Ah cool, I missed that.

[magjed_webrtc, 2016-11-10 07:43:28]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-11-10 07:43:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-10 07:45:14]

Description was changed from

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337
==========

to

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337
==========

[commit-bot: I haz the power, 2016-11-10 07:45:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-10 07:45:25]

Description was changed from

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337
==========

to

==========
Stop caching supported codecs in WebRtcVideoEngine2

We currently cache the result of GetSupportedCodecs in a member variable
|video_codecs_| in WebRtcVideoEngine2. This means we need to keep
|video_codecs_| and the result of GetSupportedCodecs in sync, which is
error prone. It's simpler to just call GetSupportedCodecs when we need
it, and we actually end up making fewer calls, so it's faster as well.
This CL also returns all std::vectors by-value instead of by-ref. Move
semantic together with in-place filtering of codecs actually end up with
fewer copies, and it's also simpler to not return references.

BUG=webrtc:6337

Committed: https://crrev.com/9f71ec5a3e3175751f4475b126cfda89767363f2
Cr-Commit-Position: refs/heads/master@{#15007}
==========

[commit-bot: I haz the power, 2016-11-10 07:45:26]

Patchset 1 (id:??) landed as
https://crrev.com/9f71ec5a3e3175751f4475b126cfda89767363f2
Cr-Commit-Position: refs/heads/master@{#15007}

[magjed_webrtc, 2016-11-10 11:36:42]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.webrtc.org/2489173004/ by magjed@webrtc.org.

The reason for reverting is: This CL probably broke Chromium FYI..