drawDisplayList should notify the region tracker.

drawDisplayList should notify the region tracker.


BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182002

[Stephen Chennney, 2014-09-11 17:39:34]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-09-11 17:39:35]

I'm running an experiment where we disable region tracking when recording but
apply it when drawing the picture (as in this case). If it resolves my clipping
bug, crbug.com/412932, then I think we need to land something like this.

But then I worry that our region tracking will be much worse because we won't
always draw within the entire picture bounds. I need to look into the
consequences.

It would be nice if all the layout tests ran correctly on my trusty box.

[danakj, 2014-09-11 17:42:15]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-11 17:42:15]

Implside painting doesn't even use that tracked region in the compositor. Is
there something in blink that makes use of the opaque info that it stores? If
not we could consider just removing it. But it sounds like maybe there is?

[Stephen Chennney, 2014-09-11 18:22:39]

So disabling region tracking does indeed fix my bug, and doesn't seem to cause
any layout test regressions (I do apply the fix in this patch).

I'll look into where that info gets used. Last time I checked I thought it was
used, but maybe that's old info or maybe it's in the process of going away. If
either of those are the case, I say we apply this patch + disable for display
list record and suffer the consequences until the code is removed entirely.

[Stephen Chennney, 2014-09-11 20:39:40]

This doc, Chromium accounts please, has my afternoon's investigation.

https://docs.google.com/a/chromium.org/document/d/137hjQyBH7C8J36sHmVdy2Mj9Cu...

Ultimately, I believe we need to maintain a tracking region for each display
list, and merge the region with the existing region when we draw the picture. We
need this because the display list can both create and destroy opaque coverage,
so it's not correct to just call didDrawBounded. We could simply call
didDrawUnbounded, but that would probably destroy too much opacity.

[f(malita), 2014-09-11 20:43:18]

On 2014/09/11 20:39:40, Stephen Chenney wrote:
> This doc, Chromium accounts please, has my afternoon's investigation.
> 
>
https://docs.google.com/a/chromium.org/document/d/137hjQyBH7C8J36sHmVdy2Mj9Cu...
> 
> Ultimately, I believe we need to maintain a tracking region for each display
> list, and merge the region with the existing region when we draw the picture.
We
> need this because the display list can both create and destroy opaque
coverage,
> so it's not correct to just call didDrawBounded.

Isn't that what didDrawBounded implies though (as opposed to didDrawRect)? At
least that's what I gather looking at other users: drawOval, drawRRect,
fillBetweenRoundedRects & fillRoundedRect.

[danakj, 2014-09-11 20:58:10]

On Thu, Sep 11, 2014 at 4:43 PM, <fmalita@chromium.org> wrote:

> Reviewers: Stephen Chenney, danakj,
>
> Message:
> On 2014/09/11 20:39:40, Stephen Chenney wrote:
>
>> This doc, Chromium accounts please, has my afternoon's investigation.
>>
>
>
> https://docs.google.com/a/chromium.org/document/d/
> 137hjQyBH7C8J36sHmVdy2Mj9CutqITyRSKncklc7iDo/edit?usp=sharing
>
>  Ultimately, I believe we need to maintain a tracking region for each
>> display
>> list, and merge the region with the existing region when we draw the
>> picture.
>>
> We
>
>> need this because the display list can both create and destroy opaque
>>
> coverage,
>
>> so it's not correct to just call didDrawBounded.
>>
>
> Isn't that what didDrawBounded implies though (as opposed to didDrawRect)?
> At
> least that's what I gather looking at other users: drawOval, drawRRect,
> fillBetweenRoundedRects & fillRoundedRect.
>

I believe didDrawUnbounded should either leave the occlusion alone
(unbounded opaque-preserving draw) or completely clear the opaque region
(unbounded opaque-destroying draw).

Merging gives you more data, but clearing the opaque region should never
give an incorrect rendering result, only potentially increase cost to draw
it.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Stephen Chennney, 2014-09-11 21:06:19]

On 2014/09/11 20:39:40, Stephen Chenney wrote:
> This doc, Chromium accounts please, has my afternoon's investigation.
> 
>
https://docs.google.com/a/chromium.org/document/d/137hjQyBH7C8J36sHmVdy2Mj9Cu...
> 
> Ultimately, I believe we need to maintain a tracking region for each display
> list, and merge the region with the existing region when we draw the picture.
We
> need this because the display list can both create and destroy opaque
coverage,
> so it's not correct to just call didDrawBounded. We could simply call
> didDrawUnbounded, but that would probably destroy too much opacity.

Correction, the effect of this patch is to explicitly destroy opacity for the
entire picture area in the not-<canvas> case. It doesn't do anything in the
<canvas> case. But I think that <canvas> case is wrong if the intent is to
detect if any pixel at all is drawn in the region.

The problem with doing this right for "everything is a Display List" is that we
need to track three regions: "definitely opaque", "definitely not opaque", and
"don't know". You can't do that with just a single rect, so next best correct
thing is to explicitly assume "don't know" is "definitely not opaque".

To achieve that, the Display List can track definitely opaque, then when it is
drawn into the canvas, it does the current RegionTracker::didDraw with
fillsBounds true. That's at least better than this patch.

[f(malita), 2014-09-11 21:10:19]

On 2014/09/11 20:58:10, danakj wrote:
> On Thu, Sep 11, 2014 at 4:43 PM, <mailto:fmalita@chromium.org> wrote:
> 
> > Reviewers: Stephen Chenney, danakj,
> >
> > Message:
> > On 2014/09/11 20:39:40, Stephen Chenney wrote:
> >
> >> This doc, Chromium accounts please, has my afternoon's investigation.
> >>
> >
> >
> > https://docs.google.com/a/chromium.org/document/d/
> > 137hjQyBH7C8J36sHmVdy2Mj9CutqITyRSKncklc7iDo/edit?usp=sharing
> >
> >  Ultimately, I believe we need to maintain a tracking region for each
> >> display
> >> list, and merge the region with the existing region when we draw the
> >> picture.
> >>
> > We
> >
> >> need this because the display list can both create and destroy opaque
> >>
> > coverage,
> >
> >> so it's not correct to just call didDrawBounded.
> >>
> >
> > Isn't that what didDrawBounded implies though (as opposed to didDrawRect)?
> > At
> > least that's what I gather looking at other users: drawOval, drawRRect,
> > fillBetweenRoundedRects & fillRoundedRect.
> >
> 
> I believe didDrawUnbounded should either leave the occlusion alone
> (unbounded opaque-preserving draw) or completely clear the opaque region
> (unbounded opaque-destroying draw).
> 
> Merging gives you more data, but clearing the opaque region should never
> give an incorrect rendering result, only potentially increase cost to draw
> it.

So to err on the conservative side (while minimizing the affected region) we
should use didDrawBounded + an opacity destroying paint, right?

I'm hesitant about per-display-list region tracking (per comments it seems
unclear how much it's being used these days): since we don't know whether we're
going to need it at drawDisplayList time, we would have to always enable it for
all DLs.

[f(malita), 2014-09-11 21:17:16]

On 2014/09/11 21:06:19, Stephen Chenney wrote:
> The problem with doing this right for "everything is a Display List" is that
we
> need to track three regions: "definitely opaque", "definitely not opaque", and
> "don't know". You can't do that with just a single rect, so next best correct
> thing is to explicitly assume "don't know" is "definitely not opaque".

Isn't RegionTracker only dealing with a conservative estimate of the opaque
region? Everything else is "don't know", so it's fine to err on the side of "not
opaque" -- like we do for a bunch of primitives. We don't need "definitely not
opaque" AFAICT.

[Stephen Chennney, 2014-09-11 21:18:15]

On 2014/09/11 20:43:18, Florin Malita wrote:
> On 2014/09/11 20:39:40, Stephen Chenney wrote:
> > This doc, Chromium accounts please, has my afternoon's investigation.
> > 
> >
>
https://docs.google.com/a/chromium.org/document/d/137hjQyBH7C8J36sHmVdy2Mj9Cu...
> > 
> > Ultimately, I believe we need to maintain a tracking region for each display
> > list, and merge the region with the existing region when we draw the
picture.
> We
> > need this because the display list can both create and destroy opaque
> coverage,
> > so it's not correct to just call didDrawBounded.
> 
> Isn't that what didDrawBounded implies though (as opposed to didDrawRect)? At
> least that's what I gather looking at other users: drawOval, drawRRect,
> fillBetweenRoundedRects & fillRoundedRect.

didDrawBounded can only mark as not-opaque in the non-<canvas> case:
If the paint does not allow bounding
  a. But it is opaque nothing is done
  b. Otherwise the rect is marked not-opaque
Otherwise, if the paint allows bounding
  c. If the clip is not a rect or the rect we are drawing is clipped, do nothing
  d. Otherwise if the transfer mode maintains opacity, do nothing
  e. Otherwise mark the rect as not-opaque

For display list we always do (a), I think.

[Stephen Chennney, 2014-09-11 21:20:41]

On 2014/09/11 21:17:16, Florin Malita wrote:
> On 2014/09/11 21:06:19, Stephen Chenney wrote:
> > The problem with doing this right for "everything is a Display List" is that
> we
> > need to track three regions: "definitely opaque", "definitely not opaque",
and
> > "don't know". You can't do that with just a single rect, so next best
correct
> > thing is to explicitly assume "don't know" is "definitely not opaque".
> 
> Isn't RegionTracker only dealing with a conservative estimate of the opaque
> region? Everything else is "don't know", so it's fine to err on the side of
"not
> opaque" -- like we do for a bunch of primitives. We don't need "definitely not
> opaque" AFAICT.

I think we sometimes need to explicitly mark as not-opaque if the display list
contains content that destroys opacity, like a non-opacity-preserving transfer
mode.

[f(malita), 2014-09-11 21:23:02]

On 2014/09/11 21:20:41, Stephen Chenney wrote:
> On 2014/09/11 21:17:16, Florin Malita wrote:
> > On 2014/09/11 21:06:19, Stephen Chenney wrote:
> > > The problem with doing this right for "everything is a Display List" is
that
> > we
> > > need to track three regions: "definitely opaque", "definitely not opaque",
> and
> > > "don't know". You can't do that with just a single rect, so next best
> correct
> > > thing is to explicitly assume "don't know" is "definitely not opaque".
> > 
> > Isn't RegionTracker only dealing with a conservative estimate of the opaque
> > region? Everything else is "don't know", so it's fine to err on the side of
> "not
> > opaque" -- like we do for a bunch of primitives. We don't need "definitely
not
> > opaque" AFAICT.
> 
> I think we sometimes need to explicitly mark as not-opaque if the display list
> contains content that destroys opacity, like a non-opacity-preserving transfer
> mode.

Right, and the suggestion is to always/conservatively mark the whole DL rect as
non-opaque, at least initially. This is correct RegionTracker semantics IUUC.

[danakj, 2014-09-11 23:48:06]

On Thu, Sep 11, 2014 at 5:23 PM, <fmalita@chromium.org> wrote:

> On 2014/09/11 21:20:41, Stephen Chenney wrote:
>
>> On 2014/09/11 21:17:16, Florin Malita wrote:
>> > On 2014/09/11 21:06:19, Stephen Chenney wrote:
>> > > The problem with doing this right for "everything is a Display List"
>> is
>>
> that
>
>> > we
>> > > need to track three regions: "definitely opaque", "definitely not
>> opaque",
>> and
>> > > "don't know". You can't do that with just a single rect, so next best
>> correct
>> > > thing is to explicitly assume "don't know" is "definitely not opaque".
>> >
>> > Isn't RegionTracker only dealing with a conservative estimate of the
>> opaque
>> > region? Everything else is "don't know", so it's fine to err on the
>> side of
>> "not
>> > opaque" -- like we do for a bunch of primitives. We don't need
>> "definitely
>>
> not
>
>> > opaque" AFAICT.
>>
>
>  I think we sometimes need to explicitly mark as not-opaque if the display
>> list
>> contains content that destroys opacity, like a non-opacity-preserving
>> transfer
>> mode.
>>
>
> Right, and the suggestion is to always/conservatively mark the whole DL
> rect as
> non-opaque, at least initially. This is correct RegionTracker semantics
> IUUC.
>

Yeh. It's a worst-case scenario for tracking, but it also should not be
possible to be wrong, because it's only when something is opaque we deviate
and skip things below it and turn off blending and such. If nothing is
considered opaque we must try show all the things even if it's waste.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2014-09-12 01:00:55]

On Thu, Sep 11, 2014 at 4:39 PM, <schenney@chromium.org> wrote:

> This doc, Chromium accounts please, has my afternoon's investigation.
>
> https://docs.google.com/a/chromium.org/document/d/
> 137hjQyBH7C8J36sHmVdy2Mj9CutqITyRSKncklc7iDo/edit?usp=sharing
>
> Ultimately, I believe we need to maintain a tracking region for each
> display
> list, and merge the region with the existing region when we draw the
> picture. We
> need this because the display list can both create and destroy opaque
> coverage,
> so it's not correct to just call didDrawBounded. We could simply call
> didDrawUnbounded, but that would probably destroy too much opacity.
>

I've put up CLs to remove the opaque rect completely from the chromium side
(minus need some cross repo hopping for the API boundary), and to remove
the OpaqueRectTrackingContentLayerDelegate. When those are gone, it's not
clear to me if the other callers of GraphicsContext::setRegionTrackingMode
can all go too, but probably they can? I'll leave that to you.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Stephen Chennney, 2014-09-15 20:33:08]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2014-09-15 20:33:09]

lgtm

[commit-bot: I haz the power, 2014-09-15 20:34:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555703004/50001

[commit-bot: I haz the power, 2014-09-15 20:39:08]

Committed patchset #4 (id:50001) as 182002