Fix memory leak in VideoToolbox encoder.

Fix memory leak in VideoToolbox encoder.

We were leaking a fragmentation header object on each frame.

BUG=webrtc:8132
Review-Url: https://codereview.webrtc.org/3004013002
Cr-Commit-Position: refs/heads/master@{#19602}
Committed: https://chromium.googlesource.com/external/webrtc/+/f8084d485b87e49a68ff8e46e07bc01d375586be

[kthelgason, 2017-08-29 09:44:10]

kthelgason@webrtc.org changed reviewers:
+ henrika@webrtc.org

[kthelgason, 2017-08-29 09:44:11]

This fix just affects H264, and there may be other leaks, but at least this is a
start.

[kthelgason, 2017-08-29 09:47:42]

kthelgason@webrtc.org changed reviewers:
+ magjed@webrtc.org

[henrika_webrtc, 2017-08-29 10:37:07]

Nice. lgtm

[henrika_webrtc, 2017-08-29 10:37:12]

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc (right):

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc:155:
out_header->reset(header);
Should it be documented that we deleted and release here?

[magjed_webrtc, 2017-08-29 11:35:02]

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc (right):

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc:146: auto header
= new webrtc::RTPFragmentationHeader();
Can't we put it in |out_header| immediately, i.e. out_header->reset(new
webrtc::RTPFragmentationHeader())?

[kthelgason, 2017-08-29 11:39:46]

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc (right):

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc:146: auto header
= new webrtc::RTPFragmentationHeader();
On 2017/08/29 11:35:02, magjed_webrtc wrote:
> Can't we put it in |out_header| immediately, i.e. out_header->reset(new
> webrtc::RTPFragmentationHeader())?

Sure, I just did this to keep the diff small and to avoid having to write
(*out_header)->thing = other_thing; 
This looks nicer :)

[magjed_webrtc, 2017-08-30 09:06:10]

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc (right):

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc:146: auto header
= new webrtc::RTPFragmentationHeader();
On 2017/08/29 11:39:45, kthelgason wrote:
> On 2017/08/29 11:35:02, magjed_webrtc wrote:
> > Can't we put it in |out_header| immediately, i.e. out_header->reset(new
> > webrtc::RTPFragmentationHeader())?
> 
> Sure, I just did this to keep the diff small and to avoid having to write
> (*out_header)->thing = other_thing; 
> This looks nicer :)

I want a variable always owning the memory in an std::unique_ptr. I agree typing
is ugly (*out_header), so you can also do:
std::unique_ptr<webrtc::RTPFragmentationHeader> header(
    new webrtc::RTPFragmentationHeader());
  ...
  header->...
  ...
*out_header = std::move(header);

But I don't want the memory to be held in a raw pointer, even if it's just
locally. It will be more robust against future changes if we always keep it in a
std::unique_ptr.

[kthelgason, 2017-08-30 09:13:41]

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc (right):

https://codereview.webrtc.org/3004013002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/VideoToolbox/nalu_rewriter.cc:146: auto header
= new webrtc::RTPFragmentationHeader();
On 2017/08/30 09:06:10, magjed_webrtc wrote:
> On 2017/08/29 11:39:45, kthelgason wrote:
> > On 2017/08/29 11:35:02, magjed_webrtc wrote:
> > > Can't we put it in |out_header| immediately, i.e. out_header->reset(new
> > > webrtc::RTPFragmentationHeader())?
> > 
> > Sure, I just did this to keep the diff small and to avoid having to write
> > (*out_header)->thing = other_thing; 
> > This looks nicer :)
> 
> I want a variable always owning the memory in an std::unique_ptr. I agree
typing
> is ugly (*out_header), so you can also do:
> std::unique_ptr<webrtc::RTPFragmentationHeader> header(
>     new webrtc::RTPFragmentationHeader());
>   ...
>   header->...
>   ...
> *out_header = std::move(header);
> 
> But I don't want the memory to be held in a raw pointer, even if it's just
> locally. It will be more robust against future changes if we always keep it in
a
> std::unique_ptr.

You're right, I've fixed this now. Thanks!

[henrika_webrtc, 2017-08-30 09:19:49]

lgtm++

[magjed_webrtc, 2017-08-30 10:58:44]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-08-30 10:58:45]

lgtm

[commit-bot: I haz the power, 2017-08-30 10:58:49]

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, 2017-08-30 11:47:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1504090724634490,
"parent_rev": "16adf03d2555f186ae7c986a405c657c35aaca6d", "commit_rev":
"f8084d485b87e49a68ff8e46e07bc01d375586be"}

[commit-bot: I haz the power, 2017-08-30 11:47:20]

Description was changed from

==========
Fix memory leak in VideoToolbox encoder.

We were leaking a fragmentation header object on each frame.

BUG=webrtc:8132
==========

to

==========
Fix memory leak in VideoToolbox encoder.

We were leaking a fragmentation header object on each frame.

BUG=webrtc:8132

Review-Url: https://codereview.webrtc.org/3004013002
Cr-Commit-Position: refs/heads/master@{#19602}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f8084d485b87e49a68ff8e46e...
==========

[commit-bot: I haz the power, 2017-08-30 11:47:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/f8084d485b87e49a68ff8e46e...