Fix rendering images with nearest sampling and 2^n size #11162

SnailBones · 2021-10-22T01:12:09Z

This code appears to be attempting to use mipmaps when textures are a power of two square:

Lines 94 to 96 in 1333ef1

    
           if (minFilter === gl.LINEAR_MIPMAP_NEAREST && !this.isSizePowerOfTwo()) { 
        
               minFilter = gl.LINEAR; 
        
           }

In the case where we pass in bind(gl.NEAREST, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST), we get:

            gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
            gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR_MIPMAP_NEAREST);

In this case rendering doesn't work as intended, instead drawing every pixel black.

By replacing gl.LINEAR_MIPMAP_NEAREST with gl.LINEAR this issue appears to be fixed.

Before:

Image on the right has paint property 'raster-resampling': 'nearest'

After:

Launch Checklist

briefly describe the changes in this PR
include before/after visuals or gifs if this PR includes visual changes
write tests for all new functionality
manually test the debug page
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix rendering issue with power of two square images and 'raster-resampling': 'nearest'</changelog>

karimnaaji · 2021-10-26T19:23:01Z

src/render/draw_raster.js


        context.activeTexture.set(gl.TEXTURE0);
-        tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
+        tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE, minFilter);


Are we fixing this at the correct level, have you considered whether there is something incorrect within the internal texture class?

I feel like the texture class could be safer for the developer based on intent. At the moment it's possible to call texture.bind(filter, a_mip_map_filter), without necessarily having created the texture with a call that falls through generateMipMap(), resulting in an incoherent mipmap filter set on a texture that doesn't have any mipmap created.

We could assert or warn if from the callee we request that combination to prevent silent failure.

The flag this.useMipmap in that class may be misleading, because it may be set to true, but might not represent the internal state of whether that texture uses mipmaps. I'd suggest replacing it with this.canUseMipMaps = Boolean(options && options.useMipmap) && isPowerOfTwo().

Then, at binding:

bind(...): if (minFilter is mipmap filter && !canUseMipMaps) { incoherent filter request: no mipmap created and requested a mipmap filter, fallback to minFilter = filter } ...

SnailBones · 2021-10-26T22:53:30Z

I feel like the texture class could be safer for the developer based on intent. At the moment it's possible to call texture.bind(filter, a_mip_map_filter), without necessarily having created the texture with a call that falls through generateMipMap(), resulting in an incoherent mipmap filter set on a texture that doesn't have any mipmap created.

I agree @karimnaaji , this is exactly what's causing this issue.

There's currently a check that's replacing gl.LINEAR_MIPMAP_NEAREST with gl.LINEAR unless size is a power of two. (This replacement is skipped regardless of filter, I'm not sure why rendering fails when is combining a failed mipmap lookup with gl.NEAREST but not gl.LINEAR)

mapbox-gl-js/src/render/texture.js

Lines 94 to 96 in 1cf7180

    
           if (minFilter === gl.LINEAR_MIPMAP_NEAREST && !this.isSizePowerOfTwo()) { 
        
               minFilter = gl.LINEAR; 
        
           }

In my latest commits, I've refactored this check to change gl.LINEAR to gl.LINEAR_MIPMAP_NEAREST if and only if this.useMipmap is true, which now corresponds to whether gl.generateMipmap is called. (so I think sticking with the name "useMipmap is appropriate.) This allowed me to remove minFilteras an argument to texture.bind().

I see two potential issues with my change, both seem unlikely:

This could be a breaking change if users are making use of texture.useMipmap.
If there's a time when we would NOT want to use mipmaps after generating them, this change breaks that.

karimnaaji

Nice find and fix! One nit about keeping mipmap sampling when we use NEAREST after requesting a texture with mipmap as an option. Otherwise LGTM.

karimnaaji · 2021-10-27T15:42:41Z

src/render/texture.js

            gl.generateMipmap(gl.TEXTURE_2D);
        }
    }

-    bind(filter: TextureFilter, wrap: TextureWrap, minFilter: ?TextureFilter) {
+    bind(filter: TextureFilter, wrap: TextureWrap) {


+1 on removing the extra parameter as it really simplifies use of that class (basically infers that from the option request to use mipmaps).

karimnaaji · 2021-10-27T15:51:34Z

src/render/texture.js

        if (filter !== this.filter) {
            gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, filter);
-            gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, minFilter || filter);
+            gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, (this.useMipmap && filter === gl.LINEAR) ? gl.LINEAR_MIPMAP_NEAREST : filter);


👍 That's the correct fallback a filtering request on LINEAR.

For NEAREST, if we request texture.bind(NEAREST, ...) I would still expect as a user of this class to fallback to NEAREST_MIPMAP_NEAREST (nearest within the mip level and nearest between mip levels).

It seems that most of our use cases need to stay non-linear between mip level as a default (we're not using trilinear, but we could extend that when we need it for some cases, e.g. LINEAR_MIPMAP_LINEAR), so we can stick to that for now but still hint the requested filter for within the mip (e.g. filter_MIPMAP_NEAREST).

In practice what that means as a fallback in this case:

LINEAR + mipmap -> LINEAR_MIPMAP_NEAREST

NEAREST + mipmap -> NEAREST_MIPMAP_NEAREST

Great! Happy to add the condition for nearest with mipmap. In practice I don't think this is ever happening, but not a bad idea to future-proof it.

I think it's a possible combination when a user chooses nearest as a paint properties with raster-resampling because we always request mipmaps for it here.

That makes sense! I noticed that code path didn't run in my example, but it should now correctly use mipmaps when it is called. (Repeating textures?)

karimnaaji · 2021-10-27T16:00:25Z

This could be a breaking change if users are making use of texture.useMipmap.
If there's a time when we would NOT want to use mipmaps after generating them, this change breaks that.

This class is only used in internal code so no worry about that. There's only one case at the moment where we explicitly request mipmaps (raster_tile_source) so this isn't breaking usage.

* Added render tests for image with a power of 2 size * Added nearest resampling, test is failing * Fix by disabling mipmaps * Keeping same behavior with linear sampling * Correcting expected.ping to use nearest filtering * Adding check for useMipMap * Refactoring * Removing third argument from * Fix broken refactoring * Removing stray console.log * Readd mipmaps * minor refactor * Mipmaps with nearest

SnailBones added 3 commits October 21, 2021 19:10

Added render tests for image with a power of 2 size

b7bb196

Added nearest resampling, test is failing

73b2487

Fix by disabling mipmaps

49f45e0

SnailBones changed the title ~~Added render test for image with a power of 2 size~~ Fix rendering images with nearest sampling and 2^n size Oct 22, 2021

Keeping same behavior with linear sampling

bebd455

SnailBones added the bug 🐞 label Oct 26, 2021

Correcting expected.ping to use nearest filtering

0dcfc2c

karimnaaji reviewed Oct 26, 2021

View reviewed changes

SnailBones added 6 commits October 26, 2021 14:27

Adding check for useMipMap

19896ce

Refactoring

57d18b8

Removing third argument from

0b81246

Fix broken refactoring

69dbf01

Removing stray console.log

976b8e1

Readd mipmaps

f21aba4

SnailBones marked this pull request as ready for review October 26, 2021 22:54

minor refactor

144e4c6

SnailBones requested a review from karimnaaji October 26, 2021 23:02

SnailBones added the refactoring 🏗️ label Oct 26, 2021

karimnaaji reviewed Oct 27, 2021

View reviewed changes

Mipmaps with nearest

c98c5a9

karimnaaji approved these changes Oct 27, 2021

View reviewed changes

karimnaaji merged commit 309c54b into main Oct 27, 2021

karimnaaji deleted the aidan/image-black branch October 27, 2021 16:32

SnailBones self-assigned this Oct 27, 2021

ansis mentioned this pull request Nov 5, 2021

update changelogs and versions for v2.6.0-beta.2 #11231

Merged

SnailBones mentioned this pull request Jan 29, 2022

Canvas sources sized in exact powers of 2 (e.g. 512x512) render as black squares #9873

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rendering images with nearest sampling and 2^n size #11162

Fix rendering images with nearest sampling and 2^n size #11162

SnailBones commented Oct 22, 2021 •

edited

Loading

karimnaaji Oct 26, 2021

SnailBones commented Oct 26, 2021 •

edited

Loading

karimnaaji left a comment

karimnaaji Oct 27, 2021

karimnaaji Oct 27, 2021 •

edited

Loading

karimnaaji Oct 27, 2021

SnailBones Oct 27, 2021

karimnaaji Oct 27, 2021 •

edited

Loading

SnailBones Oct 27, 2021 •

edited

Loading

karimnaaji commented Oct 27, 2021

	if (minFilter === gl.LINEAR_MIPMAP_NEAREST && !this.isSizePowerOfTwo()) {
	minFilter = gl.LINEAR;
	}

Fix rendering images with nearest sampling and 2^n size #11162

Fix rendering images with nearest sampling and 2^n size #11162

Conversation

SnailBones commented Oct 22, 2021 • edited Loading

Before:

After:

Launch Checklist

karimnaaji Oct 26, 2021

Choose a reason for hiding this comment

SnailBones commented Oct 26, 2021 • edited Loading

karimnaaji left a comment

Choose a reason for hiding this comment

karimnaaji Oct 27, 2021

Choose a reason for hiding this comment

karimnaaji Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

karimnaaji Oct 27, 2021

Choose a reason for hiding this comment

SnailBones Oct 27, 2021

Choose a reason for hiding this comment

karimnaaji Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

SnailBones Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

karimnaaji commented Oct 27, 2021

SnailBones commented Oct 22, 2021 •

edited

Loading

SnailBones commented Oct 26, 2021 •

edited

Loading

karimnaaji Oct 27, 2021 •

edited

Loading

karimnaaji Oct 27, 2021 •

edited

Loading

SnailBones Oct 27, 2021 •

edited

Loading