Unify image loader classes #99
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
There seems to be a problem with CI Windows builds:
|
Member
|
Yes, there is an issue with the Qt install for the Windows builds. I'm looking into it. |
Member
|
Build issue is fixed on develop, you might want to rebase :) |
In a later commit WorkerThread should also replace classes similar to ImageLoader: PageLoader, ImageLoaderGL and ImageLoaderByteArrayGL. Bugs fixed: 1. Eliminated a data race between ImageLoader::run() and ComicFlow::updateImageData()->ImageLoader::result(). Specifically when ImageLoader::busy() returns false, then ImageLoader::run() sets ImageLoader::working to true, loads the image and starts assigning it to ImageLoader::img, while ImageLoader::result() is accessed without locking from updateImageData(). Making ImageLoader::working atomic is clearly insufficient to eliminate this data race. The fix is to set 'working' to true immediately and synchronously as soon as a new task is assigned to the worker. 2. Replaced thread termination with graceful thread exit. ComicFlow destructor called QThread::terminate(), using which is discouraged by Qt documentation. The application exited without errors in Release mode. In Debug mode, however, it received the SIG32 signal on exit and printed the following warning - "QWaitCondition: mutex destroy failure: Device or resource busy". The loop in WorkerThread::run() is no longer endless. The worker thread properly ends and is joined in WorkerThread destructor. Design decisions: 1. WorkerThread could emit a signal when it completes a task. Thus updateTimer could be removed from ComicFlow and GoToFlow. However, there is no obvious way to use this new signal in the two GL classes. Also I don't know whether updateTimer is just an inefficient polling substitute for notification or an intentional animation mechanism. 2. The index variable is no longer stored in the worker class, but in ComicFlow directly. Thing is, this data member was never actually accessed by the worker, but ComicFlow went so far as to lock worker's mutex to "protect" access to the index. 3. The common ImageLoader implementation turned out to be very general. So I converted it into the WorkerThread class template that is not restricted to producing QImage results and can be reused elsewhere. 4. I used standard classes (such as std::thread) instead of their Qt equivalents (e.g. QThread) because they are more thoroughly documented. The standard classes should also be more efficient as they were more carefully designed and provide much fewer unnecessary features. 5. Release-Acquire ordering is safe for the WorkerThread::working use case and is more efficient than the std::atomic-default Sequentially-consistent ordering. 6. condition.notify_one() is called while the mutex is unlocked to improve performance. This is safe in both cases: a) if the worker thread exits due to a spurious wakeup just before the condition.notify_one() call in WorkerThread destructor, so much the better; b) if a spurious wakeup lets the worker thread finish the task and start waiting on the condition again just before the condition.notify_one() call in WorkerThread::performTask(), the second waking will be ignored by the worker thread as 'working' and 'abort' will be false then.
The timer pointer forced error-prone manual memory management without any benefits.
vedgy
force-pushed
the
unify-image-loader-classes
branch
from
fe5624b to
524495d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first pull request in a series of two. It introduces the common worker class and uses it instead of one of the 4 image loader classes. See commit messages for details.
Note: I don't like some of the formatting changes clang-format does. There aren't many templates, lambdas or default member initializers in YACReader code yet. Consider the results and see if you like them. If not, you could change some of the formatting rules. I'd be happy to reformat changes in this pull request afterwards :). These are clang-format changes to the initial version of this pull request: clang-format-changes.patch.zip.