Enforce calling RenderingDevice code from rendering thread in TextureRD classes

[clayjohn]

Needed for #87590

This fixes an unreported bug in the TextureRD classes where RenderingDevice functions could get called from any thread. Currently we allow doing this as we guard every RD function that cares about threads behind a mutex. However in #87590 we are moving to a much more efficient approach which is to have thread guards on single-threaded functions and to have nothing on the functions that can be called from any thread.

Accordingly, certain functions will need to be called from the rendering thread using call_on_render_thread(). The overall impact of this change is minimal.

[BastiaanOlij]

Good catch. I would change this slightly because this means the property doesn't change at all until the rendering thread can process this. Which means that I can call set_texture_rd_rid, but still get the old rid afterwards when I call get_texture_rd_rid.

The same with the texture object, it will now be created late, while we likely need it earlier when it is being assigned to materials etc. This is why texture_rd_create creates the RID regardless of the thread, even if it doesn't do anything else.

So I think we should assign the texture_rd_rid variable, and call texture_rd_create if !texture.is_valid (and probably the emit changes thing) on the main thread, and then do the rest on the rendering thread. This could mean that we're assigning an invalid rid but in that case the texture will just not be fully setup.

[clayjohn]

Good catch. I would change this slightly because this means the property doesn't change at all until the rendering thread can process this. Which means that I can call set_texture_rd_rid, but still get the old rid afterwards when I call get_texture_rd_rid.

The same with the texture object, it will now be created late, while we likely need it earlier when it is being assigned to materials etc. This is why texture_rd_create creates the RID regardless of the thread, even if it doesn't do anything else.

We can't call texture_rd_create() if texture_rd_rid is invalid. And we can't check the validity of texture_rd_rid on the main thread.

That being said, I'm not too worried about creating the texture object. We create a placeholder if the user calls get_rid() before this code runs.

This has made me realize that texture_rd_create() is actually a bit of a problem. It is defined using FUNCRIDTEX2:

godot/servers/rendering/rendering_server_default.h

Lines 159 to 168 in 4577dfd

	#define FUNCRIDTEX2(m_type, m_type1, m_type2) \
	virtual RID m_type##_create(m_type1 p1, m_type2 p2) override { \
	RID ret = RSG::texture_storage->texture_allocate(); \
	if (Thread::get_caller_id() == server_thread || RSG::texture_storage->can_create_resources_async()) { \
	RSG::texture_storage->m_type##_initialize(ret, p1, p2); \
	} else { \
	command_queue.push(RSG::texture_storage, &RendererTextureStorage::m_type##_initialize, ret, p1, p2); \
	} \
	return ret; \
	}

Which will create the texture on whatever thread it is called from. But it has to be called from the rendering thread because it immediately calls texture_is_shared() which has to be called from the rendering thread.

godot/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp

Lines 1469 to 1474 in 4577dfd

	void TextureStorage::texture_rd_initialize(RID p_texture, const RID &p_rd_texture, const RS::TextureLayeredType p_layer_type) {
	ERR_FAIL_COND(!RD::get_singleton()->texture_is_valid(p_rd_texture));
	// TODO : investigate if we can support this, will need to be able to obtain the order and obtain the slice info
	ERR_FAIL_COND_MSG(RD::get_singleton()->texture_is_shared(p_rd_texture), "Please create the texture object using the original texture");

So I think we should assign the texture_rd_rid variable, and call texture_rd_create if !texture.is_valid (and probably the emit changes thing) on the main thread, and then do the rest on the rendering thread. This could mean that we're assigning an invalid rid but in that case the texture will just not be fully setup.

So I think the correct solution here is going to be a mix of things

Let's assign texture_rd_rid right away and then call texture_rd_create on the rendering thread. We can call notify_property_list_changed right away, but we should wait to emit changed just in case texture_rid isn't assigned yet.

Then, we need to also stop using FUNCRIDTEX2 for texture_rd_create to ensure that it gets called from the rendering thread.

[BastiaanOlij]

@clayjohn it only calls texture_allocate on the thread its called from. It will call texture_rd_initialize on the render thread, thats what the

if (Thread::get_caller_id() == server_thread || RSG::texture_storage->can_create_resources_async()) {    \ 
	RSG::texture_storage->m_type##_initialize(ret, p1, p2);                                              \ 
} else {                                                                                                 \ 
	command_queue.push(RSG::texture_storage, &RendererTextureStorage::m_type##_initialize, ret, p1, p2); \ 
}       

does?

[BastiaanOlij]

Or is it the can_create_resources_async that causes issues when that returns true?

[clayjohn]

Or is it the can_create_resources_async that causes issues when that returns true?

Ya, in the RD renderers can_create_resources_async is true so the function gets called immediately from the calling thread. That's why we need a different macro for it

[akien-mga]

Thanks!