You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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
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.
Generally speaking, libopenshot is well written and quite efficient but I noticed a few things that could be optimized:
QImage::bits()
andQImage::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 useQImage::constBits()
orQImage::constScanline()
instead. Potential gains are inFrame::GetPixels()
,Frame::AddImage(std::shared_ptr<QImage> new_image, bool only_odd_lines)
,Frame::GetPixels(int row)
. and maybe a few other places.for (n = 0; n < image->columns() * image->rows(); n += 1)
. This is highly inefficient becauseQImage::rows()
andQImage::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 inTimeline::add_layer
and in most of the effect classes. A similar problem can be found inFrame::AddMagickImage
in the loop that converts all pixels of the Magick::Image to RGBA.The text was updated successfully, but these errors were encountered: