Fix Slimming Paint WebGL printing

Fix Slimming Paint WebGL printing

Slimming Paint uses a recording canvas when printing, which triggered
an assert in GraphcisContext::drawBitmapRect which previously
did not trigger because we were not recording when printing.

This patch modifies the assert to check for printing, when we
are only recording temporarily before immediate playback into
a canvas on the main thread.

The patch also modifies DisplayItemListRecorderContext to copy
global properties fromt he primary context to the temporary
recording context.

R=danakj@chromium.org,chrishtr@chromium.org
BUG=471100
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195818

[Stephen Chennney, 2015-05-19 21:09:01]

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

[commit-bot: I haz the power, 2015-05-19 21:09:34]

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

[commit-bot: I haz the power, 2015-05-19 22:26:52]

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

[commit-bot: I haz the power, 2015-05-19 22:26:55]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2015-05-20 13:52:35]

Fixing WebGL printing.

[chrishtr, 2015-05-20 18:20:40]

https://codereview.chromium.org/1145843003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1145843003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.cpp:1102: ASSERT(!isRecording() ||
!bitmap.getTexture() || printing());
I assume that when slimming paint is off, and printing is on, the code that
generates an SkBitmap and passes it to drawBitmapRect always does so without a
texture. Is that right?

And if so, it seems we should have the same sort of logic with slimming paint.
Maybe the SkBitmap-creation logic checks for isRecording(), and we should check
for isRecording() || printing()?

[Stephen Chennney, 2015-05-20 18:30:14]

https://codereview.chromium.org/1145843003/diff/1/Source/platform/graphics/Gr...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1145843003/diff/1/Source/platform/graphics/Gr...
Source/platform/graphics/GraphicsContext.cpp:1102: ASSERT(!isRecording() ||
!bitmap.getTexture() || printing());
On 2015/05/20 18:20:40, chrishtr wrote:
> I assume that when slimming paint is off, and printing is on, the code that
> generates an SkBitmap and passes it to drawBitmapRect always does so without a
> texture. Is that right?

There is a texture during printing, probably because it's WebGL and there needs
to be a texture to capture the content. I'll look further into where this
texture comes from.

> And if so, it seems we should have the same sort of logic with slimming paint.
> Maybe the SkBitmap-creation logic checks for isRecording(), and we should
check
> for isRecording() || printing()?

I'll look.

It is the case that the thing the assert is checking for, use of a texture off
the Blink main thread, is not a problem when printing. So on some fundamental
level the assert is too aggressive.

[Stephen Chennney, 2015-05-20 21:00:10]

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

[commit-bot: I haz the power, 2015-05-20 21:00:26]

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

[Stephen Chennney, 2015-05-20 21:00:31]

schenney@chromium.org changed reviewers:
+ junov@chromium.org
- danakj@chromium.org

[Stephen Chennney, 2015-05-20 21:03:14]

So I added a check in HTMLCanvasElement::shouldAccelerate to turn of
acceleration for printing, which seems right. It locally fixes the Slimming
Paint crash and not break anything else.

Also forcing an unaccelerated buffer in the is3D() case in
HTMLCanvasElement::createImageBufferSurface did cause a failure in the
virtual/threaded tests.

[danakj, 2015-05-20 21:06:17]

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

[danakj, 2015-05-20 21:06:17]

LGTM

[chrishtr, 2015-05-20 21:08:05]

Does WebGL printing also work now? e.g. if you just ctrl-p from Google Maps?

[Stephen Chennney, 2015-05-20 21:09:32]

But not a better fix, because we still crash when printing WebGl content in the
browser. e.g. go to http://neuroid.co.uk/lab/cub/ and print the WebGL.

[Stephen Chennney, 2015-05-20 21:29:28]

On 2015/05/20 21:08:05, chrishtr wrote:
> Does WebGL printing also work now? e.g. if you just ctrl-p from Google Maps.

No. So I think we need the original fix, or else we need some way of saying, in
GraphicsContext, "I'm recording but I'm going to play immediately into a raster
canvas".

[commit-bot: I haz the power, 2015-05-20 21:46:45]

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

[commit-bot: I haz the power, 2015-05-20 21:46:46]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[chrishtr, 2015-05-20 22:39:47]

On 2015/05/20 at 21:29:28, schenney wrote:
> On 2015/05/20 21:08:05, chrishtr wrote:
> > Does WebGL printing also work now? e.g. if you just ctrl-p from Google Maps.
> 
> No. So I think we need the original fix, or else we need some way of saying,
in GraphicsContext, "I'm recording but I'm going to play immediately into a
raster canvas".

We still don't know why the WebGL code works in non-SP also. Presumably it's
checking for recording canvases or printing somewhere?

[danakj, 2015-05-20 22:46:24]

On Wed, May 20, 2015 at 3:39 PM, <chrishtr@chromium.org> wrote:

> On 2015/05/20 at 21:29:28, schenney wrote:
>
>> On 2015/05/20 21:08:05, chrishtr wrote:
>> > Does WebGL printing also work now? e.g. if you just ctrl-p from Google
>> Maps.
>>
>
>  No. So I think we need the original fix, or else we need some way of
>> saying,
>>
> in GraphicsContext, "I'm recording but I'm going to play immediately into a
> raster canvas".
>
> We still don't know why the WebGL code works in non-SP also. Presumably
> it's
> checking for recording canvases or printing somewhere?
>

It must be using GL textures and doing a readback later?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2015-05-21 14:28:22]

On 2015/05/20 22:46:24, danakj wrote:
> On Wed, May 20, 2015 at 3:39 PM, <mailto:chrishtr@chromium.org> wrote:
> 
> > On 2015/05/20 at 21:29:28, schenney wrote:
> >
> >> On 2015/05/20 21:08:05, chrishtr wrote:
> >> > Does WebGL printing also work now? e.g. if you just ctrl-p from Google
> >> Maps.
> >>
> >
> >  No. So I think we need the original fix, or else we need some way of
> >> saying,
> >>
> > in GraphicsContext, "I'm recording but I'm going to play immediately into a
> > raster canvas".
> >
> > We still don't know why the WebGL code works in non-SP also. Presumably
> > it's
> > checking for recording canvases or printing somewhere?
> >
> 
> It must be using GL textures and doing a readback later?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

In the WebGL case, HTMLCanvasElement::paint() should work by doing a readback
(DrawingBuffer->ImageBuffer) that is what the call to
paintRenderingResultsToCanvas is for.  If the ImageBuffer is accelerated in that
case, then that is a bug. The problem might be that the image buffer was created
prior to printing, at which time shouldAccelerate would have returned true.

Try this: at the beginning of HTMLCanvasElement::paint() call
discardImageBuffer() if the context is 3d, and the document is printing. This
will force a new imagebuffer to be created, allowing your change to
shouldAccelerate() to kick in. Then call discardImageBuffer() again at the end
of HTMLCanvasElement::paint(), under the same conditions, to allow the
ImageBuffer to go back to being accelerated when done printing.

[Stephen Chennney, 2015-05-21 16:45:48]

On 2015/05/21 14:28:22, Justin Novosad wrote:
> On 2015/05/20 22:46:24, danakj wrote:
> > On Wed, May 20, 2015 at 3:39 PM, <mailto:chrishtr@chromium.org> wrote:
> > 
> > > On 2015/05/20 at 21:29:28, schenney wrote:
> > >
> > >> On 2015/05/20 21:08:05, chrishtr wrote:
> > >> > Does WebGL printing also work now? e.g. if you just ctrl-p from Google
> > >> Maps.
> > >>
> > >
> > >  No. So I think we need the original fix, or else we need some way of
> > >> saying,
> > >>
> > > in GraphicsContext, "I'm recording but I'm going to play immediately into
a
> > > raster canvas".
> > >
> > > We still don't know why the WebGL code works in non-SP also. Presumably
> > > it's
> > > checking for recording canvases or printing somewhere?
> > >
> > 
> > It must be using GL textures and doing a readback later?
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> In the WebGL case, HTMLCanvasElement::paint() should work by doing a readback
> (DrawingBuffer->ImageBuffer) that is what the call to
> paintRenderingResultsToCanvas is for.  If the ImageBuffer is accelerated in
that
> case, then that is a bug. The problem might be that the image buffer was
created
> prior to printing, at which time shouldAccelerate would have returned true.
> 
> Try this: at the beginning of HTMLCanvasElement::paint() call
> discardImageBuffer() if the context is 3d, and the document is printing. This
> will force a new imagebuffer to be created, allowing your change to
> shouldAccelerate() to kick in. Then call discardImageBuffer() again at the end
> of HTMLCanvasElement::paint(), under the same conditions, to allow the
> ImageBuffer to go back to being accelerated when done printing.

I tried the above but WebGL printing fails, doesn't crash, with a blank canvas.
That makes sense to me; I can't see how WebGL content can produce anything
without a 3D backing.

The original patch#1 is functionally the same as the old pipeline so at this
point I'm back to thinking we should fix things that way.

If we want to be super clear about what's going on, I suggest adding a
query-able mode for "recording for immediate playback on same thread".

[Justin Novosad, 2015-05-22 01:18:56]

On 2015/05/21 16:45:48, Stephen Chenney wrote:
> On 2015/05/21 14:28:22, Justin Novosad wrote:
> > On 2015/05/20 22:46:24, danakj wrote:
> > > On Wed, May 20, 2015 at 3:39 PM, <mailto:chrishtr@chromium.org> wrote:
> > > 
> > > > On 2015/05/20 at 21:29:28, schenney wrote:
> > > >
> > > >> On 2015/05/20 21:08:05, chrishtr wrote:
> > > >> > Does WebGL printing also work now? e.g. if you just ctrl-p from
Google
> > > >> Maps.
> > > >>
> > > >
> > > >  No. So I think we need the original fix, or else we need some way of
> > > >> saying,
> > > >>
> > > > in GraphicsContext, "I'm recording but I'm going to play immediately
into
> a
> > > > raster canvas".
> > > >
> > > > We still don't know why the WebGL code works in non-SP also. Presumably
> > > > it's
> > > > checking for recording canvases or printing somewhere?
> > > >
> > > 
> > > It must be using GL textures and doing a readback later?
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
> > 
> > In the WebGL case, HTMLCanvasElement::paint() should work by doing a
readback
> > (DrawingBuffer->ImageBuffer) that is what the call to
> > paintRenderingResultsToCanvas is for.  If the ImageBuffer is accelerated in
> that
> > case, then that is a bug. The problem might be that the image buffer was
> created
> > prior to printing, at which time shouldAccelerate would have returned true.
> > 
> > Try this: at the beginning of HTMLCanvasElement::paint() call
> > discardImageBuffer() if the context is 3d, and the document is printing.
This
> > will force a new imagebuffer to be created, allowing your change to
> > shouldAccelerate() to kick in. Then call discardImageBuffer() again at the
end
> > of HTMLCanvasElement::paint(), under the same conditions, to allow the
> > ImageBuffer to go back to being accelerated when done printing.
> 
> I tried the above but WebGL printing fails, doesn't crash, with a blank
canvas.
> That makes sense to me; I can't see how WebGL content can produce anything
> without a 3D backing.

The ImageBuffer is not the backing for the WebGL canvas (it is for 2D canvases,
but not for WebGL). The WebGL backing is in DrawingBuffer. In the case of a
WebGL canvas, the ImageBuffer is only used for storing presentation copies for
some indirect code paths (like printing).
> 
> The original patch#1 is functionally the same as the old pipeline so at this
> point I'm back to thinking we should fix things that way.
> 
> If we want to be super clear about what's going on, I suggest adding a
> query-able mode for "recording for immediate playback on same thread".

[chrishtr, 2015-05-22 18:13:49]

On 2015/05/22 at 01:18:56, junov wrote:
> On 2015/05/21 16:45:48, Stephen Chenney wrote:
> > On 2015/05/21 14:28:22, Justin Novosad wrote:
> > > On 2015/05/20 22:46:24, danakj wrote:
> > > > On Wed, May 20, 2015 at 3:39 PM, <mailto:chrishtr@chromium.org> wrote:
> > > > 
> > > > > On 2015/05/20 at 21:29:28, schenney wrote:
> > > > >
> > > > >> On 2015/05/20 21:08:05, chrishtr wrote:
> > > > >> > Does WebGL printing also work now? e.g. if you just ctrl-p from
Google
> > > > >> Maps.
> > > > >>
> > > > >
> > > > >  No. So I think we need the original fix, or else we need some way of
> > > > >> saying,
> > > > >>
> > > > > in GraphicsContext, "I'm recording but I'm going to play immediately
into
> > a
> > > > > raster canvas".
> > > > >
> > > > > We still don't know why the WebGL code works in non-SP also.
Presumably
> > > > > it's
> > > > > checking for recording canvases or printing somewhere?
> > > > >
> > > > 
> > > > It must be using GL textures and doing a readback later?
> > > > 
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:blink-reviews+unsubscribe@chromium.org.
> > > 
> > > In the WebGL case, HTMLCanvasElement::paint() should work by doing a
readback
> > > (DrawingBuffer->ImageBuffer) that is what the call to
> > > paintRenderingResultsToCanvas is for.  If the ImageBuffer is accelerated
in
> > that
> > > case, then that is a bug. The problem might be that the image buffer was
> > created
> > > prior to printing, at which time shouldAccelerate would have returned
true.
> > > 
> > > Try this: at the beginning of HTMLCanvasElement::paint() call
> > > discardImageBuffer() if the context is 3d, and the document is printing.
This
> > > will force a new imagebuffer to be created, allowing your change to
> > > shouldAccelerate() to kick in. Then call discardImageBuffer() again at the
end
> > > of HTMLCanvasElement::paint(), under the same conditions, to allow the
> > > ImageBuffer to go back to being accelerated when done printing.
> > 
> > I tried the above but WebGL printing fails, doesn't crash, with a blank
canvas.
> > That makes sense to me; I can't see how WebGL content can produce anything
> > without a 3D backing.
> 
> The ImageBuffer is not the backing for the WebGL canvas (it is for 2D
canvases, but not for WebGL). The WebGL backing is in DrawingBuffer. In the case
of a WebGL canvas, the ImageBuffer is only used for storing presentation copies
for some indirect code paths (like printing).
> > 
> > The original patch#1 is functionally the same as the old pipeline so at this
> > point I'm back to thinking we should fix things that way.
> > 
> > If we want to be super clear about what's going on, I suggest adding a
> > query-able mode for "recording for immediate playback on same thread".

My view: I just want to make sure we know how the non-SP code is working
correctly. That way we know for sure whether a proposed fix here is fully
correct.

[Stephen Chennney, 2015-05-22 18:36:53]

On 2015/05/22 18:13:49, chrishtr wrote:
> On 2015/05/22 at 01:18:56, junov wrote:
> > On 2015/05/21 16:45:48, Stephen Chenney wrote:
> > > I tried the above but WebGL printing fails, doesn't crash, with a blank
> canvas.
> > > That makes sense to me; I can't see how WebGL content can produce anything
> > > without a 3D backing.
> > 
> > The ImageBuffer is not the backing for the WebGL canvas (it is for 2D
> canvases, but not for WebGL). The WebGL backing is in DrawingBuffer. In the
case
> of a WebGL canvas, the ImageBuffer is only used for storing presentation
copies
> for some indirect code paths (like printing).
> > > 
> > > The original patch#1 is functionally the same as the old pipeline so at
this
> > > point I'm back to thinking we should fix things that way.
> > > 
> > > If we want to be super clear about what's going on, I suggest adding a
> > > query-able mode for "recording for immediate playback on same thread".
> 
> My view: I just want to make sure we know how the non-SP code is working
> correctly. That way we know for sure whether a proposed fix here is fully
> correct.

The non-SP code works correctly because the printing paint pipeline paints
directly into a bitmap-backed SkCanvas, not a recording canvas. Thus all the
image copies inside Skia needed to read back the WebGL texture content happen
synchronously on the main Blink thread.

The assert is there to protect against an attempt to copy out of the WebGL
texture in a raster thread. Only printing uses the pipeline that tries to do the
bitmap copy, so only printing can hit this assert. The "!isRecording()" check in
the assert is effectively a proxy for isPrinting() in a world where we have
impl-side-painting everywhere.

The only thing that Slimming Paint changes is that each page is now recorded
into a recording canvas and then synchronously played back into a bitmap-backed
canvas on the same thread. We've added a level of indirection without
fundamentally changing how printing works. We haven't violated the condition the
assert is protecting against because we are synchronous, playback into a bitmap,
and are on main blink thread.

The assert should really be ASSERT(onBlinkMainThread() || !bitmap.getTexture())
but I don't think we have isOnBlinkMainThread() available to use. Instead we can
use ASSERT((printing() || !isRecording()) || !bitmap.getTexture()), where the
first bracketed clause captures isOnBlinkMainThread.

[chrishtr, 2015-05-22 18:40:56]

On 2015/05/22 at 18:36:53, schenney wrote:
> On 2015/05/22 18:13:49, chrishtr wrote:
> > On 2015/05/22 at 01:18:56, junov wrote:
> > > On 2015/05/21 16:45:48, Stephen Chenney wrote:
> > > > I tried the above but WebGL printing fails, doesn't crash, with a blank
> > canvas.
> > > > That makes sense to me; I can't see how WebGL content can produce
anything
> > > > without a 3D backing.
> > > 
> > > The ImageBuffer is not the backing for the WebGL canvas (it is for 2D
> > canvases, but not for WebGL). The WebGL backing is in DrawingBuffer. In the
case
> > of a WebGL canvas, the ImageBuffer is only used for storing presentation
copies
> > for some indirect code paths (like printing).
> > > > 
> > > > The original patch#1 is functionally the same as the old pipeline so at
this
> > > > point I'm back to thinking we should fix things that way.
> > > > 
> > > > If we want to be super clear about what's going on, I suggest adding a
> > > > query-able mode for "recording for immediate playback on same thread".
> > 
> > My view: I just want to make sure we know how the non-SP code is working
> > correctly. That way we know for sure whether a proposed fix here is fully
> > correct.
> 
> The non-SP code works correctly because the printing paint pipeline paints
directly into a bitmap-backed SkCanvas, not a recording canvas. Thus all the
image copies inside Skia needed to read back the WebGL texture content happen
synchronously on the main Blink thread.
> 
> The assert is there to protect against an attempt to copy out of the WebGL
texture in a raster thread. Only printing uses the pipeline that tries to do the
bitmap copy, so only printing can hit this assert. The "!isRecording()" check in
the assert is effectively a proxy for isPrinting() in a world where we have
impl-side-painting everywhere.
> 
> The only thing that Slimming Paint changes is that each page is now recorded
into a recording canvas and then synchronously played back into a bitmap-backed
canvas on the same thread. We've added a level of indirection without
fundamentally changing how printing works. We haven't violated the condition the
assert is protecting against because we are synchronous, playback into a bitmap,
and are on main blink thread.
> 
> The assert should really be ASSERT(onBlinkMainThread() ||
!bitmap.getTexture()) but I don't think we have isOnBlinkMainThread() available
to use. Instead we can use ASSERT((printing() || !isRecording()) ||
!bitmap.getTexture()), where the first bracketed clause captures
isOnBlinkMainThread.

Ok makes sense. I had missed some of the logical reasoning.

I support changing the ASSERT. I think you can revert the HTMLCanvasElement
change also.

[Stephen Chennney, 2015-05-22 21:21:05]

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

[Stephen Chennney, 2015-05-22 21:21:05]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1145843003/#ps40001
(title: "Agreed upon version")

[Stephen Chennney, 2015-05-22 21:21:29]

Agreed-upon fix up.

[commit-bot: I haz the power, 2015-05-22 21:21:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1145843003/40001

[Rik, 2015-05-22 21:21:58]

Hi,

I'm currently on sick leave. For urgent matters, please contact Jacob Goldstein
or Ethan
Malasky.

Thanks!
   Rik?

[chrishtr, 2015-05-22 21:21:59]

lgtm

[commit-bot: I haz the power, 2015-05-22 23:03:10]

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

[commit-bot: I haz the power, 2015-05-22 23:03:11]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2015-05-22 23:03:38]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-05-22 23:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1145843003/40001

[commit-bot: I haz the power, 2015-05-22 23:09:33]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195818