Page MenuHome

Compositor: Add vars and methods for easier image looping
ClosedPublic

Authored by Manuel Castilla (manzanilla) on Apr 19 2021, 6:08 PM.

Details

Summary

These variables and methods should make it easier to loop through buffers elements/pixels. They take into account single element buffers.
Single element buffers can be used for set operations to reduce memory usage.

Usage example: P2078

Diff Detail

Repository
rB Blender
Branch
cmp-loop-image-vars (branched from master)
Build Status
Buildable 14239
Build 14239: arc lint + arc unit

Event Timeline

Manuel Castilla (manzanilla) requested review of this revision.Apr 19 2021, 6:08 PM
Manuel Castilla (manzanilla) created this revision.
Manuel Castilla (manzanilla) retitled this revision from add vars and methods for easier image looping to [WIP] Compositor: add vars and methods for easier image looping.Apr 19 2021, 6:21 PM
Manuel Castilla (manzanilla) edited the summary of this revision. (Show Details)
  • Inherit public BaseBuffer
  • Remove mostly unused variables
  • Minimize changes
  • Take into account single elem buffers
Manuel Castilla (manzanilla) retitled this revision from [WIP] Compositor: add vars and methods for easier image looping to Compositor: add vars and methods for easier image looping.Apr 26 2021, 1:05 PM

What do you think of adding some BLI_asserts?
stride and offset are more common words than jumps.
get_offset might then become something else.

naming single elem is a
We need to see how we could eliminate some logic so the compiler could optimize them. (but that is not related to this patch).

Some methods should be implemented in the header so the compiler could optimize these better.

Another idea is to use a template based on the num of channels.

source/blender/compositor/intern/COM_MemoryBuffer.cc
39

can't we decode is_single_elem from the rect. or do we mean constant data.

Jeroen Bakker (jbakker) requested changes to this revision.Apr 26 2021, 4:01 PM
This revision now requires changes to proceed.Apr 26 2021, 4:01 PM
  • Renamings
  • Add asserts
source/blender/compositor/intern/COM_MemoryBuffer.cc
39

Cannot because when is_a_single_elem=true, buffer may virtually have any resolution. For example a SetColorOperation buffer may be:

m_rect={0,500,0,500}, getWidth()=500, get_memory_width()=1, elem_offset=0, row_offset=0, is_a_single_elem=true

This allows writing an output buffer without worrying if inputs are single elem buffers, just need to correctly use their offset variables.

We need to see how we could eliminate some logic so the compiler could optimize them. (but that is not related to this patch).

Some methods should be implemented in the header so the compiler could optimize these better.

Another idea is to use a template based on the num of channels.

I agree. I'd like to make it generic too, not only for floats. And maybe moving pixel operations methods like readEWA and read bilinear to a header specific for pixel operations. We can see this later on.

Jeroen Bakker (jbakker) requested changes to this revision.Apr 28 2021, 8:17 AM

Naming is important so good spend just another round to have this correct and consistent. Rest of the patch is fine.

source/blender/compositor/intern/COM_MemoryBuffer.h
61

Isn't an offset, but a stride. elem_stride

70

same here.
an offset is used to shift a buffer/elem. an stride move to next elem/row.

eg

buffer_index = y * buffer.row_stride + x * buffer.elem_stride;

In this case an offset can be used as a channel selector. (offset of second channel)

buffer_index = y * buffer.row_stride + x * buffer.elem_stride + offset_channel;

Just to keep things aligned to other areas.

This revision now requires changes to proceed.Apr 28 2021, 8:17 AM
  • Rename offset vars to stride
Manuel Castilla (manzanilla) marked 2 inline comments as done.Apr 28 2021, 10:27 AM
Manuel Castilla (manzanilla) retitled this revision from Compositor: add vars and methods for easier image looping to Compositor: Add vars and methods for easier image looping.May 9 2021, 7:01 PM
This revision is now accepted and ready to land.May 10 2021, 8:31 AM