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

threaded image prefetch #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gdzhu
Copy link
Contributor

@gdzhu gdzhu commented Aug 26, 2021

WHY: On devices with slower I/O, loading images to QPixmap may take a long time (in my case the image directory is mapped to cloud storage). And since the timer is in its separate thread and fires signal to update the image with the configured timeout, adding image loading/processing time to the total images rotation duration negatively impacts the experience of the slide show by:

  • causing images to be displayed with different durations, and/or
  • when transition animation is added, potentially causing the animation to get cut off halfway.

WHAT:

  • This PR moves image loading action from within the image update handler (MainWindow::setImage) to its own function in ImageSwitcher that can be parallelized to the main window image update thread.
  • The image loading action is wired to a new imageUpdated signal, emitted when MainWindow::setImage finished executing. It fetches the next filename and loads the image into memory in the background while the QWidget is being repainted, so that when the timeout timer fires, the main window update don't need to wait for the image to load.

NOTE:

  • C++ is not my expertise, and I'm not sure the threaded solution I chose ( a combination of QFuture, QFutureWatcher and QtConcurrent) is ideal.

  • One caveat is that if the image is not successfully loaded when ImageSwitcher::updateImage is being invoked by the timer, the same (old) image will be displayed, effectively double the duration for that image and skipping the failed-to-load image (because the image update action will trigger another image load action), but with the original logic the same behavior would also be expected (but worse because of the added image processing time).
    One potential workaround is to block on ImageSwitcher::updateImage to update the image only when a new image is successfully loaded, but my lack of C++ knowledge makes it hard for me to have it implemented.

    The main drawing thread can block on image loading with QFutureWatcher.isRunning().

  • This PR also includes changes from configurable transition time #42 and skip overlay processing when no overlay is specified #43, and those should be reviewed prior to this one.
    I was able to refactor the PR and remove unrelated changes.

@gdzhu gdzhu force-pushed the gdzhu/threaded_image_prefetch branch from ddd605e to 3a23c81 Compare August 26, 2021 05:45
@gdzhu gdzhu marked this pull request as ready for review August 26, 2021 05:47
@@ -151,7 +152,7 @@ int MainWindow::getImageRotation()

void MainWindow::updateImage(bool immediately)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't we draw the image here and immediately render the next one in the same thread? That way we just had to make sure we don't run updateImage() again before the previous run is finished (when the render took longer than the timeout)

Copy link
Contributor Author

@gdzhu gdzhu Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would definitely be better. Tried this idea previously but could not get it to work, reason being (it seems to me) QWidget::update() will not draw the image until the function ImageSwitcher::updateImage() returns. Any suggestions or it's just me not familiar with QT?
Also tried offloading the rendering to a third thread, but that adds more coordination and I can open up another PR for it if desire.

@gdzhu gdzhu requested a review from NautiluX September 11, 2021 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants