Read back WebGL canvases directly into shared surface

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

7 years ago
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
Attachment #490623 - Flags: review?(jones.chris.g)
(Assignee)

Comment 1

7 years ago
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
Attachment #490623 - Attachment is obsolete: true
Attachment #490623 - Flags: review?(jones.chris.g)
(Assignee)

Comment 2

7 years ago
Created attachment 490656 [details] [diff] [review]
Add y-flip for remote sw canvas
Attachment #490656 - Flags: review?(jones.chris.g)
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.
Attachment #490656 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Blocks: 619056

Comment 4

6 years ago
can you detect that case and only flip y for the 3d case?
(Assignee)

Comment 5

6 years ago
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
Attachment #490656 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
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
Assignee: nobody → romaxa
Attachment #490636 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #502206 - Flags: review?(jones.chris.g)
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.
Attachment #502206 - Flags: review?(jones.chris.g) → review-
(Assignee)

Updated

6 years ago
Summary: Remove one memcopy from remote canvas rendering pipeline → Render remote canvas directly into shared surface
(Assignee)

Comment 8

6 years ago
Created attachment 536205 [details] [diff] [review]
Optimize remote canvas rendering into shared surface
Attachment #502206 - Attachment is obsolete: true
Attachment #536205 - Flags: review?(bjacob)
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #536205 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

6 years ago
With simple top command I can see that this patch drop plugin-container CPU usage from 30%->20%
(Assignee)

Comment 11

6 years ago
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
Attachment #536205 - Attachment is obsolete: true
Attachment #536243 - Flags: review?(bjacob)
Attachment #536205 - Flags: review?(jones.chris.g)
Attachment #536205 - Flags: review?(bjacob)
(Assignee)

Updated

6 years ago
Attachment #536243 - Flags: review?(jones.chris.g)
Summary: Render remote canvas directly into shared surface → Read back WebGL canvases directly into shared surface
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.
Attachment #536243 - Flags: review?(jones.chris.g) → review-
(Assignee)

Comment 13

6 years ago
> > 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?
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.
(Assignee)

Comment 15

6 years ago
Created attachment 537626 [details] [diff] [review]
Fixed comments
Attachment #536243 - Attachment is obsolete: true
Attachment #537626 - Flags: review?(jones.chris.g)
Attachment #536243 - Flags: review?(bjacob)
Comment on attachment 537626 [details] [diff] [review]
Fixed comments

I think this is OK.  Please be sure to run through tryserver.
Attachment #537626 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 538299 [details] [diff] [review]
Fixed normal canvas rendering (mDirty issue)

Ok, I found android canvas reftests failing because mDirty is always false.
Attachment #538299 - Flags: review?(jones.chris.g)
(Assignee)

Comment 18

6 years ago
Created attachment 538300 [details] [diff] [review]
interdiff from previous r+ patch
Attachment #538299 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 19

6 years ago
Try build looks good with last patch
http://tbpl.mozilla.org/?tree=Try&rev=620d9b977955
(Assignee)

Comment 20

6 years ago
http://hg.mozilla.org/mozilla-central/rev/9f18296db571
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.