Fix: No image GPU texture update for different requests #105547

Omar Emara · 2023-03-07T19:35:49+01:00

Omar Emara commented

2023-03-07 19:35:49 +01:00

The image_get_gpu_texture function checks if the requested properties
are different from the last requested ones, yet it does nothing to
update the texture if it is indeed the case. This patch marks the image
for an update in that case. While marking an update during a request
might seem out of place, that full update marking is immediately handled
by freeing the no longer valid cached GPU texture in
image_gpu_texture_try_partial_update().

The image_get_gpu_texture function checks if the requested properties are different from the last requested ones, yet it does nothing to update the texture if it is indeed the case. This patch marks the image for an update in that case. While marking an update during a request might seem out of place, that full update marking is immediately handled by freeing the no longer valid cached GPU texture in image_gpu_texture_try_partial_update().

Omar Emara added the

Module

EEVEE & Viewport

label 2023-03-07 19:35:49 +01:00

Omar Emara added 1 commit 2023-03-07 19:35:59 +01:00

c44c68598d Fix: No image GPU texture update for different requests

The image_get_gpu_texture function checks if the requested properties
are different from the last requested ones, yet it does nothing to
update the texture if it is indeed the case. This patch marks the image
for an update in that case. While marking an update during a request
might seem out of place, that full update marking is immediately handled
by freeing the no longer valid cached GPU texture in
image_gpu_texture_try_partial_update().

Omar Emara requested review from Clément Foucault 2023-03-07 19:36:21 +01:00

Omar Emara referenced this pull request

2023-03-07 19:38:55 +01:00

Viewport Compositor #99210

Omar Emara commented

2023-03-07 19:44:06 +01:00

Here is a demonstration of the bug at the user level. Notice how even after connecting the Diffuse Indirect pass to the output, the combined pass is still shown. Then, making any change to trigger an update works around the issue.

20230307-204102.mp4

Here is a demonstration of the bug at the user level. Notice how even after connecting the Diffuse Indirect pass to the output, the combined pass is still shown. Then, making any change to trigger an update works around the issue. [20230307-204102.mp4](/attachments/10b3569d-e5b9-4231-ab48-26dc81e24481)

Brecht Van Lommel commented

2023-03-07 21:31:05 +01:00

I don't remember the details, but this might have been intentional to avoid performance issues. If it's loading and unloading images every frame it can get very slow.

If both are needed then we need to be caching both, but there are limitations in the image cache system that need to be solved to support that properly.

I don't remember the details, but this might have been intentional to avoid performance issues. If it's loading and unloading images every frame it can get very slow. If both are needed then we need to be caching both, but there are limitations in the image cache system that need to be solved to support that properly.

Clément Foucault commented

2023-03-08 00:12:07 +01:00

Better ask @Jeroen-Bakker to review this.

Clément Foucault refused to review 2023-03-08 00:12:12 +01:00

Omar Emara requested review from Jeroen Bakker 2023-03-08 07:20:57 +01:00

Omar Emara commented

2023-03-08 07:37:10 +01:00

I understand the performance implications of the change, but currently the bug is apparent even when only a single pass is used, that is, if the user temporarily used a pass then started using another. So I think it is worth merging that change to correct the behavior temporarily until we have a proper caching system. A slower implementation is still better than a buggy one.

Jeroen Bakker commented

2023-03-08 16:25:44 +01:00

I am not able to view the video, (file not found). Can you re-upload it, or sent it via blender.chat?
The implication is that the more local variations of the image could be marked invalid.
Just want to be sure that we are not adding a lot of overhead to fix a local issue.

I am not able to view the video, (file not found). Can you re-upload it, or sent it via blender.chat? The implication is that the more local variations of the image could be marked invalid. Just want to be sure that we are not adding a lot of overhead to fix a local issue.

Omar Emara commented

2023-03-08 16:53:55 +01:00

I sent the video on Blender Chat. What do you mean by "the image could be marked invalid"?

Jeroen Bakker commented

2023-03-09 08:46:19 +01:00

I have watched the video, and in this case the system is failing as there is no setting for the active pass for the viewport compositor. Until we have support for renderpasses this can happen.

In this case I would propose to have a separate check to ensuring the right pass. Eg BKE_image_ensure_gpu_texture and call this function in the viewport compositor just before the BKE_image_get_gpu_texture. The ensure would based on the same settings call the BKE_image_partial_update_mark_full_update.

This could be done in DRW_shgroup_add_material_resources or even better in the ImageOperation::execute. This way we add it where the issue can happen and it makes it easier to remove it after using multiple passes are supported.

Does this makes sense?

I have watched the video, and in this case the system is failing as there is no setting for the active pass for the viewport compositor. Until we have support for renderpasses this can happen. In this case I would propose to have a separate check to ensuring the right pass. Eg `BKE_image_ensure_gpu_texture` and call this function in the viewport compositor just before the `BKE_image_get_gpu_texture`. The ensure would based on the same settings call the `BKE_image_partial_update_mark_full_update`. This could be done in `DRW_shgroup_add_material_resources` or even better in the `ImageOperation::execute`. This way we add it where the issue can happen and it makes it easier to remove it after using multiple passes are supported. Does this makes sense?

Omar Emara commented

2023-03-09 08:50:50 +01:00

Sounds good. I will update the patch accordingly.

Omar Emara added 1 commit 2023-03-11 20:01:54 +01:00

09138963cd Add a BKE function to ensure GPU image cache

Omar Emara commented

2023-03-11 20:03:48 +01:00

@Jeroen-Bakker Not sure if you get notified, but I updated the pull request.

Jeroen Bakker requested changes 2023-03-13 20:49:27 +01:00

Jeroen Bakker left a comment

I didn't test drive it. Code seems fine, just some adjustments to the comments.

source/blender/blenkernel/BKE_image.h Outdated

						
				@ -499,2 +513,4 @@

				 * `iuser` and `ibuf` are mutual exclusive parameters. The caller can pass the `ibuf` when already

				 * available. It is also required when requesting the #GPUTexture for a render result.

				 *

				 * Call BKE_image_ensure_gpu_texture first if the requested layer, pass, and view might differ

Jeroen Bakker commented

2023-03-13 20:49:02 +01:00

This is more the exception. First we should try to fix it in the RNA/Operator. If this isn't possible we can use this function.

OmarEmaraDev marked this conversation as resolved

source/blender/blenkernel/intern/image_gpu.cc Outdated

						
				@ -365,6 +379,9 @@ static GPUTexture *image_get_gpu_texture(Image *ima,

				    ima->gpu_pass = requested_pass;

				    ima->gpu_layer = requested_layer;

				    ima->gpu_view = requested_view;

				    /* The cache should be inavlidated here, but it is inmtentionally isn't due to possible

Jeroen Bakker commented

2023-03-13 20:45:57 +01:00

Small typo. A life saver could be to use "Code Spell Checker" when using VSCode.

OmarEmaraDev marked this conversation as resolved

Omar Emara reviewed 2023-03-13 21:29:19 +01:00

source/blender/blenkernel/BKE_image.h Outdated

						
				@ -499,2 +513,4 @@

				 * `iuser` and `ibuf` are mutual exclusive parameters. The caller can pass the `ibuf` when already

				 * available. It is also required when requesting the #GPUTexture for a render result.

				 *

				 * Call BKE_image_ensure_gpu_texture first if the requested layer, pass, and view might differ

Omar Emara commented

2023-03-13 21:29:10 +01:00

Can you clarify? Why operators are we talking about?

OmarEmaraDev marked this conversation as resolved

Jeroen Bakker reviewed 2023-03-14 07:59:58 +01:00

source/blender/blenkernel/BKE_image.h Outdated

						
				@ -499,2 +513,4 @@

				 * `iuser` and `ibuf` are mutual exclusive parameters. The caller can pass the `ibuf` when already

				 * available. It is also required when requesting the #GPUTexture for a render result.

				 *

				 * Call BKE_image_ensure_gpu_texture first if the requested layer, pass, and view might differ

Jeroen Bakker commented

2023-03-14 07:59:43 +01:00

Any operator that alters an image is responsible to invalidate the area of the image that is effected. For example reload, resize, flip etc. Same for any UI change done via the RNA layer. Change type, change render pass etc.

Only mentioning BKE_image_ensure_gpu_texture without describing the desired implementation will perhaps point developers in to use the workaround in places where it isn't required.

The issue we are solving is temporary and should eventually be supported differently. Viewport compositor should be able to construct a multipass image. and don't rely on the cached gpu texture. This is because the life-time cannot be guaranteed for the whole draw pipeline. You might want to combine multiple renderpasses of the same image. This is out of scope for this fix and release.

Does this make sense?

Any operator that alters an image is responsible to invalidate the area of the image that is effected. For example reload, resize, flip etc. Same for any UI change done via the RNA layer. Change type, change render pass etc. Only mentioning `BKE_image_ensure_gpu_texture` without describing the desired implementation will perhaps point developers in to use the workaround in places where it isn't required. The issue we are solving is temporary and should eventually be supported differently. Viewport compositor should be able to construct a multipass image. and don't rely on the cached gpu texture. This is because the life-time cannot be guaranteed for the whole draw pipeline. You might want to combine multiple renderpasses of the same image. This is out of scope for this fix and release. Does this make sense?

OmarEmaraDev marked this conversation as resolved

Omar Emara added 1 commit 2023-03-21 12:49:46 +01:00

67f733af0c Update documentation and fix spelling

Omar Emara reviewed 2023-03-21 12:51:22 +01:00

source/blender/blenkernel/BKE_image.h

						
				@ -499,2 +513,4 @@

				 * `iuser` and `ibuf` are mutual exclusive parameters. The caller can pass the `ibuf` when already

				 * available. It is also required when requesting the #GPUTexture for a render result.

				 *

				 * The requested GPU texture will be cached for subsequent calls, but only a single layer, pass,

Omar Emara commented

2023-03-21 12:50:27 +01:00

Yes, makes sense. I have updated the description to describe that.

OmarEmaraDev marked this conversation as resolved

Jeroen Bakker approved these changes 2023-03-21 13:57:44 +01:00

Jeroen Bakker added this to the 3.6 LTS milestone 2023-03-21 13:58:23 +01:00

Jeroen Bakker added this to the EEVEE & Viewport project 2023-03-21 13:58:27 +01:00

Omar Emara merged commit 04d128f545 into main

2023-03-21 14:57:24 +01:00

Omar Emara deleted branch fix-no-update-for-multi-pass-gpu-textures

2023-03-21 14:57:25 +01:00

Omar Emara referenced this issue from a commit

2023-03-21 14:57:25 +01:00

Fix: Realtime compositor outputs wrong EXR passes

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix: No image GPU texture update for different requests #105547