blender
/
blender
136
407
Fork 591
Code Issues 6.3k Pull Requests 775 Projects 19 Wiki Activity

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
pull from: OmarEmaraDev/blender:refactor-imbuf-byte-to-float
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 7 Commits 8 Files Changed 3 +34 -103
3 changed files with 34 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
source/blender/imbuf/IMB_imbuf.hh
View File

@ -453,9 +453,7 @@ void IMB_buffer_byte_from_float_mask(unsigned char *rect_to,
*/
void IMB_buffer_float_from_byte(float *rect_to,
const unsigned char *rect_from,
int profile_to,
int profile_from,
bool predivide,
int channels,
int width,
int height,
int stride_to,

42
source/blender/imbuf/intern/colormanagement.cc
View File

@ -1651,15 +1651,8 @@ static void *do_display_buffer_apply_thread(void *handle_v)
}
if (display_buffer) {
IMB_buffer_float_from_byte(display_buffer,
handle->byte_buffer,
IB_PROFILE_SRGB,
IB_PROFILE_SRGB,
false,
width,
height,
width,
width);
IMB_buffer_float_from_byte(
display_buffer, handle->byte_buffer, channels, width, height, width, width);
}
OmarEmaraDev marked this conversation as resolved
Sergey Sharybin commented 2024-03-05 15:39:05 +01:00
Review
Copy Link

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.
}
else {
@ -1903,15 +1896,7 @@ static void *do_processor_transform_thread(void *handle_v)
const bool float_from_byte = handle->float_from_byte;
if (float_from_byte) {
IMB_buffer_float_from_byte(float_buffer,
byte_buffer,
IB_PROFILE_SRGB,
IB_PROFILE_SRGB,
false,
width,
height,
width,
width);
IMB_buffer_float_from_byte(float_buffer, byte_buffer, channels, width, height, width, width);
IMB_colormanagement_processor_apply(
handle->cm_processor, float_buffer, width, height, channels, predivide);
IMB_premultiply_rect_float(float_buffer, 4, width, height);
@ -2063,15 +2048,7 @@ void IMB_colormanagement_transform_from_byte(float *float_buffer,
const char *from_colorspace,
const char *to_colorspace)
{
IMB_buffer_float_from_byte(float_buffer,
byte_buffer,
IB_PROFILE_SRGB,
IB_PROFILE_SRGB,
true,
width,
height,
width,
width);
IMB_buffer_float_from_byte(float_buffer, byte_buffer, channels, width, height, width, width);
IMB_colormanagement_transform(
float_buffer, width, height, channels, from_colorspace, to_colorspace, true);
}
@ -3618,15 +3595,8 @@ static void partial_buffer_update_rect(ImBuf *ibuf,
else {
if (display_buffer_float) {
/* huh, for dither we need float buffer first, no cheaper way. currently */
IMB_buffer_float_from_byte(display_buffer_float,
byte_buffer,
IB_PROFILE_SRGB,
IB_PROFILE_SRGB,
true,
width,
height,
width,
display_stride);
IMB_buffer_float_from_byte(
display_buffer_float, byte_buffer, channels, width, height, width, display_stride);
}
else {
int i;

91
source/blender/imbuf/intern/divers.cc
View File

@ -349,58 +349,19 @@ void IMB_buffer_byte_from_float_mask(uchar *rect_to,
void IMB_buffer_float_from_byte(float *rect_to,
const uchar *rect_from,
int profile_to,
int profile_from,
bool predivide,
int channels,
int width,
int height,
int stride_to,
int stride_from)
{
float tmp[4];
int x, y;
/* we need valid profiles */
BLI_assert(profile_to != IB_PROFILE_NONE);
BLI_assert(profile_from != IB_PROFILE_NONE);
/* RGBA input */
for (y = 0; y < height; y++) {
const uchar *from = rect_from + size_t(stride_from) * y * 4;
float *to = rect_to + size_t(stride_to) * y * 4;
if (profile_to == profile_from) {
/* no color space conversion */
for (x = 0; x < width; x++, from += 4, to += 4) {
rgba_uchar_to_float(to, from);
}
}
else if (profile_to == IB_PROFILE_LINEAR_RGB) {
/* convert sRGB to linear */
if (predivide) {
for (x = 0; x < width; x++, from += 4, to += 4) {
srgb_to_linearrgb_uchar4_predivide(to, from);
}
}
else {
for (x = 0; x < width; x++, from += 4, to += 4) {
srgb_to_linearrgb_uchar4(to, from);
}
}
}
else if (profile_to == IB_PROFILE_SRGB) {
/* convert linear to sRGB */
if (predivide) {
for (x = 0; x < width; x++, from += 4, to += 4) {
rgba_uchar_to_float(tmp, from);
linearrgb_to_srgb_predivide_v4(to, tmp);
}
}
else {
for (x = 0; x < width; x++, from += 4, to += 4) {
rgba_uchar_to_float(tmp, from);
linearrgb_to_srgb_v4(to, tmp);
}
for (int y = 0; y < height; y++) {
for (int x = 0; x < width; x++) {
for (int i = 0; i < channels; i++) {
/* Bytes buffers are always 4 channel in image buffers. */
const size_t from_index = (y * stride_from + x) * 4 + i;
const size_t to_index = (y * stride_to + x) * channels + i;
rect_to[to_index] = float(rect_from[from_index]) / 255.0f;
}
}
}
@ -755,7 +716,7 @@ void IMB_float_from_rect_ex(ImBuf *dst, const ImBuf *src, const rcti *region_to_
"Source buffer should have a byte buffer assigned.");
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 > 0, "Destination buffer should have at least one channel.");
OmarEmaraDev marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-03-14 15:14:46 +01:00
Outdated
Review
Copy Link

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.
BLI_assert_msg(region_to_update->xmin >= 0,
"Region to update should be clipped to the given buffers.");
BLI_assert_msg(region_to_update->ymin >= 0,
@ -766,29 +727,22 @@ void IMB_float_from_rect_ex(ImBuf *dst, const ImBuf *src, const rcti *region_to_
"Region to update should be clipped to the given buffers.");
float *rect_float = dst->float_buffer.data;
rect_float += (region_to_update->xmin + region_to_update->ymin * dst->x) * 4;
rect_float += (region_to_update->xmin + region_to_update->ymin * dst->x) * dst->channels;
uchar *rect = src->byte_buffer.data;
rect += (region_to_update->xmin + region_to_update->ymin * dst->x) * 4;
rect += (region_to_update->xmin + region_to_update->ymin * dst->x) * dst->channels;
const int region_width = BLI_rcti_size_x(region_to_update);
const int region_height = BLI_rcti_size_y(region_to_update);
/* Convert byte buffer to float buffer without color or alpha conversion. */
IMB_buffer_float_from_byte(rect_float,
rect,
IB_PROFILE_SRGB,
IB_PROFILE_SRGB,
false,
region_width,
region_height,
src->x,
dst->x);
IMB_buffer_float_from_byte(
rect_float, rect, dst->channels, region_width, region_height, src->x, dst->x);
/* Perform color space conversion from rect color space to linear. */
float *float_ptr = rect_float;
for (int i = 0; i < region_height; i++) {
IMB_colormanagement_colorspace_to_scene_linear(
float_ptr, region_width, 1, dst->channels, src->byte_buffer.colorspace, false);
float_ptr += 4 * dst->x;
float_ptr += dst->channels * dst->x;
}
/* Perform alpha conversion. */
@ -796,7 +750,7 @@ void IMB_float_from_rect_ex(ImBuf *dst, const ImBuf *src, const rcti *region_to_
float_ptr = rect_float;
for (int i = 0; i < region_height; i++) {
IMB_premultiply_rect_float(float_ptr, dst->channels, region_width, 1);
float_ptr += 4 * dst->x;
float_ptr += dst->channels * dst->x;
}
}
}
@ -814,15 +768,24 @@ void IMB_float_from_rect(ImBuf *ibuf)
*/
float *rect_float = ibuf->float_buffer.data;
if (rect_float == nullptr) {
const size_t size = IMB_get_rect_len(ibuf) * sizeof(float[4]);
/* For color spaces other than sRGB and linear, we force full RGBA channels since color space
* conversion might introduce color shifts even for greyscale images. */
if (!(IMB_colormanagement_space_is_srgb(ibuf->byte_buffer.colorspace) ||
IMB_colormanagement_space_is_scene_linear(ibuf->byte_buffer.colorspace)))
{
OmarEmaraDev marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-03-05 15:40:05 +01:00
Outdated
Review
Copy Link

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.
ibuf->channels = 4;
}
else {
OmarEmaraDev marked this conversation as resolved Outdated
Sergey Sharybin commented 2024-03-14 15:17:50 +01:00
Outdated
Review
Copy Link

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?
Omar Emara commented 2024-03-15 07:40:24 +01:00
Outdated
Review
Copy Link

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.
ibuf->channels = ibuf->planes / 8;
}
const size_t size = IMB_get_rect_len(ibuf) * ibuf->channels * sizeof(float);
rect_float = static_cast<float *>(MEM_callocN(size, "IMB_float_from_rect"));
if (rect_float == nullptr) {
return;
}
ibuf->channels = 4;
IMB_assign_float_buffer(ibuf, rect_float, IB_TAKE_OWNERSHIP);
}