Fix #120175: File Output node has wrong BW output #120206

Merged

Omar Emara merged 2 commits from OmarEmaraDev/blender:fix-120175 into main

2024-04-04 08:52:09 +02:00

Omar Emara commented

2024-04-03 13:40:02 +02:00

Member

The File Output node produces wrong output when the output is set to BW.
This is because the image saving logic assumes float buffers always have
4 channels and ignores the image buffer channels member. So for single
channel images, the code weighted sum neighbouring pixels and asigned
them as a single pixel, producing bleeding at best and crashes at worst.

To fix this, we skip Color to BW conversion if the image is already BW.
And, we fix the functions that assume 4 channels for float buffers.

The File Output node produces wrong output when the output is set to BW. This is because the image saving logic assumes float buffers always have 4 channels and ignores the image buffer channels member. So for single channel images, the code weighted sum neighbouring pixels and asigned them as a single pixel, producing bleeding at best and crashes at worst. To fix this, we skip Color to BW conversion if the image is already BW. And, we fix the functions that assume 4 channels for float buffers.

Omar Emara added the

labels 2024-04-03 13:40:02 +02:00

Omar Emara requested review from Sergey Sharybin 2024-04-03 13:40:48 +02:00

Sergey Sharybin requested changes 2024-04-03 14:32:32 +02:00

Dismissed

Sergey Sharybin left a comment

Owner

This seems a bit strange, as the other usages of IMB_color_to_bw are not covered, and with this change you potentially leave byte buffer as color and not BW.

I think doing something like this in the IMB_color_to_bw is fine:

  if (rct_fl) {
    if (ibuf->channels >= 3) {
      for (i = IMB_get_rect_len(ibuf); i > 0; i--, rct_fl += ibuf->channels) {
        rct_fl[0] = rct_fl[1] = rct_fl[2] = IMB_colormanagement_get_luminance(rct_fl);
      }
    }
  }

The PR should also be against the main branch. See https://developer.blender.org/docs/handbook/release_process/release_branch/#after-a-release

This seems a bit strange, as the other usages of `IMB_color_to_bw` are not covered, and with this change you potentially leave byte buffer as color and not BW. I think doing something like this in the `IMB_color_to_bw` is fine: ``` if (rct_fl) { if (ibuf->channels >= 3) { for (i = IMB_get_rect_len(ibuf); i > 0; i--, rct_fl += ibuf->channels) { rct_fl[0] = rct_fl[1] = rct_fl[2] = IMB_colormanagement_get_luminance(rct_fl); } } } ``` The PR should also be against the main branch. See https://developer.blender.org/docs/handbook/release_process/release_branch/#after-a-release

Omar Emara commented

2024-04-03 14:43:55 +02:00

Author

Member

The only other IMB_color_to_bw usage is guaranteed to be a byte buffer, so it is not affected. As for the byte buffer, I just assumed float and byte buffers would be mutually exclusive as many parts of the code already do.

I wanted to update IMB_color_to_bw and IMB_saturation directly, but I thought it is best to leave that out of 4.1. But if you think this is best, I will update the patch. The other thing that we will be doing a redundant buffer duplication in most cases.

The only other `IMB_color_to_bw` usage is guaranteed to be a byte buffer, so it is not affected. As for the byte buffer, I just assumed float and byte buffers would be mutually exclusive as many parts of the code already do. I wanted to update `IMB_color_to_bw` and `IMB_saturation` directly, but I thought it is best to leave that out of 4.1. But if you think this is best, I will update the patch. The other thing that we will be doing a redundant buffer duplication in most cases.

OmarEmaraDev changed target branch from blender-v4.1-release to main

2024-04-03 14:52:14 +02:00

Omar Emara force-pushed fix-120175 from f64d9008f1 to 4489aa978c

2024-04-03 14:54:15 +02:00

Compare

Sergey Sharybin commented

2024-04-03 15:05:21 +02:00

Owner

Byte and float buffers are not mutually exclusive. Accessing data will choose the best suitable buffer, but pixel transforms are expected to keep both in sync. For example. in the IMB_color_to_bw you see if (rct_fl) { ... } if (rct) { ... }.

Maybe with the way how some other areas work it is fine to have check on a higher level, but it somehow does not feel fully reliable.

The corrective release is scheduled for 16 April, so it seems to be OK amount of time to possibly catch unexpected issues with a change in IMB_color_to_bw. The change in IMB_saturation we can limit to the main only.

P.S. If we want to be extra careful, we can follow the IMB_gpu_get_texture_format and use channels = image_buffer->channels == 0 ? 4 : image_buffer->channels.

Byte and float buffers are not mutually exclusive. Accessing data will choose the best suitable buffer, but pixel transforms are expected to keep both in sync. For example. in the `IMB_color_to_bw` you see `if (rct_fl) { ... } if (rct) { ... }`. Maybe with the way how some other areas work it is fine to have check on a higher level, but it somehow does not feel fully reliable. The corrective release is scheduled for 16 April, so it seems to be OK amount of time to possibly catch unexpected issues with a change in `IMB_color_to_bw`. The change in `IMB_saturation` we can limit to the main only. P.S. If we want to be extra careful, we can follow the `IMB_gpu_get_texture_format` and use `channels = image_buffer->channels == 0 ? 4 : image_buffer->channels`.

Omar Emara commented

2024-04-03 15:08:29 +02:00

Author

Member

@Sergey I added !(ibuf->float_buffer.data && !ibuf->byte_buffer.data && ibuf->channels == 1) to avoid redundant duplication, not sure if you would be pro adding it.

@Sergey I added `!(ibuf->float_buffer.data && !ibuf->byte_buffer.data && ibuf->channels == 1)` to avoid redundant duplication, not sure if you would be pro adding it.

Sergey Sharybin approved these changes 2024-04-03 15:44:59 +02:00

Sergey Sharybin left a comment

Owner

I think it is fine to have that check, as it avoids unnecessary ImBug duplications if the ImBuf is a single-channel already.

I think it is fine to have that check, as it avoids unnecessary ImBug duplications if the `ImBuf` is a single-channel already.

Omar Emara merged commit 0c91ad9008 into main

2024-04-04 08:52:09 +02:00

Omar Emara deleted branch fix-120175

2024-04-04 08:52:12 +02:00

Omar Emara referenced this issue from a commit

2024-04-04 08:52:14 +02:00

Fix #120175: File Output node has wrong BW output

Thomas Dinges referenced this issue from a commit

2024-04-09 16:11:47 +02:00

Fix #120175: File Output node has wrong BW output

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project (Legacy)

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

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#120206

No description provided.