Video: Fix race condition during rapid video frame navigation - #333
Conversation
…cking the latest requested frame.
abdullah-ukaea
changed the title
|
ready for code review @jblake42 |
jblake42
left a comment
jblake42
left a comment
There was a problem hiding this comment.
This looks good and I'm happy for it to merge. Could I ask you to look how easy it would be for the frame display to show the requested frame when a user is quickly clicking through the frames as at the moment it remains on the current frame until the user stops clicking and the frame loads.
This isn't blocking and if it is more than a few lines of code I'm happy for it to be pulled into a new PR.
yep this is possible by just displaying "desired frame" turned out to be a 1 line code change 70e371e The problem now, is that when the user clicks next e.g. from frame 0 to frame 1 there is a delay so that while the user sees the frame display is at frame 1 they are still at frame 0 for a few seconds until it updates. So I added a very small spinner above the frame display to give the user some indication they are not at the frame they think they are. Let me know what you think about this change in commit e149d98 @jblake42 |
I think the spinner is a good idea but I am not a fan of the implementation as it is not very obvious the spinner is there. My recommendation would be a larger spinner over the actual image as that is incredibly clear that something is loading. Not sure on the feasibility so open to other implementations 2026-07-30.07-06-10.mp4 |
jblake42
left a comment
jblake42
left a comment
There was a problem hiding this comment.
Request changes relating to the spinner as per above comment
|
Regarding the second feature you requested to disable annotations during frame loading @jblake42 I confirmed that users can still draw annotations while a new frame is loading, but I have not fixed this in this PR on purpose. The video view has been significantly refactored in PR #334, so I would prefer to disable annotation editing during frame loading there to avoid conflicting changes. If you are happy with the new spinner, I think this PR is ready to merge. I have documented this in that PR already here #334 (comment) |
jblake42
left a comment
jblake42
left a comment
There was a problem hiding this comment.
Please make use of react spectrum for the spinner e.g.:
<ProgressCircle aria-label="Loading…" size="L" isIndeterminate /> instead of a custom component
After that I am happy for you to merge
2 participants
Summary
Rapid frame navigation could cause stale responses to overwrite newer frames, leaving the video view flickering and unusable.
This change tracks the latest requested frame, cancels older requests, and only allows the newest response to update the UI. Rapid Next and Previous clicks now resolve to the expected frame.
This will close issue #332
Current dev branch behaviour
dev_branch.mp4
This branch behaviour
latest_branch_fix.mp4