AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801
Committed: https://crrev.com/1e4d8b574cde64d93b98d89c7b817fb93185a307
Cr-Commit-Position: refs/heads/master@{#14274}

[kwiberg-webrtc, 2016-09-16 11:01:51]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now almost unused.

BUG=webrtc:5801
==========

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801
==========

[kwiberg-webrtc, 2016-09-16 11:02:19]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, ossu@webrtc.org

[kwiberg-webrtc, 2016-09-16 11:04:18]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-09-16 11:04:28]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-09-16 11:04:37]

Patchset #1 (id:40001) has been deleted

[kwiberg-webrtc, 2016-09-16 11:04:48]

Patchset #1 (id:60001) has been deleted

[kwiberg-webrtc, 2016-09-16 11:04:56]

Patchset #1 (id:80001) has been deleted

[ossu, 2016-09-16 11:39:32]

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:449: ci.plfreq = di->IsRed() ||
di->IsDtmf() ? 8000 : di->SampleRateHz();
This looks like a place where something might go wrong in the confluence of my
(upcoming) changes to DecoderDatabase, these changes, and solenberg's change
adding DTMF support at other clockrates. I'm not certain what, if anything, can
be done about this right here and now, though.

[kwiberg-webrtc, 2016-09-16 11:50:39]

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:449: ci.plfreq = di->IsRed() ||
di->IsDtmf() ? 8000 : di->SampleRateHz();
On 2016/09/16 11:39:32, ossu wrote:
> This looks like a place where something might go wrong in the confluence of my
> (upcoming) changes to DecoderDatabase, these changes, and solenberg's change
> adding DTMF support at other clockrates. I'm not certain what, if anything,
can
> be done about this right here and now, though.

It sounds like my best strategy is to land this quickly, then, so that I won't
be the one sitting with a broken CL when the dust settles. :-)

Seriously though, adding support for multiple DTMF clockrates would probably
involve letting NetEq keep track of the DTMF clockrate, meaning we could
actually get rid of one of the two special cases here. ...see, a perfectly
rational-sounding reason for why I should land this CL as-is. :-)

[ossu, 2016-09-16 11:55:59]

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:449: ci.plfreq = di->IsRed() ||
di->IsDtmf() ? 8000 : di->SampleRateHz();
On 2016/09/16 11:50:38, kwiberg-webrtc wrote:
> On 2016/09/16 11:39:32, ossu wrote:
> > This looks like a place where something might go wrong in the confluence of
my
> > (upcoming) changes to DecoderDatabase, these changes, and solenberg's change
> > adding DTMF support at other clockrates. I'm not certain what, if anything,
> can
> > be done about this right here and now, though.
> 
> It sounds like my best strategy is to land this quickly, then, so that I won't
> be the one sitting with a broken CL when the dust settles. :-)
> 
> Seriously though, adding support for multiple DTMF clockrates would probably
> involve letting NetEq keep track of the DTMF clockrate, meaning we could
> actually get rid of one of the two special cases here. ...see, a perfectly
> rational-sounding reason for why I should land this CL as-is. :-)

Hmm. I think di->GetFormat().clockrate_hz instead of 8000 should keep you safe
including upcoming changes.

[kwiberg-webrtc, 2016-09-16 12:04:20]

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:449: ci.plfreq = di->IsRed() ||
di->IsDtmf() ? 8000 : di->SampleRateHz();
On 2016/09/16 11:55:59, ossu wrote:
> On 2016/09/16 11:50:38, kwiberg-webrtc wrote:
> > On 2016/09/16 11:39:32, ossu wrote:
> > > This looks like a place where something might go wrong in the confluence
of
> my
> > > (upcoming) changes to DecoderDatabase, these changes, and solenberg's
change
> > > adding DTMF support at other clockrates. I'm not certain what, if
anything,
> > can
> > > be done about this right here and now, though.
> > 
> > It sounds like my best strategy is to land this quickly, then, so that I
won't
> > be the one sitting with a broken CL when the dust settles. :-)
> > 
> > Seriously though, adding support for multiple DTMF clockrates would probably
> > involve letting NetEq keep track of the DTMF clockrate, meaning we could
> > actually get rid of one of the two special cases here. ...see, a perfectly
> > rational-sounding reason for why I should land this CL as-is. :-)
> 
> Hmm. I think di->GetFormat().clockrate_hz instead of 8000 should keep you safe
> including upcoming changes.

No, because (1) that's the RTP timestamp rate, not the sample rate (since it's
taken directly from the SdpAudioFormat), and (2) GetFormat() only works if the
DecoderInfo was created from a NetEqDecoder that makes
RentACodec::NetEqDecoderToSdpAudioFormat return a non-empty value.

[ossu, 2016-09-16 12:10:06]

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/include/neteq.h:255: virtual
rtc::Optional<CodecInst> GetDecoder(int payload_type) const = 0;
On second thought, should this one maybe be called GetCodecInst or the like,
instead? We're not getting a NetEqDecoder nor an AudioDecoder from it.

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2339953002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:449: ci.plfreq = di->IsRed() ||
di->IsDtmf() ? 8000 : di->SampleRateHz();
On 2016/09/16 12:04:20, kwiberg-webrtc wrote:
> On 2016/09/16 11:55:59, ossu wrote:
> > On 2016/09/16 11:50:38, kwiberg-webrtc wrote:
> > > On 2016/09/16 11:39:32, ossu wrote:
> > > > This looks like a place where something might go wrong in the confluence
> of
> > my
> > > > (upcoming) changes to DecoderDatabase, these changes, and solenberg's
> change
> > > > adding DTMF support at other clockrates. I'm not certain what, if
> anything,
> > > can
> > > > be done about this right here and now, though.
> > > 
> > > It sounds like my best strategy is to land this quickly, then, so that I
> won't
> > > be the one sitting with a broken CL when the dust settles. :-)
> > > 
> > > Seriously though, adding support for multiple DTMF clockrates would
probably
> > > involve letting NetEq keep track of the DTMF clockrate, meaning we could
> > > actually get rid of one of the two special cases here. ...see, a perfectly
> > > rational-sounding reason for why I should land this CL as-is. :-)
> > 
> > Hmm. I think di->GetFormat().clockrate_hz instead of 8000 should keep you
safe
> > including upcoming changes.
> 
> No, because (1) that's the RTP timestamp rate, not the sample rate (since it's
> taken directly from the SdpAudioFormat), and (2) GetFormat() only works if the
> DecoderInfo was created from a NetEqDecoder that makes
> RentACodec::NetEqDecoderToSdpAudioFormat return a non-empty value.

Yes, but for DTMF the clockrate == sample rate, so that would be correct. For
RED, though, that would probably not work. :/ Should this method really return a
non-empty value for RED?

[ossu, 2016-09-16 12:22:23]

lgtm

[kwiberg-webrtc, 2016-09-16 12:23:11]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-16 12:23:15]

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-09-16 12:55:14]

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

[commit-bot: I haz the power, 2016-09-16 12:55:15]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-09-17 14:46:39]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-17 14:46:47]

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-09-17 15:40:16]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801
==========

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801
==========

[commit-bot: I haz the power, 2016-09-17 15:40:17]

Committed patchset #1 (id:100001)

[commit-bot: I haz the power, 2016-09-17 15:40:21]

Description was changed from

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801
==========

to

==========
AcmReceiver: Look up last decoder in NetEq's table of decoders

AcmReceiver::decoders_ is now one step closer to being unused.

BUG=webrtc:5801

Committed: https://crrev.com/1e4d8b574cde64d93b98d89c7b817fb93185a307
Cr-Commit-Position: refs/heads/master@{#14274}
==========

[commit-bot: I haz the power, 2016-09-17 15:40:22]

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

[kwiberg-webrtc, 2016-09-18 12:35:35]

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

The reason for reverting is: Seems to have broken Chromium tests..