Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be shared, and shared_ptr is currently off the table, so this CL changes the current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

BUG=webrtc:5805
Committed: https://crrev.com/e725f7c73e68fe3dfcf6b953863ff651e0687d3e
Cr-Commit-Position: refs/heads/master@{#12815}

[ossu, 2016-05-18 11:59:04]

Description was changed from

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

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

to

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

[ossu, 2016-05-18 11:59:04]

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

[ossu, 2016-05-18 11:59:56]

I decided to turn my set of changes on its head and introduce the last one
first, i.e. changing to scoped_refptr. None of the other changes would be
applicable without it, since shared_ptr is not currently allowed.

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

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:35: int AddRef()
const override {
Put these in .cc-file?
Pro: cleaner
Con: slower (and most code already uses a template for this)

[kwiberg-webrtc, 2016-05-18 14:03:15]

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

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:35: int AddRef()
const override {
On 2016/05/18 11:59:56, ossu wrote:
> Put these in .cc-file?
> Pro: cleaner
> Con: slower (and most code already uses a template for this)

Slower how? It's a virtual call either way.

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:48: mutable volatile
int ref_count_;
Danger, Will Robinson! State in an interface!

This will make it ugly to have an encoder factory and a decoder factory in the
same object.

How about pure virtual AddRef and Release methods, and forcing subclasses to
implement them? Have you considered that? ;-)

[ossu, 2016-05-18 14:42:21]

Some moaning on the general state of things...
I guess I'll look at hamburger inheritance next.

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

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:35: int AddRef()
const override {
On 2016/05/18 14:03:15, kwiberg-webrtc wrote:
> On 2016/05/18 11:59:56, ossu wrote:
> > Put these in .cc-file?
> > Pro: cleaner
> > Con: slower (and most code already uses a template for this)
> 
> Slower how? It's a virtual call either way.

Yeah, I guess as long as we inherit from rtc::RefCountInterface, the compiler
can't do anything more clever than a virtual call. We don't have to
(rtc::scoped_refptr doesn't enforce it), but I guess it's for documentation, or
something. I'd gladly remove the inheritance and bury AddRef/Release
implementations in the .cc file.

I guess if I made AddRef/Release final, the compiler could just ignore the
virtual call when used with scoped_refptr; the static type would be
AudioDecoderFactory and nothing could change the implementation of
AddRef/Release. At that point, inheriting from rtc::RefCountInterface is 100%
cosmetic. Now it's 99% cosmetic.

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:48: mutable volatile
int ref_count_;
On 2016/05/18 14:03:15, kwiberg-webrtc wrote:
> Danger, Will Robinson! State in an interface!
> 
> This will make it ugly to have an encoder factory and a decoder factory in the
> same object.
> 
> How about pure virtual AddRef and Release methods, and forcing subclasses to
> implement them? Have you considered that? ;-)

Oh, right... We'd get two reference counts and everything would blow up, unless
the implementer chose to work around that.

I don't like forcing subclasses to implement this either, since it relies on
everyone getting it right every time (not too difficult) or creating their
factory through RefCountedObject<T>. Neither is very pretty for something that
is really tangential to the matter at hand (implementing a factory for creating
decoders).

It might be the only way to go, though.

[ossu, 2016-05-18 15:16:52]

Removed the implementation in AudioDecoderFactory and started relying on
RefCountedObject<T> instead. It's not too horrible! :)

[kwiberg-webrtc, 2016-05-18 15:48:27]

lgtm

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

https://codereview.webrtc.org/1990803004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder_factory.h:35: int AddRef()
const override {
On 2016/05/18 14:42:20, ossu wrote:
> On 2016/05/18 14:03:15, kwiberg-webrtc wrote:
> > On 2016/05/18 11:59:56, ossu wrote:
> > > Put these in .cc-file?
> > > Pro: cleaner
> > > Con: slower (and most code already uses a template for this)
> > 
> > Slower how? It's a virtual call either way.
> 
> Yeah, I guess as long as we inherit from rtc::RefCountInterface, the compiler
> can't do anything more clever than a virtual call. We don't have to
> (rtc::scoped_refptr doesn't enforce it), but I guess it's for documentation,
or
> something. I'd gladly remove the inheritance and bury AddRef/Release
> implementations in the .cc file.
> 
> I guess if I made AddRef/Release final, the compiler could just ignore the
> virtual call when used with scoped_refptr; the static type would be
> AudioDecoderFactory and nothing could change the implementation of
> AddRef/Release. At that point, inheriting from rtc::RefCountInterface is 100%
> cosmetic. Now it's 99% cosmetic.

Making them final would essentially make them not virtual, so you might as well
do that instead. But that would prevent subclasses from overriding them, so that
e.g. a combined encoder+decoder factory would be impossible.

The thing you ended up doing is the only sane thing, I'd say. Especially since
the compiler will complain if you don't wrap constructor calls in
RefCountedObject.

[ossu, 2016-05-18 16:54:44]

As a side-note, should this set of CLs reference webrtc:5805 instead of
webrtc:5801?
The ACM stuff is a blocker for 5805, sure, but this is not directly tied to
that, I'd say.

On 2016/05/18 15:48:27, kwiberg-webrtc wrote:
> lgtm

yay!
 
> Making them final would essentially make them not virtual, so you might as
well
> do that instead.

Yeah, except that inheriting from RefCountInterface would maybe act as
documentation on how the factory is supposed to be used.
Moot point now, though!

[kwiberg-webrtc, 2016-05-19 11:36:57]

On 2016/05/18 16:54:44, ossu wrote:
> As a side-note, should this set of CLs reference webrtc:5805 instead
> of webrtc:5801? The ACM stuff is a blocker for 5805, sure, but this
> is not directly tied to that, I'd say.

Yes, referencing bug 5805 sounds like a good fit.

> On 2016/05/18 15:48:27, kwiberg-webrtc wrote:
>
> > Making them final would essentially make them not virtual, so you
> > might as well do that instead.
>
> Yeah, except that inheriting from RefCountInterface would maybe act
> as documentation on how the factory is supposed to be used.

I think one common way to do it is to have the destructor be
protected---that way, anyone who tries to use the object (and doesn't
simply leak it...) will get a compilation error unless they use
Release() to destroy the object.

> Moot point now, though!

Yes. The present implementation is the one we want, until or unless we
can switch to using shared_ptr.

[ossu, 2016-05-19 11:37:51]

Description was changed from

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

to

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

[ossu, 2016-05-19 12:08:37]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-19 12:08:43]

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

[commit-bot: I haz the power, 2016-05-19 12:13:14]

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

[commit-bot: I haz the power, 2016-05-19 12:13:15]

Try jobs failed on following builders:
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3275)

[ossu, 2016-05-19 13:42:13]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-05-19 13:42:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1990803004/#ps40001 (title:
"Removed AudioDecoderFactory destructor declaration.")

[commit-bot: I haz the power, 2016-05-19 13:42:25]

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

[commit-bot: I haz the power, 2016-05-19 15:42:54]

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

[commit-bot: I haz the power, 2016-05-19 15:42:55]

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

[ossu, 2016-05-19 16:09:17]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-19 16:09:26]

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

[commit-bot: I haz the power, 2016-05-19 17:48:08]

Description was changed from

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

to

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

[commit-bot: I haz the power, 2016-05-19 17:48:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-19 17:48:17]

Description was changed from

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

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

to

==========
Turned AudioDecoderFactory into a RefCounted thing to use with scoped_refptr.

First step of AudioDecoderFactory injection CLs. AudioDecoderFactories will be
shared, and shared_ptr is currently off the table, so this CL changes the
current uses of AudioDecoderFactory from std::unique_ptr to rtc::scoped_refptr.

BUG=webrtc:5805

Committed: https://crrev.com/e725f7c73e68fe3dfcf6b953863ff651e0687d3e
Cr-Commit-Position: refs/heads/master@{#12815}
==========

[commit-bot: I haz the power, 2016-05-19 17:48:18]

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