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

Stop using `glBlitFramebuffer()` for scaling tasks. #3098

Merged
merged 1 commit into from Sep 25, 2018

Conversation

@pcwalton
Copy link
Collaborator

pcwalton commented Sep 21, 2018

The driver is allowed to read outside of the source rect, and this can cause
incorrect results.

This patch makes preserve-3d.png test have some minor rounding differences in
its colors from the previous implementation, so I regenerated the reference
image.

Closes #3088.

r? @gw3583


This change is Reviewable

@pcwalton pcwalton requested a review from gw3583 Sep 21, 2018
@gw3583
gw3583 approved these changes Sep 21, 2018
Copy link
Collaborator

gw3583 left a comment

Looks good to me, we should do a try run for safety though.

@gw3583
Copy link
Collaborator

gw3583 commented Sep 21, 2018

CI warnings error:

error: unused import: `ScalingInfo`
  --> webrender/src/renderer.rs:74:53
   |
74 | use tiling::{Frame, RenderTarget, RenderTargetKind, ScalingInfo, TextureCacheRenderTarget};
   |                                                     ^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

@pcwalton pcwalton force-pushed the pcwalton:scaling branch from 4ed2968 to 4cfd099 Sep 21, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 21, 2018

@gw3583 OK, I fixed that issue. I still can't push to try yet because my hg access was disabled for inactivity, though.

@kvark
kvark approved these changes Sep 21, 2018
Copy link
Member

kvark left a comment

Code looks good, minus a few nits. The try push is all red.

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pcwalton)


webrender/res/cs_scale.glsl, line 49 at r1 (raw file):

    vec2 uv0 = src_rect.p0 / texture_size;
    vec2 uv1 = (src_rect.p0 + src_rect.size) / texture_size;
    vUv.xy = mix(uv0, uv1, aPosition.xy);

nit: could just do vUv.xy = (src_rect.p0 + src_rect.size * aPosition.xy) / texture_size;


webrender/src/renderer.rs, line 2864 at r1 (raw file):

        }

        match source {

nit: let shader = match source {...}; then shader.bind(...);

@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 21, 2018

I don't understand what's going on on try.

Copy link
Collaborator Author

pcwalton left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pcwalton)


webrender/res/cs_scale.glsl, line 49 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could just do vUv.xy = (src_rect.p0 + src_rect.size * aPosition.xy) / texture_size;

This code was copied from cs_blur.glsl. I'll rewrite the corresponding code in cs_blur.glsl when I get around to updating that file too.

Copy link
Collaborator Author

pcwalton left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark and @pcwalton)


webrender/src/renderer.rs, line 2864 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: let shader = match source {...}; then shader.bind(...);

It won't borrow check that way because of the lack of NLL (self.draw_instanced_batch is &mut self). Do you still want me to make the change with an extra {} level to work around lack of NLL or is that too ugly?

@kvark
kvark approved these changes Sep 21, 2018
Copy link
Member

kvark left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/renderer.rs, line 2864 at r1 (raw file):

Previously, pcwalton (Patrick Walton) wrote…

It won't borrow check that way because of the lack of NLL (self.draw_instanced_batch is &mut self). Do you still want me to make the change with an extra {} level to work around lack of NLL or is that too ugly?

I see, nvm then :) we'll clean this up later

@pcwalton pcwalton force-pushed the pcwalton:scaling branch from 4cfd099 to b9d7eae Sep 21, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 21, 2018

Kicked off a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef0e6a9b321a1197d5eeee9aac514b4ab960f85

I verified that the browser works, so I'm not sure why the previous build failed.

@pcwalton pcwalton force-pushed the pcwalton:scaling branch from b9d7eae to 4d7d196 Sep 21, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 21, 2018

Doh, I didn't realize new shaders have to be explicitly deinitialized now. I added a comment so that others won't be as confused as I was in the future and kicked off a new try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c4bc7e1093d447d091c8531d398df7f66b616cb

@pcwalton pcwalton force-pushed the pcwalton:scaling branch 2 times, most recently from fa93f6a to a559a71 Sep 21, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Sep 23, 2018

Looks like a lot of orange on that last try build - is that the correct try run?

@gw3583
Copy link
Collaborator

gw3583 commented Sep 24, 2018

@bors-servo r+ 🚀

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2018

📌 Commit a559a71 has been approved by gw3583

bors-servo added a commit that referenced this pull request Sep 24, 2018
Stop using `glBlitFramebuffer()` for scaling tasks.

The driver is allowed to read outside of the source rect, and this can cause
incorrect results.

This patch makes `preserve-3d.png` test have some minor rounding differences in
its colors from the previous implementation, so I regenerated the reference
image.

Closes #3088.

r? @gw3583

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

bors-servo commented Sep 24, 2018

Testing commit a559a71 with merge fa2214b...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2018

💔 Test failed - status-taskcluster

@gw3583
Copy link
Collaborator

gw3583 commented Sep 24, 2018

CI failures:

REFTEST INFO | 308 passing, 3 failing
Reftests with unexpected results:
	reftests/boxshadow/inset-border-radius.yaml == reftests/boxshadow/inset-border-radius.png
	reftests/filters/filter-blur.yaml == reftests/filters/filter-blur.png
	reftests/snap/preserve-3d.yaml == reftests/snap/preserve-3d.png

@pcwalton pcwalton force-pushed the pcwalton:scaling branch 2 times, most recently from 5950e9f to 183f40d Sep 25, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 25, 2018

The test failures look like imperceptible differences.

The driver is allowed to read outside of the source rect, and this can cause
incorrect results.

This patch makes `preserve-3d.png` test have some minor rounding differences in
its colors from the previous implementation, so I regenerated the reference
image.

Closes #3088.
@pcwalton pcwalton force-pushed the pcwalton:scaling branch from 183f40d to 3c50dc4 Sep 25, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Sep 25, 2018

@bors-servo: r=gw3583

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

📌 Commit 3c50dc4 has been approved by gw3583

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

Testing commit 3c50dc4 with merge 43ffae1...

bors-servo added a commit that referenced this pull request Sep 25, 2018
Stop using `glBlitFramebuffer()` for scaling tasks.

The driver is allowed to read outside of the source rect, and this can cause
incorrect results.

This patch makes `preserve-3d.png` test have some minor rounding differences in
its colors from the previous implementation, so I regenerated the reference
image.

Closes #3088.

r? @gw3583

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

bors-servo commented Sep 25, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 43ffae1 to master...

@bors-servo bors-servo merged commit 3c50dc4 into servo:master Sep 25, 2018
2 of 4 checks passed
2 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
code-review/reviewable 7 files, 2 discussions left (kvark, pcwalton)
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.