cc: Reformat renderers to make them work with mask tiling.

cc: Reformat renderers to make them work with mask tiling.

This CL makes the code that computes RPDQ uniforms easier to udnerstand.
Also the new code works fine with mask tiling patch.

BUG=567293
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2716893002
Cr-Commit-Position: refs/heads/master@{#455473}
Committed: https://chromium.googlesource.com/chromium/src/+/5e6bda5447afa39cc476898bcdd2ee560b1620bc

[sunxd, 2017-02-24 18:38:47]

Description was changed from

==========
cc: Reformat renderers to make them work with mask tiling.

This CL makes the code that computes RPDQ uniforms easier to udnerstand.
Also the new code works fine with mask tiling patch.

BUG=567293
==========

to

==========
cc: Reformat renderers to make them work with mask tiling.

This CL makes the code that computes RPDQ uniforms easier to udnerstand.
Also the new code works fine with mask tiling patch.

BUG=567293
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunxd, 2017-02-24 18:38:59]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 18:39:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunxd, 2017-02-24 18:39:42]

sunxd@chromium.org changed reviewers:
+ enne@chromium.org, trchen@chromium.org

[sunxd, 2017-02-24 18:39:43]

I re-upload the same changes on renderers, because of some local branch
structures matters.

[commit-bot: I haz the power, 2017-02-24 20:33:31]

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

[commit-bot: I haz the power, 2017-02-24 20:33:32]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunxd, 2017-02-28 15:48:02]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 15:48:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 16:59:44]

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

[commit-bot: I haz the power, 2017-02-28 16:59:45]

Dry run: This issue passed the CQ dry run.

[sunxd, 2017-02-28 18:52:12]

PTAL.

[trchen, 2017-03-02 00:32:56]

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1321: gfx::Size texture_size;
nits: move this down right before #1328

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1369: SkMatrix::kFill_ScaleToFit);
Thanks for saving everyone's brain cells!

However it looks like it introduced some behavioral changes. I think the
original code was equivalent to:

SkMatrix tex_to_mask = 
    SkMatrix::MakeRectToRect(SkRect::makeWH(tex_rect.width(),
tex_rect.height()),
        RectFToSkRect(mask_uv_rect),
        SkMatrix::kFill_ScaleToFit);

I think your version is more correct, and is probably related to the bug your
test exposed:
https://codereview.chromium.org/2689253002/diff/200001/third_party/WebKit/Lay...
I'm surprised your test didn't fail. Could you investigate and add test if it is
a separate issue?

[sunxd, 2017-03-02 17:21:40]

PTAL.

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1321: gfx::Size texture_size;
On 2017/03/02 00:32:56, trchen wrote:
> nits: move this down right before #1328

Done.

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1369: SkMatrix::kFill_ScaleToFit);
On 2017/03/02 00:32:56, trchen wrote:
> Thanks for saving everyone's brain cells!
> 
> However it looks like it introduced some behavioral changes. I think the
> original code was equivalent to:
> 
> SkMatrix tex_to_mask = 
>     SkMatrix::MakeRectToRect(SkRect::makeWH(tex_rect.width(),
> tex_rect.height()),
>         RectFToSkRect(mask_uv_rect),
>         SkMatrix::kFill_ScaleToFit);
> 
> I think your version is more correct, and is probably related to the bug your
> test exposed:
>
https://codereview.chromium.org/2689253002/diff/200001/third_party/WebKit/Lay...
> I'm surprised your test didn't fail. Could you investigate and add test if it
is
> a separate issue?

Yes I noticed that this didn't fix the bug in that test. The new logic here
should be correct as tex_rect can have an offset, this should fix bugs that when
we tile masks, we apply textures from the same (0, 0) offset for every tile.

I tried playing with that test a while and found that even if we disable masks,
we get the same result: you can see the tiny difference by adding/removing
will-change: transform in devtools quickly.

I assume this is a bug with filters. Moreover, in my UI patterns work I
discovered that blur filters cannot work correctly with backface visibility
(while firefox do), so I guess the code is buggy somewhere.

A small demo: https://jsfiddle.net/579v9aho/3/
Blurred divs seem to be chopped off at certain rotation degrees and backface
visibility is not correctly displayed.

[trchen, 2017-03-04 02:59:40]

lgtm

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1369: SkMatrix::kFill_ScaleToFit);
On 2017/03/02 17:21:39, sunxd wrote:
> On 2017/03/02 00:32:56, trchen wrote:
> > Thanks for saving everyone's brain cells!
> > 
> > However it looks like it introduced some behavioral changes. I think the
> > original code was equivalent to:
> > 
> > SkMatrix tex_to_mask = 
> >     SkMatrix::MakeRectToRect(SkRect::makeWH(tex_rect.width(),
> > tex_rect.height()),
> >         RectFToSkRect(mask_uv_rect),
> >         SkMatrix::kFill_ScaleToFit);
> > 
> > I think your version is more correct, and is probably related to the bug
your
> > test exposed:
> >
>
https://codereview.chromium.org/2689253002/diff/200001/third_party/WebKit/Lay...
> > I'm surprised your test didn't fail. Could you investigate and add test if
it
> is
> > a separate issue?
> 
> Yes I noticed that this didn't fix the bug in that test. The new logic here
> should be correct as tex_rect can have an offset, this should fix bugs that
when
> we tile masks, we apply textures from the same (0, 0) offset for every tile.
> 
> I tried playing with that test a while and found that even if we disable
masks,
> we get the same result: you can see the tiny difference by adding/removing
> will-change: transform in devtools quickly.

Alright. I see why. As GLRenderer::UpdateRPDQWithSkiaFilters changes the
geometry of the quad, the corresponding mask_uv_rect should be updated too (but
didn't). It was a double fail before your CL, and it became a single fail after
yours. (So blur+mask still fails, but fail in a subtly different way.)

For non-filtered code path, tex_rect.location() was always zero because our code
is smart enough to never over-draw. With tiled masks, there will be non-trivial
tex_rect.location(). I'm convinced that tests with tiled mask will catch this
behavioral change.

> I assume this is a bug with filters. Moreover, in my UI patterns work I
> discovered that blur filters cannot work correctly with backface visibility
> (while firefox do), so I guess the code is buggy somewhere.

Nah, backface visibility is another story. Basically all 4 major vendors treat
3D context differently, and none implements backface-visibility according to the
spec. Your demo kind of works (or not working) as intended.

> A small demo: https://jsfiddle.net/579v9aho/3/
> Blurred divs seem to be chopped off at certain rotation degrees and backface
> visibility is not correctly displayed.

Yea, that chopping looks incorrect, and there are weird edge bleeding at some
certain degrees. Anyway, let's ignore it for another day. :)

[sunxd, 2017-03-06 15:47:51]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2017-03-06 15:47:51]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2716893002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-06 15:48:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 15:56:16]

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

[commit-bot: I haz the power, 2017-03-06 15:56:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sunxd, 2017-03-06 16:17:05]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org

[sunxd, 2017-03-06 16:17:05]

Oops, the owners. Need an lgtm from either enne@ or ajuma@, I think.

[enne (OOO), 2017-03-06 19:10:54]

Am I understanding the discussion that this exposes some preexisting filter bug
in a test, but is otherwise correct?

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1322: // The tex_coord_rect is not produced in
RenderSufaceImpl::AppendQuads.
It seems like it should be?

[sunxd, 2017-03-06 19:27:46]

On 2017/03/06 19:10:54, enne wrote:
> Am I understanding the discussion that this exposes some preexisting filter
bug
> in a test, but is otherwise correct?

Yes, we seem to have problems with filters before this patch.

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1322: // The tex_coord_rect is not produced in
RenderSufaceImpl::AppendQuads.
On 2017/03/06 19:10:54, enne wrote:
> It seems like it should be?

Some of the information needed to generate tex_coord_rect, like dst_rect,
relates to resources that are not accessible (or very difficult to access if my
understanding is wrong), e.g. if we have a filter
(https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?rcl=cdafce67dcb...).

In these cases, we leave tex rect to be blank.

[enne (OOO), 2017-03-06 20:44:20]

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1322: // The tex_coord_rect is not produced in
RenderSufaceImpl::AppendQuads.
On 2017/03/06 at 19:27:45, sunxd wrote:
> On 2017/03/06 19:10:54, enne wrote:
> > It seems like it should be?
> 
> Some of the information needed to generate tex_coord_rect, like dst_rect,
relates to resources that are not accessible (or very difficult to access if my
understanding is wrong), e.g. if we have a filter
(https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?rcl=cdafce67dcb...).
> 
> In these cases, we leave tex rect to be blank.

I think if this parameter is filled in by the renderer itself, it shouldn't be
on the quad then.

[sunxd, 2017-03-06 20:52:18]

On 2017/03/06 20:44:20, enne wrote:
> https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/2716893002/diff/60001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:1322: // The tex_coord_rect is not produced in
> RenderSufaceImpl::AppendQuads.
> On 2017/03/06 at 19:27:45, sunxd wrote:
> > On 2017/03/06 19:10:54, enne wrote:
> > > It seems like it should be?
> > 
> > Some of the information needed to generate tex_coord_rect, like dst_rect,
> relates to resources that are not accessible (or very difficult to access if
my
> understanding is wrong), e.g. if we have a filter
>
(https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?rcl=cdafce67dcb...).
> > 
> > In these cases, we leave tex rect to be blank.
> 
> I think if this parameter is filled in by the renderer itself, it shouldn't be
> on the quad then.

OK, I talked with Ali about setting filter bounds in RenderSurfaceImpl, it seems
doable. I'll make the change in https://codereview.chromium.org/2717533005/
because RPDQ->tex_coord_rect would be non-0 only after that patch. Wdyt?

[enne (OOO), 2017-03-06 20:55:36]

> OK, I talked with Ali about setting filter bounds in RenderSurfaceImpl, it
seems doable. I'll make the change in
https://codereview.chromium.org/2717533005/ because RPDQ->tex_coord_rect would
be non-0 only after that patch. Wdyt?

Ok, lgtm, but would you please add a "TODO(sunxd): make this never be empty" to
both the gl and software branches on it?

[sunxd, 2017-03-06 21:26:56]

On 2017/03/06 20:55:36, enne wrote:
> > OK, I talked with Ali about setting filter bounds in RenderSurfaceImpl, it
> seems doable. I'll make the change in
> https://codereview.chromium.org/2717533005/ because RPDQ->tex_coord_rect would
> be non-0 only after that patch. Wdyt?
> 
> Ok, lgtm, but would you please add a "TODO(sunxd): make this never be empty"
to
> both the gl and software branches on it?

Sure will do.

[sunxd, 2017-03-06 21:29:24]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2017-03-06 21:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2716893002/#ps80001
(title: "Add todos for tex_coord_rect computation.")

[commit-bot: I haz the power, 2017-03-06 21:29:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 23:47:57]

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

[commit-bot: I haz the power, 2017-03-06 23:48:00]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sunxd, 2017-03-08 15:26:34]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2017-03-08 15:26:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 16:51:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488986794355400,
"parent_rev": "757f6620368c7503f90cb6ed0a61c617b9810c94", "commit_rev":
"5e6bda5447afa39cc476898bcdd2ee560b1620bc"}

[commit-bot: I haz the power, 2017-03-08 16:51:45]

Description was changed from

==========
cc: Reformat renderers to make them work with mask tiling.

This CL makes the code that computes RPDQ uniforms easier to udnerstand.
Also the new code works fine with mask tiling patch.

BUG=567293
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Reformat renderers to make them work with mask tiling.

This CL makes the code that computes RPDQ uniforms easier to udnerstand.
Also the new code works fine with mask tiling patch.

BUG=567293
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2716893002
Cr-Commit-Position: refs/heads/master@{#455473}
Committed:
https://chromium.googlesource.com/chromium/src/+/5e6bda5447afa39cc476898bcdd2...
==========

[commit-bot: I haz the power, 2017-03-08 16:51:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5e6bda5447afa39cc476898bcdd2...