Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Currently, ownership of the wrapped hardware encoder is handled outside
VideoEncoderSoftwareFallbackWrapper. It's easier if
VideoEncoderSoftwareFallbackWrapper owns and relases it instead.

BUG=webrtc:7925
Review-Url: https://codereview.webrtc.org/3007683002
Cr-Commit-Position: refs/heads/master@{#19572}
Committed: https://chromium.googlesource.com/external/webrtc/+/f52d34d682a049028ef5f2d126721dce03a50d8b

[magjed_webrtc, 2017-08-28 15:45:23]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-28 15:45:29]

Dry run: 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-28 16:13:40]

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

[commit-bot: I haz the power, 2017-08-28 16:13:41]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-08-28 16:22:49]

Description was changed from

==========
Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Currently, ownership of the wrapped hardware encoder is handled outside
VideoEncoderSoftwareFallbackWrapper. It's easier if
VideoEncoderSoftwareFallbackWrapper owns and relases it instead.

BUG=webrtc:7925
==========

to

==========
Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Currently, ownership of the wrapped hardware encoder is handled outside
VideoEncoderSoftwareFallbackWrapper. It's easier if
VideoEncoderSoftwareFallbackWrapper owns and relases it instead.

BUG=webrtc:7925
==========

[magjed_webrtc, 2017-08-28 16:22:50]

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

[magjed_webrtc, 2017-08-28 16:23:01]

Anders - please take a look.

[andersc, 2017-08-29 07:27:34]

Added a n00b C++ question, otherwise lgtm

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:1604: codec,
std::move(external_encoder)));
Is it normal to call std::move both here and in the initializer list in
videoencodersoftwarefallbackwrapper.cc? If there is no harm I think it makes it
more readable, but I don't understand exactly what std::move does.

[magjed_webrtc, 2017-08-29 07:56:48]

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:1604: codec,
std::move(external_encoder)));
On 2017/08/29 07:27:33, andersc wrote:
> Is it normal to call std::move both here and in the initializer list in
> videoencodersoftwarefallbackwrapper.cc? If there is no harm I think it makes
it
> more readable, but I don't understand exactly what std::move does.

Yes, it won't compile otherwise. Every time you create std::unique_ptr a from
another std::unique_ptr you need to call std::move, (it makes it a
std::unique_ptr&&). I move it twice actually, once here when moving it into the
argument of the VideoEncoderSoftwareFallbackWrapper ctor, and once when the ctor
moves it into its member variable.

[magjed_webrtc, 2017-08-29 07:57:00]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-08-29 07:57:10]

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-29 07:58:52]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1503993420173060, "parent_rev":
"d4495312dc7f6b5d5fc1c163026c52d56e6230f9", "commit_rev":
"f52d34d682a049028ef5f2d126721dce03a50d8b"}

[commit-bot: I haz the power, 2017-08-29 07:58:57]

Description was changed from

==========
Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Currently, ownership of the wrapped hardware encoder is handled outside
VideoEncoderSoftwareFallbackWrapper. It's easier if
VideoEncoderSoftwareFallbackWrapper owns and relases it instead.

BUG=webrtc:7925
==========

to

==========
Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder

Currently, ownership of the wrapped hardware encoder is handled outside
VideoEncoderSoftwareFallbackWrapper. It's easier if
VideoEncoderSoftwareFallbackWrapper owns and relases it instead.

BUG=webrtc:7925

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

[commit-bot: I haz the power, 2017-08-29 07:58:58]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/f52d34d682a049028ef5f2d12...

[brandtr, 2017-08-29 11:54:26]

brandtr@webrtc.org changed reviewers:
+ brandtr@webrtc.org

[brandtr, 2017-08-29 11:54:27]

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/videoenco...
File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right):

https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/videoenco...
webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:100:
std::unique_ptr<webrtc::VideoEncoder> encoder_;
drive-by question: Does this mean that it's OK to delete all our encoders
directly, rather than through WebRtcVideoEncoderFactory::DestroyVideoEncoder?

[brandtr, 2017-08-29 12:07:01]

On 2017/08/29 11:54:27, brandtr wrote:
>
https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/videoenco...
> File webrtc/media/engine/videoencodersoftwarefallbackwrapper.h (right):
> 
>
https://codereview.webrtc.org/3007683002/diff/1/webrtc/media/engine/videoenco...
> webrtc/media/engine/videoencodersoftwarefallbackwrapper.h:100:
> std::unique_ptr<webrtc::VideoEncoder> encoder_;
> drive-by question: Does this mean that it's OK to delete all our encoders
> directly, rather than through WebRtcVideoEncoderFactory::DestroyVideoEncoder?

nvm, just found https://codereview.webrtc.org/3007643002.