Skip to content

/ webrender

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

Fix artifacts on box shadows that don't use nine-patch stretching. #2523

Merged
merged 1 commit into from Mar 16, 2018
Merged

Fix artifacts on box shadows that don't use nine-patch stretching. #2523

merged 1 commit into from Mar 16, 2018

Conversation

@glennw
Copy link
Member

glennw commented Mar 15, 2018 •
edited by larsbergstrom

Fixes #2467.

This change is Reviewable
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=defcfa9b14cb578332570a45e4d4653a021695c6&selectedJob=168128618

There's two new failures here:
box-decoration-break-with-inset-box-shadow-1.html - fuzzy test (28, 5270) <= (23, 14016) <= (28, 5270)``` - the max difference is lower, but far more pixels are different.

boxshadow-large-border-radius.html - fuzzy test (1, 238) <= (5, 1440) <= (1, 238) - both the max difference and pixel count have increased.

Visually, I can't see any differences between the two images. I'm tempted to get this merged, since it fixes an obvious artifact (#2467). But we can wait for an opinion from @staktrace - the PR queue is backed up, so I could investigate this further tomorrow if you'd prefer.

r? @kvark
@Darkspirit
Copy link

Darkspirit commented Mar 15, 2018 •
edited

mozregression --repo try --launch defcfa9b14cb578332570a45e4d4653a021695c6 --pref gfx.webrender.all:true

I just opened some new tabs, wikipedia, reloaded, close some tabs. Then all type of text disappeared in the whole window.

Edit: on about:support, only the text on the righthand side of the table is invisible. "Nightly" in the locationbar is visible, about:support not. Tabs do not have text. No critical log, no crash, WebRender is still active.

Edit 2: If I open 2 newtab pages, both initially have text. If I close one, the other one lost its text.
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

That's caused by a bug in #2516 - there is a fix in the queue - #2520. We're having some CI merging issues today though 😭
@Darkspirit
Copy link

Darkspirit commented Mar 15, 2018

The shape seems to be fine now. But it's too sharp at 30% zoom.
screenshot_20180315_053150
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

I think we can probably open that as a separate issue from #2467.
@staktrace
Copy link
Contributor

staktrace commented Mar 15, 2018

I think the reftest differences in #2523 (comment) are acceptable, so feel free to merge.
@kvark
kvark reviewed Mar 15, 2018
View changes
webrender/res/cs_clip_box_shadow.glsl
case MODE_STRETCH: {
uv = clamp(vUv.xy, vec2(0.0), vEdge.xy);
uv += max(vec2(0.0), vUv.xy - vEdge.zw);
uv = mix(vUvBounds_NoClamp.xy, vUvBounds_NoClamp.zw, uv);

This comment has been minimized.

Sign in to view
@kvark

kvark Mar 15, 2018

Member

nit: could go straight to vUvBounds and move the follow up clamping into the MODE_SIMPLE case

This comment has been minimized.

Sign in to view
@glennw

glennw Mar 15, 2018

Author Member

It's not quite clear to me what you mean here, since that statement is a mix. Could you expand on the above?

This comment has been minimized.

Sign in to view
@kvark

kvark Mar 15, 2018

Member

on a second thought, ignore that :)
webrender/src/clip.rs Outdated
let mut stretch_mode = BoxShadowStretchMode::Stretch;
if shadow_rect.size.width < minimal_shadow_rect.size.width ||
shadow_rect.size.height < minimal_shadow_rect.size.height {
minimal_shadow_rect.size.width = shadow_rect.size.width;

This comment has been minimized.

Sign in to view
@kvark

kvark Mar 15, 2018

Member

why not just minimal_shadow_rect.size = shadow_rect.size?
@gw3583
Fix artifacts on box shadows that don't use nine-patch stretching.
Loading status checks…
b44d960 
Fixes #2467.
@glennw glennw force-pushed the glennw:fix-bs-stretch branch from cdd6e0c to b44d960 Mar 15, 2018
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

Fixed the second comment. Could you expand on the first comment, I'm not quite sure what you mean?
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

Oh wait, I see what you're saying - will fix now.
@kvark
Copy link
Member

kvark commented Mar 15, 2018

please r=me once CI is happy
@glennw
Copy link
Member Author

glennw commented Mar 15, 2018

I think the CI issue is remnants of the servo-tidy issue, hopefully it'll pass now.

@bors-servo r=kvark
@bors-servo
Copy link
Contributor

bors-servo commented Mar 15, 2018

📌 Commit b44d960 has been approved by kvark
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2018

⌛ Testing commit b44d960 with merge ea61b05...
bors-servo added a commit that referenced this pull request Mar 16, 2018
@bors-servo
Auto merge of #2523 - glennw:fix-bs-stretch, r=kvark
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ea61b05 
Fix artifacts on box shadows that don't use nine-patch stretching.

Fixes #2467.

<!-- 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/2523)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing ea61b05 to master...
@bors-servo bors-servo merged commit b44d960 into servo:master Mar 16, 2018
2 of 3 checks passed
2 of 3 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@glennw @Darkspirit @staktrace @kvark @bors-servo @gw3583
You can’t perform that action at this time.