WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

This removes one more place where we were unable to handle codecs not
in the built-in set.

BUG=webrtc:5805
Review-Url: https://codereview.webrtc.org/2686043006
Cr-Commit-Position: refs/heads/master@{#17370}
Committed: https://chromium.googlesource.com/external/webrtc/+/1724cfbdbafef4736fedda37312b0286f1eb03d0

[kwiberg-webrtc, 2017-02-10 13:50:26]

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

[commit-bot: I haz the power, 2017-02-10 13:50:30]

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, 2017-02-10 15:23:46]

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

[commit-bot: I haz the power, 2017-02-10 15:23:48]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-27 09:21:57]

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

[commit-bot: I haz the power, 2017-02-27 09:21:59]

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, 2017-02-27 09:50:21]

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

[commit-bot: I haz the power, 2017-02-27 09:50:21]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-27 09:58:24]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org, solenberg@webrtc.org

[the sun, 2017-02-27 10:10:19]

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2261: // TODO(solenberg): Remove once
"no codecs" is the default state of a stream.
Is there any change required to VoE::Channel to make sure there are no codecs
added by default?

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
std::move(decoder_map);
hmm, is this a move or a swap?

[the sun, 2017-02-27 11:48:52]

lgtm % a test that doesn't do what the comment says

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:846: // Test that changes to
recv codecs are applied to all streams.
It doesn't look like the test is doing this - it must have been broken sometime.
Can you please add one or more streams?

[ossu, 2017-03-02 16:55:09]

So, just to clarify, receive codecs weren't properly set for streams added after
the set of codecs had been determined?

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
std::move(decoder_map);
On 2017/02/27 10:10:19, the sun wrote:
> hmm, is this a move or a swap?

Since we're not holding on to decoder_map outside of this scope, I don't think
it matters. The storage from decoder_map will be moved into decoder_map_ and the
storage previously in decoder_map_ will be destroyed, regardless.

[kwiberg-webrtc, 2017-03-09 20:14:26]

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

[commit-bot: I haz the power, 2017-03-09 20:14:28]

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, 2017-03-09 20:16:18]

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

[commit-bot: I haz the power, 2017-03-09 20:16:19]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18390)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18197)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23671)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22444)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[kwiberg-webrtc, 2017-03-09 20:28:51]

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

[commit-bot: I haz the power, 2017-03-09 20:28:58]

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

[kwiberg-webrtc, 2017-03-09 20:29:19]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-09 20:44:24]

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

[commit-bot: I haz the power, 2017-03-09 20:44:25]

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

[the sun, 2017-03-10 12:12:28]

On 2017/03/02 16:55:09, ossu wrote:
> So, just to clarify, receive codecs weren't properly set for streams added
after
> the set of codecs had been determined?
> 
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
> std::move(decoder_map);
> On 2017/02/27 10:10:19, the sun wrote:
> > hmm, is this a move or a swap?
> 
> Since we're not holding on to decoder_map outside of this scope, I don't think
> it matters. The storage from decoder_map will be moved into decoder_map_ and
the
> storage previously in decoder_map_ will be destroyed, regardless.

ping ... is  there a new PS on the way? I'd really like to see this landed!

[perkj_webrtc, 2017-03-10 13:39:04]

On 2017/03/10 12:12:28, the sun wrote:
> On 2017/03/02 16:55:09, ossu wrote:
> > So, just to clarify, receive codecs weren't properly set for streams added
> after
> > the set of codecs had been determined?
> > 
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
> > std::move(decoder_map);
> > On 2017/02/27 10:10:19, the sun wrote:
> > > hmm, is this a move or a swap?
> > 
> > Since we're not holding on to decoder_map outside of this scope, I don't
think
> > it matters. The storage from decoder_map will be moved into decoder_map_ and
> the
> > storage previously in decoder_map_ will be destroyed, regardless.
> 
> ping ... is  there a new PS on the way? I'd really like to see this landed!

yes ping, me to :->

[kwiberg-webrtc, 2017-03-10 21:06:55]

On 2017/03/10 13:39:04, perkj_webrtc wrote:
> On 2017/03/10 12:12:28, the sun wrote:
> > On 2017/03/02 16:55:09, ossu wrote:
> > > So, just to clarify, receive codecs weren't properly set for streams added
> > after
> > > the set of codecs had been determined?
> > > 
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
> > > std::move(decoder_map);
> > > On 2017/02/27 10:10:19, the sun wrote:
> > > > hmm, is this a move or a swap?
> > > 
> > > Since we're not holding on to decoder_map outside of this scope, I don't
> think
> > > it matters. The storage from decoder_map will be moved into decoder_map_
and
> > the
> > > storage previously in decoder_map_ will be destroyed, regardless.
> > 
> > ping ... is  there a new PS on the way? I'd really like to see this landed!
> 
> yes ping, me to :->

Yes, #5 that I uploaded yesterday has a bug: it replaces the entire set of
decoders in NetEq, which causes a crash if we have queued packets for a decoder
that we remove. I'm currently attempting to fix that by not replacing the codecs
that are the same in the old and new map.

[the sun, 2017-03-11 14:59:17]

On 2017/03/10 21:06:55, kwiberg-webrtc wrote:
> On 2017/03/10 13:39:04, perkj_webrtc wrote:
> > On 2017/03/10 12:12:28, the sun wrote:
> > > On 2017/03/02 16:55:09, ossu wrote:
> > > > So, just to clarify, receive codecs weren't properly set for streams
added
> > > after
> > > > the set of codecs had been determined?
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > > webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
> > > > std::move(decoder_map);
> > > > On 2017/02/27 10:10:19, the sun wrote:
> > > > > hmm, is this a move or a swap?
> > > > 
> > > > Since we're not holding on to decoder_map outside of this scope, I don't
> > think
> > > > it matters. The storage from decoder_map will be moved into decoder_map_
> and
> > > the
> > > > storage previously in decoder_map_ will be destroyed, regardless.
> > > 
> > > ping ... is  there a new PS on the way? I'd really like to see this
landed!
> > 
> > yes ping, me to :->
> 
> Yes, #5 that I uploaded yesterday has a bug: it replaces the entire set of
> decoders in NetEq, which causes a crash if we have queued packets for a
decoder
> that we remove. I'm currently attempting to fix that by not replacing the
codecs
> that are the same in the old and new map.

FYI: According to standard we cannot replace payload type mappings, only add new
ones, for a session. The problem applies to packets in transit as well as those
being queued in the receiving client. See e.g.:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/engin...

[kwiberg-webrtc, 2017-03-13 08:30:44]

On 2017/03/11 14:59:17, the sun wrote:
> On 2017/03/10 21:06:55, kwiberg-webrtc wrote:
> > On 2017/03/10 13:39:04, perkj_webrtc wrote:
> > > On 2017/03/10 12:12:28, the sun wrote:
> > > > On 2017/03/02 16:55:09, ossu wrote:
> > > > > So, just to clarify, receive codecs weren't properly set for streams
> added
> > > > after
> > > > > the set of codecs had been determined?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > > > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
> > > > > webrtc/media/engine/webrtcvoiceengine.cc:1896: decoder_map_ =
> > > > > std::move(decoder_map);
> > > > > On 2017/02/27 10:10:19, the sun wrote:
> > > > > > hmm, is this a move or a swap?
> > > > > 
> > > > > Since we're not holding on to decoder_map outside of this scope, I
don't
> > > think
> > > > > it matters. The storage from decoder_map will be moved into
decoder_map_
> > and
> > > > the
> > > > > storage previously in decoder_map_ will be destroyed, regardless.
> > > > 
> > > > ping ... is  there a new PS on the way? I'd really like to see this
> landed!
> > > 
> > > yes ping, me to :->
> > 
> > Yes, #5 that I uploaded yesterday has a bug: it replaces the entire set of
> > decoders in NetEq, which causes a crash if we have queued packets for a
> decoder
> > that we remove. I'm currently attempting to fix that by not replacing the
> codecs
> > that are the same in the old and new map.
> 
> FYI: According to standard we cannot replace payload type mappings, only add
new
> ones, for a session. The problem applies to packets in transit as well as
those
> being queued in the receiving client. See e.g.:
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/engin...

I had a feeling that was the case, but good to know for sure. One more reason to
not pre-load channels with a decoder map that we later replace...

[kwiberg-webrtc, 2017-03-14 11:20:35]

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

[commit-bot: I haz the power, 2017-03-14 11:20:45]

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, 2017-03-14 11:43:19]

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

[commit-bot: I haz the power, 2017-03-14 11:43:20]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-03-14 14:22:37]

New patch set posted!

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2686043006/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:846: // Test that changes to
recv codecs are applied to all streams.
On 2017/02/27 11:48:52, the sun wrote:
> It doesn't look like the test is doing this - it must have been broken
sometime.
> Can you please add one or more streams?

Done.

[the sun, 2017-03-14 20:07:01]

Generally looking good! I think we can make things a bit simpler still, PTAL.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/base/annotations.h
File webrtc/base/annotations.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/base/annotations....
webrtc/base/annotations.h:11: #ifndef WEBRTC_BASE_ANNOTATIONS_H_
Please introduce this in a separate CL so we can discuss where it belongs.
"annotations.h" is rather broad - are there other things in webrtc/base/ that
fit here as well? Or does this actually fit somewhere else?

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/acm_receiver.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_receiver.h:82: // Replace the current set
of deocders with the specified set.
s/deocders/decoders/g

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_receiver.h:83: void SetCodecs(const
std::map<int, SdpAudioFormat>& codecs);
nit: these aren't exactly "Codecs"...

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/include/audio_coding_module.h:488: // Replace any
existing decoders with the given payload type -> decoder map.
nit: how about renaming the argument so that the comment can be removed?

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/decoder_database.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/decoder_database.cc:128: // First collect all
payload types that we'll remove or reassign, then remove
As discussed, we're not allowed to remove or reassign PT mappings. Besides, this
should all be taken care of in WVoMC (the API boundary) - at this level we
should be able to "just" latch in the new map (ignoring previous PT
assignments). May be a good idea to verify previous mappings are still there
though.

Or, did I miss something?

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:123: bool
RTPPayloadRegistry::SetAudioReceivePayloads(
Like with DecoderDatabase, we should be able to trust the upper layer (WVoMC) to
provide us a valid set of mappings. Here, we should DCHECK that the set is good,
since that is in the method's contract, but we needn't work around.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:139: return false;  //
Trying to overwrite a video PT with audio.
This should never happen, since we have an RTPPayloadRegistry instance per
voe::Channel.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:149: // Remove existing
audio payload types.
This shouldn't be allowed.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:161: const
SdpAudioFormat& audio_format = kv.second;
I suggest you do this conversion in voe::Channel, to avoid the dependency on
audio_codec_api in the rtp_rtcp_module. Then this function could just take a
vector of CodecInst.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:170: return true;
If you address the above I don't think we even need a a return value.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1327: if
(!rtp_payload_registry_->SetAudioReceivePayloads(codecs)) {
If you remove the return value from the above call, we don't need it in
Channel::SetReceiveCodecs(), or the ChannelProxy either.

[kwiberg-webrtc, 2017-03-16 10:24:13]

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/base/annotations.h
File webrtc/base/annotations.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/base/annotations....
webrtc/base/annotations.h:11: #ifndef WEBRTC_BASE_ANNOTATIONS_H_
On 2017/03/14 20:07:00, the sun wrote:
> Please introduce this in a separate CL so we can discuss where it belongs.
> "annotations.h" is rather broad - are there other things in webrtc/base/ that
> fit here as well? Or does this actually fit somewhere else?

Would do, except I think following some of your other advice will mean I no
longer need this.

It was extremely helpful when changing the return value of a bunch of methods
from void to bool, though...

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/acm_receiver.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_receiver.h:82: // Replace the current set
of deocders with the specified set.
On 2017/03/14 20:07:00, the sun wrote:
> s/deocders/decoders/g

Done.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/acm_receiver.h:83: void SetCodecs(const
std::map<int, SdpAudioFormat>& codecs);
On 2017/03/14 20:07:00, the sun wrote:
> nit: these aren't exactly "Codecs"...

What should these methods be called? SetDecoders? SetDecoderMap?

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/include/audio_coding_module.h:488: // Replace any
existing decoders with the given payload type -> decoder map.
On 2017/03/14 20:07:00, the sun wrote:
> nit: how about renaming the argument so that the comment can be removed?

payload_type_to_decoder_format_map? That's a mouthful... and not as easy to read
as the comment because _ is the only available punctuation.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/decoder_database.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/decoder_database.cc:128: // First collect all
payload types that we'll remove or reassign, then remove
On 2017/03/14 20:07:01, the sun wrote:
> As discussed, we're not allowed to remove or reassign PT mappings. Besides,
this
> should all be taken care of in WVoMC (the API boundary) - at this level we
> should be able to "just" latch in the new map (ignoring previous PT
> assignments). May be a good idea to verify previous mappings are still there
> though.
> 
> Or, did I miss something?

I tried to do this (DCHECK that the old map is a subset of the new one), but it
caused ~75 tests to fail. I then tried to also create channels with an empty
decoder map, but that didn't solve the problem.

For good reasons or bad, NetEq already had the ability to remove and reassign
payload type mappings, and our tests make use of it. I'm sure that could be
fixed (since, as you say, it's not allowed), but fixing it as part of this CL is
probably not a good idea---it's big enough already.

Should I post a bug about it (removing the remove/reassign capabilities from
NetEq, and dealing with the fallout), and leave the capability in for now? The
alternative for this CL would be to not make a SetCodecs() method in NetEq, but
use the old RegisterPayload(), Remove() etc.---which would not really solve the
problem at all.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:123: bool
RTPPayloadRegistry::SetAudioReceivePayloads(
On 2017/03/14 20:07:01, the sun wrote:
> Like with DecoderDatabase, we should be able to trust the upper layer (WVoMC)
to
> provide us a valid set of mappings. Here, we should DCHECK that the set is
good,
> since that is in the method's contract, but we needn't work around.

Done.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:139: return false;  //
Trying to overwrite a video PT with audio.
On 2017/03/14 20:07:01, the sun wrote:
> This should never happen, since we have an RTPPayloadRegistry instance per
> voe::Channel.

Acknowledged. I'll try adding DCHECKs that verify this.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:149: // Remove existing
audio payload types.
On 2017/03/14 20:07:01, the sun wrote:
> This shouldn't be allowed.

In principle, yes. See other comments...

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:161: const
SdpAudioFormat& audio_format = kv.second;
On 2017/03/14 20:07:01, the sun wrote:
> I suggest you do this conversion in voe::Channel, to avoid the dependency on
> audio_codec_api in the rtp_rtcp_module. Then this function could just take a
> vector of CodecInst.

Well, yes... but I'm trying to reduce the use of CodecInst. Adding another use
of it in an interface seems like a bad idea.

I think that the rtp_rtcp module needs only a subset of the information in
CodecInst/SdpAudioFormat, but actually switching to a type that's appropriate
may be more churn than we want in this CL, so a follow-up CL seems called for.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc:170: return true;
On 2017/03/14 20:07:01, the sun wrote:
> If you address the above I don't think we even need a a return value.

Acknowledged.

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1327: if
(!rtp_payload_registry_->SetAudioReceivePayloads(codecs)) {
On 2017/03/14 20:07:01, the sun wrote:
> If you remove the return value from the above call, we don't need it in
> Channel::SetReceiveCodecs(), or the ChannelProxy either.

Acknowledged. I'll remove as many of them as I can.

[ossu, 2017-03-20 17:01:41]

Just peeking in to let you know I've had a look through. There seems to be
enough work underway on a new patch set, so I've only left a single, menial
comment. I'll look through that next patch set, though!

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.h:255: // TODO(kwiberg): decoder_map_ and
recv_codecs_ store the exact same
In WebRtcVoiceEngine, the ordering of codecs in recv_codecs_ matters. Here it
doesn't, so for this usage, they store the exact same information. (Just had to
remind myself with a comment.)

[kwiberg-webrtc, 2017-03-22 13:05:22]

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

[commit-bot: I haz the power, 2017-03-22 13:05:32]

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, 2017-03-22 13:13:02]

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

[commit-bot: I haz the power, 2017-03-22 13:13:03]

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

[perkj_webrtc, 2017-03-23 10:50:22]

On 2017/03/20 17:01:41, ossu wrote:
> Just peeking in to let you know I've had a look through. There seems to be
> enough work underway on a new patch set, so I've only left a single, menial
> comment. I'll look through that next patch set, though!
> 
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> File webrtc/media/engine/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> webrtc/media/engine/webrtcvoiceengine.h:255: // TODO(kwiberg): decoder_map_
and
> recv_codecs_ store the exact same
> In WebRtcVoiceEngine, the ordering of codecs in recv_codecs_ matters. Here it
> doesn't, so for this usage, they store the exact same information. (Just had
to
> remind myself with a comment.)

Pretty ping please! I would like to use this during this quarter if possible.

[kwiberg-webrtc, 2017-03-23 12:30:02]

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

[commit-bot: I haz the power, 2017-03-23 12:30:06]

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, 2017-03-23 12:40:52]

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

[commit-bot: I haz the power, 2017-03-23 12:40:54]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

[kwiberg-webrtc, 2017-03-23 13:29:34]

On 2017/03/23 10:50:22, perkj_webrtc wrote:
> On 2017/03/20 17:01:41, ossu wrote:
> > Just peeking in to let you know I've had a look through. There seems to be
> > enough work underway on a new patch set, so I've only left a single, menial
> > comment. I'll look through that next patch set, though!
> > 
> >
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> > File webrtc/media/engine/webrtcvoiceengine.h (right):
> > 
> >
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> > webrtc/media/engine/webrtcvoiceengine.h:255: // TODO(kwiberg): decoder_map_
> and
> > recv_codecs_ store the exact same
> > In WebRtcVoiceEngine, the ordering of codecs in recv_codecs_ matters. Here
it
> > doesn't, so for this usage, they store the exact same information. (Just had
> to
> > remind myself with a comment.)
> 
> Pretty ping please! I would like to use this during this quarter if possible.

As of the latest patch set, I think I'm pretty much done. I'm just looking into
some newly broken tests...

[kwiberg-webrtc, 2017-03-24 02:18:03]

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

[commit-bot: I haz the power, 2017-03-24 02:18:14]

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, 2017-03-24 02:19:44]

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

[commit-bot: I haz the power, 2017-03-24 02:19:45]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18675)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24162)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22965)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24164)

[kwiberg-webrtc, 2017-03-24 02:30:24]

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

[commit-bot: I haz the power, 2017-03-24 02:30:33]

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, 2017-03-24 02:57:43]

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

[commit-bot: I haz the power, 2017-03-24 02:57:44]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-03-24 04:04:16]

On 2017/03/23 13:29:34, kwiberg-webrtc wrote:
> On 2017/03/23 10:50:22, perkj_webrtc wrote:
> > On 2017/03/20 17:01:41, ossu wrote:
> > > Just peeking in to let you know I've had a look through. There seems to be
> > > enough work underway on a new patch set, so I've only left a single,
menial
> > > comment. I'll look through that next patch set, though!
> > > 
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> > > File webrtc/media/engine/webrtcvoiceengine.h (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2686043006/diff/140001/webrtc/media/engine/webr...
> > > webrtc/media/engine/webrtcvoiceengine.h:255: // TODO(kwiberg):
decoder_map_
> > and
> > > recv_codecs_ store the exact same
> > > In WebRtcVoiceEngine, the ordering of codecs in recv_codecs_ matters. Here
> it
> > > doesn't, so for this usage, they store the exact same information. (Just
had
> > to
> > > remind myself with a comment.)
> > 
> > Pretty ping please! I would like to use this during this quarter if
possible.
> 
> As of the latest patch set, I think I'm pretty much done. I'm just looking
into
> some newly broken tests...

OK, bots say review it now!

[the sun, 2017-03-24 09:53:21]

still LGTM!

[ossu, 2017-03-24 10:10:11]

This is great! LGTM! Ship it! All the words!

[kwiberg-webrtc, 2017-03-24 10:12:58]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-03-24 10:13:09]

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, 2017-03-24 10:16:05]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490350378692660,
"parent_rev": "f137e97adbc00895c6485fb8ddee407312d0d775", "commit_rev":
"1724cfbdbafef4736fedda37312b0286f1eb03d0"}

[commit-bot: I haz the power, 2017-03-24 10:16:10]

Description was changed from

==========
WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

This removes one more place where we were unable to handle codecs not
in the built-in set.

BUG=webrtc:5805
==========

to

==========
WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

This removes one more place where we were unable to handle codecs not
in the built-in set.

BUG=webrtc:5805

Review-Url: https://codereview.webrtc.org/2686043006
Cr-Commit-Position: refs/heads/master@{#17370}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1724cfbdbafef4736fedda373...
==========

[commit-bot: I haz the power, 2017-03-24 10:16:12]

Committed patchset #13 (id:260001) as
https://chromium.googlesource.com/external/webrtc/+/1724cfbdbafef4736fedda373...

[kwiberg-webrtc, 2017-03-24 12:56:03]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.webrtc.org/2772043002/ by kwiberg@webrtc.org.

The reason for reverting is: Makes perf and Chromium FYI bots unhappy..