Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14322017:
Revert "cc: Move canvas clear from picture to picture_pile_impl" (Closed)
|
Created:
7 years, 7 months ago by vmpstr Modified:
7 years, 7 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRevert "cc: Move canvas clear from picture to picture_pile_impl
This moves a clear per picture into a clear-to-background color
per picture pile. We need this to ensure that a solid color
page in low res is considered solid, since all the draws are
of the same color.
BUG=233622
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198272
"
TBR=reveman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198826
Patch Set 1 #
Total comments: 1
Patch Set 2 : review #
Total comments: 7
Patch Set 3 : tomhudson's review #Patch Set 4 : #Patch Set 5 : undo unittest change #Patch Set 6 : unittest compile fix #Patch Set 7 : revert #
Messages
Total messages: 37 (0 generated)
Please take a look.
lgtm https://codereview.chromium.org/14322017/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14322017/diff/1/cc/resources/picture.cc#newco... cc/resources/picture.cc:170: SkRect layer_skrect = SkRect::MakeXYWH(layer_rect_.x(), While you're here, can you change this to RectToSkRect(layer_rect)?
+junov for skia/ext owner's review
Driveby OWNER: does this increase total clear() workload at all? That's already a bit of a bottleneck when we look at Skia debugger analyses. https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture.cc#ne... cc/resources/picture.cc:170: canvas->clipRect(RectToSkRect(layer_rect_)); Is this clip actually necessary, since we've created the canvas to be exactly layer_rect size and offset? https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... cc/resources/picture_pile_impl.cc:107: canvas->drawRect(RectToSkRect(inflated_content_rect), background_paint); canvas->clear(background_color_) may be more efficient, or equally efficient and more legible? (2 forms of efficiency: (1) it used to be strictly more performant; we've tried to fix that; (2) it keeps the picture simpler, with fewer unique paints.) https://codereview.chromium.org/14322017/diff/5001/skia/ext/analysis_canvas.cc File skia/ext/analysis_canvas.cc (right): https://codereview.chromium.org/14322017/diff/5001/skia/ext/analysis_canvas.c... skia/ext/analysis_canvas.cc:225: // the tile remains solid. This could be a different CL? But nice catch! Does it show up in practice?
PTAL https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture.cc#ne... cc/resources/picture.cc:170: canvas->clipRect(RectToSkRect(layer_rect_)); On 2013/04/30 09:43:59, Tom Hudson wrote: > Is this clip actually necessary, since we've created the canvas to be exactly > layer_rect size and offset? I had it there initially, since I wasn't sure. But it makes that we don't need it, I removed it. https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... cc/resources/picture_pile_impl.cc:107: canvas->drawRect(RectToSkRect(inflated_content_rect), background_paint); On 2013/04/30 09:43:59, Tom Hudson wrote: > canvas->clear(background_color_) may be more efficient, or equally efficient and > more legible? > > (2 forms of efficiency: (1) it used to be strictly more performant; we've tried > to fix that; (2) it keeps the picture simpler, with fewer unique paints.) Fixed. https://codereview.chromium.org/14322017/diff/5001/skia/ext/analysis_canvas.cc File skia/ext/analysis_canvas.cc (right): https://codereview.chromium.org/14322017/diff/5001/skia/ext/analysis_canvas.c... skia/ext/analysis_canvas.cc:225: // the tile remains solid. On 2013/04/30 09:43:59, Tom Hudson wrote: > This could be a different CL? But nice catch! Does it show up in practice? This shows up on low res tiles. However, with the changes in picture/picture_pile_impl this might be not necessary for that case. It's good to keep though, I think :) The reason it's in the same CL is that it at least _was_ required to fix the bug in description. I'll have to see if it is still required.
https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... cc/resources/picture_pile_impl.cc:107: canvas->drawRect(RectToSkRect(inflated_content_rect), background_paint); On 2013/04/30 17:06:23, vmpstr wrote: > On 2013/04/30 09:43:59, Tom Hudson wrote: > > canvas->clear(background_color_) may be more efficient, or equally efficient > and > > more legible? > > > > (2 forms of efficiency: (1) it used to be strictly more performant; we've > tried > > to fix that; (2) it keeps the picture simpler, with fewer unique paints.) > > Fixed. Sometimes we are only rastering a sub-rectangle of a texture. If you do an unclipped clear here, this will likely end up rastering unneeded pixels. (For hardware the clear's faster, but maybe not for software?) If you're going to change drawRect to a clear, should it not move after the clipRect?
On 2013/04/30 17:13:52, enne wrote: > https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... > File cc/resources/picture_pile_impl.cc (right): > > https://codereview.chromium.org/14322017/diff/5001/cc/resources/picture_pile_... > cc/resources/picture_pile_impl.cc:107: > canvas->drawRect(RectToSkRect(inflated_content_rect), background_paint); > On 2013/04/30 17:06:23, vmpstr wrote: > > On 2013/04/30 09:43:59, Tom Hudson wrote: > > > canvas->clear(background_color_) may be more efficient, or equally efficient > > and > > > more legible? > > > > > > (2 forms of efficiency: (1) it used to be strictly more performant; we've > > tried > > > to fix that; (2) it keeps the picture simpler, with fewer unique paints.) > > > > Fixed. > > Sometimes we are only rastering a sub-rectangle of a texture. If you do an > unclipped clear here, this will likely end up rastering unneeded pixels. (For > hardware the clear's faster, but maybe not for software?) If you're going to > change drawRect to a clear, should it not move after the clipRect? According to skia docs, the clear doesn't care about clip rect. Here's the comment on the clear function in SkCanvas: """ This erases the entire drawing surface to the specified color, irrespective of the clip. It does not blend with the previous pixels, but always overwrites them. It is roughly equivalent to the following: canvas.save(); canvas.clipRect(hugeRect, kReplace_Op); paint.setColor(color); paint.setXfermodeMode(kSrc_Mode); canvas.drawPaint(paint); canvas.restore(); though it is almost always much more efficient. """ I think the "roughly equivalent part", is what I replaced (more or less).
Ok, sure. I'm really asking to be convinced that changing a drawRect(subrect) to clear(larger rect) is more efficient.
On 2013/04/30 17:24:42, enne wrote: > Ok, sure. I'm really asking to be convinced that changing a drawRect(subrect) > to clear(larger rect) is more efficient. From tracing on a simple page, it seems to fairly equivalent. I can't really say the same for any page. In the pre-patch version, we expect that all pixels in the tile will be painted, and it's done with draw layer rect in picture + zero or more draws in picture pile impl. Here, the clear will paint at most each pixel in the canvas. In patch #1 vs #3, the rect is scaled tiling total size. If the tiling is meant to cover the whole canvas, wouldn't that be equivalent?
On 2013/04/30 17:58:47, vmpstr wrote: > On 2013/04/30 17:24:42, enne wrote: > > Ok, sure. I'm really asking to be convinced that changing a drawRect(subrect) > > to clear(larger rect) is more efficient. > > From tracing on a simple page, it seems to fairly equivalent. I can't really say > the same for any page. > > In the pre-patch version, we expect that all pixels in the tile will be painted, > and it's done with draw layer rect in picture + zero or more draws in picture > pile impl. Here, the clear will paint at most each pixel in the canvas. > > In patch #1 vs #3, the rect is scaled tiling total size. If the tiling is meant > to cover the whole canvas, wouldn't that be equivalent? The content rect does not always cover the whole SkCanvas during playback.
> The content rect does not always cover the whole SkCanvas during playback. My understanding of this was wrong. It turns out that sometimes the rect we draw can be smaller than the canvas, in which case clear might do more work than necessary. I've changed the code to rect for the time being. Tom, do you know if clear is significantly faster than rect? I mean, if we clear the canvas, vs draw a rect half the canvas size (for instance), would clear still be faster?
On 2013/04/30 18:53:00, vmpstr wrote: > > The content rect does not always cover the whole SkCanvas during playback. > > My understanding of this was wrong. It turns out that sometimes the rect we draw > can be smaller than the canvas, in which case clear might do more work than > necessary. I've changed the code to rect for the time being. Tom, do you know if > clear is significantly faster than rect? I mean, if we clear the canvas, vs draw > a rect half the canvas size (for instance), would clear still be faster? Drawing a rect a quarter the canvas size should be faster than clearing the entire canvas. Part of me wants to profile it, but if it isn't that's a bug, and presumably it's not the common case? But the clear is of total_content_rect, which is based on tiling_size. Is that smaller than the canvas? (Presumably because we're reusing older larger canvases?)
On 2013/05/01 11:54:21, Tom Hudson wrote: > But the clear is of total_content_rect, which is based on tiling_size. Is that > smaller than the canvas? (Presumably because we're reusing older larger > canvases?) Yes, it is (often) smaller than the canvas for edge tiles. Maybe that is an edge case (haha), but you asked to change this line of code for performance reasons. Given that I don't understand Skia performance characteristics very well, I just wanted to be convinced that this was the right thing to do.
No, since we're reusing larger tiles, it's no longer obviously the right thing to do. Forget the suggestion.
I've reverted that part. PTAL
lgtm
Tom, can you take a look as well, please?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/22001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Tom, you're listed as the owner on your @google.com account. I'm assuming that's why precommit is not happy. Could you please review from that account?
lgtm Oops!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/22001
lgtm Oops!
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/53001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/53001
Message was sent while issue was closed.
Change committed as 198272
Reverting due to crbug.com/238226
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/73001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/73001
Retried try job too often on linux_rel for step(s) browser_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14322017/73001
Message was sent while issue was closed.
Change committed as 198826 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
