Sporadic test_array_to_qimage_rgb failure using pyside2

[ievacerny]

The test sporadically fails both on Travis and during a manual test suite run after switching to unittest runner (#515) (edit: happens with both unittest and nose runners). I haven't managed to reproduce the error when running just that single test or the test module, seems to only happen when the whole test suite is run.

For some reason the following pixels have different values than expected (0xff4488cc):

i=0, j=0: 0xff000000
i=1, j=0: 0xff000000
i=2, j=0: value differs between failures
i=4, j=0: 0xff000000
i=5, j=0: 0xff000000
i=6, j=0: value differs between failures

Non-informative failure traceback:

FAIL: test_array_to_qimage_rgb (pyface.ui.qt4.util.tests.test_image_helpers.TestImageHelpers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/travis/.edm/envs/pyface-test-3.6-pyside2/lib/python3.6/site-packages/pyface-7.1.0.dev10-py3.6.egg/pyface/ui/qt4/util/tests/test_image_helpers.py", line 79, in test_array_to_qimage_rgb
    for i in range(32) for j in range(64)
AssertionError: False is not true

[corranwebster]

That's really odd. There really shouldn't be anything more than a (channel-shuffled) memory copy for these pixel values.

I'm tempted to say that this is a Qt/PySide bug, but maybe I'm missing something.

[corranwebster]

Idea for debugging these issues: re-write the test to save the QImage out if it doesn't match what we expect so we can visually inspect it.

[ievacerny]

Here's the image from a failed test:

It should be all blue, but some pixels at the top are different. Strangely enough, the pixels in the image don't seem to match printed out hex values.

I thought that maybe it is another test interaction brought upon by the change in test order, but I managed to replicate the error with nose runner, so it is most likely an issue with Qt/PySide.

[ievacerny]

I think I found the reason behind the failure. It's not a Qt bug as the behaviour is mentioned in the documentation:

The buffer must remain valid throughout the life of the QImage and all copies that have not been modified or otherwise detached from the original buffer. The image does not delete the buffer at destruction. You can provide a function pointer cleanupFunction along with an extra pointer cleanupInfo that will be called when the last copy is destroyed.

The buffer in this case is data.data defined in array_to_QImage. QImage constructor doesn't take ownership of the buffer, so when qimage is returned by this function and data goes out of scope, the buffer is no longer valid and the memory can be overwritten. And I'm guessing that PySide is worse with memory management than PyQt, that's why the error has only been seen using Pyside.

Note: to reproduce this issue locally it helps if the UI is kept in the background

[corranwebster]

Right - so I think when I was writing this I assumed that the constructor copied the bytes of the image. Maybe modifying the code to use loadFromData to construct the QImage will resolve this. No loadFromData is for file formatted data (eg. bytes of a .png).

[kitchoi]

I saw the same error again on a recent build for this PR: #552
The test failure looks like before:

======================================================================
FAIL: test_array_to_qimage_rgb (pyface.ui.qt4.util.tests.test_image_helpers.TestImageHelpers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/travis/.edm/envs/pyface-test-3.6-pyside2/lib/python3.6/site-packages/pyface-7.1.0.dev17-py3.6.egg/pyface/ui/qt4/util/tests/test_image_helpers.py", line 80, in test_array_to_qimage_rgb
    for i in range(32) for j in range(64)
AssertionError: False is not true

I believe CI is doing a PR build which means the fix from #546 should have been applied. While normally I would have merged master to the PR to be sure, PR #552 is targeting another PR so I don't want to mess things up by merging master. Just wanted to note the occurrence. If this happens again, we might have to reopen the issue.

[kitchoi]

I believe CI is doing a PR build which means the fix from #546 should have been applied. While normally I would have merged master to the PR to be sure, PR #552 is targeting another PR so I don't want to mess things up by merging master. Just wanted to note the occurrence. If this happens again, we might have to reopen the issue.

Because PR #552 is targeting #545, the CI is using the merge commit based on #545. Since #545 was open before #546 was merged and has not pulled in the latest master, CI for #552 was built without the fix. That should explain the sporadic failure seen in #552. Nothing needs to be done here, the issue does not need to be reopened.