Invert DSF to map from delegated frame to layer space

Invert DSF to map from delegated frame to layer space

The size of the frame data from a delegated source will not in general match up
exactly with the delegated layer's bounds. Frame sizes and layer bounds are both
expressed as integers, but frame sizes are in physical pixels and layer bounds
are in device-independent pixels. If the device scale is not an integer, there
may not be an exact mapping between the two spaces. For instance, if the layer
size is 105 x 211 and the device scale is 1.5, the delegated content is
responsible for filling 157.5 x 316.5 physical pixels in the final output. This
isn't really possible so what actually happens is the delegated renderer
produces a frame with a physical size of 158 x 317 pixels and sends that up to
the DRLI. To map this frame into physical space, the DRLI should apply the
inverse of the device scale factor to reflect that the frame covers 105.333 x
211.3333 logical pixels.

Before this patch, DRLI would attempt to scale the frame by (105/158, 211/317)
resulting in the frame being scaled down non-uniformly to try to make 158 x 317
pixels fit into a 157.5 x 316.6 space. This scaling looks bad and ends up
mapping pixels partially outside of the layer's logical bounds into the layer's
logical bounds.

BUG=370074
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275544

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276139

[jamesr, 2014-06-04 22:46:29]

This fixes the remaining scaling problems observable on windows with a 1.5 scale
factor.  There are still a pair of unit test failures checking the math that I
can't quite understand - comments inline. I think we probably should have some
more tests with non-integer scale factors - it seems we're only really testing
1.0 and 2.0 in most suites.

One open question with this is whether we should use the host's device scale
factor or the device scale factor from the delegating renderer, but I can't
think of any situation where we would want to combine data from compositor
instances using different DSFs.  Maybe there are some races with dragging
windows across monitors that this code should be aware of? If so, the right
behavior is probably to not draw at all with mismatched DSF's since scaling will
never look good.

https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
cc/layers/delegated_renderer_layer_impl_unittest.cc:714: expected,
root_delegated_shared_quad_state->content_to_target_transform);
this test produces an actual total translation of 17,17 instead of the expected
18,18, but I can't figure out why.

https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
cc/layers/delegated_renderer_layer_impl_unittest.cc:770: expected,
root_delegated_shared_quad_state->content_to_target_transform);
this one fails in the same way.

[jamesr, 2014-06-05 20:54:26]

Please double-check carefully, but with the TransformOrigin part removed the
rest of the delegated renderer impl unittest math seems pretty reasonable.

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (left):

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl_unittest.cc:514:
delegated_renderer_layer->SetTransformOrigin(
deleting this (which doesn't seem all that relevant to the rest of the test)
makes the math easier to follow for me. legit?

[danakj, 2014-06-05 21:52:51]

On Jun 4, 2014 6:46 PM, <jamesr@chromium.org> wrote:
>
> Reviewers: enne,
>
> Message:
> This fixes the remaining scaling problems observable on windows with a
1.5 scale
> factor.  There are still a pair of unit test failures checking the math
that I
> can't quite understand - comments inline. I think we probably should have
some
> more tests with non-integer scale factors - it seems we're only really
testing
> 1.0 and 2.0 in most suites.
>
> One open question with this is whether we should use the host's device
scale
> factor or the device scale factor from the delegating renderer, but I
can't
> think of any situation where we would want to combine data from compositor
> instances using different DSFs.  Maybe there are some races with dragging
> windows across monitors that this code should be aware of? If so, the
right
> behavior is probably to not draw at all with mismatched DSF's since
scaling will
> never look good.

Given the renderer can take arbitrarily long to produce a frame I'd prefer
that we scale the last frame when we have a mismatch and let it crispen up
later.
The DPI can change from 2 ways: 1 moving between monitors. 2 changing the
scale factor of the monitor (control alt +- on ChromeOS). Maybe other ways
I'm not thinking of..

>
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):
>
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> cc/layers/delegated_renderer_layer_impl_unittest.cc:714: expected,
> root_delegated_shared_quad_state->content_to_target_transform);
> this test produces an actual total translation of 17,17 instead of the
> expected 18,18, but I can't figure out why.
>
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> cc/layers/delegated_renderer_layer_impl_unittest.cc:770: expected,
> root_delegated_shared_quad_state->content_to_target_transform);
> this one fails in the same way.
>
> Description:
> Invert DSF to map from delegated frame to layer space
>
> The size of the frame data from a delegated source will not in general
match
> up exactly with the delegated layer's bounds. Frame sizes and layer
bounds are
> both expressed as integers, but frame sizes are in physical pixels and
layer
> bounds
> are in device-independent pixels. If the device scale is not an integer,
there
> may
> not be an exact mapping between the two spaces. For instance, if the
layer size
> is
> 105 x 211 and the device scale is 1.5, the delegated content is
responsible for
> filling
> 157.5 x 316.6 physical pixels in the final output. This isn't really
possible so
> what
> actually happens is the delegated renderer produces a frame with a
physical size
> of
> 158 x 317 pixels and sends that up to the DRLI. To map this frame into
physical
> space,
> the DRLI should apply the inverse of the device scale factor to reflect
that the
> frame covers 105.333 x 211.3333 logical pixels.
>
> Before this patch, DRLI would attempt to scale the frame by (105/158,
211/317)
> resulting
> in the frame being scaled down non-uniformly to try to make 158 x 317
pixels fit
> into a
> 157.5 x 316.6 space. This scaling looks bad and ends up mapping pixels
partially
> outside
> of the layer's logical bounds into the layer's logical bounds.
>
> BUG=370074
>
> Please review this at https://codereview.chromium.org/311253004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+29, -79 lines):
>   M cc/layers/delegated_renderer_layer_impl.h
>   M cc/layers/delegated_renderer_layer_impl.cc
>   M cc/layers/delegated_renderer_layer_impl_unittest.cc
>   M cc/trees/layer_tree_host_unittest_delegated.cc
>
>

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

[chromium-reviews, 2014-06-05 21:55:47]

On Thu, Jun 5, 2014 at 2:52 PM, Dana Jansens <danakj@chromium.org> wrote:

>
> On Jun 4, 2014 6:46 PM, <jamesr@chromium.org> wrote:
> >
> > Reviewers: enne,
> >
> > Message:
> > This fixes the remaining scaling problems observable on windows with a
> 1.5 scale
> > factor.  There are still a pair of unit test failures checking the math
> that I
> > can't quite understand - comments inline. I think we probably should
> have some
> > more tests with non-integer scale factors - it seems we're only really
> testing
> > 1.0 and 2.0 in most suites.
> >
> > One open question with this is whether we should use the host's device
> scale
> > factor or the device scale factor from the delegating renderer, but I
> can't
> > think of any situation where we would want to combine data from
> compositor
> > instances using different DSFs.  Maybe there are some races with dragging
> > windows across monitors that this code should be aware of? If so, the
> right
> > behavior is probably to not draw at all with mismatched DSF's since
> scaling will
> > never look good.
>
> Given the renderer can take arbitrarily long to produce a frame I'd prefer
> that we scale the last frame when we have a mismatch and let it crispen up
> later.
>
Scaling always looks really terrible with web content, though.  If that's
what we really want (and I kinda doubt it) then the way to do that would be
to apply the inverse of the delegated frame's DSF here, which I can wire
up.

> The DPI can change from 2 ways: 1 moving between monitors. 2 changing the
> scale factor of the monitor (control alt +- on ChromeOS). Maybe other ways
> I'm not thinking of..
>
> >
> >
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> > File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> > cc/layers/delegated_renderer_layer_impl_unittest.cc:714: expected,
> > root_delegated_shared_quad_state->content_to_target_transform);
> > this test produces an actual total translation of 17,17 instead of the
> > expected 18,18, but I can't figure out why.
> >
> >
>
https://codereview.chromium.org/311253004/diff/1/cc/layers/delegated_renderer...
> > cc/layers/delegated_renderer_layer_impl_unittest.cc:770: expected,
> > root_delegated_shared_quad_state->content_to_target_transform);
> > this one fails in the same way.
> >
> > Description:
> > Invert DSF to map from delegated frame to layer space
> >
> > The size of the frame data from a delegated source will not in general
> match
> > up exactly with the delegated layer's bounds. Frame sizes and layer
> bounds are
> > both expressed as integers, but frame sizes are in physical pixels and
> layer
> > bounds
> > are in device-independent pixels. If the device scale is not an integer,
> there
> > may
> > not be an exact mapping between the two spaces. For instance, if the
> layer size
> > is
> > 105 x 211 and the device scale is 1.5, the delegated content is
> responsible for
> > filling
> > 157.5 x 316.6 physical pixels in the final output. This isn't really
> possible so
> > what
> > actually happens is the delegated renderer produces a frame with a
> physical size
> > of
> > 158 x 317 pixels and sends that up to the DRLI. To map this frame into
> physical
> > space,
> > the DRLI should apply the inverse of the device scale factor to reflect
> that the
> > frame covers 105.333 x 211.3333 logical pixels.
> >
> > Before this patch, DRLI would attempt to scale the frame by (105/158,
> 211/317)
> > resulting
> > in the frame being scaled down non-uniformly to try to make 158 x 317
> pixels fit
> > into a
> > 157.5 x 316.6 space. This scaling looks bad and ends up mapping pixels
> partially
> > outside
> > of the layer's logical bounds into the layer's logical bounds.
> >
> > BUG=370074
> >
> > Please review this at https://codereview.chromium.org/311253004/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files (+29, -79 lines):
> >   M cc/layers/delegated_renderer_layer_impl.h
> >   M cc/layers/delegated_renderer_layer_impl.cc
> >   M cc/layers/delegated_renderer_layer_impl_unittest.cc
> >   M cc/trees/layer_tree_host_unittest_delegated.cc
> >
> >
>

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

[enne (OOO), 2014-06-05 23:02:14]

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl.cc:150: damage_in_layer.Scale(1.0 /
layer_tree_impl()->device_scale_factor());
nit: 1.0 => 1.f, here and elsewhere.  I don't think you're intending to do the
division as a double and convert back to float?

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl.cc:235: double
inverse_device_scale_factor =
Transforms are all floats.  Is double intentional here?

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (left):

https://codereview.chromium.org/311253004/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl_unittest.cc:514:
delegated_renderer_layer->SetTransformOrigin(
On 2014/06/05 20:54:26, jamesr wrote:
> deleting this (which doesn't seem all that relevant to the rest of the test)
> makes the math easier to follow for me. legit?

Yeah, I don't think it's relevant to DRLI and makes this easier to follow.  It
seems legit after I've stared at it a little.

[enne (OOO), 2014-06-05 23:02:19]

PS lgtm

[jamesr, 2014-06-05 23:37:34]

PTAL - this passes the DSF along with the delegated frame data so the parent can
map accurately and adds a unit test with mismatched parent/child DSF's to check
the math

[enne (OOO), 2014-06-05 23:47:55]

lgtmstill

[jam, 2014-06-06 00:13:13]

On 2014/06/05 23:47:55, enne wrote:
> lgtmstill

lgtm

security folks should probably change their owners to list this file, i'll ping
them

[jamesr, 2014-06-06 00:14:15]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-06 00:15:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/311253004/40001

[commit-bot: I haz the power, 2014-06-06 05:08:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-06 06:25:48]

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

[commit-bot: I haz the power, 2014-06-06 06:25:49]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[jamesr, 2014-06-06 06:36:50]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-06 06:38:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/311253004/60001

[commit-bot: I haz the power, 2014-06-06 09:59:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 11:52:39]

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

[commit-bot: I haz the power, 2014-06-06 11:52:39]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[danakj, 2014-06-06 16:00:52]

https://codereview.chromium.org/311253004/diff/60001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl.cc (left):

https://codereview.chromium.org/311253004/diff/60001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl.cc:201: gfx::Size display_size =
display_size_.IsEmpty() ? bounds() : display_size_;
Is display_size_ used now? Should it be removed?

[danakj, 2014-06-06 17:17:12]

https://codereview.chromium.org/311253004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/311253004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:489: // Should create a total
amount of gfx::Rect(2, 2, 10, 6) damage.
Add a comment here explaining why 2,2,10,6 becomes 2,2,8,6?

https://codereview.chromium.org/311253004/diff/60001/content/common/cc_messag...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/311253004/diff/60001/content/common/cc_messag...
content/common/cc_messages.cc:714: WriteParam(m, p.device_scale_factor);
Can you move this below the call to Reserve() and include this size in the
reserved amount?

[jamesr, 2014-06-06 18:57:15]

The CQ bit was checked by jamesr@chromium.org

[jamesr, 2014-06-06 18:59:04]

On 2014/06/06 16:00:52, danakj wrote:
>
https://codereview.chromium.org/311253004/diff/60001/cc/layers/delegated_rend...
> File cc/layers/delegated_renderer_layer_impl.cc (left):
> 
>
https://codereview.chromium.org/311253004/diff/60001/cc/layers/delegated_rend...
> cc/layers/delegated_renderer_layer_impl.cc:201: gfx::Size display_size =
> display_size_.IsEmpty() ? bounds() : display_size_;
> Is display_size_ used now? Should it be removed?

I believe it can go away, but it's used in lots of places not otherwise touched
in this patch so I'll remove it separately.

[commit-bot: I haz the power, 2014-06-06 19:01:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/311253004/100001

[danakj, 2014-06-06 21:41:43]

Ok, thanks!

LGTM

[commit-bot: I haz the power, 2014-06-06 22:24:41]

Change committed as 275544

[acleung1, 2014-06-07 03:20:42]

A revert of this CL has been created in
https://codereview.chromium.org/313403005/ by acleung@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=381982.

[jamesr, 2014-06-09 17:21:34]

On 2014/06/07 03:20:42, acleung1 wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/313403005/ by mailto:acleung@chromium.org.
> 
> The reason for reverting is:
> https://code.google.com/p/chromium/issues/detail?id=381982.

This breaks on android because android never sets the device scale factor in the
browser compositor (it just stays 1).  Will address in a separate patch then
revisit this one.

[jamesr, 2014-06-09 23:00:13]

Patch set 7 should fix the android issue.  aelias@ - could you check it over? 
The comment in render_widget_host_view_android.cc tries to explain the issue, if
that's not clear enough please let me know so that we can have a record of what
is going on in the code.

[aelias_OOO_until_Jul13, 2014-06-09 23:10:44]

lgtm, thanks for the cleanup.

[danakj, 2014-06-09 23:13:50]

thanks for removing display size :D

[jamesr, 2014-06-10 00:51:16]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-10 00:54:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/311253004/120001

[commit-bot: I haz the power, 2014-06-10 15:18:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[danakj, 2014-06-10 15:44:58]

https://codereview.chromium.org/311253004/diff/120001/cc/output/delegated_fra...
File cc/output/delegated_frame_data.h (right):

https://codereview.chromium.org/311253004/diff/120001/cc/output/delegated_fra...
cc/output/delegated_frame_data.h:22: float device_scale_factor;
This is already available in CompositorFrameMetadata
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/composit...

[commit-bot: I haz the power, 2014-06-10 15:46:15]

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

[commit-bot: I haz the power, 2014-06-10 15:46:16]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[jamesr, 2014-06-10 18:16:05]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-06-10 18:18:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/311253004/120001

[commit-bot: I haz the power, 2014-06-10 18:36:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-10 20:57:38]

Change committed as 276139