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

Fix buggy texelFetchOffset calls on Android #1696

Merged
merged 1 commit into from Sep 13, 2017

Conversation

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Sep 12, 2017

Fixes servo/servo#17621, servo/servo#17851 and #1694

texelFetchOffset is very buggy on some Android GPUs...


This change is Reviewable

@@ -21,6 +21,14 @@
#define TEX_SAMPLE(sampler, tex_coord) textureLod(sampler, tex_coord, 0.0)
#endif

// texelFetchOffset has driver bugs on some Android GPUs (see issue #1694).
// Fallback to texelFetch on mobile GPUs.
#if defined(GL_ES) && GL_ES == 1

This comment has been minimized.

@glennw

glennw Sep 12, 2017

Member

I think we've had a bug previously where some (mobile) shader compilers didn't handle the && properly. Could you switch it to use the same #ifdef setup for HIGHP_SAMPLER_FLOAT in shared.glsl?

This comment has been minimized.

@MortimerGoro

MortimerGoro Sep 12, 2017

Author Contributor

Done

@leeoniya
Copy link

leeoniya commented Sep 12, 2017

@glennw
Copy link
Member

glennw commented Sep 12, 2017

@MortimerGoro MortimerGoro force-pushed the MortimerGoro:texel_fetch_android branch from e0e5a0d to 207eedd Sep 12, 2017
@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Sep 12, 2017

Ok, used nested #ifs to workaround the parser bug. What a driver party!

@glennw
Copy link
Member

glennw commented Sep 12, 2017

Looks good, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 12, 2017

📌 Commit 207eedd has been approved by glennw

@leeoniya
Copy link

leeoniya commented Sep 12, 2017

@glennw ah, cool.

is there any benefit in linking related issues in the wiki since they may surface in different contexts.

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Sep 12, 2017

@glennw I think that @leeoniya meant to add the texelFetchOffset related bug

@glennw
Copy link
Member

glennw commented Sep 12, 2017

@leeoniya Yep, I think that would be useful.

@glennw
Copy link
Member

glennw commented Sep 12, 2017

@MortimerGoro @leeoniya Ah, yes - we should definitely add the texelFetch bug to that page :)

@leeoniya
Copy link

leeoniya commented Sep 12, 2017

yeah, sry i meant the texelFetchOffset bug

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2017

Testing commit 207eedd with merge 059c15c...

bors-servo added a commit that referenced this pull request Sep 13, 2017
Fix buggy texelFetchOffset calls on Android

Fixes servo/servo#17621, servo/servo#17851 and #1694

texelFetchOffset is very buggy on some Android GPUs...

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

bors-servo commented Sep 13, 2017

💔 Test failed - status-travis

@glennw
Copy link
Member

glennw commented Sep 13, 2017

@bors-servo retry

@glennw
glennw approved these changes Sep 13, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2017

Testing commit 207eedd with merge 046cdf6...

bors-servo added a commit that referenced this pull request Sep 13, 2017
Fix buggy texelFetchOffset calls on Android

Fixes servo/servo#17621, servo/servo#17851 and #1694

texelFetchOffset is very buggy on some Android GPUs...

<!-- 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/1696)
<!-- Reviewable:end -->
@leeoniya
Copy link

leeoniya commented Sep 13, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2017

☀️ Test successful - status-travis
Approved by: glennw
Pushing 046cdf6 to master...

@bors-servo bors-servo merged commit 207eedd into servo:master Sep 13, 2017
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
homu Test successful
Details
@kvark
Copy link
Member

kvark commented Sep 13, 2017

Hm

OSError: [Errno 13] Permission denied: '/usr/local/lib/python2.7/dist-packages/markupsafe'

@bors-servo retry

@kvark
Copy link
Member

kvark commented Sep 13, 2017

oh wow, looks like github has screwed me up a bit

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.