Last Comment Bug 612335 - Read back WebGL canvases directly into shared surface
: Read back WebGL canvases directly into shared surface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 619056
  Show dependency treegraph
 
Reported: 2010-11-15 11:31 PST by Oleg Romashin (:romaxa)
Modified: 2011-06-10 07:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ReadPixels directly to mBackBuffer (4.24 KB, patch)
2010-11-15 11:31 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Right patch (4.33 KB, patch)
2010-11-15 11:48 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Add y-flip for remote sw canvas (672 bytes, patch)
2010-11-15 12:43 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated patch (4.33 KB, patch)
2011-01-08 01:01 PST, Oleg Romashin (:romaxa)
cjones.bugs: review-
Details | Diff | Splinter Review
Optimize remote canvas rendering into shared surface (14.84 KB, patch)
2011-05-30 18:36 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Optimize remote canvas rendering into shared surface (15.04 KB, patch)
2011-05-31 00:15 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review-
Details | Diff | Splinter Review
Fixed comments (12.17 KB, patch)
2011-06-06 13:42 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
Fixed normal canvas rendering (mDirty issue) (12.21 KB, patch)
2011-06-09 11:26 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
interdiff from previous r+ patch (828 bytes, patch)
2011-06-09 11:26 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2010-11-15 11:31:15 PST
Created attachment 490623 [details] [diff] [review]
ReadPixels directly to mBackBuffer

I think we can create overloaded ::Updated method for BasicShadowableCanvasLayer and make it rendering with -1 composite
Comment 1 Oleg Romashin (:romaxa) 2010-11-15 11:48:20 PST
Created attachment 490636 [details] [diff] [review]
Right patch

oh, previous patch was a bit hacky and crashy...

Also we still need to do y-flip
Comment 2 Oleg Romashin (:romaxa) 2010-11-15 12:43:57 PST
Created attachment 490656 [details] [diff] [review]
Add y-flip for remote sw canvas
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-15 14:23:31 PST
Comment on attachment 490656 [details] [diff] [review]
Add y-flip for remote sw canvas

Don't we only want to y-flip for webgl canvases?  Remember that this code is used for both 2d and 3d.
Comment 4 Doug Turner (:dougt) 2011-01-07 11:28:12 PST
can you detect that case and only flip y for the 3d case?
Comment 5 Oleg Romashin (:romaxa) 2011-01-08 00:57:45 PST
Comment on attachment 490656 [details] [diff] [review]
Add y-flip for remote sw canvas

This patch is obsolete, and right patch about canvas y-flip is in bug 606218
Comment 6 Oleg Romashin (:romaxa) 2011-01-08 01:01:52 PST
Created attachment 502206 [details] [diff] [review]
Updated patch

I know nobody care about canvas webgl rendering right now, because in long future we are going to move WegGL context to Chrome, and do IPC calls for each webgl command... anyway here is the patch which allow us cutoff one memcpy in remote fennec
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-01-10 15:44:50 PST
Comment on attachment 502206 [details] [diff] [review]
Updated patch

We don't want to duplicate all this code; it should be easy to refactor BasicCanvasLayer::Updated to avoid duplication.

Please request review on v2 from :vlad, since judging by bug 606218 comment 6 he might not want this patch yet.
Comment 8 Oleg Romashin (:romaxa) 2011-05-30 18:36:56 PDT
Created attachment 536205 [details] [diff] [review]
Optimize remote canvas rendering into shared surface
Comment 9 Oleg Romashin (:romaxa) 2011-05-30 18:39:48 PDT
Comment on attachment 536205 [details] [diff] [review]
Optimize remote canvas rendering into shared surface

Canvas YFlip moved to Chrome process, that should not be a problem because it just different composite direction in SW mode, and GPU does it for free.
Comment 10 Oleg Romashin (:romaxa) 2011-05-30 20:12:10 PDT
With simple top command I can see that this patch drop plugin-container CPU usage from 30%->20%
Comment 11 Oleg Romashin (:romaxa) 2011-05-31 00:15:08 PDT
Created attachment 536243 [details] [diff] [review]
Optimize remote canvas rendering into shared surface

Ups, previous version was breaking normal canvas2D which was stored in mSurface, this version fixing that problem
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-06 09:55:37 PDT
Comment on attachment 536243 [details] [diff] [review]
Optimize remote canvas rendering into shared surface

> void
> BasicShadowableCanvasLayer::Paint(gfxContext* aContext)
> {
>-  BasicCanvasLayer::Paint(aContext);
>   if (!HasShadow())
>     return;

This will completely break same-process <canvas> rendering ...

>-  // It'd be nice to draw directly into the shmem back buffer.
>-  // Doing so is complex -- for 2D canvases, we'd need to copy
>-  // changed areas, much like we do for Thebes layers, as well as
>-  // do all sorts of magic to swap out the surface underneath the
>-  // canvas' thebes/cairo context.

This comment is still valid, but I don't think it's really
helping anyone.

>+  if (!backSurface || backSurface->GetType() != gfxASurface::SurfaceTypeImage) {

Probably better to drop the ImageSurface check and let
UpdateSurface() deal.  We'll soon be sharing X surfaces, e.g.
I'm fine fixing this in a followup if you file it.

r- for breaking same-process rendering.
Comment 13 Oleg Romashin (:romaxa) 2011-06-06 10:52:45 PDT
> > void
> > BasicShadowableCanvasLayer::Paint(gfxContext* aContext)
> > {
> >-  BasicCanvasLayer::Paint(aContext);
> >   if (!HasShadow())
> >     return;
> 
> This will completely break same-process <canvas> rendering ...
err... how? BasicShadowableCanvasLayer::Paint - shadowable is only for cross process rendering isn't it?
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-06 11:13:32 PDT
In MOZ_IPC and now all builds since MOZ_IPC is gone, all layers are Shadowable by default, except for those used by temporary layer managers.  With same-process rendering, Shadowable layers just don't have shadows, the |!HasShadow()| there.
Comment 15 Oleg Romashin (:romaxa) 2011-06-06 13:42:23 PDT
Created attachment 537626 [details] [diff] [review]
Fixed comments
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 15:02:09 PDT
Comment on attachment 537626 [details] [diff] [review]
Fixed comments

I think this is OK.  Please be sure to run through tryserver.
Comment 17 Oleg Romashin (:romaxa) 2011-06-09 11:26:28 PDT
Created attachment 538299 [details] [diff] [review]
Fixed normal canvas rendering (mDirty issue)

Ok, I found android canvas reftests failing because mDirty is always false.
Comment 18 Oleg Romashin (:romaxa) 2011-06-09 11:26:57 PDT
Created attachment 538300 [details] [diff] [review]
interdiff from previous r+ patch
Comment 19 Oleg Romashin (:romaxa) 2011-06-09 13:45:36 PDT
Try build looks good with last patch
http://tbpl.mozilla.org/?tree=Try&rev=620d9b977955
Comment 20 Oleg Romashin (:romaxa) 2011-06-10 07:36:17 PDT
http://hg.mozilla.org/mozilla-central/rev/9f18296db571

Note You need to log in before you can comment on or make changes to this bug.