Prepare for injectable SW decoders

Prepare for injectable SW decoders

Pretty much mirrors the work done on the encoding side in CLs:

"Clean up ownership of webrtc::VideoEncoder"
https://codereview.webrtc.org/3007643002/

"Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder"
https://codereview.webrtc.org/3007683002/

"WebRtcVideoEngine: Encapsulate logic for unifying internal and external video codecs"
https://codereview.webrtc.org/3006713002/

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

[andersc, 2017-09-01 09:26:37]

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

[commit-bot: I haz the power, 2017-09-01 09:26:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[andersc, 2017-09-01 09:33:01]

Description was changed from

==========
Prepare for injectable SW decoders

Pretty much mirrors the work done on the encoding side in CLs:

"Clean up ownership of webrtc::VideoEncoder"
https://codereview.webrtc.org/3007643002/

"Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder"
https://codereview.webrtc.org/3007683002/

"WebRtcVideoEngine: Encapsulate logic for unifying internal and external video
codecs"
https://codereview.webrtc.org/3006713002/

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

to

==========
Prepare for injectable SW decoders

Pretty much mirrors the work done on the encoding side in CLs:

"Clean up ownership of webrtc::VideoEncoder"
https://codereview.webrtc.org/3007643002/

"Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder"
https://codereview.webrtc.org/3007683002/

"WebRtcVideoEngine: Encapsulate logic for unifying internal and external video
codecs"
https://codereview.webrtc.org/3006713002/

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

[andersc, 2017-09-01 09:33:02]

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

[commit-bot: I haz the power, 2017-09-01 10:09:53]

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

[commit-bot: I haz the power, 2017-09-01 10:09:54]

Dry run: Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27354)

[magjed_webrtc, 2017-09-01 11:15:04]

This looks very good! I just have a few nit comments.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/videodeco...
File webrtc/media/engine/videodecodersoftwarefallbackwrapper.h (right):

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/videodeco...
webrtc/media/engine/videodecodersoftwarefallbackwrapper.h:26:
VideoDecoderSoftwareFallbackWrapper(const VideoCodecType codec_type,
nit: We send small objects like ints and enums by-value (without const), and
bigger objects as const refs (const &). So VideoCodecType should not have const
since it's an enum and we don't pass it by reference.

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

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:83: bool is_hardware_accelerated;
The variable 'is_hardware_accelerated' is not needed to store for decoders, so
you can remove the whole AllocatedDecoder struct and work directly with
webrtc::VideoDecoder.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2174: const StreamParams stream_params)
const {
This should be a reference, i.e. 'const StreamParams& stream_params'. We could
also change it to pass in a 'const VideoDecoderParams&' directly, i.e. the thing
we create with {stream_params.id}.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2189:
CreateScopedVideoDecoder(internal_decoder_factory_.get(), type,
I looked at the InternalDecoderFactory::DestroyVideoDecoder() implementation and
it's a pure 'delete decoder', so we don't have to wrap it with
CreateScopedVideoDecoder().

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2199: *old_decoders =
std::move(allocated_decoders_);
Hmm, this is a very interesting use case of std::move to optimize performance,
and it's valid :). std::move is mostly used for handling non-copyable objects
like std::unique_ptr.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2210: old_decoders->erase(type);
You have an iterator to the object now, so you should do:
new_decoder = std::move(it->second);
old_decoders->erase(it);

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2218: // TODO(andersc): are we using
the is_hardware_accelerated field for
Indeed this does not seem to be used.

[andersc, 2017-09-01 12:43:45]

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/videodeco...
File webrtc/media/engine/videodecodersoftwarefallbackwrapper.h (right):

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/videodeco...
webrtc/media/engine/videodecodersoftwarefallbackwrapper.h:26:
VideoDecoderSoftwareFallbackWrapper(const VideoCodecType codec_type,
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> nit: We send small objects like ints and enums by-value (without const), and
> bigger objects as const refs (const &). So VideoCodecType should not have
const
> since it's an enum and we don't pass it by reference.

Acknowledged.

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

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:83: bool is_hardware_accelerated;
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> The variable 'is_hardware_accelerated' is not needed to store for decoders, so
> you can remove the whole AllocatedDecoder struct and work directly with
> webrtc::VideoDecoder.

Removed in CL https://codereview.webrtc.org/3011823002

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2174: const StreamParams stream_params)
const {
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> This should be a reference, i.e. 'const StreamParams& stream_params'. We could
> also change it to pass in a 'const VideoDecoderParams&' directly, i.e. the
thing
> we create with {stream_params.id}.

Yes, that is nicer

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2189:
CreateScopedVideoDecoder(internal_decoder_factory_.get(), type,
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> I looked at the InternalDecoderFactory::DestroyVideoDecoder() implementation
and
> it's a pure 'delete decoder', so we don't have to wrap it with
> CreateScopedVideoDecoder().

Acknowledged.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2199: *old_decoders =
std::move(allocated_decoders_);
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> Hmm, this is a very interesting use case of std::move to optimize performance,
> and it's valid :). std::move is mostly used for handling non-copyable objects
> like std::unique_ptr.

Makes sense.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2210: old_decoders->erase(type);
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> You have an iterator to the object now, so you should do:
> new_decoder = std::move(it->second);
> old_decoders->erase(it);

Acknowledged.

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:2218: // TODO(andersc): are we using
the is_hardware_accelerated field for
On 2017/09/01 11:15:04, magjed_webrtc wrote:
> Indeed this does not seem to be used.

Acknowledged.

[magjed_webrtc, 2017-09-01 13:04:30]

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

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:83: bool is_hardware_accelerated;
On 2017/09/01 12:43:45, andersc wrote:
> On 2017/09/01 11:15:04, magjed_webrtc wrote:
> > The variable 'is_hardware_accelerated' is not needed to store for decoders,
so
> > you can remove the whole AllocatedDecoder struct and work directly with
> > webrtc::VideoDecoder.
> 
> Removed in CL https://codereview.webrtc.org/3011823002

Since you are already changing this logic I think it's best if you merge that
removal into this CL.

[andersc, 2017-09-01 13:16:09]

Merged that CL into this one

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

https://codereview.webrtc.org/3009973002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine.cc:83: bool is_hardware_accelerated;
On 2017/09/01 13:04:30, magjed_webrtc wrote:
> On 2017/09/01 12:43:45, andersc wrote:
> > On 2017/09/01 11:15:04, magjed_webrtc wrote:
> > > The variable 'is_hardware_accelerated' is not needed to store for
decoders,
> so
> > > you can remove the whole AllocatedDecoder struct and work directly with
> > > webrtc::VideoDecoder.
> > 
> > Removed in CL https://codereview.webrtc.org/3011823002
> 
> Since you are already changing this logic I think it's best if you merge that
> removal into this CL.

Done.

[magjed_webrtc, 2017-09-01 14:04:59]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-09-01 14:05:00]

lgtm

Looks great!

[commit-bot: I haz the power, 2017-09-01 14:05:05]

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-09-01 15:38:28]

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

[commit-bot: I haz the power, 2017-09-01 15:38:29]

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

[magjed_webrtc, 2017-09-01 16:42:51]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-09-01 16:42:59]

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-09-01 17:38:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1504284171888020,
"parent_rev": "85d18d43adc679d8d3160106a918fe05fdbc0755", "commit_rev":
"084c55a63a2d9bdc71579458406d44f8bab9f454"}

[commit-bot: I haz the power, 2017-09-01 17:38:21]

Description was changed from

==========
Prepare for injectable SW decoders

Pretty much mirrors the work done on the encoding side in CLs:

"Clean up ownership of webrtc::VideoEncoder"
https://codereview.webrtc.org/3007643002/

"Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder"
https://codereview.webrtc.org/3007683002/

"WebRtcVideoEngine: Encapsulate logic for unifying internal and external video
codecs"
https://codereview.webrtc.org/3006713002/

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

to

==========
Prepare for injectable SW decoders

Pretty much mirrors the work done on the encoding side in CLs:

"Clean up ownership of webrtc::VideoEncoder"
https://codereview.webrtc.org/3007643002/

"Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder"
https://codereview.webrtc.org/3007683002/

"WebRtcVideoEngine: Encapsulate logic for unifying internal and external video
codecs"
https://codereview.webrtc.org/3006713002/

BUG=webrtc:7925

Review-Url: https://codereview.webrtc.org/3009973002
Cr-Commit-Position: refs/heads/master@{#19641}
Committed:
https://chromium.googlesource.com/external/webrtc/+/084c55a63a2d9bdc715794584...
==========

[commit-bot: I haz the power, 2017-09-01 17:38:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/084c55a63a2d9bdc715794584...

[perkj_webrtc, 2017-09-04 07:45:08]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/3010953002/ by perkj@webrtc.org.

The reason for reverting is: Tentative revert since it seems to cause problems
in Chrome, MAC.

https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds...

.