Page MenuHome

Add methods foreach_get() and foreach_set() to pyrna_prop_array class for fast access to array data.
ClosedPublic

Authored by Bogdan Nagirniak (bnagirniak) on Mar 6 2020, 8:39 PM.

Details

Summary

Hello,

I'm developer of Blender plugin and currently in Blender getting data from pyrna_prop_array is quite slow operation. It is especially related of getting pixels data from Image object (field Image.pixels).
For example pyrna_prop_collection has foreach_get/set methods for such fast data access.

In this diff request I'm proposing changes which extend pyrna_prop_array with foreach_get/set methods.

Please review, thank you.

Diff Detail

Repository
rB Blender

Event Timeline

Fyi @Campbell Barton (campbellbarton) and @Bastien Montagne (mont29) Bogdan works on the ProRender plugin with us at AMD. But this should be useful for most plugins/users of the Python api.

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 9 2020, 3:08 PM

The functionality looks good to me. Thanks for working on this much needed feature.
It would be nice if you could provide some benchmark results.
Furthermore, can you add tests for this in tests/python/? The tests could look similar to bl_pyapi_idprop.py.

source/blender/python/intern/bpy_rna.c
5532

always use braces

This revision now requires changes to proceed.Mar 9 2020, 3:08 PM

Besides points raised by @Jacques Lucke (JacquesLucke), generally LGTM, and nice addition indeed.

Wouldn't mind having @Campbell Barton (campbellbarton)'s check on this one too though, py code looks fine to me, but am far from being a Py C API master...

source/blender/python/intern/bpy_rna.c
5427

int set -> const bool do_set

5559

0 -> false (and same below with true for set func).

Thanks for reviewing. I've fixed review comments, also added test script tests/python/bl_pyapi_prop_array.py which tests this new API.

Thanks. The test should be added to tests/python/CMakeLists.txt as well. This way it will run when make test is run.

add_blender_test(
  script_pyapi_prop_array
  --python ${CMAKE_CURRENT_LIST_DIR}/bl_pyapi_prop_array.py
)

I also noticed that one test is failing:

Traceback (most recent call last):
  File "/home/jacques/blender-git/blender/tests/python/bl_pyapi_prop_array.py", line 27, in test_foreach_getset_i
    self.array_i.foreach_set(np.arange(10, dtype=np.int64))
AssertionError: TypeError not raised

It would be nice if you could provide some benchmark results.

I did comparison tests of reading image pixels to numpy array. I used 1024x1024 image. Getting pixels data took:

  • using pixels.foreach_get() - 0.02 sec
  • using np.fromiter(pixels, dtype=np.float32): 1.7 sec
  • using np.array(pixels, dtype=np.float32): 1.8 sec

So, foreach_get is approximately 90 times faster.

Fixed unit test failing (seems it was platform specific), added unit test to CMakeLists.txt.

I forgot to mention before that there is also a warning:

/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c: In function ‘pyprop_array_foreach_getset’:
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_BOOLEAN’ not handled in switch [-Wswitch]
 5466 |     switch (prop_type) {
      |     ^~~~~~
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_STRING’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_ENUM’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_POINTER’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_COLLECTION’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5531:5: warning: enumeration value ‘PROP_BOOLEAN’ not handled in switch [-Wswitch]
 5531 |     switch (prop_type) {
      |     ^~~~~~
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5531:5: warning: enumeration value ‘PROP_STRING’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5531:5: warning: enumeration value ‘PROP_ENUM’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5531:5: warning: enumeration value ‘PROP_POINTER’ not handled in switch [-Wswitch]
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5531:5: warning: enumeration value ‘PROP_COLLECTION’ not handled in switch [-Wswitch]

Since you checked for those cases before, I think it is appropriate to simply add a default case with BLI_assert(false); break;.

Also, the test is still failing for me. In my case the reason is that an int64 buffer has the format letter l, which you seem to assume is int32 as well. I see that the Python documentation says that, but it's not true on my system. The tests runs successfully when I remove f != 'l' && from your code. If l is actually an int32 on your system it might be better to add an additional check like buf.itemsize == sizeof(int).

I forgot to mention before that there is also a warning:

/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c: In function ‘pyprop_array_foreach_getset’:
/home/jacques/blender-git/blender/source/blender/python/intern/bpy_rna.c:5466:5: warning: enumeration value ‘PROP_BOOLEAN’ not handled in switch [-Wswitch]
 5466 |     switch (prop_type) {
      |     ^~~~~~

Since you checked for those cases before, I think it is appropriate to simply add a default case with BLI_assert(false); break;.

No, please DO NOT use default: here. This is straight way to Hell when adding a new type, and you forget to check all switch/cases. Please use an explicit stack of case PROP_ENUM: etc. instead.

No, please DO NOT use default: here. This is straight way to Hell when adding a new type, and you forget to check all switch/cases. Please use an explicit stack of case PROP_ENUM: etc. instead.

I tried to follow the code style of other functions bpy_rna.c file and there are lot of 'default' keywords there in switching property types. Also in my function there is a check in the beginning:

if (prop_type != PROP_INT && prop_type != PROP_FLOAT) {
    PyErr_Format(PyExc_TypeError, "foreach_get/set available only for int and float");
    return NULL;
  }

Of course I can do what ever you ask, but ARE YOU SURE that using explicit check of all types here is good idea?

Added buf.itemsize check and default to switch.

Hello,
What is the resolution of this diff request? Is there anything I have to do here?
Thanks.

Besides the inline comment, this is looking good to me.

I do not fully agree that default must not be used here, because this code will not become wrong when a new property type is added. Not using default just means more work when a new type is added.

I'll commit this without default now, so that everyone is happy.

source/blender/python/intern/bpy_rna.c
5508

This is missing a break. Your tests crash in a debug build.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 13 2020, 1:04 PM
This revision was automatically updated to reflect the committed changes.

Thank you for this useful addition.
There's one gotcha that I think should be mentioned here for people like me struggling with foreach_get still being slow for Image.pixels
Since googling often suggests this page I'll post it here, hopefully it isn't inappropriate.

Major Gotcha: Do not ever copy pixels data from one image to another directly. This is 100x times slower. Use temporary buffer instead.

Slow example:

t0 = datetime.now()
# Save Viewer image data to temporary image
bpy.data.images['Viewer Node'].pixels.foreach_get(bpy.data.images.get("TempImage").pixels)
print("Time spent: %s seconds" % ( (datetime.now() - t0).total_seconds() ))

This single operation takes several minutes, depending on image size.

Fast example:

tempBuffer = numpy.empty(imageWidth * imageHeight * 4, dtype=numpy.float32)

t0 = datetime.now()
# Save Viewer image data to temporary image
bpy.data.images['Viewer Node'].pixels.foreach_get(tempBuffer)
bpy.data.images['TempImage'].pixels.foreach_set(tempBuffer)
print("Time spent: %s seconds" % ( (datetime.now() - t0).total_seconds() ))

This takes several seconds, for the same image size!