Attribute a finished render's thumbnail to its own frame - #800
Merged
Conversation
Renders are asynchronous and a full-res decode takes seconds, so one can land after the user has clicked on to the next frame. The thumbnail update took the pixels from last_metrics -- the render that just finished -- but keyed them by uploaded_files[selected_file_idx], whichever frame is selected *now*. Clicking through a folder faster than it decodes therefore cached one frame's picture under another frame's thumbnail, where it stayed until that cell was clicked and re-rendered. The render memo a few lines below already guards for this, comparing the render's source_hash against the current file, which is why renders arriving for a frame that is no longer current was a known possibility. The thumbnail now follows the render's own hash. Looking the asset up rather than merely skipping the update means a late render still refreshes the right cell instead of being dropped. With no source_hash it falls back to the selection, which is the active_file_changing caller -- that one fires before selected_file_idx moves, so the outgoing frame is still selected there and was never affected.
Contributor
|
Great! This has been bugging me for some time. |
marcinz606
added a commit
that referenced
this pull request
Aug 12, 2026
#800 stopped a finished render being attributed to whatever frame was selected, but only closed half the hole: it checked whether the render's source_hash matched an open asset and, on a miss, still fell back to selected_file_idx. That fallback fires whenever the list has moved on -- a different folder opened, or a merge swapping frames for a composite -- and _update_thumbnail_from_state runs on file switch, on save and after an export, not only from the render that produced the buffer. So last_metrics can hold a render whose frame is no longer open, and its picture is filed under whichever frame is selected now. The write is persisted, so it outlives the session and stays wrong until that frame is rendered again. Found on disk rather than reasoned about: an audit of 68 raw files under one shoot compared every cached thumbnail against its own file by structural correlation, robust to a rendered positive differing in tone from the source preview. One frame scored 0.10 -- slide-07's beach sunset carrying a sideways slide-09-style render -- and the mis-filed image matched, at 1.00, the thumbnail legitimately owned by a frame in slide-08. One image, two keys. A render that cannot be attributed to an open frame now updates nothing. The cost is a skipped refresh in exactly the case where the alternative is writing someone else's picture; the next render of that frame fills it in. The no-source_hash fallback goes too. It was justified on the grounds that active_file_changing snapshots the outgoing frame, which is still selected -- but that cannot be told apart from a different folder being open now, which is the failure above. The render worker sets source_hash unconditionally on a required task field, so refusing costs nothing real. Three existing fixtures omitted source_hash and leaned on the fallback to attribute; they now set the hash their asset carries, which is what the worker always does. Co-authored-by: marcinz606 <99659443+marcinz606@users.noreply.github.com>
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
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.
The bug
The filmstrip sometimes shows one frame's picture on another frame's thumbnail, and it stays wrong until that cell is clicked and re-rendered.
Renders are asynchronous and a full-res decode takes seconds, so one can land after the user has clicked on to the next frame.
_update_thumbnail_from_statetook the pixels fromlast_metrics— the render that just finished — but keyed them byuploaded_files[selected_file_idx], whichever frame is selected now:Select A, start its render, click B before the decode finishes, and A's pixels get filed under B's key.
That this can happen is already known a few lines below, where the render memo guards for exactly it:
The memo path had the guard; the thumbnail path never got one.
The fix
The thumbnail now follows the render's own
source_hash. Looking the asset up by that hash, rather than merely skipping the update when it mismatches, means a late render still refreshes the correct cell instead of being dropped.With no
source_hashit falls back to the selection. That is theactive_file_changingcaller, which fires beforeselected_file_idxmoves — the outgoing frame is still selected there, so that path was never affected and keeps working.Testing
make allclean. Newtests/test_thumbnail_attribution.pycovers the race (a render for A landing while B is selected updates A), the ordinary case, the no-hash fallback, and a render whose frame has been unloaded. Two of them fail against the previous code.tests/test_thumbnail_readback_thread.py's stub gained the new helper; no behaviour change there.