Fix #126108: Crash when EXR image is loaded in image list #126234

Sergey Sharybin · 2024-08-12T16:21:51+02:00

Sergey Sharybin commented

2024-08-12 16:21:51 +02:00

Technically the regression was caused by #124472 which made it so
duplicating ImBuf allocates the exact amount of memory needed to
hold the pixels, while before IMB_dupImBuf() would leave the float
buffer over-allocated for images that are less than 4 channels per
pixel.

At the same time IMB_scalefastImBuf() was hard-coded to use 4
channels per pixels, for both byte and float buffers. It did not
crash in Blender 4.1 as it was accessing memory that is over-allocated,
but it also did not generate proper preview.

This fix makes the IMB_scalefastImBuf() to operate on an arbitrary
number of channels in the float buffer.

Technically the regression was caused by #124472 which made it so duplicating ImBuf allocates the exact amount of memory needed to hold the pixels, while before IMB_dupImBuf() would leave the float buffer over-allocated for images that are less than 4 channels per pixel. At the same time IMB_scalefastImBuf() was hard-coded to use 4 channels per pixels, for both byte and float buffers. It did not crash in Blender 4.1 as it was accessing memory that is over-allocated, but it also did not generate proper preview. This fix makes the IMB_scalefastImBuf() to operate on an arbitrary number of channels in the float buffer.

Sergey Sharybin added 1 commit 2024-08-12 16:21:59 +02:00

Fix #126108 : Crash when EXR image is loaded in image list

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

86f12008c4

Technically the regression was caused by #124472 which made it so
duplicating ImBuf allocates the exact amount of memory needed to
hold the pixels, while before IMB_dupImBuf() would leave the float
buffer over-allocated for images that are less than 4 channels per
pixel.

At the same time IMB_scalefastImBuf() was hard-coded to use 4
channels per pixels, for both byte and float buffers. It did not
crash in Blender 4.1 as it was accessing memory that is over-allocated,
but it also did not generate proper preview.

This fix makes the IMB_scalefastImBuf() to operate on an arbitrary
number of channels in the float buffer.

Sergey Sharybin commented

2024-08-12 16:22:27 +02:00

@blender-bot build

Sergey Sharybin requested review from Jesse Yurkovich 2024-08-12 16:22:36 +02:00

Jesse Yurkovich commented

2024-08-12 18:41:37 +02:00

This fixes the crash for me as well (and build bot failures are not your fault). That said, there's 2 minor issues I have for this code in general:

I don't fully understand the use of the 65536 magic number (and related right shifts by 16). Can this be made less obtuse?
For any images with an aspect ratio > 32:1, it will technically generate a div-by-0 for the stepx and stepy calculations. Though things happen to still work...

Not sure how far you want to go in this PR to address the initial crash though. Maybe a followup is best.

This fixes the crash for me as well (and build bot failures are not your fault). That said, there's 2 minor issues I have for this code in general: - I don't fully understand the use of the 65536 magic number (and related right shifts by 16). Can this be made less obtuse? - For any images with an aspect ratio > 32:1, it will technically generate a div-by-0 for the `stepx` and `stepy` calculations. Though things happen to still work... Not sure how far you want to go in this PR to address the initial crash though. Maybe a followup is best.

Sergey Sharybin commented

2024-08-13 15:50:23 +02:00

@deadpin That is a great points you're raising. The magic numbers and such is indeed unclear, and we share the same thoughts. The devision by zero I didn't notice before, nice catch!

I'd like this to go to 4.2.1 scheduled for Thursday, so I wanted to avoid deeper changes, and have the change as safe as possible.
So from my perspective I think doing a followup on top of a fix works better. Unless you have stronger opinion.

The thing I am not sure about is whether doing a followup is something I can do in the near future. But maybe it would be something interesting for @aras_p to help with?

@deadpin That is a great points you're raising. The magic numbers and such is indeed unclear, and we share the same thoughts. The devision by zero I didn't notice before, nice catch! I'd like this to go to 4.2.1 scheduled for Thursday, so I wanted to avoid deeper changes, and have the change as safe as possible. So from my perspective I think doing a followup on top of a fix works better. Unless you have stronger opinion. The thing I am not sure about is whether doing a followup is something I can do in the near future. But maybe it would be something interesting for @aras_p to help with?

Aras Pranckevicius commented

2024-08-13 16:35:24 +02:00

@Sergey maybe! Any bets on whether IMB_scalefastImBuf is actually fast? :P

@Sergey maybe! Any bets on whether `IMB_scalefastImBuf` is actually fast? :P

Jesse Yurkovich approved these changes 2024-08-13 18:24:42 +02:00

Jesse Yurkovich left a comment

Approving since the primary bug is fixed. The cosmetic changes to make the code more clear can definitely come later.

Sergey Sharybin commented

2024-08-14 11:34:06 +02:00

Thanks for the review and inputs. I'll land the PR now and backport for 4.2.1.

Sergey Sharybin merged commit 1fc6a5b9bd into main

2024-08-14 11:34:47 +02:00

Sergey Sharybin deleted branch fix_126108

2024-08-14 11:34:51 +02:00

Sergey Sharybin referenced this issue from a commit

2024-08-14 11:34:53 +02:00

Fix #126108: Crash when EXR image is loaded in image list

Sergey Sharybin referenced this issue from a commit

2024-08-14 11:51:33 +02:00

Fix #126108: Crash when EXR image is loaded in image list

Jeroen Bakker referenced this issue from a commit

2024-08-15 14:44:50 +02:00

Fix #126108: Crash when EXR image is loaded in image list

Aras Pranckevicius referenced this pull request

2024-08-16 07:15:57 +02:00

IMB: Speedups, fixes and cleanups to various image scaling functions #126390

Aras Pranckevicius commented

2024-08-18 11:58:22 +02:00

FYI the fix caused a slight performance regression in using IMB_scalefastImBuf on float images, due to more complex per-pixel work with the channels loop. I'm fixing it as part of #126390 rabbit hole (performance test I've added there on my machine was taking 230ms before this fix, 300ms with this fix, and back to ~220ms in #126390)

FYI the fix caused a slight performance regression in using `IMB_scalefastImBuf` on float images, due to more complex per-pixel work with the channels loop. I'm fixing it as part of #126390 rabbit hole (performance test I've added there on my machine was taking 230ms before this fix, 300ms with this fix, and back to ~220ms in #126390)

Aras Pranckevicius referenced this issue from a commit

2024-08-19 16:50:52 +02:00

IMB: Speedups, fixes and cleanups to various image scaling functions

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #126108: Crash when EXR image is loaded in image list #126234