The Wayback Machine - https://web.archive.org/web/20210124113125/https://github.com/OpenShot/libopenshot/issues/202
Skip to content

/ libopenshot

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A few optimizations #202

Open
schauveau opened this issue Mar 15, 2019 · 1 comment
Open

A few optimizations #202

schauveau opened this issue Mar 15, 2019 · 1 comment
Labels
code enhancement

Comments

@schauveau
Copy link

@schauveau schauveau commented Mar 15, 2019

Generally speaking, libopenshot is well written and quite efficient but I noticed a few things that could be optimized:

  • ImageReader is creating a Magick::Image that is converted into a new QImage for each frame (via Frame::AddMagickImage). That conversion is not very efficient and could be avoided by reusing the same QImage.
  • QImage::bits() and QImage::scanline() provide a non-const pointer to the image data. If the QImage is shared (because of a shallow copy), this causes an automatic duplication of the data. Consequently, if the data is not going to be modified then it could be beneficial to use QImage::constBits() or QImage::constScanline() instead. Potential gains are in Frame::GetPixels(), Frame::AddImage(std::shared_ptr<QImage> new_image, bool only_odd_lines), Frame::GetPixels(int row). and maybe a few other places.
  • In several places, the pixels of a QImage are processed with a loop of the form for (n = 0; n < image->columns() * image->rows(); n += 1). This is highly inefficient because QImage::rows() and QImage::columns() are functions that cannot be inlined by the compiler. Consequently, the compiler cannot assume that their values are constant and must perform the 2 calls at each iteration. Also, the code cannot be vectorized. This is found in Timeline::add_layer and in most of the effect classes. A similar problem can be found in Frame::AddMagickImage in the loop that converts all pixels of the Magick::Image to RGBA.
musteresel added a commit to musteresel/libopenshot that referenced this issue Nov 26, 2019
@musteresel
Frame.cpp: Avoid unnecessary copy of image data
Loading status checks…
4b76c1e 
As mentioned in issue OpenShot#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.

https://doc.qt.io/qt-5/qimage.html#constBits
@musteresel musteresel mentioned this issue Nov 26, 2019
Merged
@ferdnyc
Copy link
Collaborator

@ferdnyc ferdnyc commented Nov 27, 2019 •
edited

  • QImage::bits() and QImage::scanline() provide a non-const pointer to the image data. If the QImage is shared (because of a shallow copy), this causes an automatic duplication of the data. Consequently, if the data is not going to be modified then it could be beneficial to use QImage::constBits() or QImage::constScanline() instead. Potential gains are in Frame::GetPixels(), Frame::AddImage(std::shared_ptr<QImage> new_image, bool only_odd_lines), Frame::GetPixels(int row). and maybe a few other places.

On this point: As I noted in #375, QImage::bits() and QImage::scanLine() are overloaded functions, and in const context they're equivalent to constBits() and constScanLine() automatically. We merged the change to use the const...() forms explicitly, for the sake of code readability and clarity, but it was purely code-cosmetic — the const context of the previous calls meant that they were already avoiding any unnecessary copying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code enhancement
2 participants
@ferdnyc @schauveau