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

Don't panic when rendering huge border corners #2916

Merged
merged 1 commit into from Jul 30, 2018

Conversation

@nical
Copy link
Collaborator

nical commented Jul 19, 2018

This is a simple workaround for huge border corners which currently try to allocate very large surfaces to render the mask.

The basic idea is to render them at a lower resolution (the maximum resolution being 2048x2048 device pixels per corner) and let the brush stretch them. As a result huge corners (with a radius bigger than 2048 device pixels) can render blurry, however we don't crash anymore and the code is very simple.
I toyed with rendering only the visible portion of the corner without down scaling, but it required adding a lot of code for a use case that I think isn't worth the maintenance cost in my opinion.

The patch also sets a limit on the number of dots/dashes so that we don't hit the maximum number of segments per primitive. This isn't great but fixing it properly isn't our highest priority. I filed #2915.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 19, 2018

With this, gecko can survive the crashtest from bug 1474885. Fixes #2884.

@kvark
kvark approved these changes Jul 19, 2018
// No point in pushing more clips as it will blow up the maximum amount of
// segments per primitive later down the road.
// See #2915 for a better fix.
let max_clip_count = self.max_clip_count.min(512);

This comment has been minimized.

@kvark

kvark Jul 19, 2018

Member

nit: should be computed from constants, e.g. MAX_VERTEX_TEXTURE_WIDTH divided by the blocks per segment

// the value must be chosen so that the corners will not use an
// unreasonable amount of memory but should allow crips corners in the
// common cases.
LayoutToDeviceScale::new(2048.0 / r)

This comment has been minimized.

@kvark

kvark Jul 19, 2018

Member

nit: define it as a constant (with sound name)

@nical nical force-pushed the nical:large-border branch from 99dbb36 to d842618 Jul 20, 2018
@kvark
kvark approved these changes Jul 30, 2018
@kvark
Copy link
Member

kvark commented Jul 30, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2018

📌 Commit d842618 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 30, 2018

Testing commit d842618 with merge 2f35640...

bors-servo added a commit that referenced this pull request Jul 30, 2018
Don't panic when rendering huge border corners

This is a simple workaround for huge border corners which currently try to allocate very large surfaces to render the mask.

The basic idea is to render them at a lower resolution (the maximum resolution being 2048x2048 device pixels per corner) and let the brush stretch them. As a result huge corners (with a radius bigger than 2048 device pixels) can render blurry, however we don't crash anymore and the code is very simple.
I toyed with rendering only the visible portion of the corner without down scaling, but it required adding a lot of code for a use case that I think isn't worth the maintenance cost in my opinion.

The patch also sets a limit on the number of dots/dashes so that we don't hit the maximum number of segments per primitive. This isn't great but fixing it properly isn't our highest priority. I filed #2915.

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

bors-servo commented Jul 30, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 2f35640 to master...

@bors-servo bors-servo merged commit d842618 into servo:master Jul 30, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@nical nical deleted the nical:large-border branch Aug 2, 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.