gfx: Converge scale precision related differences to single class.

gfx: Converge scale precision related differences to single class.

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.

This patch adds gfx::Scale class, which converts the scale factor
with given floating point to fixed precision floating point. This makes
above two scale factors appear as equal. The above two scales will get
converted as given below, when precision is set as 3.

7.33907556533813 -> 7.339000225067138671875
7.33907508850098 -> 7.339000225067138671875

This patch just adds Scale class and its unit test. Using gfx::Scale
class the precision related differences can be brought into single
class and in it by converting the scale to fixed precision, the
peformance hit could be solved.

BUG=327166

[prashant.n, 2015-08-24 17:07:35]

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

[prashant.n, 2015-08-24 17:07:35]

PTAL.

[enne (OOO), 2015-08-24 18:57:59]

This seems a bit like overkill.  I think this could just be a few lines of code
in PictureLayerImpl if this was something we wanted.  Do you see another use for
this?

[prashant.n, 2015-08-25 00:08:16]

On 2015/08/24 18:57:59, enne wrote:
> This seems a bit like overkill.  I think this could just be a few lines of
code
> in PictureLayerImpl if this was something we wanted.  Do you see another use
for
> this?
I initially thought the same, we could have the small fix in find tilings and
add tilings. But it would cause rendering artifacts as some other scales used in
picture layer tiling are not fixed precision values.

Better would be to move scale precision related issues to single file, where we
can have solutions like fixed precision values. So whole cc module talks scale
in gfx::Scale instead of float. Also later if needed single precision values can
be changed double precision or some other.

This is first patch which provides basic class. In subsequent patches we can add
other files use gfx::Scale and check with runtime switch. Once stable we can
enable this as by default.

[enne (OOO), 2015-08-25 00:11:43]

Using gfx::Scale elsewhere is also overkill.  The only place that needs this
snapping is really in picking a tiling scale, I think.

[danakj, 2015-08-25 00:20:55]

Ya I'm not sure. Can you give other examples that would be useful to have
this for?

On Mon, Aug 24, 2015 at 5:11 PM, <enne@chromium.org> wrote:

> Using gfx::Scale elsewhere is also overkill.  The only place that needs
> this
> snapping is really in picking a tiling scale, I think.
>
> https://codereview.chromium.org/1315633002/
>

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

[prashant.n, 2015-08-25 15:46:52]

On 2015/08/25 00:20:55, danakj wrote:
> Ya I'm not sure. Can you give other examples that would be useful to have
> this for?
> 

The need for having uniform Scale class across the code base is due to following
reasons.


1. PictureLayerImpl::UpdateTiles() depends on
PictureLayerImpl::RecalculateRasterScales(),
PictureLayerImpl::AddTilingsForRasterScale() and
PictureLayerImpl::ShouldAdjustRasterScale().

So updating snapped scale factor at one place will not help.
e.g. PictureLayerImpl::ShouldAdjustRasterScale() would break as it depends on
ideal_page_scale_
ideal_device_scale_
ideal_source_scale_
ideal_contents_scale_
raster_page_scale_
raster_device_scale_
raster_source_scale_
raster_contents_scale_
MaximumContentsScale()
MinimumContentsScale()

There are many functions in PictureLayerImpl and one function in
PictureImageLayerImpl
which are dependent on MaximumContentsScale() and MinimumContentsScale().

Also PictureLayerTilingSet::GetSnappedContentsScale(), AppendQuads(),
PushPropertiesTo(),
LayerTreeImpl class also need to be taken care.

2. The scales for different types of layers should be in sync. e.g. picture
layer and
video layer if scaled on page should have same scales.

More, at any object where we have scale information should be same as with the
other,
if we provide the same float value to these different objects.

3. JS scrollto, scrollby methods should yield same %of distance in all scale
factors.

4. ScrollBar layer thumb rect and scaled contents scrolled should be in sync.

For last two the scaling is handled separately than functions getting called, so
not sure.

[danakj, 2015-08-25 18:30:12]

On 2015/08/25 15:46:52, prashant.n wrote:
> On 2015/08/25 00:20:55, danakj wrote:
> > Ya I'm not sure. Can you give other examples that would be useful to have
> > this for?
> > 
> 
> The need for having uniform Scale class across the code base is due to
following
> reasons.
> 
> 
> 1. PictureLayerImpl::UpdateTiles() depends on
> PictureLayerImpl::RecalculateRasterScales(),
> PictureLayerImpl::AddTilingsForRasterScale() and
> PictureLayerImpl::ShouldAdjustRasterScale().
> 
> So updating snapped scale factor at one place will not help.
> e.g. PictureLayerImpl::ShouldAdjustRasterScale() would break as it depends on
> ideal_page_scale_
> ideal_device_scale_
> ideal_source_scale_
> ideal_contents_scale_
> raster_page_scale_
> raster_device_scale_
> raster_source_scale_
> raster_contents_scale_
> MaximumContentsScale()
> MinimumContentsScale()
> 
> There are many functions in PictureLayerImpl and one function in
> PictureImageLayerImpl
> which are dependent on MaximumContentsScale() and MinimumContentsScale().
> 
> Also PictureLayerTilingSet::GetSnappedContentsScale(), AppendQuads(),
> PushPropertiesTo(),
> LayerTreeImpl class also need to be taken care.

I don't understand what Min/Max scale have to do with things. We're talking
about choosing a scale at which to raster content for a PictureLayerImpl. That
scale needs to be <= Max and >= Min. How does snapping those to a fixed value
help? I don't see why we would create extra tilings we didn't mean/need to by
not snapping the min/max values. They are only outer bounds.

Why does AppendQuads need to care? It just draws based on the scale of the
content and the scale it wants to display. The problem is that we're making
content at different scales. If we don't do that AppendQuads should work fine
without any changes?

> 2. The scales for different types of layers should be in sync. e.g. picture
> layer and
> video layer if scaled on page should have same scales.

I don't understand what video layer would have to do with this. The video
content is at a fixed scale. If you draw it at some different-than-ideal scale,
you won't fill the space for the video with video, or you'll clip it. I think we
just want to draw it at the exact scale always.

> More, at any object where we have scale information should be same as with the
> other,
> if we provide the same float value to these different objects.
> 
> 3. JS scrollto, scrollby methods should yield same %of distance in all scale
> factors.

JS/scrolling happens in a page-scale-independent space. Not sure what this has
to do with it.

> 4. ScrollBar layer thumb rect and scaled contents scrolled should be in sync.

If you're worried about making a different method for applying scale here, maybe
a helper method could solve that. Not sure it's a problem here though. Maybe you
could demonstrate with pictures.

> For last two the scaling is handled separately than functions getting called,
so
> not sure.

[prashant.n, 2015-08-26 04:37:22]

> 
> If you're worried about making a different method for applying scale here,
maybe
> a helper method could solve that. Not sure it's a problem here though. Maybe
you
> could demonstrate with pictures.
> 

I'll put the patch for local fix in PictureLayerImpl and will check for the
rendering
artifacts and if they are appearing I'll upload them in bug.

[prashant.n, 2015-08-26 16:31:12]

> 
> I'll put the patch for local fix in PictureLayerImpl and will check for the
> rendering
> artifacts and if they are appearing I'll upload them in bug.

https://codereview.chromium.org/1321503002 created with quick fix. The bug is
not reproducible with the fix added.

Unfortunately I did not see any rendering artifacts I mentioned on chrome:/gpu
page. I'll continue to test and update if any rendering artifact is seen.

[prashant.n, 2015-08-27 17:03:43]

The problems I've mentioned are very rare edge cases as based on following
assumption.

1.f / scale_factor, will give float value which will have decimal places more
than 4,
which can result into different float numbers.

Another quick fix pushed, fixes the issue mentioned in the bug 327166. So kindly
review
that CL.