Allow canvases to be GPU-accelerated in Workers

Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
Committed: https://crrev.com/83ac5ff37da4ea1c6d052649ccea46dd4bd453d6
Cr-Commit-Position: refs/heads/master@{#416929}

[Justin Novosad, 2016-08-31 21:35:37]

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

[commit-bot: I haz the power, 2016-08-31 21:36:22]

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

[Justin Novosad, 2016-08-31 21:36:45]

junov@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Justin Novosad, 2016-08-31 21:36:46]

PTAL

[commit-bot: I haz the power, 2016-08-31 23:06:32]

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

[commit-bot: I haz the power, 2016-08-31 23:06:33]

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_...)

[Justin Novosad, 2016-09-01 14:37:35]

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

[commit-bot: I haz the power, 2016-09-01 14:37:46]

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, 2016-09-01 14:40:48]

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

[commit-bot: I haz the power, 2016-09-01 14:40:49]

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

[Stephen White, 2016-09-01 15:43:54]

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:33:
return adoptRef(new AcceleratedStaticBitmapImage(image, std::move(grContext),
mailbox, syncToken));
Note: now calling std::move() on a bare ptr.

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:53:
if (!grContext || grContext == sharedGr) {
As discussed, use this create() method only for out-of-context creation, and the
other one for in-context creation? Then you could get rid of the GrContext
parameter entirely.

In the future, it also might be worth investigating splitting the use cases into
two subclasses (one mailbox, one non-mailbox).

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:139:
RefPtr<SkImage> newImage =
fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext, backendTexture));
Nit: could also use an sk_sp<> for newImage, then do fromSkSp() below, if we're
moving away from using RefPtrs on Skia objects.

(Actually, m_image could be sk_sp<> too, for that matter. Doesn't have to be
this patch, though.)

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:199:
image->unref();
It's unfortunate to have explit unref() here. Could we use std::move() to
transfer ownership in the caller, and pass an sk_sp<> in this function param?

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp
(right):

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp:18:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<SharedGpuContext>,
threadSpecificInstance, new ThreadSpecific<SharedGpuContext>);
As discussed, it might be nice to move this into some other per-worker owned
object, rather than using a TLS singleton. Also, please add a test for two
OffscreenCanvases in two separate workers, if that doesn't already exist.

[haraken, 2016-09-01 17:33:05]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-09-01 17:33:05]

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp
(right):

https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp:18:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<SharedGpuContext>,
threadSpecificInstance, new ThreadSpecific<SharedGpuContext>);
On 2016/09/01 15:43:54, Stephen White wrote:
> As discussed, it might be nice to move this into some other per-worker owned
> object, rather than using a TLS singleton. Also, please add a test for two
> OffscreenCanvases in two separate workers, if that doesn't already exist.

Yeah, agreed. We want to avoid this kind of thread-local storage usage.

[Justin Novosad, 2016-09-01 18:15:00]

On 2016/09/01 17:33:05, haraken wrote:
>
https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp
> (right):
> 
>
https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp:18:
> DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<SharedGpuContext>,
> threadSpecificInstance, new ThreadSpecific<SharedGpuContext>);
> On 2016/09/01 15:43:54, Stephen White wrote:
> > As discussed, it might be nice to move this into some other per-worker owned
> > object, rather than using a TLS singleton. Also, please add a test for two
> > OffscreenCanvases in two separate workers, if that doesn't already exist.
> 
> Yeah, agreed. We want to avoid this kind of thread-local storage usage.

I agree too, I just could not find an appropriate class to own the instance. 
WebThread could work, or ExecutionContext. These just don't seem like fantastic
places to add dependencies on Gpu stuff.  I am open to suggestions.

[haraken, 2016-09-01 18:29:36]

On 2016/09/01 18:15:00, Justin Novosad wrote:
> On 2016/09/01 17:33:05, haraken wrote:
> >
>
https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2300633004/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/graphics/gpu/SharedGpuContext.cpp:18:
> > DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<SharedGpuContext>,
> > threadSpecificInstance, new ThreadSpecific<SharedGpuContext>);
> > On 2016/09/01 15:43:54, Stephen White wrote:
> > > As discussed, it might be nice to move this into some other per-worker
owned
> > > object, rather than using a TLS singleton. Also, please add a test for two
> > > OffscreenCanvases in two separate workers, if that doesn't already exist.
> > 
> > Yeah, agreed. We want to avoid this kind of thread-local storage usage.
> 
> I agree too, I just could not find an appropriate class to own the instance. 
> WebThread could work, or ExecutionContext. These just don't seem like
fantastic
> places to add dependencies on Gpu stuff.  I am open to suggestions.

I looked at the code base, but it looks like that it's common to use
ThreadSpecific for per-thread things. This doesn't look nice but that's how
things are working today.

https://cs.chromium.org/search/?q=DEFINE_THREAD_SAFE_STATIC_LOCAL+ThreadSpeci...

Maybe we should introduce a platform-level abstraction class per thread. Either
way, I'm okay with using ThreadSpecific in this CL.

[Justin Novosad, 2016-09-02 21:00:25]

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

[Justin Novosad, 2016-09-02 21:00:46]

The CQ bit was unchecked by junov@chromium.org

[commit-bot: I haz the power, 2016-09-02 21:00:50]

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

[Justin Novosad, 2016-09-02 21:04:28]

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

[commit-bot: I haz the power, 2016-09-02 21:05:06]

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, 2016-09-02 21:08:40]

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

[commit-bot: I haz the power, 2016-09-02 21:08:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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_...)

[Justin Novosad, 2016-09-02 21:16:56]

New patch.

Addressed all comments.  The additional test actually found bugs.  \o/

[Justin Novosad, 2016-09-06 17:16:33]

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

[commit-bot: I haz the power, 2016-09-06 17:16:50]

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

[Justin Novosad, 2016-09-06 17:19:58]

New patch.  rebased

[Justin Novosad, 2016-09-06 17:50:47]

junov@chromium.org changed reviewers:
+ kbr@chromium.org
- senorblanco@chromium.org

[Justin Novosad, 2016-09-06 17:50:48]

Switching reviewers because senorblanco is OOO
@kbr: PTAL

[Justin Novosad, 2016-09-06 17:54:50]

Description was changed from

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
==========

to

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
==========

[commit-bot: I haz the power, 2016-09-06 19:22:36]

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

[commit-bot: I haz the power, 2016-09-06 19:22:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Justin Novosad, 2016-09-06 20:31:27]

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

[commit-bot: I haz the power, 2016-09-06 20:31:52]

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, 2016-09-06 22:42:22]

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

[commit-bot: I haz the power, 2016-09-06 22:42:23]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-07 04:20:43]

This is awesome. I don't have confidence though in my ability to look at this
code and gauge its correctness. The tests will have to do that. Assuming that
the tests are all passing, then lgtm. The thread handling in
AcceleratedStaticBitmapImage does look OK.

[Justin Novosad, 2016-09-07 13:55:22]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-09-07 13:55:42]

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, 2016-09-07 13:59:59]

Description was changed from

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
==========

to

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
==========

[commit-bot: I haz the power, 2016-09-07 14:00:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-07 14:01:30]

Description was changed from

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514
==========

to

==========
Allow canvases to be GPU-accelerated in Workers

This change sets up a per-thread shared GPU context that is managed by
a thread-specific singleton SharedGpuContext.  This means that all 2D
contexts on a given worker will use the same context, which avoids
having to rely on mailboxes all the time.  Also this CL makes deep changes
to AcceleratedStaticBitmapImage in order to support the use of
the current thread's shared context, and it handles transfers
between threads in order to support the transferrable behavior of
ImageBitmap objects.

BUG=593514

Committed: https://crrev.com/83ac5ff37da4ea1c6d052649ccea46dd4bd453d6
Cr-Commit-Position: refs/heads/master@{#416929}
==========

[commit-bot: I haz the power, 2016-09-07 14:01:31]

Patchset 5 (id:??) landed as
https://crrev.com/83ac5ff37da4ea1c6d052649ccea46dd4bd453d6
Cr-Commit-Position: refs/heads/master@{#416929}

[lushnikov, 2016-09-08 00:13:14]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2323573002/ by lushnikov@chromium.org.

The reason for reverting is: This makes the following tests fail on Linux MSAN
bot:

virtual/gpu/fast/canvas/canvas-createImageBitmap-invalid-blob-in-workers.html
virtual/display_list_2d_canvas/fast/canvas/canvas-createImageBitmap-invalid-blob-in-workers.html

Build link:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu...

.