cc: Do not create separate tilings for almost equal scale factors.

cc: Do not create separate tilings for almost equal scale factors.

Different scale factors which are almost equal (i.e. differ by some
threshold magnitude of floating point epsilon) create different
tilings when content is rigourously zoomed.

e.g. If scales are 7.33907556533813f and 7.33907508850098f, different
tilings get created, when scale is changed from one to other. Although
tiles are shared between different tilings, the contents at previous
scale needs to be re-rastered with new scale. This causes unnecessary
performance hit.

The new function added converts the scale factor with given floating
point to fixed precision floating point. This makes above two scale
factors considered as equal. The above two scales will get converted
as given below, when precision is set as 4.

7.33907556533813 -> 7.339000225067138671875
7.33907508850098 -> 7.339000225067138671875

BUG=327166
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[prashant.n, 2015-08-26 16:32:17]

prashant.n@samsung.com changed reviewers:
+ danakj@chromium.org, enne@chromium.org, vmpstr@chromium.org

[prashant.n, 2015-08-27 16:57:50]

PTAL.

[enne (OOO), 2015-08-27 18:08:34]

Test?

https://codereview.chromium.org/1321503002/diff/40001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:79: const float factor = pow(10,
kScalePrecision);
It is weird to do math with base 10.  Surely there's some more standard way to
do this.

https://codereview.chromium.org/1321503002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:944: raster_contents_scale_ =
GetFixedPrecisionValue(ideal_contents_scale_);
Do we really need fixed precision for all of these? I think contents is really
the only important one.

[prashant.n, 2015-09-01 16:55:25]

I'll add the test once RoundTo method is finalized.

https://codereview.chromium.org/1321503002/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:65: float fractional = std::modf(value,
&integral);
I tried checking standard way for doing this. Definitely there should be based
on logarithmic function. But I still did not get it. I'll try to work on it.

Meanwhile enne@/danakj, if you know this rounding can you pls. suggest here.

https://codereview.chromium.org/1321503002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:930: raster_contents_scale_ =
RoundToFixedPrecision(ideal_contents_scale_);
I thought of rounding all raster scales, so that all are in sync. When checked
by applying only for contents scale I did not see any issue.

[prashant.n, 2015-10-16 14:44:44]

PTAL.

[vmpstr, 2015-10-16 17:58:17]

https://codereview.chromium.org/1321503002/diff/100001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/100001/cc/base/math_util.cc#n...
cc/base/math_util.cc:124: const int kScalePrecision = 4;
Can you just write const float kFactor = 1e4 instead? And remove the dcheck?

Or better yet, take the precision as a parameter to this function.
MathUtil::RoundToFixedPrecision(foo) is not clear at all that it will round to 4
decimal places. (and why 4, btw?)

https://codereview.chromium.org/1321503002/diff/100001/cc/base/math_util.cc#n...
cc/base/math_util.cc:132: fractional *= factor;
nit: I'd just write these three things in one line

fractional = std::round(factor * fractional) / factor;

[prashant.n, 2015-10-17 01:28:13]

https://codereview.chromium.org/1321503002/diff/100001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/100001/cc/base/math_util.cc#n...
cc/base/math_util.cc:124: const int kScalePrecision = 4;
On 2015/10/16 17:58:17, vmpstr wrote:
> Can you just write const float kFactor = 1e4 instead? And remove the dcheck?
> 
> Or better yet, take the precision as a parameter to this function.
> MathUtil::RoundToFixedPrecision(foo) is not clear at all that it will round to
4
> decimal places. (and why 4, btw?)

I'll add precision as a parameter, I was thinking to add this earlier, but then
thought of 3 digits fractional part is sufficient so let it be fixed value here.
4 is for having 3 digits fractional part, e.g. 3.336f for scale factor.

[prashant.n, 2015-10-17 14:44:05]

PTAL.

[ericrk, 2015-10-19 16:40:50]

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

[ericrk, 2015-10-19 16:40:50]

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.cc#n...
cc/base/math_util.cc:125: if (precision < 1 || precision >
(std::numeric_limits<float>::digits10 - 1))
drive-by-nit: Should we DCHECK that precision >= 1? Or clamp to 1? returning the
full-precision number when a caller asks for precision 0 seems weird?

[vmpstr, 2015-10-19 18:58:38]

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.cc#n...
cc/base/math_util.cc:125: if (precision < 1 || precision >
(std::numeric_limits<float>::digits10 - 1))
On 2015/10/19 16:40:50, ericrk wrote:
> drive-by-nit: Should we DCHECK that precision >= 1? Or clamp to 1? returning
the
> full-precision number when a caller asks for precision 0 seems weird?

I agree, this should be a DCHECK instead of an early out.

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util.h#ne...
cc/base/math_util.h:100: // Rounds the given |value| to fixed precision. This
makes amlost euqal
typo: amlost euqal -> almost equal

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util_unit...
File cc/base/math_util_unittest.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util_unit...
cc/base/math_util_unittest.cc:388: float f1 = 7.33907556533813f;
nit: f1 -> first_value, f2 -> second_value (or something more descriptive)

https://codereview.chromium.org/1321503002/diff/200001/cc/base/math_util_unit...
cc/base/math_util_unittest.cc:398: float f3 =
MathUtil::RoundToFixedPrecision(f1, i);
nit: f3 -> rounded_first_value, f2 -> rounded_second_value (or something more
descriptive)

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:954:
MathUtil::RoundToFixedPrecision(ideal_contents_scale_, kScalePrecision);
My understanding is that with this, something like 1.004 would become 1.0,
right? (or maybe 1.0004 becomes 1.0). In either case, wouldn't this cause
blurriness? Since 1.0 is actually not the ideal scale, but 1.004 is. I'm not
saying the blurriness would be excessive, but it's probably also not too small
(.004 is 1 pixel in every 250, right?)

Can you fake a pinch scale of 1.004 before and after your patch and have two
screenshots to compare? Something with a lot of text probably would show any
artifacts. I don't know if that's easy to do though...

[prashant.n, 2015-10-20 18:45:24]

Pls. take a look at rounding algorithm modified. I'll check the contents
rastered with different almost equal scale factors soon.

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:954:
MathUtil::RoundToFixedPrecision(ideal_contents_scale_, kScalePrecision);
We set scale precision as 4. This will give 4 digits in fractional part, IMO,
which is sufficient for distinguishing two scales. e.g. 1.1234. The ideal scale
gets used further as raster contents scale. So there would not occur blurriness.
However I'll check contents with two different scales.

[enne (OOO), 2015-10-20 18:54:57]

On 2015/10/20 at 18:45:24, prashant.n wrote:
> Pls. take a look at rounding algorithm modified. I'll check the contents
rastered with different almost equal scale factors soon.
> 
>
https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
> cc/layers/picture_layer_impl.cc:954:
MathUtil::RoundToFixedPrecision(ideal_contents_scale_, kScalePrecision);
> We set scale precision as 4. This will give 4 digits in fractional part, IMO,
which is sufficient for distinguishing two scales. e.g. 1.1234. The ideal scale
gets used further as raster contents scale. So there would not occur blurriness.
However I'll check contents with two different scales.

When we shipped impl-side painting, we introduced an additional 0.5 pixels to
the total height of root layers which caused terrible text smearing.  If you
make a guess that webpages are 2k pixels tall, that's a 1.00025 scale.  I am a
bit nervous about rounding these values off and just assuming there will not be
blurriness.

How often does this near-collision of scales happen?

[vmpstr, 2015-10-20 18:55:23]

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:954:
MathUtil::RoundToFixedPrecision(ideal_contents_scale_, kScalePrecision);
On 2015/10/20 18:45:24, prashant.n wrote:
> We set scale precision as 4. This will give 4 digits in fractional part, IMO,
> which is sufficient for distinguishing two scales. e.g. 1.1234. The ideal
scale
> gets used further as raster contents scale. So there would not occur
blurriness.
> However I'll check contents with two different scales.

Yeah you're probably right, but I'd still like to see concrete examples (or at
least some verification). Mostly because it's hard to reason about small float
difference when they affect large layers.

https://codereview.chromium.org/1321503002/diff/220001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/220001/cc/base/math_util.cc#n...
cc/base/math_util.cc:134: int digits10_in_integral_part =
I wouldn't worry about the precision being too high for us to even be able to
represent the number by a float. If that happens that's a bug in the calling
code.

I would say delete this code (since it's also making the function slower to try
and catch caller code bugs). Just say DCHECK_GE(precision, 0); and
DCHECK_LE(precision, std::numeric_limits<float>::digits10);

After that, just assume that the input is sane. You can leave a detailed comment
about this math in the header near this function to explain that the bigger the
float that you have, the smaller the precision needs to be to make a difference.

[prashant.n, 2015-10-26 15:53:49]

> When we shipped impl-side painting, we introduced an additional 0.5 pixels to
> the total height of root layers which caused terrible text smearing.  If you
> make a guess that webpages are 2k pixels tall, that's a 1.00025 scale.  I am a
> bit nervous about rounding these values off and just assuming there will not
be
> blurriness.

IMO, the chromium is able to render given scale. Let's say instead of scale1 =
7.33907556533813, if ideal scale computed comes as scale_rounded =
7.339000225067138671875, then rastering should happen in the later scale only. I
did not assume content to be 2k pixels tall.

1. Do you mean to say by computation of scale factors it will not come across
the value of scale_rounded and that would cause blurriness? I don't think so.
2. IMO, both GPU and CPU raster methods should be able to raster the contents in
the given scale.

WDYT?

However I've been checking with different near scales on desktop and android.


> How often does this near-collision of scales happen?

This is frequently reproducible when pinch zoom is done rigourously such a way
that touch move points cross edges.

[prashant.n, 2015-10-26 15:54:19]

https://codereview.chromium.org/1321503002/diff/220001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1321503002/diff/220001/cc/base/math_util.cc#n...
cc/base/math_util.cc:134: int digits10_in_integral_part =
This additional code adds ~25% overhead. Without this code, it takes ~ 80 nano
seconds and with this additional code, it takes ~ 110 nano seconds on an
average. However we'll decide on this, once the rest of the code is finalized.

[prashant.n, 2015-10-26 16:35:04]

https://codereview.chromium.org/1321503002/diff/240001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1321503002/diff/240001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:1203: std::max(GetIdealContentsScale(),
min_contents_scale), kScalePrecision);
Modifying ideal page and contents scales are needed as they are checked against
in ShouldAdjustRasterScale() and RecalculateRasterScales() functions.

[prashant.n, 2015-10-26 19:11:35]

I guess the main problem would be smooth sequence of scale factors in zooming.

[prashant.n, 2015-10-28 14:29:34]

On 2015/10/26 19:11:35, prashant.n wrote:
> I guess the main problem would be smooth sequence of scale factors in zooming.

Dear All,

I did further analysis on this and attached my analysis in the bug. Kindly check
and do let me know the opinion for proceeding on this patch.