Page MenuHome

Fix T80859: Thread safe pixeldata concersion from float to uchar and v.v.
ClosedPublic

Authored by Martijn Versteegh (Baardaap) on Sep 17 2020, 10:24 PM.

Details

Summary

I created a bugreport T80859 earlier.

The problem is in the file source/blender/editors/sculpt_paint/paint_image_proj.c (line numbers against commit d376aea61)

at the lines 5309 and 5320 the functions IMB_rect_from_float(...) and IMB_float_from_rect(...) are called from threaded code on a shared ibuf.
However, ps->reproject_ibuf is shared between threads, so this is not thread-safe.
This is a race condition and leads to wrong output when projecting float data onto a uchar texture or vice versa on a multithreaded system.
The checks on line 5308 and 5319 are already a race condition.

I created this patch which fixes the problem , but I'm not sure if I'm overlooking something.
This makes sure reproject_ibuf contains the correct formats *before* the threadpool is started.

I replaced the original location of the conversions with BLI_asserts(). That may be overkill?

Diff Detail

Repository
rB Blender

Event Timeline

Martijn Versteegh (Baardaap) requested review of this revision.Sep 17 2020, 10:24 PM
Martijn Versteegh (Baardaap) created this revision.
Martijn Versteegh (Baardaap) retitled this revision from Thread safe pixeldata concersion from float to uchar and v.v. to Thread safe pixeldata concersion from float to uchar and v.v., fixes T80859.Sep 17 2020, 10:58 PM
Martijn Versteegh (Baardaap) edited the summary of this revision. (Show Details)
Martijn Versteegh (Baardaap) set the repository for this revision to rB Blender.

To help people reviewing and prevent them from needing to closely study the code to get a mental model of what happens I'll write a short outline of the code path here.

All functions reside in the file source/blender/editors/sculpt_paint/paint_image_proj.c

When projecting an image from the camera onto the mesh

texture_paint_camera_project_exec(...)

is called. This does some setup and builds up a PaintProjectState object (ps) , attaching (among other stuff) the reproject_ibuf (the source image) and the projImages array (the destination images). It then calls

project_paint_op(...)

this sets up a task_pool, does some more setup and fires up the taskpool with the function
do_projectpaint_thread(...)
as threadfunction.
Inside do_projectpaint_thread(...) in the branch where ps->source is not PROJ_SRC_VIEW and not PROJ_SRC_VIEW_FILL it does a check if the ps->reproject_ibuf and ps->projImages[currentlyactiveimage] both have a rect (uchar data) or a rect_float (float data) and if not create the missing data in the ps->reproj_ibuf. But since this is run in a multithreaded context, while the ps->reproj_ibuf is a shared object, will lead to a race condition, both in the check and in the generating of the missing ibuf.rect(_float) .

Now that I've written this down I think this will go wrong any time you do projection painting with float textures on a material containing uchar textures or vice versa. So probably also if you use a float texture brush on a material with uchar textures and v.v.

I hope this writeup makes it easier to understand what I'm trying to do with my patch. And also easier for someone who actually knows the code to maybe create a better patch,

source/blender/editors/sculpt_paint/paint_image_proj.c
5574

I'm not sure about this condition. I derived it from the code path in do_projectpaint_thread, but I'm not completely sure it covers all cases.

Germano Cavalcante (mano-wii) retitled this revision from Thread safe pixeldata concersion from float to uchar and v.v., fixes T80859 to Fix T80859: Thread safe pixeldata concersion from float to uchar and v.v..Sep 21 2020, 7:50 PM
Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)

Patch LGTM, besides nit-picky style comments below.

source/blender/editors/sculpt_paint/paint_image_proj.c
5308

Better be explicit with NULL comparison (ps->reproject_ibuf->rect_float != NULL).

5325

Same as above

5574

Can use instead if (!ELEM(ps->source, PROJ_SRC_VIEW, PROJ_SRC_VIEW_FILL))

5575

Comments should be full sentences, including caps and dots. ;)

5580

again explicit NULL checks

Martijn Versteegh (Baardaap) marked 5 inline comments as done.

Worked in mont29's feedback.

source/blender/editors/sculpt_paint/paint_image_proj.c
5574

While this condition is more succinct, I'm still unsure if it covers all needed cases....

Martijn Versteegh (Baardaap) marked an inline comment as not done.Sep 22 2020, 10:00 AM
Bastien Montagne (mont29) added inline comments.
source/blender/editors/sculpt_paint/paint_image_proj.c
5574

I checked code path as you did, and looks valid to me as well. the asserts should cover potential missed case anyway.

This revision is now accepted and ready to land.Sep 22 2020, 10:25 AM