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

Add a fast path for clips with uniform radii. #1661

Closed
glennw opened this issue Sep 5, 2017 · 5 comments
Closed

Add a fast path for clips with uniform radii. #1661

glennw opened this issue Sep 5, 2017 · 5 comments

Comments

@glennw
Copy link
Member

@glennw glennw commented Sep 5, 2017

The clipping shader currently evaluates an ellipse, since the x/y radii of a rounded rect can be different.

The ellipse evaluation is much slower than evaluating a clip against a circle, and elliptical clips are actually very rare in practice.

We should add a fast path clip mask shader that is used when the radii are equal that just evaluates the SDF of a circle.

@j3parker
Copy link

@j3parker j3parker commented Oct 25, 2017

The ellipse shader code handles circles in the easy/fast way (because the SDF impl for ellipses doesn't work for that special case) here, currently:

float distance_to_ellipse(vec2 p, vec2 radii) {
// sdEllipse fails on exact circles, so handle equal
// radii here. The branch coherency should make this
// a performance win for the circle case too.
if (radii.x == radii.y) {
return length(p) - radii.x;
} else {
return sdEllipse(p, radii);
}
}

Is the idea in this issue to create a separate fragment shader (a variant of cs_clip_rectangle) that only has that path? (Presumably with some #ifdefs to cut out the ellipse code.)

If so: when would we decide to use it? I see two options (I'm looking at WebRender for the first time, excuse me if this is way off the mark :) )

  1. When target.clip_batcher.rectangles is only composed of "nice" rectangles
  2. Split the current rectangles batch into two batches: one for "nice" and one for "not nice" rectangles

("nice": all corners use circles)

@j3parker
Copy link

@j3parker j3parker commented Oct 25, 2017

Hm, total list of shaders using the shared ellipse code:

  • ps_border_corner
  • brush_mask
  • cs_clip_rectangle
@glennw
Copy link
Member Author

@glennw glennw commented Oct 25, 2017

That's the basic idea, yep!

Although, the clipping code is undergoing quite a lot of changes right now, so it's perhaps not the best time to work on this task.

One step towards this task that would be super useful is to create a wrench test case that does only require circle clips, locally replace the ellipse code by a shader that only does the circle clip, and measure the performance difference. It may be that there's minimal performance gain from this anyway, in which case it won't be worth pursuing.

In terms of an actual implementation, I'm hoping to have the major clip changes done in the next 2-3 weeks, which would then be a good time to work on this. Thanks!

@j3parker
Copy link

@j3parker j3parker commented Oct 25, 2017

Alright, sounds good. I'll give the wrench test idea a go tomorrow and report back.

@gw3583
Copy link
Collaborator

@gw3583 gw3583 commented Jun 18, 2018

I did some profiling and there's no noticeable performance improvement when modifying the clip mask shader to assume only circular radii (@pcwalton did land some changes to make the ellipse evaluation use far fewer instructions, so it may be explained by this).

@gw3583 gw3583 closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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