gfx.canvas.azure.backends -> skia gfx.canvas.azure.accelerated -> true e10s -> on Results in blank 2d canvas, e.g. the graphs on https://areweslimyet.com/

I found similar bug (not enable e10s in my case) while work on bug 1001069, and find the solution for non-e10s case. For non-e10s case, we get a BasicCanvasLayer while call aManager->CreateCanvasLayer in CanvasRenderingContext2D::GetCanvasLayer(), but the related buffer copy code in CopyableCanvasLayer(shared both e10s and non e10s case) does not take BasicCanvasLayer case into consideration and make it blank. This patch handles the buffer copy for both condition in a naive way. I think I can investigate it further to check e10s case, too.

Comment on attachment 8493577 [details] [diff] [review] V1 OK, I tried e10s window and my patch works well, too. I haven't check whether e10s window goes to BasicLayerManager, yet. However, if we switch to BasicLayerManager while e10s enabled, it should be another bug, so let's file another bug if it really happens

Fix WebGL canvas

I'm a bit confused as to what exactly this patch is doing. Can you explain it for me?

OK, the problem is SkiaGL canvas uses a SurfaceStream to update screen, but BasicCanvasLayer needs a mDrawTarget/mSurface to update screen, so this patch send DrawTarget into BasicCanvasLayer basically. Another possibility is to handle stream->CopySurfaceToProducer and stream->SwapProducer (they were handled in CanvasClient, but BasicLayers do not use CompositableClients) somewhere around CopyableCanvasLayer::UpdateTarget or BasicCanvasLayer::Paint or CanvasRenderContext2D::PreTransactionCallback. The need of copy is because we do not render into the SurfaceStream directly. We render into a texture and then copy the content to the Stream. Because 2d Canvas needs retain content. (Other possibility to handle the copy is to make SurfaceStream handles buffer blit internally based on configuration, but that is another problem) As sending DrawTarget into BasicCanvasLayer seems safe and easy. My patch fix it that way :) If you prefer any other solution, please feel free to let me know. If you think I should request :nical to review this part, please forward the review request to him. Thanks!

I'm worried that there are places where we do: if (mDrawTarget) { } else if (mGLContext) { } which will no longer trigger. snorp may know off-hand, but I will take a closer look tomorrow.

I am not totally sure about this. It seems like handling stuff in BasicCanvasLayer::Paint() (and more specifically, CopyableCanvasLayer::UpdateTarget()) is the way to go. In fact, I think calling UpdateTarget(aDT) in BasicCanvasLayer::Paint() should make things Just Work?

I think nical is the only person who can keep this layer stuff straight these days :)

I'll look at it, but I don't know if it makes a lot of sense to render the canvas on the GPU and then read it back for software compositing. Perhaps we should just disable accelerated canvas when using basic layers. We disable d2d when we have software compositing on windows which is sort of equivalent.

(In reply to Nicolas Silva [:nical] from comment #9) > I'll look at it, but I don't know if it makes a lot of sense to render the > canvas on the GPU and then read it back for software compositing. Perhaps we > should just disable accelerated canvas when using basic layers. We disable > d2d when we have software compositing on windows which is sort of equivalent. Oh it's absolutely not ideal, I agree. I think the way forward in general would be to use ClientLayerManager on Linux e10s. Shouldn't that already work?

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10) > (In reply to Nicolas Silva [:nical] from comment #9) > Oh it's absolutely not ideal, I agree. I think the way forward in general > would be to use ClientLayerManager on Linux e10s. Shouldn't that already > work? I think that it already works. In fact I don't think you can get anything to the compositor without a ClientLayerManager when e10s (or even just OMTC) is enabled.

Soooo...how do we hit this bug with BLM then? Is it just when creating page thumbnails?

Sorry, I was not careful enough reducing this: This does work with basic layers, it's when acceleration is on that canvas is blank.

(In reply to John Schoenick [:johns] from comment #13) > Sorry, I was not careful enough reducing this: This does work with basic > layers, it's when acceleration is on that canvas is blank. What do you mean by "acceleration" in this case? GL compositor?

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #14) > (In reply to John Schoenick [:johns] from comment #13) > > Sorry, I was not careful enough reducing this: This does work with basic > > layers, it's when acceleration is on that canvas is blank. > > What do you mean by "acceleration" in this case? GL compositor? layers.acceleration.force-enabled

OK. George can you figure out what's going on here? I wager that SurfaceStream just doesn't work cross-process on Linux. We should either fallback to the readback path there or fix it to use a XPixmap or whatever the kids are doing these days.

Comment on attachment 8494281 [details] [diff] [review] V2 Review of attachment 8494281 [details] [diff] [review]: ----------------------------------------------------------------- r- for now based on my and snorp's comments

I think comment 9 is a acceptable solution, however, the skiaGL backend is faster in many cases. And in my local build, my 2d canvas with skiaGL is blank even if OMTC off.(default) That's why it goes to BasicLayers I think. BTW, if SurfaceStream has problem, webGL canvas should be problematic, too. So let's figure out what combination is expected, and decide how to fix all of them :)

(In reply to Chiajung Hung [:chiajung] from comment #18) > I think comment 9 is a acceptable solution, however, the skiaGL backend is > faster in many cases. And in my local build, my 2d canvas with skiaGL is > blank even if OMTC off.(default) That's why it goes to BasicLayers I think. > > BTW, if SurfaceStream has problem, webGL canvas should be problematic, too. > > So let's figure out what combination is expected, and decide how to fix all > of them :) It appears this has been fixed as a side-effect of source code revisions 209892 and 209893 (bugzilla 1066280). Can you verify?

(In reply to Lee Salzman from comment #19) > (In reply to Chiajung Hung [:chiajung] from comment #18) > > I think comment 9 is a acceptable solution, however, the skiaGL backend is > > faster in many cases. And in my local build, my 2d canvas with skiaGL is > > blank even if OMTC off.(default) That's why it goes to BasicLayers I think. > > > > BTW, if SurfaceStream has problem, webGL canvas should be problematic, too. > > > > So let's figure out what combination is expected, and decide how to fix all > > of them :) > > It appears this has been fixed as a side-effect of source code revisions > 209892 and 209893 (bugzilla 1066280). Can you verify? Oops, better info on source revisions: changeset: 209892:72ce4c2ccdcb user: jdashg <jdashg+github@gmail.com> date: Tue Oct 07 21:15:39 2014 -0700 summary: Bug 1066280 - Remove SurfaceStream. - r=kamidphish,mattwoodrow changeset: 209893:49531b0af9b8 user: jdashg <jdashg+github@gmail.com> date: Tue Oct 07 21:16:14 2014 -0700 summary: Bug 1066280 - Fixes for SurfaceStream removal. - r=kamidphish,mattwoodrow

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #14) > (In reply to John Schoenick [:johns] from comment #13) > > Sorry, I was not careful enough reducing this: This does work with basic > > layers, it's when acceleration is on that canvas is blank. > > What do you mean by "acceleration" in this case? GL compositor? Can you verify if this has been fixed?

I can confirm that canvases appear to work fine now with the combination mentioned in c0

Okay, we'll just close this out as having been fixed by those ShSurf changes I mentioned above.