WIP: IMB: Allow BW images in byte to float conversion #118622

Draft
Omar Emara wants to merge 8 commits from OmarEmaraDev/blender:refactor-imbuf-byte-to-float into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

This patch refactors the IMB_float_from_rect function to work with any
number of channels, which is useful for RGB and BW images. This
consequently also refactors the IMB_float_from_rect_ex, and
IMB_buffer_float_from_byte function to allow any number of channels.

This patch refactors the IMB_float_from_rect function to work with any number of channels, which is useful for RGB and BW images. This consequently also refactors the IMB_float_from_rect_ex, and IMB_buffer_float_from_byte function to allow any number of channels.
Omar Emara added the
Interest
Compositing
Module
Core
labels 2024-02-22 15:44:30 +01:00
Omar Emara added 1 commit 2024-02-22 15:44:40 +01:00
cd07fe75d1 IMB: Allow BW images in byte to float conversion
This patch refactors the IMB_float_from_rect function to work with any
number of channels, which is useful for RGB and BW images. This
consequently also refactors the IMB_float_from_rect_ex, and
IMB_buffer_float_from_byte function to allow any number of channels.
Omar Emara requested review from Sergey Sharybin 2024-02-22 15:44:54 +01:00

The IMB_float_from_rect is kind of a legacy function, and it is hard to justify correctness of the change about it.

The thing is, the function roots to the times prior to the more programmable OpenColorIO color management in Blender, and has a number of limitations in there. Basically, it is hardcoded to either sRGB or linear rec709 color spaces, which is not covering all cases we have. One possible improvement in clarity would be to remove the profile from the arguments, and make it a data-type-only conversion operation.

For the BW and byte<->float conversion the story is more tricky. If the color space changes (and it will typically change, because byte buffers are usually in display space, and float buffers are in linear space) you can't really guarantee that for BW input the output will be also BW, as the display-alike<->linear space conversion might introduce shift in HUE.

What is the problem you're working on solving? It would be very useful to know to see what the proper solution would be.

The `IMB_float_from_rect` is kind of a legacy function, and it is hard to justify correctness of the change about it. The thing is, the function roots to the times prior to the more programmable OpenColorIO color management in Blender, and has a number of limitations in there. Basically, it is hardcoded to either sRGB or linear rec709 color spaces, which is not covering all cases we have. One possible improvement in clarity would be to remove the profile from the arguments, and make it a data-type-only conversion operation. For the BW and byte<->float conversion the story is more tricky. If the color space changes (and it will typically change, because byte buffers are usually in display space, and float buffers are in linear space) you can't really guarantee that for BW input the output will be also BW, as the display-alike<->linear space conversion might introduce shift in HUE. What is the problem you're working on solving? It would be very useful to know to see what the proper solution would be.
Author
Member

@Sergey The actual problem is #118624, where this function is used.

@Sergey The actual problem is https://projects.blender.org/blender/blender/pulls/118624, where this function is used.

I see.

The Eevee and viewport rendering uses sime grayscale optimization, and there a more tricky check is performed: imb_is_grayscale_texture_format_compatible(). Maybe those code paths/checks could be somehow unified?

I see. The Eevee and viewport rendering uses sime grayscale optimization, and there a more tricky check is performed: `imb_is_grayscale_texture_format_compatible()`. Maybe those code paths/checks could be somehow unified?
Sergey Sharybin requested changes 2024-03-05 15:44:11 +01:00
Sergey Sharybin left a comment
Owner

There are some inlined notes.

On a bigger scale: is not always when we can convert BW byte image to BW float image. It is possible with the standard linear and sRGB spaces, but it is not impossible that conversion from byte image's color space to linear introduces some color shift. For those cases the float buffer needs to have at least 3 channels.

There are some checks in the imb_is_grayscale_texture_format_compatible: IMB_colormanagement_space_is_srgb and IMB_colormanagement_space_is_scene_linear which could help here making decision on whether we can keep float buffer 1-channel.

There are some inlined notes. On a bigger scale: is not always when we can convert BW byte image to BW float image. It is possible with the standard linear and sRGB spaces, but it is not impossible that conversion from byte image's color space to linear introduces some color shift. For those cases the float buffer needs to have at least 3 channels. There are some checks in the `imb_is_grayscale_texture_format_compatible`: `IMB_colormanagement_space_is_srgb` and `IMB_colormanagement_space_is_scene_linear` which could help here making decision on whether we can keep float buffer 1-channel.
@ -143,6 +143,7 @@ void rgb_float_set_hue_float_offset(float rgb[3], float hue_offset);
*/
void rgb_byte_set_hue_float_offset(unsigned char rgb[3], float hue_offset);
float uchar_to_float(const unsigned char value);

Not sure if we need such function, is just a simple division, which can not be SIMD-optimized (in possible contrast with rgb_uchar_to_float).

Not sure if we need such function, is just a simple division, which can not be SIMD-optimized (in possible contrast with `rgb_uchar_to_float`).
OmarEmaraDev marked this conversation as resolved
@ -1656,6 +1656,7 @@ static void *do_display_buffer_apply_thread(void *handle_v)
IB_PROFILE_SRGB,

I think we should start removing this legacy profile. Will help simplifying this change as well, and I don't think we ever do color space conversion.

I think we should start removing this legacy profile. Will help simplifying this change as well, and I don't think we ever do color space conversion.
OmarEmaraDev marked this conversation as resolved
@ -756,3 +773,3 @@
BLI_assert_msg(dst->x == src->x, "Source and destination buffer should have the same dimension");
BLI_assert_msg(dst->y == src->y, "Source and destination buffer should have the same dimension");
BLI_assert_msg(dst->channels = 4, "Destination buffer should have 4 channels.");
BLI_assert_msg(dst->channels = src->channels,

Wow, surely this is not an issue with this patch, but we should not be assigning the number of channels as part of an assert expression.

Wow, surely this is not an issue with this patch, but we should not be assigning the number of channels as part of an assert expression.
OmarEmaraDev marked this conversation as resolved
Omar Emara added 2 commits 2024-03-08 09:58:13 +01:00
Omar Emara added 1 commit 2024-03-08 15:27:16 +01:00
Omar Emara added 1 commit 2024-03-08 15:29:46 +01:00
Omar Emara added 2 commits 2024-03-14 10:07:16 +01:00
Sergey Sharybin reviewed 2024-03-14 15:20:00 +01:00
Sergey Sharybin left a comment
Owner

IMB_buffer_float_from_byte looks so much cleaner now!

I am still a bit confused about IMB_buffer_float_from_byte and IMB_float_from_rect.
Not gonna lie, some legacy roots of the situation about ibuf->planes, ibuf->channels, and ibuf->byte_buffer does not make it easier to work with those conversions.

`IMB_buffer_float_from_byte` looks so much cleaner now! I am still a bit confused about `IMB_buffer_float_from_byte` and `IMB_float_from_rect`. Not gonna lie, some legacy roots of the situation about ibuf->planes, ibuf->channels, and ibuf->byte_buffer does not make it easier to work with those conversions.
@ -756,3 +717,3 @@
BLI_assert_msg(dst->x == src->x, "Source and destination buffer should have the same dimension");
BLI_assert_msg(dst->y == src->y, "Source and destination buffer should have the same dimension");
BLI_assert_msg(dst->channels = 4, "Destination buffer should have 4 channels.");
BLI_assert_msg(dst->channels == src->channels,

I am not sure this is semantically correct in this context. The src is the image with byte buffer, the dst is the image with float buffer. The imBuf::channels as per comment is valid for float buffers. So, since the src is byte-only, I can not say what it'll have in its channels.

Do we even need this assert now? The code does no longer assume 4 elements pixels stride. Can perhaps assert dst->channels >= 1 to catch uninitialized values.

I am not sure this is semantically correct in this context. The `src` is the image with byte buffer, the `dst` is the image with float buffer. The `imBuf::channels` as per comment is valid for float buffers. So, since the `src` is byte-only, I can not say what it'll have in its `channels`. Do we even need this assert now? The code does no longer assume 4 elements pixels stride. Can perhaps assert `dst->channels >= 1` to catch uninitialized values.
OmarEmaraDev marked this conversation as resolved
@ -818,0 +775,4 @@
IMB_colormanagement_space_is_scene_linear(ibuf->byte_buffer.colorspace)))
{
ibuf->channels = 4;
}

Shouldn't we initialize ibuf->channels based on the number of planes in the else branch here? Otherwise it could left uninitialized, or initialized to some default value which is not necessarily corresponding to the desired value?

Shouldn't we initialize `ibuf->channels` based on the number of planes in the `else` branch here? Otherwise it could left uninitialized, or initialized to some default value which is not necessarily corresponding to the desired value?
Author
Member

Yah, sorry. Was remnant from the time where I though channels was shared between both byte and float buffers.

Yah, sorry. Was remnant from the time where I though channels was shared between both byte and float buffers.
OmarEmaraDev marked this conversation as resolved
Omar Emara added 1 commit 2024-03-15 07:44:26 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
8788ef8ea2
Address review

@blender-bot build

@blender-bot build

Unfortunately, it turns out IMB_colormanagement_colorspace_to_scene_linear() used by IMB_float_from_rect_ex() does not work with channels below 3. This is because it uses PackedImageDesc for which valid number of channels is either 3 or 4.

On a semi-related topic: it seems excessive to allocate and release PackedImageDesc for every scanline.

Unfortunately, it turns out `IMB_colormanagement_colorspace_to_scene_linear()` used by `IMB_float_from_rect_ex()` does not work with channels below 3. This is because it uses `PackedImageDesc` for which valid number of channels is either 3 or 4. On a semi-related topic: it seems excessive to allocate and release `PackedImageDesc` for every scanline.
Author
Member

@Sergey If this patch is not necessary for #118624, I feel like we can just abandon it for now?

@Sergey If this patch is not necessary for #118624, I feel like we can just abandon it for now?

@OmarEmaraDev I wouldn't necessarily abandon it, it might still be useful to have. But i wouldn't necessarily prioritize it, and maybe mark it as WIP.

Thing is, it feels like we're close, and "just" need to replace IMB_colormanagement_colorspace_to_scene_linear in IMB_float_from_rect_ex with some custom loop, which will convert uchar4 to float4, apply color space conversion, and copy required channels the destination. It will also avoid some non-trivial things like OCIO_createOCIO_PackedImageDesc done per-scanline. However, what I am fearing is that after that we'll find out some other limitation or part of the PR which needs attention.

@OmarEmaraDev I wouldn't necessarily abandon it, it might still be useful to have. But i wouldn't necessarily prioritize it, and maybe mark it as WIP. Thing is, it feels like we're close, and "just" need to replace `IMB_colormanagement_colorspace_to_scene_linear` in `IMB_float_from_rect_ex` with some custom loop, which will convert uchar4 to float4, apply color space conversion, and copy required channels the destination. It will also avoid some non-trivial things like `OCIO_createOCIO_PackedImageDesc` done per-scanline. However, what I am fearing is that after that we'll find out some other limitation or part of the PR which needs attention.
Omar Emara changed title from IMB: Allow BW images in byte to float conversion to WIP: IMB: Allow BW images in byte to float conversion 2024-03-26 08:57:38 +01:00
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
This pull request is marked as a work in progress.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u refactor-imbuf-byte-to-float:OmarEmaraDev-refactor-imbuf-byte-to-float
git checkout OmarEmaraDev-refactor-imbuf-byte-to-float
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#118622
No description provided.