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

Port radial gradient shader to be a brush primitive. #2433

Merged
merged 1 commit into from Feb 18, 2018

Conversation

@glennw
Copy link
Member

glennw commented Feb 16, 2018

This is mostly a straightforward conversion, with a couple of
interesting notes:

  • The previous radial gradient shader was missing a transform
    implementation so would never have worked properly with
    rotation and/or perspective.
  • We didn't (and still don't) detect if a radial gradient is
    opaque - it's always assumed transparent. So even though
    it can now opt in to segments (and save clip mask generation
    time), the inner segments will still be in the alpha pass.
    We should fix this!
  • Previously, we used to append the gradient stops directly
    after the primitive information in the main GPU cache handle.
    Now, there is a separate GPU cache handle for the stops. This
    is quite a bit more efficient in the GPU cache, since the
    size of the stops array is large, and a power of 2, it now
    fits exactly into a GPU cache slab, and allows the main
    primitive data to be allocated in a smaller row.

This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Feb 16, 2018

r? @kvark or @nical

I will kick off a gecko try shortly.

@glennw
Copy link
Member Author

glennw commented Feb 16, 2018

Gecko try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af47d705000970aa0504145c2bbdc6314b8c1a61&selectedJob=162598036

Despite the number of orange, I think this is actually good. I think the Windows gpu, mda, 2 and C items are unrelated intermittents. The fails in R4 and R8 are from previous PRs (the mipmap and local box-shadow clip ones).

The other two unexpected results in R4 are new radial gradient PASS results.

Of course, it'd be good for @staktrace or someone else to verify the above before merging.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

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

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

The windows failures in the try push don't look like intermittents to me. They seem to be all crashes of some sort, and I haven't seen that kind of failure before. I can do a try push for this change using my script and retrigger the jobs a few times to confirm one way or another.

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

Try push is in progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7e716bb5071bcb271c8a14a4c0101bc5a36b15c (I just included the windows jobs)

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

@glennw Looks like there's shader compilation failures. See output at https://treeherder.mozilla.org/logviewer.html#?job_id=162670092&repo=try&lineNumber=1904

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

I think the shader compilation failures are actually coming from #2424 which merged already. Although this PR might make it worse since the try push for this has mda job failures as well which I didn't see on the try push including just #2424.

@kvark
Copy link
Member

kvark commented Feb 16, 2018

:lgtm:


Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/prim_store.rs, line 208 at r1 (raw file):

        start_center: LayerPoint,
        end_center: LayerPoint,
        start_radius: f32,

may consider using ops::Range for some of the start/stop pairs


Comments from Reviewable

This is mostly a straightforward conversion, with a couple of
interesting notes:

* The previous radial gradient shader was missing a transform
  implementation so would never have worked properly with
  rotation and/or perspective.
* We didn't (and still don't) detect if a radial gradient is
  opaque - it's always assumed transparent. So even though
  it can now opt in to segments (and save clip mask generation
  time), the inner segments will still be in the alpha pass.
  We should fix this!
* Previously, we used to append the gradient stops directly
  after the primitive information in the main GPU cache handle.
  Now, there is a separate GPU cache handle for the stops. This
  is quite a bit more efficient in the GPU cache, since the
  size of the stops array is large, and a power of 2, it now
  fits exactly into a GPU cache slab, and allows the main
  primitive data to be allocated in a smaller row.
@glennw glennw force-pushed the glennw:gradient-repeat branch from 33b8ec9 to 621c936 Feb 18, 2018
@glennw
Copy link
Member Author

glennw commented Feb 18, 2018

Try run after rebase:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00bec7660cc074f579ba22eaa221c7b598fa9350&selectedJob=162923263

We have:

  • A few fails in border radii cases which are going to be fuzzed as a result of the ellipse distance AA change.
  • A few fails in the input tests using box-shadows related to the box-shadow local clip changes.
  • Three new passes in the radial gradient tests.

So, this should be OK to merge.

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2018

📌 Commit 621c936 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2018

Testing commit 621c936 with merge 196b9bf...

bors-servo added a commit that referenced this pull request Feb 18, 2018
Port radial gradient shader to be a brush primitive.

This is mostly a straightforward conversion, with a couple of
interesting notes:

* The previous radial gradient shader was missing a transform
  implementation so would never have worked properly with
  rotation and/or perspective.
* We didn't (and still don't) detect if a radial gradient is
  opaque - it's always assumed transparent. So even though
  it can now opt in to segments (and save clip mask generation
  time), the inner segments will still be in the alpha pass.
  We should fix this!
* Previously, we used to append the gradient stops directly
  after the primitive information in the main GPU cache handle.
  Now, there is a separate GPU cache handle for the stops. This
  is quite a bit more efficient in the GPU cache, since the
  size of the stops array is large, and a power of 2, it now
  fits exactly into a GPU cache slab, and allows the main
  primitive data to be allocated in a smaller row.

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

bors-servo commented Feb 18, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing 196b9bf to master...

@bors-servo bors-servo merged commit 621c936 into servo:master Feb 18, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 7 files, 1 discussion left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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