Frame.cpp: Avoid unnecessary copy of image data

[musteresel]

As mentioned in issue #202 QImage::bits() and QImage::scanLine() make a deep copy of the source image. This is completely unnecessary when read-only access to the pixel data is required. Changing to QImage::constBits() and QImage::constScanLine() solves this. Both functions were introduced in Qt 4.7.

Tested using some test project in OpenShot, not sure if I covered all cases, though!

https://doc.qt.io/qt-5/qimage.html#constBits

[ferdnyc]

@musteresel I looked at this a bit after #202 was filed, and at the time I wasn't completely sold on the actual issue here. (For reasons I'll come to shortly.)

Given that you'd filed this PR, though, I checked it out and ran some tests to compare the effects.

valgrind memcheck tests

I first ran openshot-test from both the current develop branch code and this PR branch through valgrind's memcheck scanner. valgrind did show slightly fewer logged 'errors' with the PR code. (Any flagged event is an error, even "potentially lost" memory blocks... and in truth the difference may be down to my not having some debug symbols installed on the first run. If I were being 100% rigorous I'd rebuild and rerun the first test in the exact same environment.)

The end-result memory stats were exactly the same, though, which I wouldn't have expected to be the case if lots of unnecessary copies were happening.

The result summary from a baseline openshot-test run without these changes:

$ cd build
$ valgrind --trace-children=yes --leak-check=full tests/openshot-test
[...]
==179087== LEAK SUMMARY:
==179087==    definitely lost: 73,246 bytes in 135 blocks
==179087==    indirectly lost: 615,383 bytes in 1,023 blocks
==179087==      possibly lost: 42,021,360 bytes in 66 blocks
==179087==    still reachable: 601,332 bytes in 2,898 blocks
==179087==                       of which reachable via heuristic:
==179087==                         newarray           : 1,536 bytes in 16 blocks
==179087==         suppressed: 0 bytes in 0 blocks
==179087== Reachable blocks (those to which a pointer was found) are not shown.
==179087== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==179087==
==179087== Use --track-origins=yes to see where uninitialised values come from
==179087== For lists of detected and suppressed errors, rerun with: -s
==179087== ERROR SUMMARY: 130 errors from 113 contexts (suppressed: 0 from 0)

For openshot-test with the PR applied:

$ cd build_pr375
$ valgrind --trace-children=yes --leak-check=full tests/openshot-test            
==180335== LEAK SUMMARY:
==180335==    definitely lost: 73,246 bytes in 135 blocks
==180335==    indirectly lost: 615,383 bytes in 1,023 blocks
==180335==      possibly lost: 42,021,360 bytes in 66 blocks
==180335==    still reachable: 601,332 bytes in 2,898 blocks
==180335==                       of which reachable via heuristic:
==180335==                         newarray           : 1,536 bytes in 16 blocks
==180335==         suppressed: 0 bytes in 0 blocks
==180335== Reachable blocks (those to which a pointer was found) are not shown.
==180335== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==180335==
==180335== Use --track-origins=yes to see where uninitialised values come from
==180335== For lists of detected and suppressed errors, rerun with: -s
==180335== ERROR SUMMARY: 118 errors from 101 contexts (suppressed: 0 from 0)

The full output from both runs:

• pr375_baseline_openshot-test_20191127.txt
• pr375_enabled_openshot-test_20191127.txt

Running valgrind did point me at an uninitialized-variable issue in FrameMapper, though, so the effort was doubly useful. I'll file a PR on that shortly.

OpenShot export memusage tests

Now, it's possible that the change would reduce only memory consumption during the program execution, but as long as all of that memory was subsequently cleaned up properly, there wouldn't have been any traces of it in valgrind. So, not seeing any change there isn't inherently conclusive.

Which is why I also re-ran one of my OpenShot export memory-consumption tests (from OpenShot/openshot-qt#2811). First on both the latest develop branch code of both projects, again as a baseline, and then on a libopenshot with these changes applied.

The results speak for themselves: Memory consumption is all but identical between the two runs. Any deviations in the graphs are mostly attributable to extra time I took in starting the export process, or terminating the application after it completed.

memusage graph comparison.pdf

What's going on here?

The secret, I suspected but am now completely certain, has to do with the fact that QImage::bits() is an overloaded function, and in const context the constBits() form is automatically used. (Edit: Same for QImage::scanLine().) So, Qt was automatically optimizing these calls on our behalf.

I'm still in favor of merging this, since it doesn't hurt to be more explicit in how we use QImage. Relying on context to choose which algorithm is used just makes the code harder to read. Thanks!

[musteresel]

Applied the suggested edits. Yes, it seems that this doesn't solve memory issues ... but as you said: being explicit doesn't hurt :)