Created attachment 132387 [details] KDE crash report SUMMARY STEPS TO REPRODUCE 0. You nee a DB with a few videos with proper video thumbnails to start with. 1. select two or more videos with video thumbnails in the browser (can't do this with the demo as it only has one video). 2. click crtl-"+" or crtl-"-" to select the next or the previous thumbnail as the static thumbnail image OBSERVED RESULT 3. crash. EXPECTED RESULT no crash and the next image should be the static image. Funny, enough the same operation seems to work sometimes. When selecting only one video the operation seems to work. I just tested again and it worked a few times with two videos selected and then crashed again when dealing with two other videos. Each of which could be advanced one by one without crash. Dr. Konqui has provided a crash report (attachment). KPA v5.7.0-25-gd2af41aa KDE Frameworks 5.71.0 Qt 5.12.7 (kompiliert gegen 5.12.7) Das xcb Fenstersystem
Thanks for the bug report, and special thanks for running kphotoalbum with assertions enabled! I could reproduce this by copying additional video files to the demo database. The stack trace shows the following function as the culprit: DB::FileName nextExistingImage(const DB::FileName &fileName, int frame, int direction) { for (int i = 1; i < 10; ++i) { const int nextIndex = (frame + 10 + direction * i) % 10; const DB::FileName file = BackgroundJobs::HandleVideoThumbnailRequestJob::frameName(fileName, nextIndex); if (file.exists()) return file; } Q_ASSERT(false && "We should always find at least the current frame"); return DB::FileName(); } I'll have to evaluate the conditions that can lead to this code path in detail...
I just compiled the latest git master (v5.7.0-31-g17fb539c) and the bug persists.
Git commit f54a5e39fadb07ee85f380e4589191b649def858 by Johannes Zarl-Zierl. Committed on 30/10/2020 at 23:38. Pushed by johanneszarl into branch 'master'. Remove incorrect assertion. Apparently, it *is* possible for the nextExistingImage() function in UpdateVideoThumbnail.cpp to not find a next frame. M +3 -1 ImageManager/ThumbnailCache.cpp M +13 -2 MainWindow/UpdateVideoThumbnail.cpp https://invent.kde.org/graphics/kphotoalbum/commit/f54a5e39fadb07ee85f380e4589191b649def858
After checking the execution paths leading up to this function I'm confident that the assertion can be removed without harm.
Created attachment 132910 [details] screenshot1 ...before
Created attachment 132911 [details] screenshot2 ...after
Hi Johannes, I can confirm 2 things: 1. The crash is gone in v5.7.0-34-gf2dd467a - this is progress! 2. It is not only possible that KPA misses the next frame. It happens in ~50% of the selected thumbnails. See attached screenshots before and after I hit crtl-+ a few times on a selection of videos. Only the videos with the red dot show a different image. The others remain as before. To my untrained eye it looks like something of a timing/syncing bug between concurrent threads. Depending on the actual selection other videos get skipped. Should I file a new bug or would you rather reopen this one here? I agree, that the current state of affairs is no show stopper anymore, but still ... Best regards, Andreas
Let's keep the two issues in the same bug - so far it seems they are two sides of the same coin...
Hi Andreas, I wanted to revisit this bug and tried to reproduce it, but to no avail. Can you check that the bug persists in the current git master? Thanks, Johannes
Hi Johannes, thanks for looking after this. I just built the latest git master (v5.7.0-178-g096dbec0) and the bug seems to be gone indeed. I tested with various sets of videothumbnails: all changed in sync and no crash. So, I guess, you could just close this. I will keep this release in "production" for the time being. Best regards, Andreas
Hi Andreas, Thanks for the feedback! Cheers, Johannes