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 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 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.

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).

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

(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.

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

Thanks for the backout! Will check.

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.

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 :).

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 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.

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

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%.

(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.

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

Could this have caused bug 935541?

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.

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

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.

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