Bug 74352 – [skia] Track a simple opaque area when painting via PlatformContextSkia and save in LayerTextureUpdater

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Construct a simplified opaque region when painting via PlatformContextSkia and save in LayerTextureUpdater

[Dana Jansens]

Created attachment 118874 [details]
Patch

Construct a simplified opaque region when painting via a PlatformContextSkia, and save it in the LayerTextureUpdater for use when updating compositor resources.

[Dana Jansens]

So I am thinking something like this. It gives back an opaque Region with bounded complexity (at most 5 separate rects).

[Dana Jansens]

Created attachment 119048 [details]
Patch

Fixed to consider the current clip and mask bitmap.

[Nat Duca]

Comment on attachment 119048 [details]
Patch

Think about handing a million rects at this. The cost of markRect* scares the crap out of me. Can we go for a single big rect at first to establish a method as well as fitness-testing infrastructure? Then go back and do another patch that makes it smarter?

[Mike Reed]

Should the detection code in drawRect() also check for gradients or other ways to fill the rectangle besides a single color?

[Dana Jansens]

(In reply to comment #4)
> (From update of attachment 119048 [details])
> Think about handing a million rects at this. The cost of markRect* scares the crap out of me. Can we go for a single big rect at first to establish a method as well as fitness-testing infrastructure? Then go back and do another patch that makes it smarter?

May I ask what part scares you about it? I had tried to design with this in mine, so that it would only compare the each new rect to at most 5 others. Maybe my intuition is just off on this being cheap?

[Nat Duca]

(In reply to comment #6)
> May I ask what part scares you about it? I had tried to design with this in mine, so that it would only compare the each new rect to at most 5 others. Maybe my intuition is just off on this being cheap?

This is pretty much as critical-path as you can get in chrome. So, zero cost is too slow. :)

[David Reveman]

(In reply to comment #2)
> So I am thinking something like this. It gives back an opaque Region with bounded complexity (at most 5 separate rects).

I like the approach of starting with something simple like a flag or a rect and then follow up with possible improvements after that. I'm not sure a region with bounded complexity is the right approach. Does it make sense to get higher detail about opaque-ness than on a per-tile basis? I'm not sure the complexity of that is worth it.

[Mike Reed]

What is a good measure of the performance cost? Anecdotally, when ever I have profiled a sites drawing, the hot spots have never been anywhere near the setup for drawRect or fillRect. I am very interested on all aspects of measure drawing performance, so I would appreciate pointers to sites or tests that would exercise the code around this CL.

[Dana Jansens]

Created attachment 119103 [details]
Patch

A new patch addressing concerns and ideas voiced here and on chats; thanks for the good feedback.

Simplied the decision/storage of the opaque region by just storing a Rect. Try to expand the rect when possible, or replace it when seeing a larger rect.

Considers gradients (and checks if they have alpha) and patterns (assumes they have alpha).

[Dana Jansens]

Created attachment 119106 [details]
Patch

Previous patch skipped initializing the new members of the PlatformContext.

[Vangelis Kokkevis]

Comment on attachment 119106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119106&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:300
> +    if (m_opaqueRect.fTop == rect.fTop && m_opaqueRect.fBottom == rect.fBottom) {

Are these really common cases in practice (e.g.  the new rect has the same height but different width or vice-versa)?  Is it really worth the time checking them?  My suspicion is that more commonly we'll be drawing a lot of non-overlapping opaque rects whose union will be one large rect.
It's definitely worth a try to put this in place but my gut feeling is that we'll want to track opaqueness as a boolean flag on the tiles themselves, maybe at the time we upload their contents as textures.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:331
> +    unsigned fillcolorNotTransparent = m_state->m_fillColor & 0xFF000000;

Don't we have to look at the m_state's xfer mode here?  We could be overwriting existing opaque pixels with transparent ones.

[Dana Jansens]

(In reply to comment #12)
> (From update of attachment 119106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119106&action=review
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:300
> > +    if (m_opaqueRect.fTop == rect.fTop && m_opaqueRect.fBottom == rect.fBottom) {
> 
> Are these really common cases in practice (e.g.  the new rect has the same height but different width or vice-versa)?  Is it really worth the time checking them?  My suspicion is that more commonly we'll be drawing a lot of non-overlapping opaque rects whose union will be one large rect.

You bring up a good point here. The rects don't need to be the same height exactly to grow the rectangle.

I can see your point about the many non-overlapping opaque rects, but my intuition is that these would be foreground elements, which this may catch but is not really intended to. It is mostly meant to catch backgrounds, and box decorations. I think the set of backgrounds typically approximately fill a layer. For instance a set of divs would make a vertical column of backgrounds that are the same width and adjacent.

> It's definitely worth a try to put this in place but my gut feeling is that we'll want to track opaqueness as a boolean flag on the tiles themselves, maybe at the time we upload their contents as textures.

Basically the region that is opaque is used to know what parts of the screen are occluded, and we track that as we move from front to back painting. Each opaque area is added to the occluded space as we go, and any tiles contained in the occluded space can skip painting/uploading.

I think this comment is directed at using this information on the impl side for draw culling? Currently it would not get the benefit of this infomation, but if we did pass it over, IMO it would be better to pass over one opaque rect per layer rather than a set of booleans for each tile. While the booleans could be used to decide blending, they would lose information wrt occlusion (half opaque tiles) and we'd end up with more overdraw, since tiles do not align between different layers. If we do end up passing opaque information gleaned here over to impl, unless we are reading back all pixels of a tile, I think that we need to decide what is opaque someplace like this (or in WebKit) regardless of how the data is handed to impl.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:331
> > +    unsigned fillcolorNotTransparent = m_state->m_fillColor & 0xFF000000;
> 
> Don't we have to look at the m_state's xfer mode here?  We could be overwriting existing opaque pixels with transparent ones.

Yes, thank you for this! Also m_alpha looks important.

[Dana Jansens]

Created attachment 119249 [details]
Patch

Includes consideration of the current state's Xfermode::Mode and alpha. Grows the tracked opaque rect to include area in a new rect in cases where it can't consume the entire rect.

[James Robinson]

Comment on attachment 119249 [details]
Patch

I like this, but where are the tests?  There are lots of cases to check for here.

[Dana Jansens]

Ah, I was expecting to excise this by testing the culling, but I'll make some tests for it, thanks.

[Vangelis Kokkevis]

Comment on attachment 119249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119249&action=review

I'm curious how this will be used in practice given that after the initial layer paint, we do get incremental updates, which all end up in small canvases with throw away GC's .  How will we track the overall opaque rect of a layer after the incremental updates?  For example:  Imagine that a layer starts half opaque and half transparent and in a subsequent incremental update, the transparent part is painted over with an opaque color.  Or, the other way around...

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:298
> +

You could presumably add another easy test here for the case where m_opaqueRect is fully inside rect:
if (rect.contains(m_opaqueRect))
  m_opaqueRect = rect;

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:300
> +        if (rect.fLeft < m_opaqueRect.fLeft && rect.fRight > m_opaqueRect.fLeft)

shouldn't this be:
rect.fLeft < m_opaqueRect.fLeft && rect.fRight >= m_opaqueRect.fLeft
                                                                       
(>= vs >  for the second check)

to catch adjacent rects?  Similarly below.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:304
> +    } else if (m_opaqueRect.fLeft <= rect.fLeft && m_opaqueRect.fRight >= rect.fRight) {

Should this be a straight "if" instead of an "else if"  ?  Although checking for containment as I mentioned above should take care of it.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:333
> +    unsigned fillcolorNotTransparent = m_state->m_fillColor & 0xFF000000;

Is the color always stored as ARGB ?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:341
> +                markRectAsOpaque(rect);

If the transparent pixels you're writing with a non opaque transfer mode end up in what you previously considered part of the opaque rect, don't you need to reset the opaque rect?

[Dana Jansens]

(In reply to comment #17)
> (From update of attachment 119249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119249&action=review
> 
> I'm curious how this will be used in practice given that after the initial layer paint, we do get incremental updates, which all end up in small canvases with throw away GC's .  How will we track the overall opaque rect of a layer after the incremental updates?  For example:  Imagine that a layer starts half opaque and half transparent and in a subsequent incremental update, the transparent part is painted over with an opaque color.  Or, the other way around...

This code is just meant to give back an opaque region for a single paint. You're right that incremental updates are something to consider, but that is something that would be considered up in the layers themselves, so it'll be the next CL :).  This is only meant to give back a value for the current painting operation. The property seems long-lived, but a new PlatformContextSkia is created each time a layer is painted, so this value is only holding something for the current paint.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:298
> > +
> 
> You could presumably add another easy test here for the case where m_opaqueRect is fully inside rect:
> if (rect.contains(m_opaqueRect))
>   m_opaqueRect = rect;

yup thanks.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:300
> > +        if (rect.fLeft < m_opaqueRect.fLeft && rect.fRight > m_opaqueRect.fLeft)
> 
> shouldn't this be:
> rect.fLeft < m_opaqueRect.fLeft && rect.fRight >= m_opaqueRect.fLeft
> 
> (>= vs >  for the second check)
>
> to catch adjacent rects?  Similarly below.

fRight is 1 px past the right edge of the rect, so it would be 1 more in the adjacent case and > will catch it. This is the same as maxX/maxY in IntRect, but the name is less clear. At least that was my understanding from reading other SkRect code.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:304
> > +    } else if (m_opaqueRect.fLeft <= rect.fLeft && m_opaqueRect.fRight >= rect.fRight) {
> 
> Should this be a straight "if" instead of an "else if"  ?  Although checking for containment as I mentioned above should take care of it.

Yeh you're right, since it is checking >= now, I'll use the contains check that you suggest to catch this case.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:333
> > +    unsigned fillcolorNotTransparent = m_state->m_fillColor & 0xFF000000;
> 
> Is the color always stored as ARGB ?

I guess so, I just changed its type from int to unsigned.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:341
> > +                markRectAsOpaque(rect);
> 
> If the transparent pixels you're writing with a non opaque transfer mode end up in what you previously considered part of the opaque rect, don't you need to reset the opaque rect?

As above, there is no state preserved across paints, that sort of smartness would happen at the layer level.

[Dana Jansens]

(In reply to comment #18)
> > If the transparent pixels you're writing with a non opaque transfer mode end up in what you previously considered part of the opaque rect, don't you need to reset the opaque rect?
> 
> As above, there is no state preserved across paints, that sort of smartness would happen at the layer level.

Sorry I see what you mean here, we might non-opaque stuff drawn over top during a single paint operation. I'll have to take that into consideration, thanks.

[Dana Jansens]

Created attachment 119329 [details]
Patch

Complete with unit test, thank you for making me do that.

PlatformContextSkia::drawRect() is only used in one specific case. Pulled the opaque tracking out to a new function (trackFilledRect()) and call this after any canvas->drawRect() calls in GraphicsContextSkia and PlatformContextSkia. It also makes use of the SkPaint used to do the filling to get accurate results.

Handles redrawing pixels over top of a previously marked opaque area, and determines if the new fill will preserve the opaque-ness there or not. If not it attempts to keep as large a rect as possible marked opaque.

I don't have timing data to assess the impact of the change but I will do that tomorrow morning. Cheers.

[Vangelis Kokkevis]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 119249 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119249&action=review
> > 
> > I'm curious how this will be used in practice given that after the initial layer paint, we do get incremental updates, which all end up in small canvases with throw away GC's .  How will we track the overall opaque rect of a layer after the incremental updates?  For example:  Imagine that a layer starts half opaque and half transparent and in a subsequent incremental update, the transparent part is painted over with an opaque color.  Or, the other way around...
> 
> This code is just meant to give back an opaque region for a single paint. You're right that incremental updates are something to consider, but that is something that would be considered up in the layers themselves, so it'll be the next CL :).  This is only meant to give back a value for the current painting operation. The property seems long-lived, but a new PlatformContextSkia is created each time a layer is painted, so this value is only holding something for the current paint.

Apologies for repeating my previous comment but the more I look at this code the more convinced I become that tracking opaqueness as a boolean value on a per-tile basis based on actual pixel values will likely be more flexible.  Imagine a large layer that's opaque everywhere except for a small area near its center.  By tracking only a single opaque rect for the entire layer we'll only be able to mark as opaque only about a quarter of the layer.  In contrast, by marking as opaque all tiles except for the one containing the transparent region will potentially give us a much bigger opaque area. 

I find marking entire tiles as opaque or non-opaque will be much less complicated and error prone than trying to concatenate and subtract rects.  Part of the win we'll get by being able to identify opaque regions is that we can turn off blending while drawing opaque layers.  In again this can only be done at the tile level.

[Dana Jansens]

(In reply to comment #21)
> Apologies for repeating my previous comment but the more I look at this code the more convinced I become that tracking opaqueness as a boolean value on a per-tile basis based on actual pixel values will likely be more flexible.  Imagine a large layer that's opaque everywhere except for a small area near its center.  By tracking only a single opaque rect for the entire layer we'll only be able to mark as opaque only about a quarter of the layer.  In contrast, by marking as opaque all tiles except for the one containing the transparent region will potentially give us a much bigger opaque area. 

FWIW it would catch ~1/2 not 1/4 in this case, but that's probably not your point :)

> I find marking entire tiles as opaque or non-opaque will be much less complicated and error prone than trying to concatenate and subtract rects.  Part of the win we'll get by being able to identify opaque regions is that we can turn off blending while drawing opaque layers.  In again this can only be done at the tile level.

You're right, of course. Reading the actual pixel values would give the best knowledge about the alpha values. It was brought up as one of 3 possible ways to approach this data collection, but that reading back all the pixels will be expensive when the tile data is in the GPU, and cause more memory bandwidth use, while the goal of culling is to use less. That's why I chose to go the PlatformContextSkia route here.

I guess two things:

1) The opaque edges but transparent interior seems like a really uncommon case to me, compared to opaque middle and transparent edges?

2) By tracking a few more rects instead of 1 we could cover this case, so if it is an important case, I'd still prefer to do it here rather than do a pixel readback from the GPU.

I don't know.. I still feel like we should do it this way right now. I like that its performance is not based on the size of the layer, and that it doesn't require any access to video memory. What do others think?

[Dana Jansens]

Comment on attachment 119329 [details]
Patch

I ran the following test both with PCSkia::setTrackOpaqueRegion enabled and disabled.

http://www.corp.google.com/~danakj/paint-culling/test-painted-foreground.html

There appears no measurable difference in time to paint, as random noise dominated the differences in results.

For example, to paint the set of layers in one frame (in milliseconds).

Without tracking for two different frames:
.420             .472
.174             .178
.150             .152
.167             .151
.170             .168

With tracking:
.443             .479              .490
.167             .175              .176
.148             .165              .147
.143             .150              .153
.165             .175              .216

Each frame is drawing the same things, just different colours, so as you can see the variation between frames with tracking enabled/disabled is as large as the difference between the two.

[Vangelis Kokkevis]

(In reply to comment #22)
> (In reply to comment #21)
> > Apologies for repeating my previous comment but the more I look at this code the more convinced I become that tracking opaqueness as a boolean value on a per-tile basis based on actual pixel values will likely be more flexible.  Imagine a large layer that's opaque everywhere except for a small area near its center.  By tracking only a single opaque rect for the entire layer we'll only be able to mark as opaque only about a quarter of the layer.  In contrast, by marking as opaque all tiles except for the one containing the transparent region will potentially give us a much bigger opaque area. 
> 
> FWIW it would catch ~1/2 not 1/4 in this case, but that's probably not your point :)

oh, right, of course :)

> 
> > I find marking entire tiles as opaque or non-opaque will be much less complicated and error prone than trying to concatenate and subtract rects.  Part of the win we'll get by being able to identify opaque regions is that we can turn off blending while drawing opaque layers.  In again this can only be done at the tile level.
> 
> You're right, of course. Reading the actual pixel values would give the best knowledge about the alpha values. It was brought up as one of 3 possible ways to approach this data collection, but that reading back all the pixels will be expensive when the tile data is in the GPU, and cause more memory bandwidth use, while the goal of culling is to use less. That's why I chose to go the PlatformContextSkia route here.
> 
> I guess two things:
> 
> 1) The opaque edges but transparent interior seems like a really uncommon case to me, compared to opaque middle and transparent edges?

It' not just about whether the opaque part is at the center or the edges.  If there is a mix of opaque and transparent regions on a single layer (e.g. images without alpha intermixed with text drawn on transparent background), doing the tests per tile will give you better granularity.  

But it's true that at this point it's anyone's guess what the common pattern on real pages is.  We could measure it, presumably.

> 
> 2) By tracking a few more rects instead of 1 we could cover this case, so if it is an important case, I'd still prefer to do it here rather than do a pixel readback from the GPU.

I'd rather not go back to that. The tile-level granularity seems easier to deal with rather than tracking independent rects.

> 
> I don't know.. I still feel like we should do it this way right now. I like that its performance is not based on the size of the layer, and that it doesn't require any access to video memory. What do others think?

One thing to note is that how we detect transparency is somewhat orthogonal to how we track it. We don't necessarily have to detect transparency by walking the pixels.  We could use code similar to what you have (without the complexity of actual rect tracking) that works on the context level when we create a context to paint the contents of a tile. The question I'm raising is whether we should be doing that on a tile-by-tile basis with a single flag that marks the entire tile or do our own rect tracking on the entire layer.  I advocate using tiles as our base unit so that we can keep both paint and draw culling based on the same flags.

One difficulty that comes to mind if we do end up painting only partial contents of a tile based on a visibility determination that happens in the main thread is the following: What happens if the rest of the tile is uncovered in the compositor thread?  Currently tiles are somewhat binary, either they have valid contents or they don't.

Anyway, interested in hearing other opinions too.

[Dana Jansens]

Nat requested some more tests using canvas and a far larger number of rectangles. The impact of this code seems minimal still, something like on the order or 0.005ms per rect.

There are 3 tests I used:
1. http://www.corp.google.com/~danakj/paint-culling/test-fillrect-collage.html
2. http://www.corp.google.com/~danakj/paint-culling/test-fillrect-border.html
3. http://www.corp.google.com/~danakj/paint-culling/test-fillrect-border-200.html

Average and median time (in seconds) to draw for each of them WITH and WITHOUT opaque tracking.

1. WITH 9.2196     9.241
   WITHOUT 9.1613  9.185

2. WITH 6.6228     6.440
   WITHOUT 6.6032  6.451

3. WITH 7.1644     6.970
   WITHOUT 6.8315  6.946

Third test had a couple high outliers for both sides so the median is probably more interesting. The data set's not huge but should give an idea. Full data below.

1. WITH 9.228 9.250 9.180 9.258 9.264 9.095 9.234 9.248
1. WITHOUT 9.089 9.119 9.205 9.237 9.218 9.185 9.076
2. WITH 6.496 6.393 6.477 6.440 7.308
2. WITHOUT 6.451 6.592 6.450 6.386 7.137
3. WITH 6.970 6.974 6.960 8.333 6.946 7.015 6.953
3. WITHOUT 6.905 6.956 6.935 6.916 8.310 6.967

[Dana Jansens]

(In reply to comment #24)
> (In reply to comment #22)
> It' not just about whether the opaque part is at the center or the edges.  If there is a mix of opaque and transparent regions on a single layer (e.g. images without alpha intermixed with text drawn on transparent background), doing the tests per tile will give you better granularity.  

I think the assumption underlying this is that we would be painting a single tile at a time. That is not the current situation, we paint the entire layer in one go. And reveman tells me this will never be the case. So if we are going to do the detection during the paint, I think we would need to pass in tile information to the PlatformContextSkia, and have it track opaque stuff, similar to how this CL does, but for each tile. Is that what you are meaning? If so, I'd rather do that in a separate iteration as it is significantly more complicated.

What we are planning to do on a per-tile basis is playback a recorded SkPicture. We could do this opaque computation in there, and it was suggested originally also. Is this what you are meaning instead? I will +you on the email thread but the current state of the thread was it was easier to do this in the context on paint and we should try that out first. We certainly have the potential to do a lot better with this method.

Determining if this method is "good enough" is another issue.. Nat has suggested doing pixel readbacks and comparing the results when running all the LayoutTests, so say if it catches some % of the truly opaque cases.

[Vangelis Kokkevis]

(In reply to comment #26)
> (In reply to comment #24)
> > (In reply to comment #22)
> > It' not just about whether the opaque part is at the center or the edges.  If there is a mix of opaque and transparent regions on a single layer (e.g. images without alpha intermixed with text drawn on transparent background), doing the tests per tile will give you better granularity.  
> 
> I think the assumption underlying this is that we would be painting a single tile at a time. That is not the current situation, we paint the entire layer in one go. And reveman tells me this will never be the case. So if we are going to do the detection during the paint, I think we would need to pass in tile information to the PlatformContextSkia, and have it track opaque stuff, similar to how this CL does, but for each tile. Is that what you are meaning? If so, I'd rather do that in a separate iteration as it is significantly more complicated.
> 
> What we are planning to do on a per-tile basis is playback a recorded SkPicture. We could do this opaque computation in there, and it was suggested originally also. Is this what you are meaning instead?

Sorry for not being more clear. Yes, that's what I meant.  When we play back the SkPicture to create the tile contents, we need to create PlatformContext and at that point we can figure out whether the tile is opaque using code similar to what you have here.

And so the question is, should we be trying to figure out the opaque rect when painting the entire layer or postpone it and do it as we fill the contents of each tile?


 I will +you on the email thread but the current state of the thread was it was easier to do this in the context on paint and we should try that out first. We certainly have the potential to do a lot better with this method.
> 
> Determining if this method is "good enough" is another issue.. Nat has suggested doing pixel readbacks and comparing the results when running all the LayoutTests, so say if it catches some % of the truly opaque cases.

[David Reveman]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > (In reply to comment #22)
> > > It' not just about whether the opaque part is at the center or the edges.  If there is a mix of opaque and transparent regions on a single layer (e.g. images without alpha intermixed with text drawn on transparent background), doing the tests per tile will give you better granularity.  
> > 
> > I think the assumption underlying this is that we would be painting a single tile at a time. That is not the current situation, we paint the entire layer in one go. And reveman tells me this will never be the case. So if we are going to do the detection during the paint, I think we would need to pass in tile information to the PlatformContextSkia, and have it track opaque stuff, similar to how this CL does, but for each tile. Is that what you are meaning? If so, I'd rather do that in a separate iteration as it is significantly more complicated.
> > 
> > What we are planning to do on a per-tile basis is playback a recorded SkPicture. We could do this opaque computation in there, and it was suggested originally also. Is this what you are meaning instead?
> 
> Sorry for not being more clear. Yes, that's what I meant.  When we play back the SkPicture to create the tile contents, we need to create PlatformContext and at that point we can figure out whether the tile is opaque using code similar to what you have here.
> 
> And so the question is, should we be trying to figure out the opaque rect when painting the entire layer or postpone it and do it as we fill the contents of each tile?

I'd like to eventually see us do this on a per-tile basis. However, (assuming that checking opaque-ness when playing back a SkPicture is a bad idea) doing it at the layer level seems like a good first step.

As the opaque-ness query is not required to be accurate (just not allowed to return false positives) I can imagine that returning a boolean value for a rect can be significantly more efficient than returning an opaque rect.

So the question I think we need to answer is; should this be done by returning a boolean or rect? Should the interface provide "isRectOpaque(IntRect)" or "IntRect opaqueRect()"?

"isRectOpaque" is not as useful when doing it at the layer level but just what we need for opaque-ness on a per-tile basis. So if "isRectOpaque" is more efficient and our goal is to do this on a per-tile basis, a boolean return value might be the better choice.

Dana, what do you think about efficiency and simplicity of  "isRectOpaque(IntRect)" vs "IntRect opaqueRect()"?

[Dana Jansens]

I think what's complicating this in my mind is that I really want both.

What this conversation has been about is deciding if a tile is opaque. That is a super useful operation since we can store that data and pass it over to the impl side. Yay. For this, returning one rect is less optimal cuz we only care about data on a per-tile basis. This information could from from the SkCanvas, computed during drawPicture (not the PlatformContextSkia as done in this patch).

What I was thinking about largely when writing this patch, however, was deciding occlusion. I've explained my planned strategy a few times to people but not in a documented place yet. So let me do that.

We will keep track of "visible" pixels in a Region. You start with the entire screen "visible". Begin at the front-most layer and work our way back.
1. For layer L, paint any tiles that intersect with the current "visible" region.
2. For layer L, decide what region O was painted opaque
3. Subtract O from the "visible" region (those pixels should never be painted again).
4. Repeat from 1.

In this way we track which pixels to paint on each layer as we go. For this, I think we want just a single opaque Rect, if it can be a decent enough approximation. If we do this on a per-tile basis, the "visible" region can become arbitrarily complex. If we add 1 rect per layer instead (with $n$ layers), the Region's complexity will be at most $n$.

The current computation is relatively cheap. A similar operation could be done for each tile if the perf would be acceptable. At the moment I am not worrying about getting opaque data for the impl side, though. If we only consider computing occluded pixels during the paint walk, is this approach better than per-tile like I feel it is?

[Nat Duca]

I think we need to simplify this discussion. The simple fact of the matter is we aren't yet shipping any skpicture-based drawing paths. We are, however, shipping "paint a tile" paths, and on those paths, we are overdraw bound.

Would it make sense to people here to get something *simple* landed that works for our production "paint tiles into a bitmap" path first? We can do followup patches for other modes afterward.

[Dana Jansens]

Created attachment 120047 [details]
Patch

when blitting an SkImage, make sure to mark the area as opaque/not. use the SkPaint shader to handle gradients/patterns properly.

[Stephen White]

Comment on attachment 120047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120047&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:313
> +    if (rect.fTop <= m_opaqueRect.fTop && rect.fBottom >= m_opaqueRect.fBottom) {
> +        if (rect.fLeft < m_opaqueRect.fLeft && rect.fRight > m_opaqueRect.fLeft)
> +            m_opaqueRect.fLeft = rect.fLeft;
> +        if (rect.fRight > m_opaqueRect.fRight && rect.fLeft < m_opaqueRect.fRight)
> +            m_opaqueRect.fRight = rect.fRight;
> +    } else if (rect.fLeft <= m_opaqueRect.fLeft && rect.fRight >= m_opaqueRect.fRight) {
> +        if (rect.fTop < m_opaqueRect.fTop && rect.fBottom > m_opaqueRect.fTop)
> +            m_opaqueRect.fTop = rect.fTop;
> +        if (rect.fBottom > m_opaqueRect.fBottom && rect.fTop < m_opaqueRect.fBottom)
> +            m_opaqueRect.fBottom = rect.fBottom;
> +    }

Out of curiosity, what would be the downside of making this a simple union?  Unnecessary redraws?  Using both overlap and area criteria seems somewhat complex.  (Although I must admit I don't fully understand how this rect interacts with the compositor).

[Stephen White]

Comment on attachment 120047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120047&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:313
> +    if (rect.fTop <= m_opaqueRect.fTop && rect.fBottom >= m_opaqueRect.fBottom) {
> +        if (rect.fLeft < m_opaqueRect.fLeft && rect.fRight > m_opaqueRect.fLeft)
> +            m_opaqueRect.fLeft = rect.fLeft;
> +        if (rect.fRight > m_opaqueRect.fRight && rect.fLeft < m_opaqueRect.fRight)
> +            m_opaqueRect.fRight = rect.fRight;
> +    } else if (rect.fLeft <= m_opaqueRect.fLeft && rect.fRight >= m_opaqueRect.fRight) {
> +        if (rect.fTop < m_opaqueRect.fTop && rect.fBottom > m_opaqueRect.fTop)
> +            m_opaqueRect.fTop = rect.fTop;
> +        if (rect.fBottom > m_opaqueRect.fBottom && rect.fTop < m_opaqueRect.fBottom)
> +            m_opaqueRect.fBottom = rect.fBottom;
> +    }

Out of curiosity, what would be the downside of making this a simple union?  Unnecessary redraws?  Using both overlap and area criteria seems somewhat complex.  (Although I must admit I don't fully understand how this rect interacts with the compositor).

[Dana Jansens]

  +--------+
  |        |
  |        |
+-|        |-+
| +--------+ |
|            |
+------------+

A rect union would include pixels not in either rect, and mark them opaque when they are not. We can allow false negatives but not positives like this.

Instead we want the result to be. It's a compromise between a Region union and storing a single Rect.

  +--------+
  |        |
  |        |
  |        |
  +        +
  |        |
  +--------+

[Stephen White]

(In reply to comment #34)
>   +--------+
>   |        |
>   |        |
> +-|        |-+
> | +--------+ |
> |            |
> +------------+
> 
> A rect union would include pixels not in either rect, and mark them opaque when they are not. We can allow false negatives but not positives like this.
> 
> Instead we want the result to be. It's a compromise between a Region union and storing a single Rect.
> 
>   +--------+
>   |        |
>   |        |
>   |        |
>   +        +
>   |        |
>   +--------+

OK, I see now:  you do a "conservative" union if possible, or pick the larger rect by area if not.  Thanks for the explanation.

[Vangelis Kokkevis]

(In reply to comment #30)
> I think we need to simplify this discussion. The simple fact of the matter is we aren't yet shipping any skpicture-based drawing paths. We are, however, shipping "paint a tile" paths, and on those paths, we are overdraw bound.
> 
> Would it make sense to people here to get something *simple* landed that works for our production "paint tiles into a bitmap" path first? We can do followup patches for other modes afterward.

(In reply to comment #30)
> I think we need to simplify this discussion. The simple fact of the matter is we aren't yet shipping any skpicture-based drawing paths. We are, however, shipping "paint a tile" paths, and on those paths, we are overdraw bound.
> 
> Would it make sense to people here to get something *simple* landed that works for our production "paint tiles into a bitmap" path first? We can do followup patches for other modes afterward.

Simple is good but I'm not sure what you're suggesting simple will be in this case. I mostly want to make sure that whatever we do produces a reasonably good indication of the areas of the layer are opaque. Depending on the order that paints happen in WebKit we could either end up with a good approximation or an overly conservative one.  For example, how does WebKit paint a layer with a semi-transparent border?  How about a layer that contains a number of opaque elements next to each other but has no opaque background? Will they be drawn in a way that will let us concatenate their rects into a larger rect?

[Dana Jansens]

Comment on attachment 120047 [details]
Patch

(In reply to comment #36)
> Simple is good but I'm not sure what you're suggesting simple will be in this case. I mostly want to make sure that whatever we do produces a reasonably good indication of the areas of the layer are opaque. Depending on the order that paints happen in WebKit we could either end up with a good approximation or an overly conservative one.  For example, how does WebKit paint a layer with a semi-transparent border?  How about a layer that contains a number of opaque elements next to each other but has no opaque background? Will they be drawn in a way that will let us concatenate their rects into a larger rect?

Ok here are some much-needed numbers.  I ran all of the compositing layout tests, and I am ignoring scroll bars (they are drawn in Chrome).

The average number of opaque pixels that we find, per layer = 83.6%
The total number of opaque pixels that we find, over all the tests = 73.2%

On most layers it is getting most of the pixels. The times it misses most are layers with opaque borders but non-opaque interiors.

This seems like a pretty decent first step towards deciding opaque-ness, and it is something we can iterate on in the future to improve.

I will throw up a new patch tomorrow.

[Dana Jansens]

Created attachment 121325 [details]
Patch

More thorough first cut at detecting opaque-ness via PlatformContextSkia.

This patch takes strokes/lines into account (they may put alpha into pixels). The unit test is beefed up to include lines, paths, ovals and image draws.

[Vangelis Kokkevis]

Comment on attachment 121325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121325&action=review

Looking good.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:454
> +    if (!m_trackOpaqueRegion)

Maybe simpler and faster to call trackDrawBounded() for the bounding box containing all the points?

[Dana Jansens]

Comment on attachment 121325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121325&action=review

Thanks.

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:454
>> +    if (!m_trackOpaqueRegion)
> 
> Maybe simpler and faster to call trackDrawBounded() for the bounding box containing all the points?

Yeh that would probably be sufficient.  At the moment it would be identical really since the function is only called once with a single line (2 points).

[Dana Jansens]

Comment on attachment 121325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121325&action=review

>>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:454
>>> +    if (!m_trackOpaqueRegion)
>> 
>> Maybe simpler and faster to call trackDrawBounded() for the bounding box containing all the points?
> 
> Yeh that would probably be sufficient.  At the moment it would be identical really since the function is only called once with a single line (2 points).

Sorry on second thought that wouldn't do the same thing. The path function takes into account stroke thickness and joining method and such. Taking the bounding box would be a smaller area than the area possibly drawn in. I think it needs to be done using an SkPath.

It would be possible to build on big SkPath always from all the points. But I'm not sure that this would always give the same (or larger) bounding box. Does anyone know?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:484
> +{

nit to self: check m_trackOpaqueRegion region here.

[Mike Reed]

Comment on attachment 121325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121325&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:354
> +

// returns true if the xfermode will force the dst to be opaque, regardless of the current dst.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:356
> +{

if (!srcIsOpaque)
    return false;

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:358
> +        return srcIsOpaque; // default to kSrcOver_Mode

return true;

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:369
> +        return srcIsOpaque;

return true;

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:381
> +

// returns true if the xfermode will keep the dst opaque, assuming the dst is already opaque.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:421
> +        if (shader)

Cannot call getFlags() w/o calling setContext() first. Instead, I think you can do this:

if (0xFF == paint.getAlpha()) {
    if (shader)
        opaque = shader->isOpaque();
    else
        opaque = !bitmap || bitmap->isOpaque();
}

[Mike Reed]

paint.computeStrokeFastBounds() takes stroking into account, and will return the bloated bounds (as needed) to take into account stroke-width and join/cap types.

If you have an array of points to be draws as a path or lines, you may be able to just compute their bounds, and then ask the paint for its bounds as a rect, assuming you set the paint's style appropriately (e.g. to kStroke if you're drawing as lines).

[Dana Jansens]

Created attachment 121510 [details]
Patch

Comments addressed.

Reworked the tracking a bit so the opaque-ness is considered for paths/points/lines/etc also. if they are only drawing opaque pixels then they shouldn't damage our current opaqueRect.

Buffed up the tests some. Added some actual pixel tests too to check for false positives. (Disabled pending skia-side fixes. [1])

There are two things that I wasn't totally sure about - I'll point them out via the review system.

[1] http://codereview.appspot.com/5494076/

[Dana Jansens]

Comment on attachment 121510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121510&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:424
> +    if (paint.getStyle() != SkPaint::kFill_Style && paint.isAntiAlias())

Is this decision okay? It passes the unit tests but I'm not sure if there's some case they are missing.

[Mike Reed]

There are a lot of methods related to just tracking opaqueness. Can all of these be moved into a separate class? PlatformContextSkia could contain an instance of one of these, so there should be no perf const due to indirection, but I would like to attempt to refactor this to try to encapsulate it more (if possible), esp. since PlatformContextSkia is already a big noisy class. Might make unittesting easier too...

[Stephen White]

(In reply to comment #46)
> There are a lot of methods related to just tracking opaqueness. Can all of these be moved into a separate class? PlatformContextSkia could contain an instance of one of these, so there should be no perf const due to indirection, but I would like to attempt to refactor this to try to encapsulate it more (if possible), esp. since PlatformContextSkia is already a big noisy class. Might make unittesting easier too...

+1 to that.

[Dana Jansens]

Thanks, great idea.

[Dana Jansens]

Created attachment 121748 [details]
Patch

This is how I propose we split it out. I've made an OpaqueRegionSkia class in .../platform/graphics/skia.

The object is pretty dumb itself, it just listens to draws and builds its rect. You can grab the object back from
the context, and ask for it as a rect with OpaqueRegionSkia::asRect().

The context still have functions to listen to drawing operations and just forwards them to the OpaqueRegionSkia
when tracking is enabled in the context.

I kept the same unit tests, because I want them to test the GraphicsContextSkia and other code in
 ...platform/graphics/skia to make sure they are calling the trackDraw*() methods on the platform
context also.

[Mike Reed]

thanks, nice refactor.

[Stephen White]

Comment on attachment 121748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121748&action=review

Looks pretty good.  At a minimum, please add a comment referencing the Skia bug, and restore the #includes.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:-37
> -#include "SkCanvas.h"

Please don't remove these.  As long as this file directly refers to those symbols, we should include what we use, and not rely on transitive includes.  That said, if it's possible to change these to be forward declarations, feel free to do so.  It looks like it won't be for SkCanvas or SkPaint, but might be for SkPath.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:46
> +    { } // FIXME: Enable this test when Skia actually gets the pixels right.

Please log a bug about this (if there isn't already), and add a comment to point to that bug.

Also, this looks like it essentially makes all the pixel tests below a no-op.  Could you use fuzzy testing instead, at least until the bug is fixed?

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:59
> +    uint32_t* pixels = static_cast<uint32_t*>(malloc(400*400*4));
> +    bitmap.setPixels(pixels);

Another option would be use to use bitmap.allocPixels() here, then an SkAutoLockPixels() and getAddr32() to get the pointer.  That would save you doing the malloc()/free() yourself, and make it less fragile against changes to width/height.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:117
> +    uint32_t* pixels = static_cast<uint32_t*>(malloc(200*200*4));
> +    bitmap.setPixels(pixels);

Same here.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:215
> +    uint32_t* pixels = static_cast<uint32_t*>(malloc(200*200*4));
> +    bitmap.setPixels(pixels);

Same here.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:278
> +    uint32_t* pixels = static_cast<uint32_t*>(malloc(200*200*4));
> +    bitmap.setPixels(pixels);

Same here.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:294
> +        static_cast<uint32_t*>(drawBitmap.getPixels())[i] = 0xFFFFFFFF;

Could use drawBitmap.getAddr32() here I think.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:300
> +        static_cast<uint32_t*>(drawBitmap.getPixels())[i] = 0x00000000;

Same here.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:363
> +    uint32_t* pixels = static_cast<uint32_t*>(malloc(200*200*4));
> +    bitmap.setPixels(pixels);

Could use allocPixels()/getAddr32().

[Dana Jansens]

Created attachment 122098 [details]
Patch

comments addressed.

[Dana Jansens]

View in context: https://bugs.webkit.org/attachment.cgi?id=121748&action=review

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:-37
>> -#include "SkCanvas.h"
> 
> Please don't remove these.  As long as this file directly refers to those symbols, we should include what we use, and not rely on transitive includes.  That said, if it's possible to change these to be forward declarations, feel free to do so.  It looks like it won't be for SkCanvas or SkPaint, but might be for SkPath.

done.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:46
>> +    { } // FIXME: Enable this test when Skia actually gets the pixels right.
> 
> Please log a bug about this (if there isn't already), and add a comment to point to that bug.
> 
> Also, this looks like it essentially makes all the pixel tests below a no-op.  Could you use fuzzy testing instead, at least until the bug is fixed?

It is actually getting the alpha right here. (Didn't expect that since LayoutTest webpage output does not.) So I will just use the exact test.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:59
>> +    bitmap.setPixels(pixels);
> 
> Another option would be use to use bitmap.allocPixels() here, then an SkAutoLockPixels() and getAddr32() to get the pointer.  That would save you doing the malloc()/free() yourself, and make it less fragile against changes to width/height.

done. getAddr32() is lovely.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:117
>> +    bitmap.setPixels(pixels);
> 
> Same here.

done everywhere.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:294
>> +        static_cast<uint32_t*>(drawBitmap.getPixels())[i] = 0xFFFFFFFF;
> 
> Could use drawBitmap.getAddr32() here I think.

done everywhere.

[Dana Jansens]

Created attachment 122119 [details]
Patch

rebase, and punt the changes in platform/graphics/chromium off to a new CL for simplicity.

[Stephen White]

Comment on attachment 122119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122119&action=review

Sorry for the bikeshed, but after thinking about it a bit, I think the trackFoo() functions should be renamed didFoo().  E.g., trackDrawRect() -> didDrawRect().  This will be more consistent with the rest of WebKit code for post-draw notifications.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:594
> +    paint.setStrokeWidth(1);
> +    platformContext()->trackDrawPath(path, paint);

This seems slightly fragile.  It would be safer to call trackDrawPath() from drawInnerPath() and drawOuterPath(), and you wouldn't need to set the stroke width here.  I presume that you did it this way for performance?

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.h:44
> +namespace WebCore {
> +class PlatformContextSkia;
> +};
> +
> +namespace WebCore {

Any reason for two namespaces here?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:613
> +void PlatformContextSkia::trackDrawRect(const SkRect& rect, const SkPaint& paint, const SkBitmap* bitmap)

Please rename to didDrawRect(), per above.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:47
> +    SkAutoLockPixels locker(bitmap); \

It would be more efficient to create a single SkAutoLockPixels than to do it before each check:  lockPixels acquires a mutex each time, IIRC.  (Since this is a test, I don't care too much, but I thought you should know.)

[Dana Jansens]

Comment on attachment 122119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122119&action=review

Thanks for the feedback.

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:594
>> +    platformContext()->trackDrawPath(path, paint);
> 
> This seems slightly fragile.  It would be safer to call trackDrawPath() from drawInnerPath() and drawOuterPath(), and you wouldn't need to set the stroke width here.  I presume that you did it this way for performance?

it was because there is no platform context in the drawInner/OuterPath functions (they are local static). I will just pass it in and do it there though. Thanks.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.h:44
>> +namespace WebCore {
> 
> Any reason for two namespaces here?

no, i'll remove it.

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:613
>> +void PlatformContextSkia::trackDrawRect(const SkRect& rect, const SkPaint& paint, const SkBitmap* bitmap)
> 
> Please rename to didDrawRect(), per above.

done.

>> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:47
>> +    SkAutoLockPixels locker(bitmap); \
> 
> It would be more efficient to create a single SkAutoLockPixels than to do it before each check:  lockPixels acquires a mutex each time, IIRC.  (Since this is a test, I don't care too much, but I thought you should know.)

ok, i wasn't sure if locking it would prevent canvas paint operations from working, but didn't try it. i'll keep it in mind.

[Dana Jansens]

Created attachment 122268 [details]
Patch

comments addressed.

- Made the asRect() give back an enclosed IntRect so the calculation of that rect is being tested by the unit tests.
- s/track/did/
- Removed the PlatformContextSkia* member from OpaqueRegionSkia in favour of just passing it in, so that now the class
  only holds in it an SkRect.

[Stephen White]

Comment on attachment 122268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122268&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:593
>      SkCanvas* canvas = platformContext()->canvas();
> -    drawOuterPath(canvas, path, paint, width);
> -    drawInnerPath(canvas, path, paint, width);
> +    drawOuterPath(platformContext(), canvas, path, paint, width);
> +    drawInnerPath(platformContext(), canvas, path, paint, width);

I think you could avoid passing in the canvas now, and retrieve it from the platform context in the callee.

[Dana Jansens]

Comment on attachment 122268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122268&action=review

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:593
>> +    drawInnerPath(platformContext(), canvas, path, paint, width);
> 
> I think you could avoid passing in the canvas now, and retrieve it from the platform context in the callee.

done.

[Dana Jansens]

Created attachment 122269 [details]
Patch

[Stephen White]

Comment on attachment 122269 [details]
Patch

Looks good.  Thanks for all the fixes.  Will let the EWS bots chew on it for a bit.  r=me

[Dana Jansens]

Created attachment 122277 [details]
Patch

added unit test to address off-by-one error with rects beside each other.

[Stephen White]

Comment on attachment 122277 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 122277 [details]
Patch

Clearing flags on attachment: 122277

Committed r104861: <http://trac.webkit.org/changeset/104861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.