The Wayback Machine - https://web.archive.org/web/20200906232831/https://github.com/mltframework/mlt/pull/573
Skip to content
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

Optimize Qimage producer #573

Open
wants to merge 2 commits into
base: master
from
Open

Conversation

@j-b-m
Copy link
Contributor

j-b-m commented May 30, 2020

Looking at the QImage producer, I found several bottlenecks that I could improve:

  • Slightly faster check if image is readable on producer init (use QImageReader methods only to check image size and ensure it is readable instead of loading full image)

  • When transferring the QImage data, use the QImage::Format_RGBA8888 or QImage::Format_RGB888 to copy the image in one pass instead of doing a pixel copy

  • Disable caching for ttl=1 (image sequences with 1 image/frame). Caching scaled image does not make sense if we have an image sequence with 1 image/frame. Disable caching for those provides a noticeable speedup, in Kdenlive these sequences can now play at 20fps instead of 7fps on my machine.

Since this touches a rather important MLT component, comments are welcome.

j-b-m added 2 commits May 30, 2020
…ng for ttl=1 (image sequences with 1 image/frame)
@j-b-m j-b-m changed the title Optimaze Qimage producer Optimize Qimage producer May 30, 2020
@ddennedy
Copy link
Member

ddennedy commented May 31, 2020

I agree with the ideas but 2 things:

  1. ttl defined does not imply it is an image sequence. producer_qimage.count > 0 is a required check. In Shotcut, when you convert an image sequence back to still it does not clear ttl.

  2. It is a heavy amount of change in some complex, stable code. Was it all necessary? How do we ensure there is no dangling pointer, bad pointer dereference, race condition, memory leak, etc.?

@bmatherly
Copy link
Member

bmatherly commented May 31, 2020

I am interested in "Slightly faster check if image is readable on producer init" because the new slideshow feature in Shotcut will compel people to open many images at once.

It would be good if you could share some rough metrics of how much the performance is improved with these changes. For example, How much faster can 100 images be loaded with this change?

@j-b-m
Copy link
Contributor Author

j-b-m commented May 31, 2020

@ddennedy I agree that this is a rather large patch which makes it difficult to track possible issues as it mixes some refactoring with other optimizations. I will maybe try to come up with several smaller patches that implement the various changes with only the necessary changes :

  • One patch to disable caching on count > 0 and ttl == 1
  • One patch to use the faster QImage::Format_RGBA8888 method to copy pixel data to MLT
  • One patch to implement faster producer init (do not load image at init)
  • Possibly a last patch to refactor some code (in current code we have several useless QImage * to MLT void* conversions).

@bmatherly In fact the speedup on producer init is very noticeable but it's because the image is not read anymore at that stage, only when requesting an image from it. I hope it's ok to proceed like this. Opening a project file with 75 images drops from 3 seconds to 0.3 seconds, but of course when displaying the images in monitor, they now need to be loaded. It also doesn't consume memory until the image is displayed.

@sandsmark
Copy link
Contributor

sandsmark commented Aug 27, 2020

I might be missing something, but doesn't the overhead from flipping every pixel in the image to another format outweigh any cost of copying?

and don't you at least need to check the stride? I don't think it's safe to assume that the lines are aligned perfectly, I'd at least memcpy scanLine() by scanLine().

#endif
}
self->current_image = ( uint8_t * )mlt_pool_alloc( image_size );
memcpy( self->current_image, scaled.constBits(), image_size);

This comment has been minimized.

@sandsmark

sandsmark Aug 27, 2020

Contributor

you should at least check that scaled.sizeInBytes() is same or bigger than image_size so you don't read out of bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.