mac: Use IOSurfaces in Canvas2DLayerBridge.

mac: Use IOSurfaces in Canvas2DLayerBridge.

This CL adds the method prepareIOSurfaceMailboxFromImage(). By default, Canvas2D
now emits mailboxes that contain textures backed by IOSurfaces. This CL adds the
switch kDisable2dCanvasImageChromium, which disables this new behavior.

This CL allows webpages that have canvas 2D elements to enter the new
CoreAnimation compositing mode on OS X.

Overview of new behavior in prepareMailbox():
1. On prepareMailbox(), grab an SkImage snapshot of the underlying SKCanvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f3d656148aa51e025094c93290d1fbf889064ca8
Cr-Commit-Position: refs/heads/master@{#380670}

[erikchen, 2016-03-02 06:26:42]

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

[commit-bot: I haz the power, 2016-03-02 06:26:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/1

[erikchen, 2016-03-02 06:41:24]

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

[commit-bot: I haz the power, 2016-03-02 06:41:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/20001

[erikchen, 2016-03-02 07:45:50]

Description was changed from

==========
Use IOSurfaces in Canvas2DLayerBridge.

BUG=
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
==========

[erikchen, 2016-03-02 07:46:17]

Description was changed from

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
==========

[erikchen, 2016-03-02 07:47:56]

Description was changed from

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

This CL adds the method prepareIOSurfaceMailboxFromImage(). The new default on
OSX is for Canvas2D to use IOSurfaces. This CL adds the switch
kDisable2dCanvasImageChromium, which disables IOSurface usage in Canvas2D.

BUG=579664
==========

[erikchen, 2016-03-02 07:48:10]

erikchen@chromium.org changed reviewers:
+ kbr@chromium.org

[erikchen, 2016-03-02 07:48:10]

kbr: Please review.

[commit-bot: I haz the power, 2016-03-02 10:32:23]

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

[commit-bot: I haz the power, 2016-03-02 10:32:23]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-03-02 16:33:44]

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

[Justin Novosad, 2016-03-02 16:33:45]

This is CL lacking tests.  You should create a LayoutTest virtual test suite
(similar to the gpu virtual suite) to run at least fast/canvas/* with this
option enabled.

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:275: if
(grContext && prepareIOSurfaceMailboxFromImage(image, outMailbox))
I think what you want here is:
if (!grContext)
    return true; // For unit tests
if (prepareIOSurfaceMailboxFromImage(image, outMailbox))
    return false;
// Note: if IOSurface backed texture creation failed we fall back to the
non-IOSurface path.

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:788: if
(releasedMailboxInfo->m_image) {
ASSERT(!releasedMailboxInfo->m_CHROMIUMImageId)

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:806:
context()->waitSyncTokenCHROMIUM(mailbox.syncToken);
This synchronisation is probably unnecessary? In the case of the other code
path, the only reason we needed to sync is because the textures got recycled by
skia, so it was important to make sure that the compositor was done consuming it
before throwing the texture into the recycle bin to prevent read/write race
between contexts. In this case you are just deleting the resource in the current
context, which is equivalent to decrementing a ref count.  In theory you
wouldn't even need to wait for mailboxReleased to delete the texture because you
are not even accessing it from the main thread context after prepareMailbox.

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:810: GLenum
target = GC3D_TEXTURE_RECTANGLE_ARB;
Does the extension for this need to be detected?

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/1752083003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:137: GLuint
m_CHROMIUMImageId = 0;
Can we use "#if OS(MACOSX)" for this (and the code that uses it)?

[Justin Novosad, 2016-03-02 16:43:36]

FWIW: I am not convinced this is the right approach. You are losing the double
buffer copy optimization by performing a copy upfront at prepareMailbox time. If
you added support for IOSurface backed textures to SkSurface_Gpu/SkImage_Gpu,
then we wouldn't need to make the upfront copy which means we could still
leverage the deferred copy on write optimization, and we would not need dual
resource management code paths in Canvas2DLayerBridge.

[erikchen, 2016-03-02 17:42:53]

On 2016/03/02 16:43:36, Justin Novosad wrote:
> FWIW: I am not convinced this is the right approach. You are losing the double
> buffer copy optimization by performing a copy upfront at prepareMailbox time.
If
> you added support for IOSurface backed textures to SkSurface_Gpu/SkImage_Gpu,
> then we wouldn't need to make the upfront copy which means we could still
> leverage the deferred copy on write optimization, and we would not need dual
> resource management code paths in Canvas2DLayerBridge.

If the underlying canvas changes between two calls to prepareMailbox(), both
approaches use exactly 1 copy. If it does not, the check  if (image->uniqueID()
== m_lastImageId) prevents the IOSurface approach from making a copy. The
approach you suggested is actually the first approach I attempted, but after
extensive discussion with ericrk, I decided the current approach is better. 

The Skia approach is surprisingly hairy, because Skia doesn't know how to handle
rectangle textures, and the Chrome command buffer doesn't actually support
rectangle textures, but rather a custom subset of commands associated with
rectangle textures.

[Justin Novosad, 2016-03-02 19:12:17]

As discussed by e-mail, I am in favor of trying this out. But first, can we
prove there is a substantial improvement and no substantial regressions by at
least launching a perf tryjob?

[erikchen, 2016-03-02 19:24:38]

On 2016/03/02 19:12:17, Justin Novosad wrote:
> As discussed by e-mail, I am in favor of trying this out. But first, can we
> prove there is a substantial improvement and no substantial regressions by at
> least launching a perf tryjob?

Launched try-jobs:
https://codereview.chromium.org/1757033002/
https://codereview.chromium.org/1754403002/
https://codereview.chromium.org/1756293002/
https://codereview.chromium.org/1756723005/

Note that no special flags were required, since the IOSurface functionality is
on by default, which a flag to turn it off.

[erikchen, 2016-03-02 21:27:56]

On 2016/03/02 19:24:38, erikchen wrote:
> On 2016/03/02 19:12:17, Justin Novosad wrote:
> > As discussed by e-mail, I am in favor of trying this out. But first, can we
> > prove there is a substantial improvement and no substantial regressions by
at
> > least launching a perf tryjob?
> 
> Launched try-jobs:
> https://codereview.chromium.org/1757033002/
> https://codereview.chromium.org/1754403002/
> https://codereview.chromium.org/1756293002/
> https://codereview.chromium.org/1756723005/
> 
> Note that no special flags were required, since the IOSurface functionality is
> on by default, which a flag to turn it off.

Another set of try jobs, this time with the right benchmark:
https://codereview.chromium.org/1756793003/
https://codereview.chromium.org/1761563002/
https://codereview.chromium.org/1758043002/
https://codereview.chromium.org/1752093003/

[erikchen, 2016-03-03 02:01:41]

On 2016/03/02 21:27:56, erikchen wrote:
> On 2016/03/02 19:24:38, erikchen wrote:
> > On 2016/03/02 19:12:17, Justin Novosad wrote:
> > > As discussed by e-mail, I am in favor of trying this out. But first, can
we
> > > prove there is a substantial improvement and no substantial regressions by
> at
> > > least launching a perf tryjob?
> > 
> > Launched try-jobs:
> > https://codereview.chromium.org/1757033002/
> > https://codereview.chromium.org/1754403002/
> > https://codereview.chromium.org/1756293002/
> > https://codereview.chromium.org/1756723005/
> > 
> > Note that no special flags were required, since the IOSurface functionality
is
> > on by default, which a flag to turn it off.
> 
> Another set of try jobs, this time with the right benchmark:
> https://codereview.chromium.org/1756793003/
> https://codereview.chromium.org/1761563002/
> https://codereview.chromium.org/1758043002/
> https://codereview.chromium.org/1752093003/

Okay, another set of benchmarks, with IOSurface caching:
https://codereview.chromium.org/1755163003/
https://codereview.chromium.org/1764433002/
https://codereview.chromium.org/1758103002/
https://codereview.chromium.org/1760753002/

Results for the first three are in:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...

Pretty much what I expected, which is that noise of tests overpowers performance
changes. Note that the main expected performance gains are in power, which isn't
being measured in these tests.
First one shows no change across the board. [really weird...]
Second one shows slight improvement for old-path.
Third one shows slight improvement for new-path.

The only worrying result is 36% performance reduction on frame-time-discrepancy
on the second set of results. Diving in, the new-path does better on 16 pages,
and the old code does better on 14. The reason for the 36% is that there is a
single page on which the old-path does 99.41% better. But when we look at the
same page in the third set of results, the new-path does 32% better. There's
another page in the same page set where the old-path does 20x worse. It looks
like there are non-deterministic, severe spikes in the metric
frame-time-discrepancy. Going through the other measurements, those also seem to
be dominated by the spikes in performance on a single page.

[erikchen, 2016-03-03 02:04:52]

On 2016/03/03 02:01:41, erikchen wrote:
> On 2016/03/02 21:27:56, erikchen wrote:
> > On 2016/03/02 19:24:38, erikchen wrote:
> > > On 2016/03/02 19:12:17, Justin Novosad wrote:
> > > > As discussed by e-mail, I am in favor of trying this out. But first, can
> we
> > > > prove there is a substantial improvement and no substantial regressions
by
> > at
> > > > least launching a perf tryjob?
> > > 
> > > Launched try-jobs:
> > > https://codereview.chromium.org/1757033002/
> > > https://codereview.chromium.org/1754403002/
> > > https://codereview.chromium.org/1756293002/
> > > https://codereview.chromium.org/1756723005/
> > > 
> > > Note that no special flags were required, since the IOSurface
functionality
> is
> > > on by default, which a flag to turn it off.
> > 
> > Another set of try jobs, this time with the right benchmark:
> > https://codereview.chromium.org/1756793003/
> > https://codereview.chromium.org/1761563002/
> > https://codereview.chromium.org/1758043002/
> > https://codereview.chromium.org/1752093003/
> 
> Okay, another set of benchmarks, with IOSurface caching:
> https://codereview.chromium.org/1755163003/
> https://codereview.chromium.org/1764433002/
> https://codereview.chromium.org/1758103002/
> https://codereview.chromium.org/1760753002/
> 
> Results for the first three are in:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
> 
> Pretty much what I expected, which is that noise of tests overpowers
performance
> changes. Note that the main expected performance gains are in power, which
isn't
> being measured in these tests.
> First one shows no change across the board. [really weird...]
> Second one shows slight improvement for old-path.
> Third one shows slight improvement for new-path.
> 
> The only worrying result is 36% performance reduction on
frame-time-discrepancy
> on the second set of results. Diving in, the new-path does better on 16 pages,
> and the old code does better on 14. The reason for the 36% is that there is a
> single page on which the old-path does 99.41% better. But when we look at the
> same page in the third set of results, the new-path does 32% better. There's
> another page in the same page set where the old-path does 20x worse. It looks
> like there are non-deterministic, severe spikes in the metric
> frame-time-discrepancy. Going through the other measurements, those also seem
to
> be dominated by the spikes in performance on a single page.

These results I just quoted are for patch set 4.
https://codereview.chromium.org/1752083003/#ps60001

[Justin Novosad, 2016-03-03 16:04:39]

Okay, the results look reasonable.  Now, since this is an interim solution and
will likely be refactored in the future, could we contain the IOSurface support
code in Canvas2DLayerBridge inside an "#if USE_IOSURFACE_FOR_2D_CANVAS"
The macro could be hardcoded to 1 in Canvas2DLayerBridge.h, but perhaps it
should be tied to build config if we expect this to be dead code on non-mac/ios
platforms. Especially since the way this is written the compiler will not be
able to optimize it out (code is theoretically reachable)

[erikchen, 2016-03-08 01:59:55]

Description was changed from

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

This CL adds the method prepareIOSurfaceMailboxFromImage(). The new default on
OSX is for Canvas2D to use IOSurfaces. This CL adds the switch
kDisable2dCanvasImageChromium, which disables IOSurface usage in Canvas2D.

BUG=579664
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

This CL adds the method prepareIOSurfaceMailboxFromImage(). The new default on
OSX is for Canvas2D to use IOSurfaces. This CL adds the switch
kDisable2dCanvasImageChromium, which disables IOSurface usage in Canvas2D.

BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-03-08 02:41:45]

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

[commit-bot: I haz the power, 2016-03-08 02:42:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/180001

[commit-bot: I haz the power, 2016-03-08 02:50:34]

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

[commit-bot: I haz the power, 2016-03-08 02:50:35]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-03-08 18:07:22]

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

[commit-bot: I haz the power, 2016-03-08 18:07:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/200001

[commit-bot: I haz the power, 2016-03-08 20:21:43]

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

[commit-bot: I haz the power, 2016-03-08 20:21:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-03-08 20:31:00]

junov: Please review.

The only test failures are for the new virtual test suite. I haven't added the
test expectations for that. I'm guessing you'll want me to do that in a separate
CL, that I land before this one?

[Justin Novosad, 2016-03-08 22:10:47]

On 2016/03/08 20:31:00, erikchen wrote:
> junov: Please review.
> 
> The only test failures are for the new virtual test suite. I haven't added the
> test expectations for that. I'm guessing you'll want me to do that in a
separate
> CL, that I land before this one?

[Justin Novosad, 2016-03-08 22:15:54]

You should really take care of the tests in the same CL. 
It should not be that much work.
Inspect the failures reported by the bots. For those that are bugs, file a bug
and add a failure line in TestExpectations. For the diffs that are not bugs, you
add a [ NeedsRebaseline ] line in TestExpectations

https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/NeverFixTests (right):

https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/NeverFixTests:124: [ Win Linux ]
virtual/iosurface_2d_canvas [ WontFix ]
This is a lie.  You intend to fix these

[Justin Novosad, 2016-03-08 22:20:13]

I looked at a handful of the failures and they were all just subtle diffs in
compositing and/or anti-aliasing. No actual failure. Looks like what is required
is mostly just simple rebaselining.

[erikchen, 2016-03-08 22:22:14]

On 2016/03/08 22:10:47, Justin Novosad wrote:
> On 2016/03/08 20:31:00, erikchen wrote:
> > junov: Please review.
> > 
> > The only test failures are for the new virtual test suite. I haven't added
the
> > test expectations for that. I'm guessing you'll want me to do that in a
> separate
> > CL, that I land before this one?

junov: I just chatted with dpranke@ and ccameron@. Right now, we have no
LayoutTests that require an actual GPU, and our infrastructure isn't ready to
handle that, so right now the new virtual test suite is a no-go. 

There are a couple of approaches we can take:
1. Write some gpu pixel tests. This will obviously only test a subset of all
canvas functionality, but we're not trying to test canvas functionality, we're
trying to test the new compositing path.
2. Try to hack together a solution that allows us to run LayoutTests that
require a real GPU. dpranke@ probably has more ideas of how to do this.

[erikchen, 2016-03-08 22:22:26]

https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/NeverFixTests (right):

https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/NeverFixTests:124: [ Win Linux ]
virtual/iosurface_2d_canvas [ WontFix ]
On 2016/03/08 22:15:54, Justin Novosad wrote:
> This is a lie.  You intend to fix these

I don't understand your statement. IOSurface tests should never run on Win and
Linux. This is, to the best of my knowledge, the appropriate way of "disabling"
the tests.

[Justin Novosad, 2016-03-08 22:26:10]

On 2016/03/08 22:22:26, erikchen wrote:
>
https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/NeverFixTests (right):
> 
>
https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/NeverFixTests:124: [ Win Linux ]
> virtual/iosurface_2d_canvas [ WontFix ]
> On 2016/03/08 22:15:54, Justin Novosad wrote:
> > This is a lie.  You intend to fix these
> 
> I don't understand your statement. IOSurface tests should never run on Win and
> Linux. This is, to the best of my knowledge, the appropriate way of
"disabling"
> the tests.

Sorry, I missed the part about [Win Linux]. LOL, okay.

The virtual test suite seems to be working fine on mac despite not using a
physical GPU. Is that not what you expected?

[erikchen, 2016-03-08 22:49:51]

On 2016/03/08 22:26:10, Justin Novosad wrote:
> On 2016/03/08 22:22:26, erikchen wrote:
> >
>
https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
> > File third_party/WebKit/LayoutTests/NeverFixTests (right):
> > 
> >
>
https://codereview.chromium.org/1752083003/diff/200001/third_party/WebKit/Lay...
> > third_party/WebKit/LayoutTests/NeverFixTests:124: [ Win Linux ]
> > virtual/iosurface_2d_canvas [ WontFix ]
> > On 2016/03/08 22:15:54, Justin Novosad wrote:
> > > This is a lie.  You intend to fix these
> > 
> > I don't understand your statement. IOSurface tests should never run on Win
and
> > Linux. This is, to the best of my knowledge, the appropriate way of
> "disabling"
> > the tests.
> 
> Sorry, I missed the part about [Win Linux]. LOL, okay.
> 
> The virtual test suite seems to be working fine on mac despite not using a
> physical GPU. Is that not what you expected?

Why do you think the virtual test suite is not using a physical GPU? The fact
that we're seeing any differences at all implies that we're not using the
fallback/previous compositing path. 

One possible problem is that we run multiple VMs on a single machine, and if
more than one VM attempts to access the GPU at the same time, the results are
not necessarily well defined.

[Justin Novosad, 2016-03-09 01:46:18]

> 
> Why do you think the virtual test suite is not using a physical GPU? The fact
> that we're seeing any differences at all implies that we're not using the
> fallback/previous compositing path. 
> 
> One possible problem is that we run multiple VMs on a single machine, and if
> more than one VM attempts to access the GPU at the same time, the results are
> not necessarily well defined.

None of the bots use physical GPUs except the ones on the GPU waterfall. The try
servers and the blink bots use osmesa to do OpenGL on the CPU. One of the
reasons for this is to avoid having test results that vary based on GPU model
and driver version, which was a nightmare in the past. That said, I am not sure
what happens on mac bots when you use an IOSurface in conjunction with
--override-use-gl-with-osmesa-for-tests.

[erikchen, 2016-03-09 01:50:15]

On 2016/03/09 01:46:18, Justin Novosad wrote:
> > 
> > Why do you think the virtual test suite is not using a physical GPU? The
fact
> > that we're seeing any differences at all implies that we're not using the
> > fallback/previous compositing path. 
> > 
> > One possible problem is that we run multiple VMs on a single machine, and if
> > more than one VM attempts to access the GPU at the same time, the results
are
> > not necessarily well defined.
> 
> None of the bots use physical GPUs except the ones on the GPU waterfall. The
try
> servers and the blink bots use osmesa to do OpenGL on the CPU. One of the
> reasons for this is to avoid having test results that vary based on GPU model
> and driver version, which was a nightmare in the past. That said, I am not
sure
> what happens on mac bots when you use an IOSurface in conjunction with
> --override-use-gl-with-osmesa-for-tests.

The VirtualTestSuite I set up has the flag --use-gl=desktop, which causes the
test to use the real GPU. The new logic in Canvas2DLayerBridge.cpp is gated by
the conditional RuntimeEnabledFeatures::canvas2dImageChromiumEnabled(), which is
only enabled for real GPUs (this prevents IOSurfaces from being used with
osmesa)

[Justin Novosad, 2016-03-09 16:44:58]

Ah, that explains the diffs then.

The CL is still missing [ NeedsRebaseline ] entries for the
virtual/iosurface_2d_canvas/fast/canvas/ on Mac.

[Justin Novosad, 2016-03-09 16:46:55]

On 2016/03/09 16:44:58, Justin Novosad wrote:
> Ah, that explains the diffs then.
> 
> The CL is still missing [ NeedsRebaseline ] entries for the
> virtual/iosurface_2d_canvas/fast/canvas/ on Mac.

Also, did you check with the test infra team that it Okay to use
--use-gl=desktop on the Mac blink bots?

[erikchen, 2016-03-09 17:13:24]

On 2016/03/09 16:46:55, Justin Novosad wrote:
> On 2016/03/09 16:44:58, Justin Novosad wrote:
> > Ah, that explains the diffs then.
> > 
> > The CL is still missing [ NeedsRebaseline ] entries for the
> > virtual/iosurface_2d_canvas/fast/canvas/ on Mac.
> 
> Also, did you check with the test infra team that it Okay to use
> --use-gl=desktop on the Mac blink bots?

I think we might be having a miscommunication. This CL, as is, is not landable.
dpranke@ has explicitly stated we should not land this CL, because we should not
use real GPUs on the layout tests. My suggestion was that I could write a gpu
pixel test (that would use the gpu), but pull out all the layout tests. There is
currently no way have LayoutTests that actually use IOSurfaces.

Does that sound reasonable to you?

[Justin Novosad, 2016-03-09 17:38:30]

On 2016/03/09 17:13:24, erikchen wrote:
> On 2016/03/09 16:46:55, Justin Novosad wrote:
> > On 2016/03/09 16:44:58, Justin Novosad wrote:
> > > Ah, that explains the diffs then.
> > > 
> > > The CL is still missing [ NeedsRebaseline ] entries for the
> > > virtual/iosurface_2d_canvas/fast/canvas/ on Mac.
> > 
> > Also, did you check with the test infra team that it Okay to use
> > --use-gl=desktop on the Mac blink bots?
> 
> I think we might be having a miscommunication. This CL, as is, is not
landable.
> dpranke@ has explicitly stated we should not land this CL, because we should
not
> use real GPUs on the layout tests. My suggestion was that I could write a gpu
> pixel test (that would use the gpu), but pull out all the layout tests. There
is
> currently no way have LayoutTests that actually use IOSurfaces.
> 
> Does that sound reasonable to you?

Yes, that sounds reasonable.  Are you going to do that in this CL?

[erikchen, 2016-03-09 22:39:58]

junov: Please review whole CL
kbr: Please review pixel test changes in content/test/gpu.

[Justin Novosad, 2016-03-09 23:13:11]

lgtm with nit for third_party/WebKit/Source/platform/
I am not an OWNER in the other areas.

https://codereview.chromium.org/1752083003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/1752083003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:47: #if
OS(MACOSX)
I just landed this minutes ago: https://codereview.chromium.org/1762783002/  Not
sure if there is a collision, but I put a similar "#if" in a slightly different
location. Could you rebase and group the macros together.

[erikchen, 2016-03-09 23:42:24]

https://codereview.chromium.org/1752083003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/1752083003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:47: #if
OS(MACOSX)
On 2016/03/09 23:13:11, Justin Novosad wrote:
> I just landed this minutes ago: https://codereview.chromium.org/1762783002/ 
Not
> sure if there is a collision, but I put a similar "#if" in a slightly
different
> location. Could you rebase and group the macros together.

Done.

[erikchen, 2016-03-09 23:46:31]

erikchen@chromium.org changed reviewers:
+ aelias@chromium.org, avi@chromium.org, dzhioev@chromium.org

[erikchen, 2016-03-09 23:46:32]

avi: Please review content/, but not content/test/, which kbr@ will review.

dzhioev: Please review chrome/browser/chromeos/login/chrome_restart_request.cc

aelias: Please review third_party/WebKit/Source/web/WebRuntimeFeatures.cpp and
third_party/WebKit/public/web/WebRuntimeFeatures.h

[aelias_OOO_until_Jul13, 2016-03-09 23:54:01]

Source/web lgtm

[Ken Russell (switch to Gerrit), 2016-03-10 01:58:04]

content/test/gpu lgtm

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:290: void
Canvas2DLayerBridge::clearCHROMIUMImageCache()
This must be called when the canvas is resized, correct? If so, where does that
happen? Is there a test for that (that a canvas drawn with one size, and then
re-drawn with another size, renders correctly)?

[Ken Russell (switch to Gerrit), 2016-03-10 02:00:03]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-03-10 02:00:04]

One more thing comes to mind. CC'ing zmo.

https://codereview.chromium.org/1752083003/diff/260001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/1752083003/diff/260001/content/child/runtime_...
content/child/runtime_features.cc:119: enable_canvas_2d_image_chromium);
I think you should add a new GPU driver bug workaround type covering this
feature, so that if you find that it's broken on a particular kind of GPU, you
can easily disable it there. Please see
src/gpu/config/gpu_driver_bug_list_json.cc and
src/gpu/config/gpu_driver_bug_workaround_type.h , and uses of these workaround
types.

[erikchen, 2016-03-10 02:03:37]

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:290: void
Canvas2DLayerBridge::clearCHROMIUMImageCache()
On 2016/03/10 01:58:04, Ken Russell wrote:
> This must be called when the canvas is resized, correct? If so, where does
that
> happen? Is there a test for that (that a canvas drawn with one size, and then
> re-drawn with another size, renders correctly)?

I am under the impression that resizing happens in another location, and
presumably involves destroying this instance of Canvas2DLayerBridge. Note that
my CL adds the "const" qualifier to the member m_size.

[erikchen, 2016-03-10 02:10:51]

On 2016/03/10 02:00:04, Ken Russell wrote:
> One more thing comes to mind. CC'ing zmo.
> 
>
https://codereview.chromium.org/1752083003/diff/260001/content/child/runtime_...
> File content/child/runtime_features.cc (right):
> 
>
https://codereview.chromium.org/1752083003/diff/260001/content/child/runtime_...
> content/child/runtime_features.cc:119: enable_canvas_2d_image_chromium);
> I think you should add a new GPU driver bug workaround type covering this
> feature, so that if you find that it's broken on a particular kind of GPU, you
> can easily disable it there. Please see
> src/gpu/config/gpu_driver_bug_list_json.cc and
> src/gpu/config/gpu_driver_bug_workaround_type.h , and uses of these workaround
> types.

Chatted with kbr@ offline about this. The main reason for his suggestion was so
that if we run into a problem, I can turn off the feature w/o reverting. 

If we run into problems and I want to avoid a revert, I'll just use the newly
added flag kDisable2dCanvasImageChromium to turn off the functionality.

[Ken Russell (switch to Gerrit), 2016-03-10 02:59:41]

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/1752083003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:290: void
Canvas2DLayerBridge::clearCHROMIUMImageCache()
On 2016/03/10 02:03:37, erikchen wrote:
> On 2016/03/10 01:58:04, Ken Russell wrote:
> > This must be called when the canvas is resized, correct? If so, where does
> that
> > happen? Is there a test for that (that a canvas drawn with one size, and
then
> > re-drawn with another size, renders correctly)?
> 
> I am under the impression that resizing happens in another location, and
> presumably involves destroying this instance of Canvas2DLayerBridge. Note that
> my CL adds the "const" qualifier to the member m_size.

OK, yes, it looks like HTMLCanvasElement::m_imageBuffer is cleared indirectly
from HTMLCanvasElement::setSurfaceSize, called from HTMLCanvasElement::reset,
called from HTMLCanvasElement::setSize -- and the ImageBuffer indirectly owns
the Canvas2DLayerBridge.

[Avi (use Gerrit), 2016-03-10 04:01:51]

content non-test lgtm

[erikchen, 2016-03-10 17:50:04]

erikchen@chromium.org changed reviewers:
+ thakis@chromium.org

[erikchen, 2016-03-10 17:50:05]

thakis: Could I get a 1-line stamp on chrome/?

[Nico, 2016-03-10 18:09:27]

here comes the one-line stamp:

lgtm stamp

CL description doesn't say why this change is being made, it only explains how
it's not worse afaiu. The bug kind of hints at this, but it's long and full of
details. Maybe motivate "why" in the cl description a bit more.

[erikchen, 2016-03-10 18:17:45]

Description was changed from

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

The current implementation of Canvas2DLayerBridge performs a copy for each call
of prepareMailbox().
1. On prepareMailbox(), grab a new SkImage snapshot of the underlying canvas
with canvas->newImageSnapshot() (no copy).
2. Skia internally performs a copy-on-write.

The IOSurface implementation also performs a copy for each call of
prepareMailbox().
1. On prepareMailbox(), grab an SkImage snapshot of the underlying canvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

This CL adds the method prepareIOSurfaceMailboxFromImage(). The new default on
OSX is for Canvas2D to use IOSurfaces. This CL adds the switch
kDisable2dCanvasImageChromium, which disables IOSurface usage in Canvas2D.

BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

This CL adds the method prepareIOSurfaceMailboxFromImage(). By default, Canvas2D
now emits mailboxes that contain textures backed by IOSurfaces. This CL adds the
switch kDisable2dCanvasImageChromium, which disables this new behavior.

This CL allows webpages that have canvas 2D elements to enter the new
CoreAnimation compositing mode on OS X.

Overview of new behavior in prepareMailbox():
1. On prepareMailbox(), grab an SkImage snapshot of the underlying SKCanvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.


BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-03-10 18:18:48]

On 2016/03/10 18:09:27, Nico wrote:
> here comes the one-line stamp:
> 
> lgtm stamp
> 
> CL description doesn't say why this change is being made, it only explains how
> it's not worse afaiu. The bug kind of hints at this, but it's long and full of
> details. Maybe motivate "why" in the cl description a bit more.

I rewrote the CL description. Quoting an excerpt that answers your question:

"""
This CL allows webpages that have canvas 2D elements to enter the new
CoreAnimation compositing mode on OS X.
"""

[erikchen, 2016-03-10 18:19:06]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-10 18:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1752083003/#ps260001
(title: "Rebase. Group macros.")

[commit-bot: I haz the power, 2016-03-10 18:19:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-10 19:39:02]

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

[commit-bot: I haz the power, 2016-03-10 19:39:05]

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

[erikchen, 2016-03-10 20:06:38]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-10 20:07:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-10 21:41:18]

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

[commit-bot: I haz the power, 2016-03-10 21:41:21]

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

[erikchen, 2016-03-10 21:44:34]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-10 21:45:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-10 22:37:25]

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

[commit-bot: I haz the power, 2016-03-10 22:37:28]

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

[erikchen, 2016-03-10 22:39:11]

On 2016/03/10 22:37:28, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

CQ is experiencing problems:
https://bugs.chromium.org/p/chromium/issues/detail?id=593891

[erikchen, 2016-03-10 22:46:15]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-10 22:47:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-10 23:17:35]

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

[commit-bot: I haz the power, 2016-03-10 23:17:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-03-11 01:30:51]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:31:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-11 02:44:49]

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

[commit-bot: I haz the power, 2016-03-11 02:44:52]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-03-11 17:15:24]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-03-11 17:15:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752083003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752083003/260001

[commit-bot: I haz the power, 2016-03-11 18:17:11]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-03-11 18:18:36]

Description was changed from

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

This CL adds the method prepareIOSurfaceMailboxFromImage(). By default, Canvas2D
now emits mailboxes that contain textures backed by IOSurfaces. This CL adds the
switch kDisable2dCanvasImageChromium, which disables this new behavior.

This CL allows webpages that have canvas 2D elements to enter the new
CoreAnimation compositing mode on OS X.

Overview of new behavior in prepareMailbox():
1. On prepareMailbox(), grab an SkImage snapshot of the underlying SKCanvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.


BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mac: Use IOSurfaces in Canvas2DLayerBridge.

This CL adds the method prepareIOSurfaceMailboxFromImage(). By default, Canvas2D
now emits mailboxes that contain textures backed by IOSurfaces. This CL adds the
switch kDisable2dCanvasImageChromium, which disables this new behavior.

This CL allows webpages that have canvas 2D elements to enter the new
CoreAnimation compositing mode on OS X.

Overview of new behavior in prepareMailbox():
1. On prepareMailbox(), grab an SkImage snapshot of the underlying SKCanvas.
2. Copy the snapshot into a new IOSurface backed GL texture, and pass that to
prepareMailbox().
3. Discard the SkImage. Skia does not perform any internal copies.

BUG=579664
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f3d656148aa51e025094c93290d1fbf889064ca8
Cr-Commit-Position: refs/heads/master@{#380670}
==========

[commit-bot: I haz the power, 2016-03-11 18:18:38]

Patchset 14 (id:??) landed as
https://crrev.com/f3d656148aa51e025094c93290d1fbf889064ca8
Cr-Commit-Position: refs/heads/master@{#380670}