Fix a crash and clean up YACReaderFlowGL #198
Conversation
Adjusting index of the item that is about to be removed is pointless. Not adjusting the last index caused an assertion failure (a crash) in Library built in Debug mode - when a comic other than the last in the current folder was deleted in ClassicComicsView with hardware acceleration enabled.
The implementation when item != -1 seems to leak images[item].texture. Let us remove rather than fix this unused branch. Make YACReaderFlowGL::insert()'s name argument const.
This index was used only in YACReaderFlowGL::drawCover(). A simple change of this function eliminates the use. The benefits of the removal: * sizeof(YACReader3DImage) is 8 bytes less now, which improves CPU cache utilization and saves a little RAM; * the code, which ensured that this index member matches the index of the image in YACReaderFlowGL::images, can be removed now; * the index member can no longer get out of sync with the index of the image. Simplify the implementation of YACReaderFlowGL::draw(). For some (most likely obsolete) reason YACReaderFlowGL::replace() checked if the index member matches the index of the image and handled the mismatch differently. I don't see how the mismatch could happen with the current implementation of YACReaderFlowGL and I couldn't trigger it during debugging. So I think this special case can be safely removed.
|
There is also a crash on exit from YACReader Library after removing a comic or a folder. I have already fixed it and will create a pull request soon. |
|
How did you tested it? Moving quickly between folders in YACReaderLibrary, or fast opening next/previous comic in YACReader while the flow is shown (using CTRL + Left/Right) can be good ways of testing this. I have some memories about the worker loading an image from the previous content while the flow is repopulated, causing a wrong image being displayed in the flow, because once the right one finishes loading the flow will reject it because it thinks it is already loaded. |
Just tested Library and Reader like you described at this branch. Haven't spotted a wrong image near the beginning. But detecting that a wrong image is displayed somewhere far from the first one is obviously difficult.
Your description resembles a symptom of a thread safety bug. Sadly, in the presence of data races any refactoring or fix can cause a regression. I still plan to implement a follow-up pull request to #99. But I have forgotten the details of that fix/refactoring, so not sure whether it would guarantee thread safety here. |
loaded[idx] is unconditionally set to true in the call to YACReaderFlowGL::replace() just before the removed line.
That's why I usually don't like changing code just for the sake of it. This worker implementation comes from the original PictureFlow implementation, and it's been years since I touched it. So I don't think the benefits of these kind of changes are worth the risks. But I am happy to see the crash fixed :), nice catch! |
|
This pull request does not touch the workers. The changes here seem to be pretty safe to me. The removed
My reimplementation of |
Just looked at the code again and realized that you might have confused the removed |
luisangelsm
force-pushed
the
develop
branch
from
6ccfc51
to
63fcde8
Compare
See the commit messages for details.