Fix memory leak in vcm_capturer.cc.

Fix memory leak in vcm_capturer.cc.
vcm_->Release won't delete vcm_ automatically since the refcount of vcm_ will be
-1 after Release call.
vcm_->AddRef() should be called after vcm_ creation.

BUG=webrtc:5229

[wangrui, 2015-11-23 02:54:02]

Description was changed from

==========
Fix memory leak in vcm_capturer.cc.
vcm_->Release won't delete vcm_ automatically since the refcount of vcm_ will be
-1 after Release call.
vcm_->AddRef() should be called after vcm_ creation.

BUG=5229
==========

to

==========
Fix memory leak in vcm_capturer.cc.
vcm_->Release won't delete vcm_ automatically since the refcount of vcm_ will be
-1 after Release call.
vcm_->AddRef() should be called after vcm_ creation.

BUG=5229

https://bugs.chromium.org/p/webrtc/issues/detail?id=5229
==========

[wangrui, 2015-11-23 02:54:02]

wangruihit@gmail.com changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-11-26 17:10:00]

Description was changed from

==========
Fix memory leak in vcm_capturer.cc.
vcm_->Release won't delete vcm_ automatically since the refcount of vcm_ will be
-1 after Release call.
vcm_->AddRef() should be called after vcm_ creation.

BUG=5229

https://bugs.chromium.org/p/webrtc/issues/detail?id=5229
==========

to

==========
Fix memory leak in vcm_capturer.cc.
vcm_->Release won't delete vcm_ automatically since the refcount of vcm_ will be
-1 after Release call.
vcm_->AddRef() should be called after vcm_ creation.

BUG=webrtc:5229
==========

[pbos-webrtc, 2015-11-26 17:12:13]

Sorry for the slow reply, a question:

https://codereview.webrtc.org/1465113002/diff/1/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/1465113002/diff/1/webrtc/test/vcm_capturer.cc#n...
webrtc/test/vcm_capturer.cc:37: vcm_->AddRef();
Shouldn't this be done inside ::Create() maybe? Creating it should imply that
you hold a reference to it.

[wangrui, 2015-11-27 04:14:49]

On 2015/11/26 17:12:13, pbos-webrtc wrote:
> Sorry for the slow reply, a question:
> 
> https://codereview.webrtc.org/1465113002/diff/1/webrtc/test/vcm_capturer.cc
> File webrtc/test/vcm_capturer.cc (right):
> 
>
https://codereview.webrtc.org/1465113002/diff/1/webrtc/test/vcm_capturer.cc#n...
> webrtc/test/vcm_capturer.cc:37: vcm_->AddRef();
> Shouldn't this be done inside ::Create() maybe? Creating it should imply that
> you hold a reference to it.

Thanks, pbos. Ref_count should be 1 after create/new/alloc methods, call
AddRef() after new/create/alloc is confusing.
plz checkout patch set 2.

[wangrui, 2015-11-27 05:40:55]

wangruihit@gmail.com changed reviewers:
+ niklas.enbom@webrtc.org

[pbos-webrtc, 2015-11-27 12:43:51]

Sorry, that's wrong again (don't modify RefCountImpl).

We shouldn't be calling AddRef or Release directly, but put the
VideoCaptureModule in a rtc::scoped_refptr (that's the thing supposed to do
AddRef and Release).

It's in here: webrtc/base/scoped_ref_ptr.h

So I think you want to replace:

44  VideoCaptureModule* vcm_;

with:

44  rtc::scoped_refptr<VideoCaptureModule> vcm_;

Which should take care of the AddRef/Releasing for you.

[pbos-webrtc, 2015-11-27 18:01:08]

On 2015/11/27 12:43:51, pbos-webrtc wrote:
> Sorry, that's wrong again (don't modify RefCountImpl).
> 
> We shouldn't be calling AddRef or Release directly, but put the
> VideoCaptureModule in a rtc::scoped_refptr (that's the thing supposed to do
> AddRef and Release).
> 
> It's in here: webrtc/base/scoped_ref_ptr.h
> 
> So I think you want to replace:
> 
> 44  VideoCaptureModule* vcm_;
> 
> with:
> 
> 44  rtc::scoped_refptr<VideoCaptureModule> vcm_;
> 
> Which should take care of the AddRef/Releasing for you.

I found out that this has wider implications (we should be returning
scoped_refptrs everywhere).

I think this is easier for me to fix, so I put up a change here:
https://codereview.webrtc.org/1477013005/

I hope this is OK with you.

[wangrui, 2015-12-02 09:33:51]

On 2015/11/27 18:01:08, pbos-webrtc wrote:
> On 2015/11/27 12:43:51, pbos-webrtc wrote:
> > Sorry, that's wrong again (don't modify RefCountImpl).
> > 
> > We shouldn't be calling AddRef or Release directly, but put the
> > VideoCaptureModule in a rtc::scoped_refptr (that's the thing supposed to do
> > AddRef and Release).
> > 
> > It's in here: webrtc/base/scoped_ref_ptr.h
> > 
> > So I think you want to replace:
> > 
> > 44  VideoCaptureModule* vcm_;
> > 
> > with:
> > 
> > 44  rtc::scoped_refptr<VideoCaptureModule> vcm_;
> > 
> > Which should take care of the AddRef/Releasing for you.
> 
> I found out that this has wider implications (we should be returning
> scoped_refptrs everywhere).
> 
> I think this is easier for me to fix, so I put up a change here:
> https://codereview.webrtc.org/1477013005/
> 
> I hope this is OK with you.

Yes, I am absolutely OK with this.
sorry for my improper patches.
Hope i can contribute next time.

[pbos-webrtc, 2015-12-02 23:44:07]

On 2015/12/02 09:33:51, wangrui wrote:
> On 2015/11/27 18:01:08, pbos-webrtc wrote:
> > On 2015/11/27 12:43:51, pbos-webrtc wrote:
> > > Sorry, that's wrong again (don't modify RefCountImpl).
> > > 
> > > We shouldn't be calling AddRef or Release directly, but put the
> > > VideoCaptureModule in a rtc::scoped_refptr (that's the thing supposed to
do
> > > AddRef and Release).
> > > 
> > > It's in here: webrtc/base/scoped_ref_ptr.h
> > > 
> > > So I think you want to replace:
> > > 
> > > 44  VideoCaptureModule* vcm_;
> > > 
> > > with:
> > > 
> > > 44  rtc::scoped_refptr<VideoCaptureModule> vcm_;
> > > 
> > > Which should take care of the AddRef/Releasing for you.
> > 
> > I found out that this has wider implications (we should be returning
> > scoped_refptrs everywhere).
> > 
> > I think this is easier for me to fix, so I put up a change here:
> > https://codereview.webrtc.org/1477013005/
> > 
> > I hope this is OK with you.
> 
> Yes, I am absolutely OK with this.
> sorry for my improper patches.
> Hope i can contribute next time.

Thanks for the patch though, I'd be glad to review a future one! This one grew
larger than I thought it would. Closing this issue now, but feel free to send me
an email or so if you think you have something else and want to discuss whether
it's easy/hard.

Best,
- Peter

[wangrui, 2015-12-10 07:23:58]

Patchset #3 (id:40001) has been deleted

[wangrui, 2015-12-10 07:58:10]

Patchset #3 (id:60001) has been deleted