WIP: IMB: Allow BW images in byte to float conversion #118622
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#118622
Loading…
Reference in New Issue
No description provided.
Delete Branch "OmarEmaraDev/blender:refactor-imbuf-byte-to-float"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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.
@Sergey The actual problem is #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?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
andIMB_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
).@ -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.
@ -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.
IMB_buffer_float_from_byte
looks so much cleaner now!I am still a bit confused about
IMB_buffer_float_from_byte
andIMB_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, thedst
is the image with float buffer. TheimBuf::channels
as per comment is valid for float buffers. So, since thesrc
is byte-only, I can not say what it'll have in itschannels
.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.@ -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 theelse
branch here? Otherwise it could left uninitialized, or initialized to some default value which is not necessarily corresponding to the desired value?Yah, sorry. Was remnant from the time where I though channels was shared between both byte and float buffers.
@blender-bot build
Unfortunately, it turns out
IMB_colormanagement_colorspace_to_scene_linear()
used byIMB_float_from_rect_ex()
does not work with channels below 3. This is because it usesPackedImageDesc
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.@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
inIMB_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 likeOCIO_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.IMB: Allow BW images in byte to float conversionto WIP: IMB: Allow BW images in byte to float conversionCheckout
From your project repository, check out a new branch and test the changes.