|
|
Descriptioncc: 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
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 5
Patch Set 3 : Nit. #Patch Set 4 : Rebase #
Total comments: 3
Patch Set 5 : Add todos for tex_coord_rect computation. #Messages
Total messages: 38 (20 generated)
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 ==========
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sunxd@chromium.org changed reviewers: + enne@chromium.org, trchen@chromium.org
I re-upload the same changes on renderers, because of some local branch structures matters.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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; 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?
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.
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. :)
The CQ bit was checked by sunxd@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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@chromium.org changed reviewers: + ajuma@chromium.org
Oops, the owners. Need an lgtm from either enne@ or ajuma@, I think.
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?
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.
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.
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?
> 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?
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.
The CQ bit was checked by sunxd@chromium.org
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.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sunxd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488986794355400, "parent_rev": "757f6620368c7503f90cb6ed0a61c617b9810c94", "commit_rev": "5e6bda5447afa39cc476898bcdd2ee560b1620bc"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5e6bda5447afa39cc476898bcdd2... |