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

Invalid mipmap size is calculated for backbuffer #56511

Closed
saki7 opened this issue Jan 5, 2022 · 0 comments · Fixed by #57682
Closed

Invalid mipmap size is calculated for backbuffer #56511

saki7 opened this issue Jan 5, 2022 · 0 comments · Fixed by #57682

Comments

@saki7
Copy link
Contributor

saki7 commented Jan 5, 2022

Godot version

master (2c7fcdd)

System information

Windows 10, GTX1080Ti (497.09), Vulkan

Issue description

When using BackBufferCopy with screen-reading shaders, the builtin backbuffer mipmap (i.e. RenderTarget::BackbufferMipmap) uses wrong mipmap size: [1px x 0px].

ERROR: Height must be equal or greater than 1 for 2D and 3D textures
   at: (drivers\vulkan\rendering_device_vulkan.cpp:1756)

Following backtraces

The error comes from this section:

image_create_info.extent.width = p_format.width;
if (image_create_info.imageType == VK_IMAGE_TYPE_3D || image_create_info.imageType == VK_IMAGE_TYPE_2D) {
ERR_FAIL_COND_V_MSG(p_format.height < 1, RID(), "Height must be equal or greater than 1 for 2D and 3D textures");
image_create_info.extent.height = p_format.height;
} else {
image_create_info.extent.height = 1;
}

Which goes to:

Size2 mm_size = Image::get_image_mipmap_size(tf.width, tf.height, Image::FORMAT_RGBA8, i);
RD::TextureFormat mmtf = tf;
mmtf.width = mm_size.width;
mmtf.height = mm_size.height;
mmtf.mipmaps = 1;
mm.mipmap_copy = RD::get_singleton()->texture_create(mmtf, RD::TextureView());

Image::get_image_mipmap_size redirects to Image::_get_dst_image_size:

int Image::_get_dst_image_size(int p_width, int p_height, Format p_format, int &r_mipmaps, int p_mipmaps, int *r_mm_width, int *r_mm_height) {

Looking at git blame, most of the lines are more than 3 years old, except for those introduced in the commit 81ed9fa (PR #49421).

Proposed fix

I assume that RendererStorageRD::_create_render_target_backbuffer must never use the invalid height of 0, regardless of backbuffer resolution.

If I understand correctly, the smallest reasonable mipmap size is [1px x 1px]. I have not checked if other mipmap variants' dimensions are calculated correctly.

I am not sure if current implementation is bugged specifically for BackBufferCopy or Image::_get_dst_image_size. There could be other root cause which secondarily makes Image::_get_dst_image_size calculate the wrong size. I've read the function, but I am not confident about showing alternate implementation code (which should involve debugging bitwise operation).

Workaround

--- a/servers/rendering/renderer_rd/renderer_storage_rd.cpp
+++ b/servers/rendering/renderer_rd/renderer_storage_rd.cpp
@@ -7741,8 +7741,8 @@ void RendererStorageRD::_create_render_target_backbuffer(RenderTarget *rt) {
                        Size2 mm_size = Image::get_image_mipmap_size(tf.width, tf.height, Image::FORMAT_RGBA8, i);

                        RD::TextureFormat mmtf = tf;
-                       mmtf.width = mm_size.width;
-                       mmtf.height = mm_size.height;
+                       mmtf.width = MAX(1, mm_size.width);
+                       mmtf.height = MAX(1, mm_size.height);
                        mmtf.mipmaps = 1;

                        mm.mipmap_copy = RD::get_singleton()->texture_create(mmtf, RD::TextureView());

Similar reports

Note that the same output was initially reported in #55097, but the ticket was (mistakenly) marked as duplicate of #50976. I believe that the error spam actually is not a single bug, but it is a mixture of 2 bugs:

  1. The copy-and-paste bug which I described in the comment of #50976, which is to be handled in that issue
  2. The mipmap calculation error which was ignored in texture(SCREEN_TEXTURE, SCREEN_UV) shader bug in 4.0 dev for Vulkan #55097.

The behavior (2) is currently not tracked, so I'm hereby opening a new issue.

Steps to reproduce

Scene tree:

BackBufferCopy
  +--Sprite2D

Shader on Sprite2D:

shader_type canvas_item;

void fragment() {
	COLOR = textureLod(SCREEN_TEXTURE, SCREEN_UV, 1.0); // error
	//COLOR = texture(SCREEN_TEXTURE, SCREEN_UV); // error
	//COLOR = texture(TEXTURE, UV); // OK
	//COLOR = vec4(1.0, 0.0, 1.0, 1.0); // OK
}

Since the minimal scene is pretty much trivial, I suppose many other developers are seeing similar errors...

Minimal reproduction project

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants