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

Use glCopyImageSubData where available to copy data between texture arrays #3338

Merged
merged 1 commit into from Nov 27, 2018

Conversation

@jamienicol
Copy link
Contributor

jamienicol commented Nov 22, 2018

Falling back to the existing glBlitFramebuffer loop. This avoids a
driver bug with texture arrays and glBlitFramebuffer on Adreno 4xx,
5xx, and 6xx devices, where the blit was failing and the texture cache
became corrupted.

Adreno 3xx seems to be affected by a similar bug, but does not support
glCopyImageSubData, so this can not be used there. Once the exact
nature of that bug has been established and a workaround identified,
we should re-evaluate whether this change is still desirable or if the
code paths can be consolidated.


This change is Reviewable

@jamienicol
Copy link
Contributor Author

jamienicol commented Nov 22, 2018

Note that this is instead of #3327, as it does not work on 3xx or 4xx, for some reason not yet known.

The bug on 3xx also seems slightly different than on later Adrenos. Once we properly understand it, we can make a better decision on how to minimise divergent code whilst supporting as many devices as possible.

@jamienicol
Copy link
Contributor Author

jamienicol commented Nov 23, 2018

The taskcluster failure seems to be because the updated gleam depends on a newer gl_generator than glutin and mozangle. (which then depends on a newer xml-rs and khronos_api).

I don't know if this is actually be a problem? I tried to update to the newest glutin to see if that would work, and it seems there are api changes so that won't be straightforward. Is it okay to just add gl_generator, khronos_api, and xml-rs, to the ignore packages in servo-tidy.toml?

@staktrace
Copy link
Contributor

staktrace commented Nov 23, 2018

@jamienicol looks like @djg has a patch to bump glutin etc. in #3343, maybe you can steal that?

@djg
Copy link
Contributor

djg commented Nov 25, 2018

@emilio
Copy link
Member

emilio commented Nov 25, 2018

Is it okay to just add gl_generator, khronos_api, and xml-rs, to the ignore packages in servo-tidy.toml?

I think given they're build-time only dependencies it should be fine.

self.bind_draw_target_impl(*draw_fbo);
self.blit_render_target(rect, rect);
if self.supports_copy_image_sub_data {
assert!(src.id != dst.id,

This comment has been minimized.

@emilio

emilio Nov 25, 2018

Member

nit: assert_neq! should give you a slightly better error message.

@atouchet
Copy link
Contributor

atouchet commented Nov 26, 2018

I think @kvark was involved with the glutin API change. Perhaps he knows what to do with that.

@jamienicol jamienicol force-pushed the jamienicol:copyimagesubdata branch from a984fa5 to 9ca10d1 Nov 26, 2018
@jamienicol
Copy link
Contributor Author

jamienicol commented Nov 26, 2018

Latest push adds duplicated package versions to the ignore list, and uses assert_ne!.

@kvark
kvark approved these changes Nov 26, 2018
@kvark
Copy link
Member

kvark commented Nov 26, 2018

Do our test bots have this extension? If so, we should see what the try results are before proceeding.
Otherwise 👍 🚢 it

@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2018

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

…rrays

Falling back to the existing glBlitFramebuffer loop. This avoids a
driver bug with texture arrays and glBlitFramebuffer on Adreno 4xx,
5xx, and 6xx devices, where the blit was failing and the texture cache
became corrupted.

Adreno 3xx seems to be affected by a similar bug, but does not support
glCopyImageSubData, so this can not be used there. Once the exact
nature of that bug has been established and a workaround identified,
we should re-evaluate whether this change is still desirable or if the
code paths can be consolidated.
@jamienicol jamienicol force-pushed the jamienicol:copyimagesubdata branch from 9ca10d1 to 85cb0a7 Nov 27, 2018
@jamienicol
Copy link
Contributor Author

jamienicol commented Nov 27, 2018

Latest push fixes conflict from #3356 in Cargo.lock

I have no idea what hardware the test machines have but I reckon the linux ones could well have that extension. I'm not sure how to do a try run including the gleam update. @staktrace?

@staktrace
Copy link
Contributor

staktrace commented Nov 27, 2018

I kicked off a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6465f51752f90f22d0cdbd2b21a246ae24702686 - for future reference the doc here describes how to do an update including dependencies.

@jamienicol
Copy link
Contributor Author

jamienicol commented Nov 27, 2018

Thanks Kats!

Looks like we have a couple tests which need fuzzed, and an unexpected pass.

@staktrace
Copy link
Contributor

staktrace commented Nov 27, 2018

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Nov 27, 2018

📌 Commit 85cb0a7 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Nov 27, 2018

Testing commit 85cb0a7 with merge 586af96...

bors-servo added a commit that referenced this pull request Nov 27, 2018
Use glCopyImageSubData where available to copy data between texture arrays

Falling back to the existing glBlitFramebuffer loop. This avoids a
driver bug with texture arrays and glBlitFramebuffer on Adreno 4xx,
5xx, and 6xx devices, where the blit was failing and the texture cache
became corrupted.

Adreno 3xx seems to be affected by a similar bug, but does not support
glCopyImageSubData, so this can not be used there. Once the exact
nature of that bug has been established and a workaround identified,
we should re-evaluate whether this change is still desirable or if the
code paths can be consolidated.

<!-- 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/3338)
<!-- Reviewable:end -->
@staktrace
Copy link
Contributor

staktrace commented Nov 27, 2018

Looks like we have a couple tests which need fuzzed, and an unexpected pass.

Actually these changes were from bug 1509973 which landed on autoland this morning and just got backed out. So your PR shouldn't need any fuzzing.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 27, 2018

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

@bors-servo bors-servo merged commit 85cb0a7 into servo:master Nov 27, 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
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 28, 2018
…fb47f1d270a1 (WR PR #3338). r=kats

servo/webrender#3338

Differential Revision: https://phabricator.services.mozilla.com/D13115

--HG--
extra : moz-landing-system : lando
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Nov 28, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…fb47f1d270a1 (WR PR #3338). r=kats

servo/webrender#3338

Differential Revision: https://phabricator.services.mozilla.com/D13115

UltraBlame original commit: 52b6028ac2b3492fd02f2e4e9ee74caea52ee2ff
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…fb47f1d270a1 (WR PR #3338). r=kats

servo/webrender#3338

Differential Revision: https://phabricator.services.mozilla.com/D13115

UltraBlame original commit: 52b6028ac2b3492fd02f2e4e9ee74caea52ee2ff
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…fb47f1d270a1 (WR PR #3338). r=kats

servo/webrender#3338

Differential Revision: https://phabricator.services.mozilla.com/D13115

UltraBlame original commit: 52b6028ac2b3492fd02f2e4e9ee74caea52ee2ff
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

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