Closed
Bug 996929
Opened 10 years ago
Closed 10 years ago
Windows XP non-PGO CanvasMark regression on inbound from bug 950372
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jwatt, Assigned: mattwoodrow)
References
Details
Attachments
(2 files)
1.14 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
5.80 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
Regression: Mozilla-Inbound-Non-PGO - CanvasMark - WINNT 5.1 (ix) - 24% decrease -------------------------------------------------------------------------------- Previous: avg 6980.042 stddev 28.433 of 12 runs up to revision e134b3aa718f New : avg 5303.083 stddev 728.320 of 12 runs since revision a54345bfd338 Change : -1676.958 (24% / z=58.979) Graph : http://mzl.la/1p8bOPT
Reporter | ||
Comment 1•10 years ago
|
||
From IRC: mattwoodrow GetSourceSurfaceForSurface(aTarget, ..) was in SFE, now it’s GetSourceSurfaceForSurface(nullptr) mattwoodrow in RasterImage mattwoodrow and it’s on XP, so the canvas backend isn’t the same as the content backend mattwoodrow I think calling OptimizeSourceSurface in SurfaceFromElement would fix the regression mattwoodrow the problem is that we’re returning a SourceSurfaceCairo that wraps a gdi surface, and we put that into the canvas image cache mattwoodrow then every time we try draw with skia we call GetDataSurface() and that makes a copy mattwoodrow repeat for every draw of that image mattwoodrow OptimizeSourceSurface will do the same, but then we put that version into the cache
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8407213 -
Flags: review?(jwatt)
Reporter | ||
Updated•10 years ago
|
Assignee: jwatt → matt.woodrow
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8407213 [details] [diff] [review] Optimize the source surface Can you make this: // The surface we return is likely to be cached. We don't want to have to // convert to a surface that's compatible with aTarget each time it's used // (that would result in bad performance), so we convert once here upfront. RefPtr<SourceSurface> optSurface = aTarget->OptimizeSourceSurface(result.mSourceSurface); if (optSurface) { result.mSourceSurface = optSurface; } At least add the comment.
Attachment #8407213 -
Flags: review?(jwatt) → review+
Reporter | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31cf0a9ae9b9
Assignee | ||
Comment 5•10 years ago
|
||
Well, it would work that way, if OptimizeSourceSurface was actually implemented.
Whiteboard: [leave open]
Assignee | ||
Comment 6•10 years ago
|
||
This should actually fix the regression.
Attachment #8407301 -
Flags: review?(bas)
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/31cf0a9ae9b9
Comment 8•10 years ago
|
||
Comment on attachment 8407301 [details] [diff] [review] fix-optimize-source-surface Review of attachment 8407301 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetD2D.cpp @@ +1196,5 @@ > + } > + > + RefPtr<SourceSurfaceD2D> newSurf = new SourceSurfaceD2D(); > + > + if (!newSurf->InitFromData(map.mData, data->GetSize(), map.mStride, data->GetFormat(), mRT)) { nit: I'd prefer: bool success = newSurf->InitFromData(map.mData, data->GetSize(), map.mStride, data->GetFormat(), mRT); data->Unmap(); if (!success) { return nullptr; } return newSurf; ::: gfx/2d/DrawTargetD2D1.cpp @@ +936,5 @@ > + } > + > + RefPtr<ID2D1Bitmap1> bitmap; > + HRESULT hr = mDC->CreateBitmap(D2DIntSize(data->GetSize()), map.mData, map.mStride, > + D2D1::BitmapProperties1(D2D1_BITMAP_OPTIONS_NONE, D2DPixelFormat(data->GetFormat())), nit: whitespace
Attachment #8407301 -
Flags: review?(bas) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/496a5b50e7a9
Comment 12•10 years ago
|
||
very nice!
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/496a5b50e7a9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•