Use ImBuf instead of a raw buffer for compositor previews #110064
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
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110064
Loading…
Reference in New Issue
No description provided.
Delete Branch "Kdaf/blender:compositor-node-preview-ImBuf"
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?
In order to better suit with the upcoming changes of shader node previews, this patch replaces the old implementation of the storage of the compositor previews. It also prevent memory loss for future modification.
@ -3136,3 +3130,1 @@
MEM_callocN(4 * xsize + xsize * ysize * sizeof(char[4]), "node preview rect"));
preview->xsize = xsize;
preview->ysize = ysize;
IMB_rect_size_set(preview->image, new uint[2]{(uint)xsize, (uint)ysize});
The
IMB_rect_size_set
calls are problematic (here and below). This will leak that 2-element array for starters, but it's also going to allocate the float buffer, if the size changes, which is unused in this scenario. Typically, if you know what the ImBuf is going to be up front (just byte, just float, or both), pass in theIB_*
flags to theIMB_allocImBuf
call above -- e.g. pass inIB_rect
in your case.If it's possible for this preview to change x,y dimensions after creation then I think it's sufficient to just call
IMB_initImBuf
with the new set of values. It will handle things from that point. Only call it if the size has changed though.I do not think they are problematic, because the float buffer is never initialised, and so the
float_buffer.data
is a nullptr, which will not be allocated. seerectop.cc:276
. What theIMB_rect_size_set
call does is just to check if the size given is the same as the size stored, and if it not the case, it will reallocate the data of the buffers already allocated.I dont think that using
IMB_initImBuf
can a good option, because it will initialise with 0 the ImBuf structure and thus change the data pointers without freeing the data.Ah, missed that memset to 0. Good spot! That leaves just the leak mentioned then.
What leak ? Could you explain me a bit more ?
Edit: I see now what is the problem, but I dont know what is the best practice in this case; creating the array right before ?
The
new uint[2]{(uint)xsize, (uint)ysize}
leaks. No one callsdelete
on that array.@ -1327,3 +1329,3 @@
/* Not a callback. */
static void node_draw_preview(bNodePreview *preview, rctf *prv)
static void node_draw_preview(Scene *sce, ImBuf *image, rctf *prv)
Maybe consider using the full word
preview
as the parameter/variable name here and elsewhere? It would negate the need to use the abbreviationpr_image
. In either case, ensure this parameter name is consistent with the others. Here justimage
is used but other functions usepr_image
.Colin Marmond referenced this pull request2023-07-14 11:28:13 +02:00
@ -3236,4 +3223,2 @@
} // namespace blender::bke
void BKE_node_preview_clear_tree(bNodeTree *ntree)
Looks like this function is removed? Don't forget to remove it from the header too
@ -3136,3 +3130,1 @@
MEM_callocN(4 * xsize + xsize * ysize * sizeof(char[4]), "node preview rect"));
preview->xsize = xsize;
preview->ysize = ysize;
uint size[2] = {(uint)xsize, (uint)ysize};
const uint size[2] = {uint(xsize), uint(ysize)};
@ -1327,3 +1329,3 @@
/* Not a callback. */
static void node_draw_preview(bNodePreview *preview, rctf *prv)
static void node_draw_preview(Scene *sce, ImBuf *preview, rctf *prv)
sce
->scene
Just a more standard variable name (many more uses) that's less ugly :)
Same below
@ -2084,3 +2082,3 @@
}
static void node_draw_extra_info_panel(TreeDrawContext &tree_draw_ctx,
static void node_draw_extra_info_panel(Scene *sce,
Can
Scene
beconst
? Might apply elsewhere in the file tooI am not exactly sure what the upcoming changes you are referring to. Is there a design task for them?
Is it to allow the GPU compositor to output
GPUTexture
and use that as a node preview?@ -516,3 +517,2 @@
unsigned char *rect;
short xsize, ysize;
struct ImBuf *image;
It would be much better to call it
ibuf
to not be confused with anImage
data-block.@Sergey I did not make a design task about it, but it is about the shader node previews #110065. It would be useful to draw previews as imbuf in the node_draw_preview function. The fact is that the render can deliver ImBuf directly instead of copying it to a buffer.
In this patch I want to prepare the ground for unified ImBuf drawing of previews.
@Kdaf Ah, I see. Thanks for clarifying.
There is one more thing in the code which is not really clear to me: the removal of the
BKE_node_preview_clear_tree
. It seems to cause more functional changes than just changing the way how pixels are stored and displayed on the screen.@Sergey
This function was only used for the material node previews which are not used (for now).
The cleaning of those shader node previews is no longer relevant (for years now).
Looks good to me now.
I had some outdated pending commits that I can't remove before submitting, will remove them after.
@ -53,3 +50,1 @@
preview_->xsize = get_width();
preview_->ysize = get_height();
preview_->rect = output_buffer_;
IMB_rect_size_set(output_image_, new uint[2]{get_width(), get_height()});
This
new
leaks memory, just define this variable on the stack instead.@ -96,10 +96,6 @@ static void rna_Material_update_previews(Main * /*bmain*/, Scene * /*scene*/, Po
{
Material *ma = (Material *)ptr->owner_id;
if (ma->nodetree) {
This is removed because material node previews are no longer stored like this. Which is ok, just noting for clarity.
I see. ideally the
BKE_node_preview_clear_tree
would be removed in a separate PR, so that there is separation between removal of unused/non-relevant code and the change of the storage type used for the previous.Less ideally it needs to be stated in the final commit message that the change removes this non-relevant function because reasons :)
The code side looks fine to me now. So more matter of logistics of how to get it into the main.