Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR introduces potential thread-safety risks by weakening atomic ordering and includes unrelated logging changes.
Reviewed changes — this PR refactors the visibility animation to use an encapsulated FadeAnimation struct and updates several atomic operations.
- Encapsulate animation logic —
FadeAnimationstruct replaces direct use ofegui'sanimate_bool_with_time. - Atomic ordering changes — multiple atomic stores in
AppUI,TinyDecoder, andPresentDataManagerwere changed fromOrdering::ReleasetoOrdering::Relaxed.
⚠️ Atomic ordering regression
The changes from Ordering::Release to Ordering::Relaxed in atomic store operations may break synchronization guarantees. If these flags are used to signal other threads to read data, Release ordering is likely necessary to ensure visibility of preceding memory writes.
Technical details
# Atomic ordering regression
## Affected sites
- `src/appui.rs:106`
- `src/appui.rs:636`
- `src/appui.rs:639`
- `src/appui.rs:651`
- `src/appui.rs:663`
- `src/appui.rs:691`
- `src/decode_engine.rs:808`
- `src/presentation.rs:210`
- `src/presentation.rs:232`
- `src/presentation.rs:270`
- `src/presentation.rs:275`
## Required outcome
- Ensure that weakening the atomic ordering is safe or revert the changes. Release ordering is essential for thread synchronization; if these flags guard access to data, they must remain `Release` (or stronger).ℹ️ Nitpicks
- The
info!logs insrc/controlbar_ui.rsandsrc/whispercpp_transcriber.rsare unrelated to the refactoring and should be moved to a separate PR.
google/gemini-3.1-flash-lite ｜ 𝕏
| self.ui_flags | ||
| .visible_flag | ||
| .store(false, std::sync::atomic::Ordering::Release); | ||
| .store(false, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| context | ||
| .current_main_stream_timestamp | ||
| .store(0, std::sync::atomic::Ordering::Release); | ||
| .store(0, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| context | ||
| .current_video_timestamp | ||
| .store(0, std::sync::atomic::Ordering::Release); | ||
| .store(0, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| context | ||
| .tip_window_flag | ||
| .store(true, std::sync::atomic::Ordering::Release); | ||
| .store(true, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| context | ||
| .tip_window_flag | ||
| .store(true, std::sync::atomic::Ordering::Release); | ||
| .store(true, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| video_play_context | ||
| .current_video_timestamp | ||
| .store(f_pts, std::sync::atomic::Ordering::Release); | ||
| .store(f_pts, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| video_play_context | ||
| .current_video_timestamp | ||
| .store(pts, std::sync::atomic::Ordering::Release); | ||
| .store(pts, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| if let Some(pts) = audio_pts { | ||
| // info!("store main timestamp:{}",pts); | ||
| main_stream_current_timestamp.store(pts, std::sync::atomic::Ordering::Release); | ||
| main_stream_current_timestamp.store(pts, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| MainStream::Video => { | ||
| let pts = current_video_timestamp.load(std::sync::atomic::Ordering::Relaxed); | ||
| main_stream_current_timestamp.store(pts, std::sync::atomic::Ordering::Release); | ||
| main_stream_current_timestamp.store(pts, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Why was this changed from Ordering::Release to Ordering::Relaxed? This could be unsafe if it is used for thread synchronization.
| sleep(Duration::from_millis(200)).await; | ||
| } else { | ||
| transcribe_task_notify_cloned.notified().await; | ||
| info!("transcribe task waked"); |
There was a problem hiding this comment.
This log is unrelated to the animation refactor. Please move to a separate PR.
1 participant
No description provided.