godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.45k stars 21.07k forks source link

Mipmap creation is not threaded (for ImageTexture::create_from_image()) #51966

Open Goblinth opened 3 years ago

Goblinth commented 3 years ago

Godot version

3.3.3 stable.mono (and 3.1.1 stable -> 3.4beta4)

System information

Windows 10, GLES3

Issue description

not sure on title

The mipmap creation in ImageTexture::create_from_image() (flags=1 or 7) runs on the main thread, even if the create_from_image() call itself is in a thread. This results in the application freezing for large resolution images (or presumably for weaker computers).

I am not certain if this is an issue, but I think it should be documented at least.

Steps to reproduce

a.jpg is a 12000x12000 image

    var p:String = "res://a.jpg"
    var t:Thread
    func test() -> void:
        t = Thread.new()
        t.start(self, "_test", 0)

    func _test(_u:int) -> void:
        var i:Image = Image.new()
        var it1:ImageTexture = ImageTexture.new()
        var it2:ImageTexture = ImageTexture.new()

        var e:int = i.load(p)
        if e == OK: 
            it1.create_from_image(i, 1) # causes lag 
            it2.create_from_image(i, 0) # does not cause lag (much less at least)
        t.call_deferred("wait_to_finish")

Minimal reproduction project

mipmap lag.zip

mortarroad commented 3 years ago

Does it actually run on the main thread? Or does it just block somehow? Also, if the lag is caused by generating the mipmaps, I expected i.generate_mipmaps() to improve performance, but it does not seem so.

clayjohn commented 3 years ago

I have a feeling the lag has more to do with the fact that you call wait_to_funish immediately after generating the mipmaps. This tells the main thread to stall until your thread has finished running.

Goblinth commented 3 years ago

I have a feeling the lag has more to do with the fact that you call wait_to_funish immediately after generating the mipmaps. This tells the main thread to stall until your thread has finished running.

The actual implementation in my program (where I noticed the issue) has threads that only exit when the application closes, so wait_to_finish() is not the issue.

clayjohn commented 3 years ago

@fire I'm reopening this. I don't think we properly determined if this was a limitation or a bug. I don't see any obvious reason why mipmap generation would stall the main thread. And the user confirmed that they were not forcing a stall. This issue should be investigated before it is closed.

mortarroad commented 3 years ago

Alright, a little breakdown: If the thread model is not set to RENDER_THREAD_UNSAFE, the visual server is wrapped in VisualServerWrapMT, which enqueues every call to the visual server to the rendering thread, and waits until that has returned.

The call in question here create_from_image https://github.com/godotengine/godot/blob/46ad2560a1a161c452b9a5a6b3b57327c09943b2/scene/resources/texture.cpp#L189-L202

makes the following blocking calls on the visual server: texture_allocate https://github.com/godotengine/godot/blob/46ad2560a1a161c452b9a5a6b3b57327c09943b2/drivers/gles3/rasterizer_storage_gles3.cpp#L545

and texture_set_data https://github.com/godotengine/godot/blob/46ad2560a1a161c452b9a5a6b3b57327c09943b2/drivers/gles3/rasterizer_storage_gles3.cpp#L702

The latter interestingly checks, whether the image has mipmaps already, and uses those instead of generating new ones. That's why I expected adding a manual generate_mipmaps() in the script before creating the texture would speed things up, since that runs on the CPU. I'm unclear why this is so much slower, since the amount of transferred data isn't much bigger. And in the case where the mipmaps have to be generated, that happens via glGenerateMipmap, which should run asynchronously on the GPU and be reasonably fast. (However, I just noticed that in this example, the texture is 12000², which is neither power-of-2, nor a reasonable size. That might indeed cause a stall in some way.)

I'm not sure what the conclusion should be here.

Zylann commented 2 years ago

I think https://github.com/godotengine/godot/pull/51969 is wrong, because I see no code enforcing Image.generate_mipmaps to run on the main thread, and I see no reason it would be a problem to run on a different thread. I also see no good reason for it to be enforced in any way. As far as Image is concerned, there is no exotic threading issue there.

As for ImageTexture, I havent fully followed nor use Godot 3 GLES3 renderer anymore, but I believe it isn't good at multithreading graphics resource creation. It is better with the Vulkan renderer though.

clayjohn commented 2 years ago

I think #51969 is wrong, because I see no code enforcing Image.generate_mipmaps to run on the main thread, and I see no reason it would be a problem to run on a different thread. I also see no good reason for it to be enforced in any way. As far as Image is concerned, there is no exotic threading issue there.

Indeed Image.generate_mipmaps() should run on the calling thread. Nothing about it enforces running on the main thread. As discussed in this issue, the issue OP is facing is that ImageTexture.create_from_image() calls rendering server functions that have to run on the main thread in Godot 3.x. So it doesn't return until those functions are run.

As for ImageTexture, I havent fully followed nor use Godot 3 GLES3 renderer anymore, but I believe it isn't good at multithreading graphics resource creation. It is better with the Vulkan renderer though.

It is still relevant for the OpenGL3 renderer, for the Vulkan renderer everything should be fine running on the calling thread.

That being said, this issue is not about 4.0 documentation it is about a lag a user experiences in 3.x. Accordingly the 4.0 documentation issue should be tracked elsewhere