Texture broken with mipmaps enabled

[KoBeWi]

Godot version

3.4 beta1

System information

W10

Issue description

I have a texture that can't be imported with mipmaps enabled. It shows this error:

 Expected data size of 1528 bytes in Image::create(), got instead 1527 bytes.
 drivers/gles3/rasterizer_storage_gles3.cpp:757 - Condition "!read.ptr()" is true.

And image format shows Lum8. The texture becomes black rectangle.
This doesn't happen with other textures, even smaller ones.

Apparently it was fine some time ago and only broke recently. Also works in 4.0.

Steps to reproduce

1. Copy this to your project
   
2. Enable Mipmaps in import options

Minimal reproduction project

No response

[akien-mga]

Confirmed on Jetpaca with this texture: https://github.com/KOBUGE-Games/jetpaca/blob/master/art/intro_sky_bg.png

It's a regression specific to 3.x (present in 3.4-beta1 and latest 3.x commit).

[akien-mga]

Had a couple older builds lying around, the bug is not reproduced in a69cc9f (May 22), but it is reproducible in 83ad0dd (July 8).

Time to bisect...

[akien-mga]

Bisected to #47854, CC @mortarroad.

[mortarroad]

Hmm, doesn't seem to be caused by my commit, just revealed by it.
The WebP unpacker returns its images as either RGB or RGBA, depending on whether the decoded image actually has transparent pixels.

However, the Texture resource loaded assumes all of its images to have the same format. It just uses the format of the first mipmap image, and copies them consecutively in memory.

godot/scene/resources/texture.cpp

Lines 582 to 599 in ab30437

	PoolVector<uint8_t> img_data;
	img_data.resize(total_size);
	{
	PoolVector<uint8_t>::Write w = img_data.write();
	int ofs = 0;
	for (int i = 0; i < mipmap_images.size(); i++) {
	PoolVector<uint8_t> id = mipmap_images[i]->get_data();
	int len = id.size();
	PoolVector<uint8_t>::Read r = id.read();
	memcpy(&w[ofs], r.ptr(), len);
	ofs += len;
	}
	}
	image->create(sw, sh, true, mipmap_images[0]->get_format(), img_data);
	return OK;

Different formats can happen, if the source image has very few transparent pixels, such that a smaller mip map ends up with no transparency.
This issue also affects the lossy WebP encoder.

A quick fix is to let WebP always decode to RGBA.
Another is to properly convert all mipmap images to the same format, before combining them.

[mortarroad]

Adding this

			if (i != 0) {
				img->convert(mipmap_images[0]->get_format()); // ensure the same format for all mipmaps
			}

right here

godot/scene/resources/texture.cpp

Line 567 in ab30437

(i.e. right before the line with the total_size) fixes it.

Also, while I'm here, let me reiterate how silly it is to store mipmaps on disk other than for VRAM compression formats.

[mortarroad]

Looking into 4.x, that seems to have equivalent code.
See:

godot/scene/resources/texture.cpp

Lines 369 to 376 in 7f0a83c

	if (first) {
	//format will actually be the format of the first image,
	//as it may have changed on compression
	format = img->get_format();
	first = false;
	} else if (img->get_format() != format) {
	img->convert(format); //all needs to be the same format
	}

My fix is shorter though, hehe.

[reduz]

Converting on load is inefficient, IMO the webp decoder should be told the desired format instead with a hint.

[mortarroad]

I disagree. I estimate that a conversion happens in less than 1% of the cases and then only for the lowest mipmap levels, which are small anyway (a few pixels at worst).
However, it could make sense to store the used channels anyway, and use then when loading to save memory. That would be a new feature, however.
(Or even better, we don't store mipmaps on disk, and compute them on the fly.)

Either way, the proper fix here is to add the conversion, as in 4.x.

Looking through the code, this issue affects at least one more encoder, possibly others too:
The png encoder also encodes without alpha, if the source image has a more exotic channel layout:

godot/drivers/png/png_driver_common.cpp

Lines 155 to 162 in 0543b71

	default:
	if (source_image->detect_alpha()) {
	source_image->convert(Image::FORMAT_RGBA8);
	png_img.format = PNG_FORMAT_RGBA;
	} else {
	source_image->convert(Image::FORMAT_RGB8);
	png_img.format = PNG_FORMAT_RGB;
	}

[reduz]

Yeah upon closer inspection you are right, this should happen so little often its not worth having a more complex workaround.

[akien-mga]

Fixed by #51555.