Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow RGBA8 + nearest filtered images in shared texture cache. #1999
Conversation
|
Code looks good to me. One thing I'd like to discuss before merging is that storing image data in a separate texture array just because it uses different sampling seems less than ideal to me in terms of efficiency. We can have both nearest and linear in the same texture cache and just switch the sampling mode per batch, if needed. This can be done by either binding a different GL sampler object (supported GL ES 3.0), or modifying the sampling state on the texture directly. Clearly, that requires batch breaks, but so does this PR, for the fact our image data goes to a separate texture array. |
|
Yea, it's less than ideal. The reason I did it this way was due to an experience I had on a previous project. I had found that on certain GPUs (mostly mobile, I think), changing the sampling state on the texture would cause massive frame spikes. In the project I was dealing with, switching sampling mode on a texture would often take > 16ms each time we drew something with that updated texture. My guess is that the driver had to rebuild the shader in some fairly significant way when the sampling mode was different. It may well be that this no longer occurs, but it's definitely something I've run into in the past. I think it's probably reasonable to merge this, even if as an interim fix, what do you think? Perhaps we could investigate switching the sampler mode on various GPUs and make sure we don't see any spikes as as a follow up? |
|
Re-linking the shader for different sampler modes sounds completely bizzare to me. Maybe some ancient hardware? If that was how mobile GPUs work, they'd reflect this somehow in Vulkan, ideally... Also, as I mentioned, we can bind a different sampler object instead of changing the state of the current texture. This should be more straightforward for the picky driver to handle. Given the concerns, it does appear reasonable to defer my requested improvements until we investigate the mobile GPU behavior for those cases. Therefore, let's merge this, even if we'll need to scrap/revert the change later. @bors-servo r+ |
|
|
|
I'm trying to recall the exact hardware - I think it might have been iPhone 4 / iPad 2 era GPUs. It was really completely bizarre, I couldn't believe it either :) Thanks for the review - I'm certainly happy to have this scrapped and a better solution implemented as a follow up! |
Allow RGBA8 + nearest filtered images in shared texture cache. This drops the draw call count when drawing the Gecko chrome UI by a fairly significant amount, where favicons are being drawn with nearest filtering enabled. Fixes #1992. <!-- 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/1999) <!-- Reviewable:end -->
|
|
glennw commentedNov 6, 2017
•
edited by larsbergstrom
This drops the draw call count when drawing the Gecko chrome UI
by a fairly significant amount, where favicons are being drawn
with nearest filtering enabled.
Fixes #1992.
This change is