Fix #114414: Alternative fix for greyscale textures retaining perf #114611

Jason Fielder · 2023-11-08T01:54:01+01:00

Jason Fielder commented

2023-11-08 01:54:01 +01:00

Alternative solution to #114414 which reduces the scope of
textures for which single-channel greyscale optimization
is removed from.

Some byte buffers are converted to float buffers during
color management. These cases should retain support in
the optimal path.

Authored by Apple: Michael Parkin-White

Alternative solution to #114414 which reduces the scope of textures for which single-channel greyscale optimization is removed from. Some byte buffers are converted to float buffers during color management. These cases should retain support in the optimal path. Authored by Apple: Michael Parkin-White

Jason Fielder added 1 commit 2023-11-08 01:54:15 +01:00

Fix #114414 : Alternative fix for greyscale textures retaining perf 7b96eb7765

Alternative solution to #114414 which reduces the scope of
textures for which single-channel greyscale optimization
is removed from.

Some byte buffers are converted to float buffers during
color management. These cases should retain support in
the optimal path.

Authored by Apple: Michael Parkin-White

Jason Fielder requested review from Jeroen Bakker 2023-11-08 01:54:22 +01:00

Michael Parkin-White commented

2023-11-08 01:58:43 +01:00

First-time contributor

For a few technical notes, in the process of implementing a more comprehensive solution which allowed single channel textures to be updated from multi-component source buffers, I noticed that in quite a few of the common cases, the colormanagement still allocates a float buffer for a given ImBuf during data preparation.

In this case, these paths could still take the optimal packed channel path, so long as the source eGPUDataType was overriden to float if the source buffer was converted during imb_gpu_get_data(..).

Loosening the check as follows to byte buffers with given colorspaces enables the optimized single channel GPUTexture to be used in most cases, while also resolving: #114414

Note that I have only been able to test this in the Metal backend. I have also not been able to test every source format, but the Wanderer appears to work, and the logic should limit this to the cases where the float buffer is later allocated and used.

For a few technical notes, in the process of implementing a more comprehensive solution which allowed single channel textures to be updated from multi-component source buffers, I noticed that in quite a few of the common cases, the colormanagement still allocates a float buffer for a given ImBuf during data preparation. In this case, these paths could still take the optimal packed channel path, so long as the source eGPUDataType was overriden to float if the source buffer was converted during `imb_gpu_get_data(..)`. Loosening the check as follows to byte buffers with given colorspaces enables the optimized single channel GPUTexture to be used in most cases, while also resolving: https://projects.blender.org/blender/blender/issues/114414 Note that I have only been able to test this in the Metal backend. I have also not been able to test every source format, but the Wanderer appears to work, and the logic should limit this to the cases where the float buffer is later allocated and used.

Michael Parkin-White referenced this pull request

2023-11-08 17:31:34 +01:00

Blender 4.0 Regression: 10% EEVEE Performance drop from #114441 #114590

Jeroen Bakker requested changes 2023-11-09 09:15:09 +01:00

Jeroen Bakker left a comment

I agree with the solution, just need to cleanup a parameter as it is always overwritten.

source/blender/imbuf/intern/util_gpu.cc

						
				@ -121,2 +131,3 @@

				                              const bool store_premultiplied,

				                              bool *r_freedata)

				                              bool *r_freedata,

				                              eGPUDataFormat *out_data_format)

Jeroen Bakker commented

2023-11-09 09:08:42 +01:00

This makes two ways to determine the data_format.
one by imb_gpu_get_format which could be overwritten by imb_gpu_get_data.

Or the correct data_format should be returned by imb_gpu_get_format, or the r_data_format parameter should be removed from that function.

I would prefer the removal of the r_data_format

This makes two ways to determine the data_format. one by `imb_gpu_get_format` which could be overwritten by `imb_gpu_get_data`. Or the correct data_format should be returned by imb_gpu_get_format, or the r_data_format parameter should be removed from that function. I would prefer the removal of the `r_data_format`

Michael Parkin-White commented

2023-11-09 12:02:00 +01:00

First-time contributor

Yeah, this is a little non-ideal as it stands.
However, part of what makes this solution work to retain the 4ch->1ch compression is that in imb_gpu_get_data, even if the source data is a byte_buffer, some of the colour conversions appear to generate floating point data, in which case, one of the original reasons for the bug was that the wrong data type was being generated.

It may still be valid to have the data type return in the original imb_gpu_get_format, as I believe this is used in certain cases without the data upload. Although the override in imb_gpu_get_data allows for the case where the data is flipped to being floating point, to ensure the correct flag is passed to GPU_texture_update(..).

Although it will likely make sense to refactor both paths to use the same thing, or defer data format determination to a point where the source data type is known.

But yes, it'll likely make the most sense to remove r_data_format from imb_gpu_get_format, and leave that to only being the source GPU tex format, to allow for the destination texture format and source data format to allow different types.

Yeah, this is a little non-ideal as it stands. However, part of what makes this solution work to retain the 4ch->1ch compression is that in `imb_gpu_get_data`, even if the source data is a byte_buffer, some of the colour conversions appear to generate floating point data, in which case, one of the original reasons for the bug was that the wrong data type was being generated. It may still be valid to have the data type return in the original `imb_gpu_get_format`, as I believe this is used in certain cases without the data upload. Although the override in `imb_gpu_get_data` allows for the case where the data is flipped to being floating point, to ensure the correct flag is passed to `GPU_texture_update(..)`. Although it will likely make sense to refactor both paths to use the same thing, or defer data format determination to a point where the source data type is known. But yes, it'll likely make the most sense to remove r_data_format from `imb_gpu_get_format`, and leave that to only being the source GPU tex format, to allow for the destination texture format and source data format to allow different types.