When tracking regions, treat sub-pixel areas as belonging to the region.

When tracking regions, treat sub-pixel areas as belonging to the region.

It is critical to display list canvases, due to the fact, that we fallback from
rendering on impl thread, when the canvas is not fully redrawn inside each
frame. On Browsermark 2 it is actually messed up due to floating point rounding.
The area in SkScalar is {0.00061, 0.00061, 1599.99997, 999.99998}, rounded down
to {1,1,1599,999} and the entire canvas area seems to be not redrawn.

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

[Sergey, 2014-10-22 02:33:43]

p.sergey@samsung.com changed reviewers:
+ junov@chromium.org

[Sergey, 2014-10-22 02:35:45]

Junov, PTAL. This actually fixes Browsermark from falling back on display list
canvases.

I've also checked and it seems this change does not break any layout tests.

[Sergey, 2014-10-22 02:36:10]

p.sergey@samsung.com changed reviewers:
+ jinho.bang@samsung.com

[zino, 2014-10-22 04:56:19]

Looks reasonable to me.

non-owner lgtm.

[Justin Novosad, 2014-10-22 14:49:53]

On 2014/10/22 04:56:19, zino wrote:
> Looks reasonable to me.
> 
> non-owner lgtm.

Thank you for finding this! I disagree with this fix. It only makes sense to
grow the rect if the bounds are very close to the next integer. For example, if
you round up fRight from 10.1 to 11, you can get rendering artifacts.

I think something like this would be more appropriate:

const float epsilon = 1.0f/512.0f; // large enough to accommodate machine
precision issues, small enough to have a negligible effect on rendered results.
int left = SkScalarCeilToInt(m_opaqueRect.fLeft - epsilon);
int top = SkScalarCeilToInt(m_opaqueRect.fTop - epsilon);
int right = SkScalarFloorToInt(m_opaqueRect.fRight + epsilon);
int bottom = SkScalarFloorToInt(m_opaqueRect.fBottom + epsilon);

[danakj, 2014-10-22 14:57:34]

On 2014/10/22 14:49:53, junov wrote:
> On 2014/10/22 04:56:19, zino wrote:
> > Looks reasonable to me.
> > 
> > non-owner lgtm.
> 
> Thank you for finding this! I disagree with this fix. It only makes sense to
> grow the rect if the bounds are very close to the next integer. For example,
if
> you round up fRight from 10.1 to 11, you can get rendering artifacts.
> 
> I think something like this would be more appropriate:
> 
> const float epsilon = 1.0f/512.0f; // large enough to accommodate machine
> precision issues, small enough to have a negligible effect on rendered
results.
> int left = SkScalarCeilToInt(m_opaqueRect.fLeft - epsilon);
> int top = SkScalarCeilToInt(m_opaqueRect.fTop - epsilon);
> int right = SkScalarFloorToInt(m_opaqueRect.fRight + epsilon);
> int bottom = SkScalarFloorToInt(m_opaqueRect.fBottom + epsilon);

To you want an enclosing or an enclosed rect?

[Justin Novosad, 2014-10-22 15:02:14]

On 2014/10/22 14:57:34, danakj wrote:
> On 2014/10/22 14:49:53, junov wrote:
> > On 2014/10/22 04:56:19, zino wrote:
> > > Looks reasonable to me.
> > > 
> > > non-owner lgtm.
> > 
> > Thank you for finding this! I disagree with this fix. It only makes sense to
> > grow the rect if the bounds are very close to the next integer. For example,
> if
> > you round up fRight from 10.1 to 11, you can get rendering artifacts.
> > 
> > I think something like this would be more appropriate:
> > 
> > const float epsilon = 1.0f/512.0f; // large enough to accommodate machine
> > precision issues, small enough to have a negligible effect on rendered
> results.
> > int left = SkScalarCeilToInt(m_opaqueRect.fLeft - epsilon);
> > int top = SkScalarCeilToInt(m_opaqueRect.fTop - epsilon);
> > int right = SkScalarFloorToInt(m_opaqueRect.fRight + epsilon);
> > int bottom = SkScalarFloorToInt(m_opaqueRect.fBottom + epsilon);
> 
> To you want an enclosing or an enclosed rect?

Enclosed

[Sergey, 2014-10-23 03:12:25]

On 2014/10/22 14:49:53, junov wrote:
> On 2014/10/22 04:56:19, zino wrote:
> > Looks reasonable to me.
> > 
> > non-owner lgtm.
> 
> Thank you for finding this! I disagree with this fix. It only makes sense to
> grow the rect if the bounds are very close to the next integer. For example,
if
> you round up fRight from 10.1 to 11, you can get rendering artifacts.

Sorry, Junov, could you please explain more for me? My thought is that right now
fRight gets rounded from 10.9999 to 10 and nobody sees any rendering artifacts.
So, why would it make a difference if we will start rounding it in another
direction?

Another question is why would not we set epsilon = 0.5f? For me it kind of seems
more reasonable... WDYT?

Do you have in mind some examples, where this change would make visual
differences? If you do, could you share them, so that I would understand your
point more clear?

[Justin Novosad, 2014-10-23 03:44:58]

Ok, let me add some clarity.

In the case of the display list 2D canvas feature, what we want to determine is
whether the current frame's draw operations completely overwrite the previous
frame.
Suppose the canvas has a size of 10x10. 
What we are looking for is an opaque region that entirely covers the (0, 0)-(10,
10) rectangular region.
An opaque region that covers (0, 0)-(10, 9.9999) does not technically meet this
criteria, but it is close enough that we can make an exception, which we
attribute to machine precision.
An opaque region that covers (0, 0)-(10, 9.7) is not good enough for two
reasons:
a) the last row of pixels may be semi-transparent due to anti-aliasing and will
therefore not occlude the previous frame.
b) eventually we want to add a special mode that allows the compositor to scale
the canvas automatically to render its contents at a resolution that matches the
display resolution. Once that scale factor is applied, that 0.3 pixel margin (in
CSS pixels) may actually represent more than 1 physical pixel on the display.

[Sergey, 2014-10-23 04:45:41]

On 2014/10/23 03:44:58, junov wrote:
> Ok, let me add some clarity.
> 
> In the case of the display list 2D canvas feature, what we want to determine
is
> whether the current frame's draw operations completely overwrite the previous
> frame.
> Suppose the canvas has a size of 10x10. 
> What we are looking for is an opaque region that entirely covers the (0,
0)-(10,
> 10) rectangular region.
> An opaque region that covers (0, 0)-(10, 9.9999) does not technically meet
this
> criteria, but it is close enough that we can make an exception, which we
> attribute to machine precision.
> An opaque region that covers (0, 0)-(10, 9.7) is not good enough for two
> reasons:
> a) the last row of pixels may be semi-transparent due to anti-aliasing and
will
> therefore not occlude the previous frame.
> b) eventually we want to add a special mode that allows the compositor to
scale
> the canvas automatically to render its contents at a resolution that matches
the
> display resolution. Once that scale factor is applied, that 0.3 pixel margin
(in
> CSS pixels) may actually represent more than 1 physical pixel on the display.

I get it, thank you.
Will check your proposal and upload new patch soon.

[Sergey, 2014-10-23 07:32:04]

On 2014/10/22 14:49:53, junov wrote:
> I think something like this would be more appropriate:
> 
> const float epsilon = 1.0f/512.0f; // large enough to accommodate machine
> precision issues, small enough to have a negligible effect on rendered
results.
> int left = SkScalarCeilToInt(m_opaqueRect.fLeft - epsilon);
> int top = SkScalarCeilToInt(m_opaqueRect.fTop - epsilon);
> int right = SkScalarFloorToInt(m_opaqueRect.fRight + epsilon);
> int bottom = SkScalarFloorToInt(m_opaqueRect.fBottom + epsilon);

PTAL. Works fine for Browsermark.

[Justin Novosad, 2014-10-23 14:29:18]

On 2014/10/23 07:32:04, Sergey wrote:
> On 2014/10/22 14:49:53, junov wrote:
> > I think something like this would be more appropriate:
> > 
> > const float epsilon = 1.0f/512.0f; // large enough to accommodate machine
> > precision issues, small enough to have a negligible effect on rendered
> results.
> > int left = SkScalarCeilToInt(m_opaqueRect.fLeft - epsilon);
> > int top = SkScalarCeilToInt(m_opaqueRect.fTop - epsilon);
> > int right = SkScalarFloorToInt(m_opaqueRect.fRight + epsilon);
> > int bottom = SkScalarFloorToInt(m_opaqueRect.fBottom + epsilon);
> 
> PTAL. Works fine for Browsermark.

lgtm

[Sergey, 2014-10-24 01:58:56]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-10-24 02:00:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666253003/20001

[commit-bot: I haz the power, 2014-10-24 02:13:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 02:13:03]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Sergey, 2014-10-24 04:29:25]

The CQ bit was checked by p.sergey@samsung.com

[commit-bot: I haz the power, 2014-10-24 04:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666253003/20001

[commit-bot: I haz the power, 2014-10-24 05:09:10]

Committed patchset #2 (id:20001) as 184327