Skip to content

Conversation

@kvark
Copy link
Member

@kvark kvark commented Feb 1, 2017

Fixes #789

Clamping the texture coordinates in pixel shaders (as well as using textureLod) to ensure sampling within original borders now.


This change is Reviewable

@kvark
Copy link
Member Author

kvark commented Feb 1, 2017

r? @glennw

BTW, I removed the distinction between requested_size and allocated_size now, following the general idea of relying on git history to restore the obsolete stuff if needed.

@kvark kvark changed the title [WIP] Remove image borders Remove image borders Feb 1, 2017
@glennw
Copy link
Member

glennw commented Feb 2, 2017

This generally looks good to me (I'll run the CSS tests locally to confirm). We'll need to apply the same clamp in the text run shader though.

@kvark
Copy link
Member Author

kvark commented Feb 2, 2017

Good point @glennw , the text is now also converted.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #776) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw
Copy link
Member

glennw commented Feb 2, 2017

This looks good, but will need to be rebased after the external texture images changes landed.

@kvark
Copy link
Member Author

kvark commented Feb 2, 2017

@glennw rebased now

@glennw
Copy link
Member

glennw commented Feb 2, 2017

Nice one!

@glennw
Copy link
Member

glennw commented Feb 2, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 5b41357 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 5b41357 with merge 2f17a52...

bors-servo pushed a commit that referenced this pull request Feb 3, 2017
Remove image borders

Fixes #789

Clamping the texture coordinates in pixel shaders (as well as using textureLod) to ensure sampling within original borders now.

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

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

@bors-servo bors-servo merged commit 5b41357 into servo:master Feb 3, 2017
@JerryShih
Copy link
Contributor

@kvark @nical
Are we use mipmap now? I don't see any mipmap generating in WR. So, I'm curious about the textureLod usage here.

@kvark kvark deleted the border branch February 3, 2017 15:39
@kvark
Copy link
Member Author

kvark commented Feb 3, 2017

@JerryShih there are 2 reasons for using textureLod in this case:

  1. It clearly states the pre-condition. Since the image border code uses a half-texel offset, it's not going to work properly for any Lod > 0.0. So now if you add any mipmap levels, the code doesn't care and will still work properly.
  2. It may be slightly faster. The shader compiler doesn't know if a texture has mipmap levels or not (unless it re-compiles the shader on the fly), so it will generate the gradient instructions and a regular sample, only to be dismissed by the texture sampler that discovers no mipmap levels. Using textureLod directly produces the efficient HW shader assembly right away.

@leeoniya
Copy link

leeoniya commented Feb 3, 2017

@kvark maybe worth adding these architectural decisions to the wiki? probably makes sense for any details that can answer "why not use X?" for common graphics tech.

@kvark
Copy link
Member Author

kvark commented Feb 3, 2017

@leeoniya that should be a good start:
https://github.com/servo/webrender/wiki/Graphics-Features

@leeoniya
Copy link

leeoniya commented Feb 3, 2017

excellent, thanks 👍

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.

5 participants