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

[WIP] Precise transformed primitive edge extrusion for AA #2263

Closed
wants to merge 5 commits into from

Conversation

@kvark
Copy link
Member

kvark commented Jan 5, 2018

What

Includes #2254

Implemented a new code path for transformed vertex shaders: FIXED_EDGE_EXTRUSION. Defining it as (2.0) makes the behavior unchanged to what we have now. Undefining it makes the vertex shaders to compute edge extrusion dynamically, which handles non-trivial transformation cases of scaling/perspective. A reftest is added to demonstrate it.

Also includes a change (that was meant to go as a separate PR, and can still be moved out) that only applies extrusion to edges that are meant to have AA enabled. Unfortunately, this also requires a few fixes in both Rust and GLSL code to make sure we pass those flags correctly, since the semantics is changed a bit (or, rather, by 4 bits).

TODO

  • get feedback on the current approach, starting with "do we really need this?" :) cc @glennw
  • if the new behavior is desired, update the relevant reftest images (quite a few failures atm, because of the better quality), then investigate the Gecko side of things
  • figure out how to only enable the new behavior for primitives in need, since it may be computationally heavy to do for all the brush vertices. Get WR_FEATURE_TRANSFORM back to affect brushes? (currently, the brush vertex shaders are unconditionally transform-enabled.

Example

Left - new path. Right - old path. Notice the lack of vertical light-red edge.
wr-adaptive-edge-aa2


This change is Reviewable

@glennw
Copy link
Member

glennw commented Jan 7, 2018

I like the idea, I think it's worth having. I don't really want to bring back the WR_FEATURE_TRANSFORM though - that's one of the nice benefits of the brush shaders, to not have to break batches in those cases. The brush (vertex) shader does do a dynamic branch based on whether there is a complex transform, perhaps that will be enough for your question above?

@kvark
Copy link
Member Author

kvark commented Jan 7, 2018

@glennw my main concern about dynamic branching would be that there is quite a bit of arithmetic-heavy code in the branch, so it will consume register space and penalize occupation of our vertex shaders.

@glennw
Copy link
Member

glennw commented Jan 7, 2018

That's a fair concern - can we profile to try and get some numbers on what effect it has?

@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2018

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

@glennw
Copy link
Member

glennw commented Mar 14, 2018

@kvark Do we want to keep this one open for now?

@kvark
Copy link
Member Author

kvark commented Mar 14, 2018

I'll need to revive it...

@glennw
Copy link
Member

glennw commented Apr 12, 2018

Should we close this until you have time to revive it?

@kvark kvark closed this Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.