Canvas DrawImage performance 40 times slower on Azure Cairo compared to Thebes.

Comment on attachment 654074 [details] [diff] [review] Part 1: Remove xlib to buffered image conversion [v2] Review of attachment 654074 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +545,5 @@ > + surf.mSurface = aSurface->CairoSurface(); > + srcBuffer = aTarget->CreateSourceSurfaceFromNativeSurface(surf); > + > + // It's cheap enough to make a new one so we won't keep it around and > + // keeping it creates a cycle. Hrm, I'm not sure how I feel about this, but it's ok for now. I'm sure we can work around the cycle if we need to.

Try run for 6f1862f4a9e4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=6f1862f4a9e4 Results (out of 269 total builds): exception: 39 success: 65 warnings: 44 failure: 121 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6f1862f4a9e4

Try run for 08f39d31ccbb is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=08f39d31ccbb Results (out of 66 total builds): exception: 14 success: 5 failure: 47 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-08f39d31ccbb

Try run for 94f4e85b0cb1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=94f4e85b0cb1 Results (out of 264 total builds): exception: 12 success: 48 warnings: 23 failure: 181 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-94f4e85b0cb1

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #6) > Created attachment 654080 [details] [diff] [review] > Part 5: Avoid readback by teeing the drawing operations Do you have numbers for how much of a performance difference this makes?

[:jrmuizel] from comment #13) > Do you have numbers for how much of a performance difference this makes? AsteroidsBench without patch 1557 frames AsteroidsBench with patch 2118 frames

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #14) > [:jrmuizel] from comment #13) > > Do you have numbers for how much of a performance difference this makes? > > AsteroidsBench without patch 1557 frames > AsteroidsBench with patch 2118 frames What were the pre-azure numbers?

These were Azure numbers before and after the tee patch. On android the difference is 266 before the tee and 274 after the tee. That is for that individual patch.

Comment on attachment 654919 [details] [diff] [review] Part 8: Fixed empty shadows Review of attachment 654919 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +319,5 @@ > transform._32 -= mTempRect.y; > > mTarget = > mCtx->mTarget->CreateShadowDrawTarget(IntSize(int32_t(mTempRect.width), int32_t(mTempRect.height)), > + FORMAT_B8G8R8A8, mSigma); Indent here is still off it seems.

I must have got too comfortable using (In reply to Bas Schouten (:bas) from comment #18) > Indent here is still off it seems. I must have got too comfortable using a formatter :-)

Comment on attachment 654080 [details] [diff] [review] Part 5: Avoid readback by teeing the drawing operations Review of attachment 654080 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCairo.cpp @@ +290,5 @@ > if (mSurface) { > + // If we're using a tee surface then we need to destroy all the children > + if (cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_TEE) { > + for (int i = 0; ; i++) { > + cairo_surface_t* child = cairo_tee_surface_index(mSurface, i); Why do you need to do this? Normally a cairo_tee_surface keeps strong references to its children and releases them when it's destroyed. @@ +405,2 @@ > > + if (cairo_surface_get_type(blursurf) == CAIRO_SURFACE_TYPE_IMAGE) { This confusing. How could this be false? If it is false, then we won't draw the blurred shadow, which would be wrong. Maybe this should be an assertion instead? @@ +799,5 @@ > + aSize.width, > + aSize.height); > + > + if (cairo_surface_status(similar) || cairo_surface_status(blursurf)) { > + return nullptr; You should destroy similar/blursurf here, otherwise this could leak.

Comment on attachment 654082 [details] [diff] [review] Part 7: Keep unblurred patches in xlib Review of attachment 654082 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +727,5 @@ > virtual TemporaryRef<DrawTarget> > CreateSimilarDrawTarget(const IntSize &aSize, SurfaceFormat aFormat) const = 0; > > /* > * Create a draw target optimized for drawing a shadow. Note that aSigma is the blur radius that must be used when we draw the shadow. Also note that this doesn't affect the size of the allocated surface, the caller is still responsible for including the shadow area in its size.

Actually, you should fold part 9 in part 5 after all.

Try run for a5ee9b2d1a6e is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=a5ee9b2d1a6e Results (out of 18 total builds): failure: 18 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-a5ee9b2d1a6e

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fa6d8742ff https://hg.mozilla.org/integration/mozilla-inbound/rev/7402befeff1c https://hg.mozilla.org/integration/mozilla-inbound/rev/ddff99db5f7c https://hg.mozilla.org/integration/mozilla-inbound/rev/4041007e49cd https://hg.mozilla.org/integration/mozilla-inbound/rev/aec0729913fe https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2797ed0518 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac3a7df2b6a https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdfdf5ae236

Comment on attachment 655550 [details] [diff] [review] Part 9: Fixed failed assertion in reftest test_canvas.html Reviewing fix for broken code to land to avoid having to back everything out.

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f183cf4b38

Try run for f4c7e27c718e is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f4c7e27c718e Results (out of 265 total builds): exception: 38 success: 155 warnings: 27 failure: 45 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f4c7e27c718e

https://tbpl.mozilla.org/?tree=Try&rev=207b3e44752d The failing reftest in this try push is part of the yet-to-be-committed bug 784573.

Comment on attachment 655844 [details] [diff] [review] Part 4: Removed double padding with AlphaBoxBlur v2 Changed int8_t aStride to int32_t aStride.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #35) > https://tbpl.mozilla.org/?tree=Try&rev=207b3e44752d Taking your word for it about the reftest. Green on Try. https://hg.mozilla.org/integration/mozilla-inbound/rev/503884273a3a https://hg.mozilla.org/integration/mozilla-inbound/rev/dc33092d9cd8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1099c9f0962d https://hg.mozilla.org/integration/mozilla-inbound/rev/682a61f34c81 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc26333c7525 https://hg.mozilla.org/integration/mozilla-inbound/rev/e823337a4f78 https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa7cdede09c

Backed out again for OSX bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a7f7115ec6

Needs a check for aTarget->GetType() == BACKEND_CAIRO

Autoland Failure Specified patches [7, 1, 2, 6, 4, 3, 5] do not exist, or are not posted to this bug.

fwiw, autoland doesn't work any more :( http://lukasblakk.com/why-isnt-autoland-working/

Try run for ba77974558ac is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ba77974558ac Results (out of 258 total builds): exception: 2 success: 232 warnings: 23 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-ba77974558ac

Part 1 hasn't received r+ from Bas yet.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b4498ed59b https://hg.mozilla.org/integration/mozilla-inbound/rev/e7878a522d8b https://hg.mozilla.org/integration/mozilla-inbound/rev/208e1a753a8b https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ab8d4a18fe https://hg.mozilla.org/integration/mozilla-inbound/rev/79df91a7ff8c https://hg.mozilla.org/integration/mozilla-inbound/rev/07c5d694b7f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c66c3997381

https://hg.mozilla.org/mozilla-central/rev/c4b4498ed59b https://hg.mozilla.org/mozilla-central/rev/e7878a522d8b https://hg.mozilla.org/mozilla-central/rev/208e1a753a8b https://hg.mozilla.org/mozilla-central/rev/b3ab8d4a18fe https://hg.mozilla.org/mozilla-central/rev/79df91a7ff8c https://hg.mozilla.org/mozilla-central/rev/07c5d694b7f3 https://hg.mozilla.org/mozilla-central/rev/6c66c3997381

Autoland Patchset: Patches: 656653, 654075, 654076, 655844, 655494, 654081, 655495 Branch: mozilla-central => try Patch 656653 could not be applied to mozilla-central. patching file gfx/thebes/gfxPlatform.cpp Hunk #1 FAILED at 529 1 out of 1 hunks FAILED -- saving rejects to file gfx/thebes/gfxPlatform.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir Patchset could not be applied and pushed.