NetEq: Simplify DecoderDatabase::DecoderInfo

NetEq: Simplify DecoderDatabase::DecoderInfo

By eliminating one of the two constructors, handling decoder ownership
with a unique_ptr instead of a raw pointer, and making all member
variables const (except one, which is made private instead).

BUG=webrtc:5801
Committed: https://crrev.com/0fa0a97cf3ff656a70e5b2955eeab5ef38982a04
Cr-Commit-Position: refs/heads/master@{#12425}

[kwiberg-webrtc, 2016-04-18 19:13:31]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-04-18 19:17:16]

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

[hlundin-webrtc, 2016-04-19 06:47:00]

Nice. Please, see comments.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc (right):

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc:59:
EXPECT_FALSE(info->external_decoder);
Lines 56 and 59 verify the same thing, in slightly different ways. Omit one.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right):

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:74: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
FYI: This is the typical place where setting an rtp_timestamp_rate other than
the sample rate makes sense, and will be necessary when NetEqDecoder is removed.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:129: 8000,
nullptr);
Hmmm, what does 8000 represent here? Sample rate or RTP timestamp rate? The
sample rate should be 16000 Hz for G.722, but the RTP timestamp rate should be
8000. I think one interpretation (of this inermediate state) is that the number
is the audio sample rate, while the NetEqDecoder type defines the RTP sample
rate. WDYT?

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:131: 8000,
nullptr);
This is where things get messy with the current design. CNGwb should (and I
believe will) produce comfort noise at a sample rate of 16000 Hz. But, I believe
that it is correct from a payload registration point of view to register it as
having an RTP sample rate of 8000. This does not make sense with what I wrote
above...

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:171: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
16000

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:200: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
16000

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:230: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
16000

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:270: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderOpus, "", 8000,
48000 for Opus.

[kwiberg-webrtc, 2016-04-19 08:57:31]

Comments addressed in patch set #2.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc (right):

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc:59:
EXPECT_FALSE(info->external_decoder);
On 2016/04/19 06:46:59, hlundin-webrtc wrote:
> Lines 56 and 59 verify the same thing, in slightly different ways. Omit one.

Done.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right):

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:74: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> FYI: This is the typical place where setting an rtp_timestamp_rate other than
> the sample rate makes sense, and will be necessary when NetEqDecoder is
removed.

Yes. But when we start using a decoder factory that's injected into NetEq, this
information can no longer come from anyplace other than that factory;
specifically, we can't just supply it as a parameter to DecoderInfo. The factory
will have to be able to answer questions about the specifications it uses to
create decoders, I guess.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:129: 8000,
nullptr);
On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> Hmmm, what does 8000 represent here? Sample rate or RTP timestamp rate? The
> sample rate should be 16000 Hz for G.722, but the RTP timestamp rate should be
> 8000. I think one interpretation (of this inermediate state) is that the
number
> is the audio sample rate, while the NetEqDecoder type defines the RTP sample
> rate. WDYT?

I guess... but I don't know if the current state of the code is coherent enough
to draw such conclusions. For one, this CL demonstrates that rtp_sample_rate_hz
was never set to anything other than fs_hz, and besides, no one was looking at
it, so effectively it didn't exist.

Regarding this particular piece of code: I put 8000 in here because that's what
the default value was before I removed it. I'm not claiming it makes sense, just
that the new code uses the same value as the old code...

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:131: 8000,
nullptr);
On 2016/04/19 06:46:59, hlundin-webrtc wrote:
> This is where things get messy with the current design. CNGwb should (and I
> believe will) produce comfort noise at a sample rate of 16000 Hz. But, I
believe
> that it is correct from a payload registration point of view to register it as
> having an RTP sample rate of 8000. This does not make sense with what I wrote
> above...

The design for audio decoder (and encoder) factories that ossu@ and I have come
up with calls for the specification to the factory to use basically untranslated
SDP parameters. The downside is that those don't always map straightforwardly to
the parameters the codec will actually use, but at least it'll be consistent and
well-defined, and we can put all the translation logic in the factory, which
actually has some business knowing about those gory details for its codecs.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:171: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
On 2016/04/19 06:46:59, hlundin-webrtc wrote:
> 16000

Done. (I updated all these Herzes to match the actual sample rate of the codec
in question. Since the values were wrong before, I'm pretty sure it doesn't
actually matter, though.)

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:200: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> 16000

Done.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:230: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
On 2016/04/19 06:46:59, hlundin-webrtc wrote:
> 16000

Done.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:270: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderOpus, "", 8000,
On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> 48000 for Opus.

Done.

[hlundin-webrtc, 2016-04-19 09:31:30]

lgtm

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc (right):

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:74: const
DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderG722, "", 8000,
On 2016/04/19 08:57:30, kwiberg-webrtc wrote:
> On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> > FYI: This is the typical place where setting an rtp_timestamp_rate other
than
> > the sample rate makes sense, and will be necessary when NetEqDecoder is
> removed.
> 
> Yes. But when we start using a decoder factory that's injected into NetEq,
this
> information can no longer come from anyplace other than that factory;
> specifically, we can't just supply it as a parameter to DecoderInfo. The
factory
> will have to be able to answer questions about the specifications it uses to
> create decoders, I guess.

Acknowledged.

https://codereview.webrtc.org/1899733002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc:129: 8000,
nullptr);
On 2016/04/19 08:57:31, kwiberg-webrtc wrote:
> On 2016/04/19 06:47:00, hlundin-webrtc wrote:
> > Hmmm, what does 8000 represent here? Sample rate or RTP timestamp rate? The
> > sample rate should be 16000 Hz for G.722, but the RTP timestamp rate should
be
> > 8000. I think one interpretation (of this inermediate state) is that the
> number
> > is the audio sample rate, while the NetEqDecoder type defines the RTP sample
> > rate. WDYT?
> 
> I guess... but I don't know if the current state of the code is coherent
enough
> to draw such conclusions. For one, this CL demonstrates that
rtp_sample_rate_hz
> was never set to anything other than fs_hz, and besides, no one was looking at
> it, so effectively it didn't exist.
> 
> Regarding this particular piece of code: I put 8000 in here because that's
what
> the default value was before I removed it. I'm not claiming it makes sense,
just
> that the new code uses the same value as the old code...

Acknowledged.

[kwiberg-webrtc, 2016-04-19 10:35:56]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-04-19 10:36:11]

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

[commit-bot: I haz the power, 2016-04-19 10:57:21]

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

[commit-bot: I haz the power, 2016-04-19 10:57:22]

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

[kwiberg-webrtc, 2016-04-19 11:04:00]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-04-19 11:04:13]

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

[commit-bot: I haz the power, 2016-04-19 12:03:49]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-19 12:03:56]

Description was changed from

==========
NetEq: Simplify DecoderDatabase::DecoderInfo

By eliminating one of the two constructors, handling decoder ownership
with a unique_ptr instead of a raw pointer, and making all member
variables const (except one, which is made private instead).

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

to

==========
NetEq: Simplify DecoderDatabase::DecoderInfo

By eliminating one of the two constructors, handling decoder ownership
with a unique_ptr instead of a raw pointer, and making all member
variables const (except one, which is made private instead).

BUG=webrtc:5801

Committed: https://crrev.com/0fa0a97cf3ff656a70e5b2955eeab5ef38982a04
Cr-Commit-Position: refs/heads/master@{#12425}
==========

[commit-bot: I haz the power, 2016-04-19 12:03:57]

Patchset 2 (id:??) landed as
https://crrev.com/0fa0a97cf3ff656a70e5b2955eeab5ef38982a04
Cr-Commit-Position: refs/heads/master@{#12425}