Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full rectangular pixel snapping #1292

Merged
merged 3 commits into from
May 26, 2017
Merged

Full rectangular pixel snapping #1292

merged 3 commits into from
May 26, 2017

Conversation

kvark
Copy link
Member

@kvark kvark commented May 24, 2017

Fixes #1279
r? @glennw

WIP TODOs:

  • test servo WPT reftests
  • add local snapping tests
  • cover write_transform_vertex

This change is Reviewable

@kvark kvark changed the title [WIP] Full rectangular pixel snapping Full rectangular pixel snapping May 24, 2017
@glennw
Copy link
Member

glennw commented May 24, 2017

@kvark This generally looks good to me. After running the CSS tests, I get these unexpected results:

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/rotate_45deg.htm
  └   → /css-transforms-1_dev/html/rotate_45deg.htm 19763a42afc94b24a1c3762e2eab58e0fc89900b
/css-transforms-1_dev/html/reference/rotate_45deg-ref.htm 44b428d2043c705a9257112a9108ae1cdec9a165
Testing 19763a42afc94b24a1c3762e2eab58e0fc89900b == 44b428d2043c705a9257112a9108ae1cdec9a165

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/rotate_y_45deg.htm
  └   → /css-transforms-1_dev/html/rotate_y_45deg.htm 3a92183be2a54f07fbbb974600b4330f5a653712
/css-transforms-1_dev/html/reference/rotate_y_45deg-ref.htm 4f53b38136f08aaf1d3bfd47fd0e693a17596dc7
Testing 3a92183be2a54f07fbbb974600b4330f5a653712 == 4f53b38136f08aaf1d3bfd47fd0e693a17596dc7

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-input-019.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/block-non-replaced-height-001.htm

  ▶ FAIL [expected PASS] /css21_dev/html4/c5525-fltmrgn-000.htm
  └   → /css21_dev/html4/c5525-fltmrgn-000.htm 578df2d1dbc3ffedc88fe346808ec5742b041a7c
/css21_dev/html4/reference/c5525-fltmrgn-000-ref.htm 653c6c85e4f06d453eab1cc2a46f85df8fbfe848
Testing 578df2d1dbc3ffedc88fe346808ec5742b041a7c == 653c6c85e4f06d453eab1cc2a46f85df8fbfe848

It's quite possible that those changes are in fact a different issue, exposed by these changes. But let's investigate them first and see if we can work out what's causing the changes.

@Gankra
Copy link
Contributor

Gankra commented May 25, 2017

I ran a try build before and after this change on my branch where I'm working on enabling borders in gecko: this fixes like 30 tests! 🎉

Just one interesting regression:

@kvark
Copy link
Member Author

kvark commented May 25, 2017

@gankro thanks for giving it a try! I looked at the failing border test, and it doesn't appear to be trivial at all. Given the number of tests fixed, I think it's a step forward to trade 30 for 1 here. Hopefully, further investigation on border rendering will clear this out.

@glennw I checked rotate_45 tests, and we are rendering them now exactly like Gecko does. Unfortunately, this doesn't match WPT reference, which means Gecko fails it as well. Thus, the answer lies in the investigation of who is right between Gecko and WPT.

@glennw
Copy link
Member

glennw commented May 25, 2017

@kvark Ah, interesting! If Gecko fails those reftests, we have a good case for disabling / marking as fails in the Servo test metadata.

@glennw
Copy link
Member

glennw commented May 25, 2017

@kvark It'd be interesting to know if Blink also fails those tests, if it's easy to check.

@kvark
Copy link
Member Author

kvark commented May 25, 2017

@glennw for all 3 failed tests (by this PR), I confirm both Gecko (52.0.2, Linus) and Chromium (57.0.2987.133, Linus) fail them as well (even though Chromium produces a little different result from Gecko). I took the upstream versions of the tests (from web-platform-tests repo) just to be sure.

@glennw
Copy link
Member

glennw commented May 25, 2017

:shipit:

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 70c099f has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 70c099f with merge 6ab41d0...

bors-servo pushed a commit that referenced this pull request May 26, 2017
Full rectangular pixel snapping

Fixes #1279
r? @glennw

WIP TODOs:
- [x] test servo WPT reftests
- [x] add local snapping tests
- [x] cover `write_transform_vertex`

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1292)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - status-travis

@Gankra
Copy link
Contributor

Gankra commented May 26, 2017

Looks like this slightly changed anti-aliasing behaviour, so you need to regenerate the test I just added (reftests/border/degenerate-curve). Or make it fuzzy (max difference: 44, number of differing pixels: 287).

@kvark
Copy link
Member Author

kvark commented May 26, 2017

Given that nothing in the code is changed, and the new reftest author gives a green light on changing the test, I consider this PR to still be approved.

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

📌 Commit d4235d4 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit d4235d4 with merge efa87af...

bors-servo pushed a commit that referenced this pull request May 26, 2017
Full rectangular pixel snapping

Fixes #1279
r? @glennw

WIP TODOs:
- [x] test servo WPT reftests
- [x] add local snapping tests
- [x] cover `write_transform_vertex`

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1292)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: glennw
Pushing efa87af to master...

@bors-servo bors-servo merged commit d4235d4 into servo:master May 26, 2017
@kvark kvark deleted the snap branch May 26, 2017 18:31
@staktrace
Copy link
Contributor

@kvark This PR (or #1296, haven't done a full bisection) appears to be causing a panic in gecko crashtests, when running dom/animation/test/crashtests/1272475-1.html. Backtrace can be found here

It might also be responsible for an increase in fuzziness in the layout/reftests/columns/column-balancing-overflow-005.html reftest, although that's less important (but perhaps unexpected?).

@kvark
Copy link
Member Author

kvark commented May 29, 2017

@staktrace are you sure? this PR doesn't touch any Rust code, it would unlikely trigger a panic in TransformedRect::new.

@staktrace
Copy link
Contributor

Sorry, my bad. I kicked off a bisection to confirm after posting the comment, and it looks like #1296 is the culprit. The same panic appears on this try push which has webrender at cset 150da47 (including #1296 but not this one).

@staktrace
Copy link
Contributor

Also for the record the column-balancing-overflow-005.html fuzziness increase is from this PR.

@kvark
Copy link
Member Author

kvark commented May 29, 2017

Thanks @staktrace ! That's the regression mentioned by @gankro here - #1292 (comment)

moz-v2v-gh pushed a commit to mozilla/gecko-projects that referenced this pull request Jun 1, 2017
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Jun 2, 2017
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Jun 5, 2017
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Jun 11, 2017
Gankra added a commit to Gankra/webrender that referenced this pull request Jun 14, 2017
This reverts servo#1339, which just reverted servo#1292 and servo#1319.

The original changes seem to behaving well now. It's unclear what was happening,
but whatever it was, it seems fine now!

Here is the full list of reverts:

Revert "Manually re-apply ANGLE workaround in 3debb57."
This reverts commit 9143f1d.

Revert "Revert "Full rectangular pixel snapping""
This reverts commit de7e944.

Revert "Revert "Snapping transformed primitives""
This reverts commit dae62c5.

Revert "Revert "Snapping ref tests""
This reverts commit d6f03ba.

Revert "Revert "Ensure that subpixel sized snap rects are at least 1 device pixel.""
This reverts commit d396a7e.

Revert "Revert "clamp_rect Angle workaround""
This reverts commit ef24b78.

Revert "Revert "Don't apply local clip rect to snap rectangle calculation.""
This reverts commit 406b102.
Gankra added a commit to Gankra/webrender that referenced this pull request Jun 14, 2017
This reverts servo#1339, which just reverted servo#1292 and servo#1319.

The original changes seem to behaving well now. It's unclear what was happening,
but whatever it was, it seems fine now!

Here is the full list of reverts:

Revert "Manually re-apply ANGLE workaround in 3debb57."
This reverts commit 9143f1d.

Revert "Revert "Full rectangular pixel snapping""
This reverts commit de7e944.

Revert "Revert "Snapping transformed primitives""
This reverts commit dae62c5.

Revert "Revert "Snapping ref tests""
This reverts commit d6f03ba.

Revert "Revert "Ensure that subpixel sized snap rects are at least 1 device pixel.""
This reverts commit d396a7e.

Revert "Revert "clamp_rect Angle workaround""
This reverts commit ef24b78.

Revert "Revert "Don't apply local clip rect to snap rectangle calculation.""
This reverts commit 406b102.
brendandahl pushed a commit to brendandahl/gecko that referenced this pull request Jul 10, 2017
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…ender#1292. r=jrmuizel

MozReview-Commit-ID: 6gc49H6Vwob

UltraBlame original commit: 90199d13a72cbe1bab0a4a58bec2404b5cf8e248
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…ender#1292. r=jrmuizel

MozReview-Commit-ID: 6gc49H6Vwob

UltraBlame original commit: 90199d13a72cbe1bab0a4a58bec2404b5cf8e248
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…ender#1292. r=jrmuizel

MozReview-Commit-ID: 6gc49H6Vwob

UltraBlame original commit: 90199d13a72cbe1bab0a4a58bec2404b5cf8e248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants