933030 - eliminate thebes use in CanvasRenderingContext2D.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Andreas Gal :gal]

Attachment: GetImageBuffer with Moz2D

Comment 2

[Andreas Gal :gal]

Comment on attachment 825242 [details] [diff] [review]
GetImageBuffer with Moz2D

The whole HOSTARGB business is really fishy. Please take a close look at BGR vs HOSTARGB.

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 825242 [details] [diff] [review]
GetImageBuffer with Moz2D

I'm not a Graphics module peer, and after looking into this patch for a bit I'm not comfortable with r+'ing anyway since there are some things I'm not sure about. Sorry. I've preemptively switched the review request to roc since from the hg log of the file he looks to have done the most recent relevant reviews, but feel free to switch it to someone else on the Graphics owners/peers list if you prefer:

https://wiki.mozilla.org/Modules/Core#Graphics

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 825242 [details] [diff] [review]
GetImageBuffer with Moz2D

Review of attachment 825242 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +1076,5 @@
> +    Factory::CreateDrawTargetForData(mTarget->GetType(),
> +                                     imageBuffer,
> +                                     IntSize(mWidth, mHeight),
> +                                     mWidth * 4,
> +                                     FORMAT_B8G8R8A8);

This is correct according to gfx2DGlue::ImageFormatToSurfaceFormat.

@@ +1085,5 @@
>  
> +  IntSize size = snapshot->GetSize();
> +  dt->DrawSurface(snapshot,
> +                  mgfx::Rect(0, 0, mWidth, mHeight),
> +                  mgfx::Rect(0, 0, size.width, size.height));

This should use OP_SOURCE for efficiency.

Comment 5

[Andreas Gal :gal]

roc, I am actually not so sure any more whether this is in general the best approach.

Right now I do the following:

1. Wrap a draw target around CPU memory.
2. Draw and possibly scale onto it. The backend may or may not fall back onto a software path here, since its going into CPU memory. A smart backend might render onto a texture and then Flush() forces a readback. This is left to the backend.

Maybe what I should do is:

1. Make a draw target that matches mTarget (GetOptimizedDrawTarget).
2. Draw/scale onto it.
3. Snapshot(), and then get the data surface.
4. Copy that.

For a software backend this adds an extra copy. Ideally, nothing should use a software backend in the future. On the upside for a GPU backend this should be much faster since scaling happens on the GPU.

What do you think? (this is a slow path, it doesn't matter much here, but I think its worth having this discussion).

Comment 6

[Andreas Gal :gal]

I guess mWidth/mHeight == snapshot->GetSize() so we never scale here. I _think_. I will stick to the current code.

Comment 7

[Andreas Gal :gal]

Attachment: Use Moz2D in Canvas2D GetImageBuffer (r=roc).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #6)
> I guess mWidth/mHeight == snapshot->GetSize() so we never scale here. I
> _think_. I will stick to the current code.

Yes, there normally wouldn't be scaling going on here. Also, this is only used for image encoding (toDataURL etc) so performance isn't too big of an issue; minimizing memory usage is probably more important.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df0f69fc244f

Comment 10

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/6ef2b64d9a0a - dunno what's going to happen elsewhere (you were on top of build bustage, and most other platforms that are building haven't run tests), but Android says a whole lot of https://tbpl.mozilla.org/php/getParsedLog.php?id=29947397&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29947341&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29947596&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29947447&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29947630&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=29947625&tree=Mozilla-Inbound

Comment 11

[Andreas Gal :gal]

Thanks for the backout! Will check.

Comment 12

[Matt Woodrow (:mattwoodrow)]

CreateDrawTargetForData only works (and can only work) for some backends. So that is failing, and you're returning early where it worked previously.

What I think you should do is:

* Call GetDataSurface() on the snapshot to get access to the pixel data or read is back.
* Assert that this is BGRA32 or BGR24 formatted (I'm pretty sure this is true for all canvases).
* Allocate the memory for the destination
* Memcpy from the snapshot to the destination, maybe taking into account differences in stride and setting the alpha channel to 255 if the source is BGR24.

Comment 13

[Andreas Gal :gal]

Attachment: Use Moz2D in Canvas2D GetImageBuffer.

Comment 14

[Andreas Gal :gal]

https://tbpl.mozilla.org/?tree=Try&rev=17ad690d0d45

Comment 15

[Andreas Gal :gal]

Attachment: patch ready for check-in

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b66c7ae1aa6

Comment 17

[Bas Schouten (:bas.schouten)]

Please avoid csets that touch both gfx/2d and other files in a single cset in the future, they make my life harder when merging :).

Comment 18

[Bas Schouten (:bas.schouten)]

Also it seems an inclusion of mozalloc slipped in here for fallible_t, mozalloc is not part of MFBT so we can't include it, I'll create a patch to fix this.

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Remove external dependency added to Moz2D.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 826519 [details] [diff] [review]
Remove external dependency added to Moz2D.

Review of attachment 826519 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, sorry, should have caught that.

Comment 21

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8783dc5333

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b66c7ae1aa6
https://hg.mozilla.org/mozilla-central/rev/6f8783dc5333

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

this patch appears to have caused a regression in android tp4m shutdown times:
android 2.2:
http://graphs.mozilla.org/graph.html#tests=[[87,63,20]]&sel=1383423553000,1383596353000&displayrange=7&datatype=running
android 4.0.4:
http://graphs.mozilla.org/graph.html#tests=[[87,63,29]]&sel=1383424130000,1383596930000&displayrange=7&datatype=running

I am not sure why, but both platforms show a fairly stable reporting value before and after this change, this change just caused the total value to go up about 14%.

Comment 24

[Geoff Brown [:gbrown] (pto Apr 11-18)]

(In reply to Joel Maher (:jmaher) from comment #23)
> this patch appears to have caused a regression in android tp4m shutdown times:

I'm pretty sure this is the annual bug 809190 -- Android tp4m shutdown is dependent on the DST time change.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

oh, that actually lines up well with the data, sorry for the confusion in this bug.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Could this have caused bug 935541?

Comment 27

[Bas Schouten (:bas.schouten)]

It seems extremely unlikely to me (at least my patch wont have caused it, but I also doubt the others did). But others might have a better idea.

Comment 28

[Matt Woodrow (:mattwoodrow)]

No, I don't think these are related at all.

Comment 29

[Andreas Gal :gal]

We might be using skiaGL where we used software before, increasing texture memory pressure, causing us to die in the pbuffer allocation. Just guessing here.

Comment 30

[Cameron Kaiser [:spectre]]

This breaks thumbnails on big-endian platforms (please see bug 998749).