Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191
Review-Url: https://codereview.chromium.org/2610493002
Cr-Commit-Position: refs/heads/master@{#449277}
Committed: https://chromium.googlesource.com/chromium/src/+/c6abc2378d27fb64de389472c5af3ac74305ea51

[fdoray, 2017-01-01 19:37:10]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-01 19:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-01 19:45:46]

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

[commit-bot: I haz the power, 2017-01-01 19:45:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-01-01 20:09:46]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-01 20:09:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-01 20:36:45]

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

[commit-bot: I haz the power, 2017-01-01 20:36:45]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-01-02 22:59:35]

fdoray@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[fdoray, 2017-01-02 22:59:36]

PTAL. WorkerPool is being deprecated in favor of TaskScheduler.

[Wez, 2017-01-04 00:37:31]

FWIW jamiewalch@ is OOO until the 7th, I believe.

Re the CL description, the wording is very jargon-heavy, and sounds like it may
duplicate comment wording on the relevant enum declarations, e.g. the
CONTINUE_ON_SHUTDOWN description is generic wording for
WorkerPool->TaskScheduler migration.

FWIW, CONTINUE_ON_SHUTDOWN does not seem an intuitive name for the corresponding
behaviour - I'd interpret it as "continue processing tasks [until no more
remain] on shutdown", which is the opposite of what it does. DETACH_ON_SHUTDOWN,
ABANDON_ON_SHUTDOWN or similar, or ALLOW_IMMEDIATE_SHUTDOWN, ALLOW_SHUTDOWN,
BLOCK_SHUTDOWN would seem a better fit.

Finally, is TaskTraits suitable to have a constexpr constructor, so we can
create a single const instance rather than re-creating it on the stack on every
PostTask?

[Wez, 2017-01-04 00:37:51]

wez@chromium.org changed reviewers:
+ sergeyu@chromium.org, wez@chromium.org

[Wez, 2017-01-04 00:37:51]

+sergeyu, since jamiewalch@ is OOO.

[Sergey Ulanov, 2017-01-07 07:54:37]

Sorry about the delay. 

Do I understand it correctly that
TaskScheduler::CreateAndSetDefaultTaskScheduler() has to be called in order to
use base::PostTaskWithTraitsAndReplyWithResult(), otherwise the calling process
will crash?
In that case this change will break chromoting Android and iOS client. They need
to be update to initialize TaskScheduler. Or it's probably better to update
WebrtcVideoRendererAdapter to use a separate background thread, without
dependency on TaskScheduler. Currently it's the only place in the Android/iOS
clients that use WorkerPool (I can make CL for this).

[fdoray, 2017-01-11 17:42:32]

On 2017/01/07 07:54:37, Sergey Ulanov wrote:
> Sorry about the delay. 
> 
> Do I understand it correctly that
> TaskScheduler::CreateAndSetDefaultTaskScheduler() has to be called in order to
> use base::PostTaskWithTraitsAndReplyWithResult(), otherwise the calling
process
> will crash?
> In that case this change will break chromoting Android and iOS client. They
need
> to be update to initialize TaskScheduler. Or it's probably better to update
> WebrtcVideoRendererAdapter to use a separate background thread, without
> dependency on TaskScheduler. Currently it's the only place in the Android/iOS
> clients that use WorkerPool (I can make CL for this).

TaskScheduler::CreateAndSet(Default|Simple)TaskScheduler() has to be called
before any function in base/task_scheduler/post_task.h can be called.

I recommend that we initialize TaskScheduler in chromoting clients by calling
base::TaskScheduler::CreateAndSetSimpleTaskScheduler(base::SysInfo::NumberOfProcessors()).
If you want me to take care of this, could you tell me from where it would be
appropriate to do so (ideally early in the main function)?

We want developers to be able to use base/task_scheduler/post_task.h from any
Chrome-related process. TaskScheduler creates/destroys threads depending on
workload, so it doesn't consume a lot of resources when it isn't used a lot. My
team would like to get rid of all special-purpose threads, which consume more
resources and can't make scheduling decisions knowing everything that is going
on in the process like TaskScheduler does.

[Wez, 2017-01-12 21:15:02]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191
==========

[Wez, 2017-01-12 21:15:02]

wez@chromium.org changed reviewers:
- wez@chromium.org

[Sergey Ulanov, 2017-01-13 08:25:42]

On 2017/01/11 17:42:32, fdoray wrote:
> On 2017/01/07 07:54:37, Sergey Ulanov wrote:
> > Sorry about the delay. 
> > 
> > Do I understand it correctly that
> > TaskScheduler::CreateAndSetDefaultTaskScheduler() has to be called in order
to
> > use base::PostTaskWithTraitsAndReplyWithResult(), otherwise the calling
> process
> > will crash?
> > In that case this change will break chromoting Android and iOS client. They
> need
> > to be update to initialize TaskScheduler. Or it's probably better to update
> > WebrtcVideoRendererAdapter to use a separate background thread, without
> > dependency on TaskScheduler. Currently it's the only place in the
Android/iOS
> > clients that use WorkerPool (I can make CL for this).
> 
> TaskScheduler::CreateAndSet(Default|Simple)TaskScheduler() has to be called
> before any function in base/task_scheduler/post_task.h can be called.
> 
> I recommend that we initialize TaskScheduler in chromoting clients by calling
>
base::TaskScheduler::CreateAndSetSimpleTaskScheduler(base::SysInfo::NumberOfProcessors()).
> If you want me to take care of this, could you tell me from where it would be
> appropriate to do so (ideally early in the main function)?
> 
> We want developers to be able to use base/task_scheduler/post_task.h from any
> Chrome-related process. TaskScheduler creates/destroys threads depending on
> workload, so it doesn't consume a lot of resources when it isn't used a lot.
My
> team would like to get rid of all special-purpose threads, which consume more
> resources and can't make scheduling decisions knowing everything that is going
> on in the process like TaskScheduler does.

This is currently the only place where WorkerPool are used in CRD (including all
dependencies). Threading in chromoting client apps for Android and iOS is quite
simple. There are just a few threads that are shared across all layers, see
remoting/client/client_context.h . TaskScheduler seems to be quite complex, with
a lot of features that we don't need, so pulling it just for this class looks
wrong. But I can also see how we can get rid of ClientContext and use
TaskScheduler for all decoding tasks. Can TaskScheduler be initialized with 1 or
2 lines of code?
You could just search all the places where ClientContext is initialized and
initialize TaskScheduler there:
 remoting/client/jni/chromoting_jni_instance.cc - Android
 remoting/client/ios/bridge/client_instance.cc - iOS
 remoting/test/test_chromoting_client.cc - test client

There is also remoting/client/plugin/chromoting_instance.h, but it won't be
using webrtc_video_renderer_adapter.cc, so you can ignore it.

[Sergey Ulanov, 2017-02-01 23:13:16]

FYI I'm adding TaskScheduler in https://codereview.chromium.org/2669893002.

[fdoray, 2017-02-08 13:23:57]

On 2017/02/01 23:13:16, Sergey Ulanov wrote:
> FYI I'm adding TaskScheduler in https://codereview.chromium.org/2669893002.

Thanks! Can you review this CL now that TaskScheduler is initialized in remoting
processes?

[Sergey Ulanov, 2017-02-08 18:35:36]

LGTM

[Sergey Ulanov, 2017-02-08 18:35:40]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2017-02-08 18:36:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 18:39:10]

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

[commit-bot: I haz the power, 2017-02-08 18:39:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-02-08 22:11:38]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-02-08 22:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2610493002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-08 22:12:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-02-08 22:13:28]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-02-08 22:13:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2610493002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-08 22:14:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:21:13]

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

[commit-bot: I haz the power, 2017-02-08 22:21:16]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-02-09 12:51:26]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-02-09 12:51:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2610493002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-09 12:51:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 13:26:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486644686185690,
"parent_rev": "0c4dc23c2c1a24df3f57347c0765b4872730c632", "commit_rev":
"c6abc2378d27fb64de389472c5af3ac74305ea51"}

[commit-bot: I haz the power, 2017-02-09 13:26:43]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in webrtc_video_renderer_adapter.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191

Review-Url: https://codereview.chromium.org/2610493002
Cr-Commit-Position: refs/heads/master@{#449277}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6abc2378d27fb64de389472c5af...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c6abc2378d27fb64de389472c5af...