NetEq: Use a BuiltinAudioDecoderFactory to create decoders

NetEq: Use a BuiltinAudioDecoderFactory to create decoders

Later steps in the refactoring will have the factory injected from the
outside rather than owned by NetEq.

BUG=webrtc:5801
Committed: https://crrev.com/5178ee86ba18ff50726f54445b41f51c02cb677e
Cr-Commit-Position: refs/heads/master@{#12604}

[kwiberg-webrtc, 2016-04-29 09:16:02]

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

[hlundin-webrtc, 2016-04-29 11:52:57]

Nice, but see my 2 comments.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h:1: /*
We used to have the structure that the mock for path/to/folder/my_class.h was
located in path/to/folder/mocks/mock_my_class.h, i.e., in a subfolder named
mocks.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.cc:53: return decoder_.get();
So now we can end up returning no decoder without issuing a DCHECK? If both
decoder_ and audio_format_ are null, we won't even try. Is this intended?

[ossu, 2016-04-29 12:21:33]

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/rent_a_codec.cc:92: return
rtc::Optional<SdpAudioFormat>(SdpAudioFormat("opus", 48000, 2));
Opus is weird and always wants to have a channel count of 2, with an optional
parameter setting stereo=1 or stereo=0 to actually decide the number of
channels.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h:24:
std::unique_ptr<AudioDecoder> MakeAudioDecoder(
Why is this? Just to remove the std::unique_ptr<> stuff from any mocks?

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:163: const
std::unique_ptr<AudioDecoderFactory> decoder_factory_;
Since the decoder_factory_ will (eventually) be created somewhere far from here,
and be pushed down into possibly many a DecoderFactory (and other places), it's
not going to fly to have a unique_ptr here. In my work of moving the decoder
factory further and further away, I've been using shared_ptr, though I'm
beginning to think we should maybe only keep it refcounted much earlier, and
keep a reference here. The lifetime of this DecoderDatabase ought to be
controlled by the WebRtcVoiceEngine, for example, so it might be enough to keep
ownership there.

I'm aware we're not supposed to use references as function parameters, but we
could always send them down as pointers. The key is we really need a non-null
AudioDecoderFactory here.

[kwiberg-webrtc, 2016-04-29 23:10:14]

New patch set uploaded.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/rent_a_codec.cc:92: return
rtc::Optional<SdpAudioFormat>(SdpAudioFormat("opus", 48000, 2));
On 2016/04/29 12:21:33, ossu wrote:
> Opus is weird and always wants to have a channel count of 2, with an optional
> parameter setting stereo=1 or stereo=0 to actually decide the number of
> channels.

I was hoping to be able to postpone the introduction of this sort of
complication, and just use the obvious mapping for now. Is that a bad idea, do
you think? (Hmm. I already have g722's whacky sample rate in here, so if this
Opus oddity is the only other one, perhaps I should just put it in there and be
done with it?)

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h:1: /*
On 2016/04/29 11:52:56, hlundin-webrtc wrote:
> We used to have the structure that the mock for path/to/folder/my_class.h was
> located in path/to/folder/mocks/mock_my_class.h, i.e., in a subfolder named
> mocks.

Laudable. Will change. But we have only one mock in a "mocks" subdirectory, and
~30 in "mock" subdirectories, so I'll go with the latter.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h:24:
std::unique_ptr<AudioDecoder> MakeAudioDecoder(
On 2016/04/29 12:21:33, ossu wrote:
> Why is this? Just to remove the std::unique_ptr<> stuff from any mocks?

Yes. gmock is pre-C++11 and doesn't properly forward the return values,
resulting in them being copied, not moved. I work around it by having the mock
return void and passing the return value in an argument.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.cc:53: return decoder_.get();
On 2016/04/29 11:52:56, hlundin-webrtc wrote:
> So now we can end up returning no decoder without issuing a DCHECK? If both
> decoder_ and audio_format_ are null, we won't even try. Is this intended?

I... ummm... oh, look over there! A pink elephant!

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:163: const
std::unique_ptr<AudioDecoderFactory> decoder_factory_;
On 2016/04/29 12:21:33, ossu wrote:
> Since the decoder_factory_ will (eventually) be created somewhere far from
here,
> and be pushed down into possibly many a DecoderFactory (and other places),
it's
> not going to fly to have a unique_ptr here.

I know. I'm just using unique_ptr for now so that I don't have to make the hard
choice about what else to use. I'd like shared_ptr, but I suspect we'll end up
having to use scoped_refptr which is sooo ugly.

> In my work of moving the decoder
> factory further and further away, I've been using shared_ptr, though I'm
> beginning to think we should maybe only keep it refcounted much earlier, and
> keep a reference here. The lifetime of this DecoderDatabase ought to be
> controlled by the WebRtcVoiceEngine, for example, so it might be enough to
keep
> ownership there.

I'm not fond of that idea. There's no machine making sure that we don't mess it
up. shared_ptr would be much safer.

Hey---Idea for a new type of smart pointer for this use case: master_ptr and
slave_ptr. master_ptr behaves like unique_ptr (i.e., moveable but not copyable),
but additionally you can fork off slave_ptrs that point to the same object.
slave_ptr behaves like shared_ptr (i.e., copyable). When the master_ptr releases
the object, it CHECKs that no slave_ptrs are still live. (This is easily
implemented by making both master_ptr and slave_ptr use a shared_ptr under the
hood; ~master_ptr will simply CHECK that it owns the last remaining reference.)

Maybe that'd be overkill though, and we should stick with shared_ptr.

For this CL right now, however, I'd like to keep using unique_ptr, to avoid
blocking on the decision about what type of smart pointer we should use.

[hlundin-webrtc, 2016-05-02 07:54:43]

lgtm

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.cc:53: return decoder_.get();
On 2016/04/29 23:10:13, kwiberg-webrtc wrote:
> On 2016/04/29 11:52:56, hlundin-webrtc wrote:
> > So now we can end up returning no decoder without issuing a DCHECK? If both
> > decoder_ and audio_format_ are null, we won't even try. Is this intended?
> 
> I... ummm... oh, look over there! A pink elephant!

Where? What!?

[ossu, 2016-05-02 10:59:40]

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/rent_a_codec.cc:92: return
rtc::Optional<SdpAudioFormat>(SdpAudioFormat("opus", 48000, 2));
On 2016/04/29 23:10:13, kwiberg-webrtc wrote:
> On 2016/04/29 12:21:33, ossu wrote:
> > Opus is weird and always wants to have a channel count of 2, with an
optional
> > parameter setting stereo=1 or stereo=0 to actually decide the number of
> > channels.
> 
> I was hoping to be able to postpone the introduction of this sort of
> complication, and just use the obvious mapping for now. Is that a bad idea, do
> you think? (Hmm. I already have g722's whacky sample rate in here, so if this
> Opus oddity is the only other one, perhaps I should just put it in there and
be
> done with it?)

Might as well do it "the right (wrong) way" from the start. :)

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock_audio_decoder_factory.h:24:
std::unique_ptr<AudioDecoder> MakeAudioDecoder(
On 2016/04/29 23:10:13, kwiberg-webrtc wrote:
> On 2016/04/29 12:21:33, ossu wrote:
> > Why is this? Just to remove the std::unique_ptr<> stuff from any mocks?
> 
> Yes. gmock is pre-C++11 and doesn't properly forward the return values,
> resulting in them being copied, not moved. I work around it by having the mock
> return void and passing the return value in an argument.

Ah, I see.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decoder_database.h (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decoder_database.h:163: const
std::unique_ptr<AudioDecoderFactory> decoder_factory_;
On 2016/04/29 23:10:13, kwiberg-webrtc wrote:
> On 2016/04/29 12:21:33, ossu wrote:
> > Since the decoder_factory_ will (eventually) be created somewhere far from
> here,
> > and be pushed down into possibly many a DecoderFactory (and other places),
> it's
> > not going to fly to have a unique_ptr here.
> 
> I know. I'm just using unique_ptr for now so that I don't have to make the
hard
> choice about what else to use. I'd like shared_ptr, but I suspect we'll end up
> having to use scoped_refptr which is sooo ugly.
> 
> > In my work of moving the decoder
> > factory further and further away, I've been using shared_ptr, though I'm
> > beginning to think we should maybe only keep it refcounted much earlier, and
> > keep a reference here. The lifetime of this DecoderDatabase ought to be
> > controlled by the WebRtcVoiceEngine, for example, so it might be enough to
> keep
> > ownership there.
> 
> I'm not fond of that idea. There's no machine making sure that we don't mess
it
> up. shared_ptr would be much safer.
> 
> Hey---Idea for a new type of smart pointer for this use case: master_ptr and
> slave_ptr. master_ptr behaves like unique_ptr (i.e., moveable but not
copyable),
> but additionally you can fork off slave_ptrs that point to the same object.
> slave_ptr behaves like shared_ptr (i.e., copyable). When the master_ptr
releases
> the object, it CHECKs that no slave_ptrs are still live. (This is easily
> implemented by making both master_ptr and slave_ptr use a shared_ptr under the
> hood; ~master_ptr will simply CHECK that it owns the last remaining
reference.)
> 
> Maybe that'd be overkill though, and we should stick with shared_ptr.
> 
> For this CL right now, however, I'd like to keep using unique_ptr, to avoid
> blocking on the decision about what type of smart pointer we should use.

Yeah, keep it like this for now. No reason to block the CL over it.
Unfortunately, shared_ptr/scoped_refptr won't solve ownership by itself. I've
made that mistake in a previous project. In the end, we had to figure out a
proper ownership structure and once we had that, keeping shared_ptrs was just
superfluous.

[kwiberg-webrtc, 2016-05-02 20:04:24]

Patchset #5 (id:80001) has been deleted

[kwiberg-webrtc, 2016-05-02 20:20:01]

New patch set posted.

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/rent_a_codec.cc (right):

https://codereview.webrtc.org/1928293002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/rent_a_codec.cc:92: return
rtc::Optional<SdpAudioFormat>(SdpAudioFormat("opus", 48000, 2));
On 2016/05/02 10:59:40, ossu wrote:
> On 2016/04/29 23:10:13, kwiberg-webrtc wrote:
> > On 2016/04/29 12:21:33, ossu wrote:
> > > Opus is weird and always wants to have a channel count of 2, with an
> optional
> > > parameter setting stereo=1 or stereo=0 to actually decide the number of
> > > channels.
> > 
> > I was hoping to be able to postpone the introduction of this sort of
> > complication, and just use the obvious mapping for now. Is that a bad idea,
do
> > you think? (Hmm. I already have g722's whacky sample rate in here, so if
this
> > Opus oddity is the only other one, perhaps I should just put it in there and
> be
> > done with it?)
> 
> Might as well do it "the right (wrong) way" from the start. :)

Done.

https://codereview.webrtc.org/1928293002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h
(right):

https://codereview.webrtc.org/1928293002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h:25: const
SdpAudioFormat& format) {
I had to remove the "override" here, because of the recently enabled
-Winconsistent-missing-override.

[ossu, 2016-05-03 08:28:34]

lgtm

[kwiberg-webrtc, 2016-05-03 08:31:25]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-05-03 08:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1928293002/#ps60001 (title:
"Rejigger Opus config")

[commit-bot: I haz the power, 2016-05-03 08:31:28]

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

[commit-bot: I haz the power, 2016-05-03 08:39:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-03 08:39:16]

Description was changed from

==========
NetEq: Use a BuiltinAudioDecoderFactory to create decoders

Later steps in the refactoring will have the factory injected from the
outside rather than owned by NetEq.

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

to

==========
NetEq: Use a BuiltinAudioDecoderFactory to create decoders

Later steps in the refactoring will have the factory injected from the
outside rather than owned by NetEq.

BUG=webrtc:5801

Committed: https://crrev.com/5178ee86ba18ff50726f54445b41f51c02cb677e
Cr-Commit-Position: refs/heads/master@{#12604}
==========

[commit-bot: I haz the power, 2016-05-03 08:39:17]

Patchset 4 (id:??) landed as
https://crrev.com/5178ee86ba18ff50726f54445b41f51c02cb677e
Cr-Commit-Position: refs/heads/master@{#12604}