fix: critical bugs in face_swapper and globals, update gitignore#1740
Open
huynhhh wants to merge 2 commits intohacksider:mainfrom
Open
fix: critical bugs in face_swapper and globals, update gitignore#1740huynhhh wants to merge 2 commits intohacksider:mainfrom
huynhhh wants to merge 2 commits intohacksider:mainfrom
Conversation
- Add missing `import logging` that caused NameError - Fix model path mismatch: pre_start() now checks inswapper_128.onnx matching pre_check() download - Add missing eyes_mask_size and eyebrows_mask_size globals to prevent AttributeError - Add INTERPOLATION_LOCK to protect PREVIOUS_FRAME_RESULT from race conditions across threads - Update .gitignore for env, claude, docs, plans
Contributor
Reviewer's GuideFixes several critical runtime issues in the face swapper pipeline: aligns the ONNX model path with the pre-check downloader, adds missing global config values, introduces a lock around frame interpolation state for thread safety, and updates .gitignore entries. Sequence diagram for thread safe frame interpolation in face_swappersequenceDiagram
actor WorkerThread1
actor WorkerThread2
participant FaceSwapper as FaceSwapperFrameProcessor
participant Globals
participant State as PREVIOUS_FRAME_RESULT
WorkerThread1->>FaceSwapper: apply_post_processing(frame1, bboxes1)
activate FaceSwapper
FaceSwapper->>FaceSwapper: acquire INTERPOLATION_LOCK
FaceSwapper->>Globals: read enable_interpolation, interpolation_weight
FaceSwapper->>State: read PREVIOUS_FRAME_RESULT
FaceSwapper->>FaceSwapper: interpolate frame1 with PREVIOUS_FRAME_RESULT
FaceSwapper->>State: write new PREVIOUS_FRAME_RESULT
FaceSwapper->>FaceSwapper: release INTERPOLATION_LOCK
deactivate FaceSwapper
par concurrent_frame_processing
WorkerThread2->>FaceSwapper: apply_post_processing(frame2, bboxes2)
activate FaceSwapper
FaceSwapper->>FaceSwapper: acquire INTERPOLATION_LOCK
FaceSwapper->>Globals: read enable_interpolation, interpolation_weight
FaceSwapper->>State: read PREVIOUS_FRAME_RESULT
FaceSwapper->>FaceSwapper: interpolate frame2 with PREVIOUS_FRAME_RESULT
FaceSwapper->>State: write new PREVIOUS_FRAME_RESULT
FaceSwapper->>FaceSwapper: release INTERPOLATION_LOCK
deactivate FaceSwapper
end
WorkerThread1->>FaceSwapper: process_frame(source_face, frameX) with opacity 0
activate FaceSwapper
FaceSwapper->>Globals: read opacity
FaceSwapper->>FaceSwapper: acquire INTERPOLATION_LOCK
FaceSwapper->>State: set PREVIOUS_FRAME_RESULT to None
FaceSwapper->>FaceSwapper: release INTERPOLATION_LOCK
FaceSwapper-->>WorkerThread1: return original frameX
deactivate FaceSwapper
Class diagram for updated face_swapper interpolation and globalsclassDiagram
class FaceSwapperFrameProcessor {
<<module>>
PREVIOUS_FRAME_RESULT np_ndarray_or_None
INTERPOLATION_LOCK threading_Lock
pre_check() bool
pre_start() bool
apply_post_processing(current_frame Frame, swapped_face_bboxes List_np_ndarray) Frame
process_frame(source_face Face, temp_frame Frame) Frame
process_frame_v2(temp_frame Frame, temp_frame_path str) Frame
process_image(source_path str, target_path str, output_path str) void
process_video(source_path str, temp_frame_paths List_str) void
}
class Globals {
<<module>>
opacity float
map_faces bool
enable_interpolation bool
interpolation_weight float
mask_down_size float
mask_size float
mouth_mask_size float
eyes_mask_size float
eyebrows_mask_size float
}
class ModelFiles {
<<filesystem>>
models_dir str
inswapper_128_onnx str
}
FaceSwapperFrameProcessor ..> Globals : uses
FaceSwapperFrameProcessor ..> ModelFiles : reads_path
class ThreadingRuntime {
<<runtime>>
acquire(lock threading_Lock) void
release(lock threading_Lock) void
}
FaceSwapperFrameProcessor ..> ThreadingRuntime : synchronized_by
Flow diagram for updated frame interpolation logic in apply_post_processingflowchart TD
A_start[Start apply_post_processing] --> B_set_final[Set final_frame to processed_frame]
B_set_final --> C_lock[Acquire INTERPOLATION_LOCK]
C_lock --> D_check_interp{enable_interpolation
and 0<weight<1?}
D_check_interp -- No --> D1_clear_prev[Set PREVIOUS_FRAME_RESULT to None]
D1_clear_prev --> E_unlock[Release INTERPOLATION_LOCK]
E_unlock --> F_return[Return final_frame]
D_check_interp -- Yes --> G_check_prev{PREVIOUS_FRAME_RESULT
is not None and
shape and dtype match?}
G_check_prev -- No --> H_set_prev_curr[Set PREVIOUS_FRAME_RESULT to processed_frame copy]
H_set_prev_curr --> E_unlock
G_check_prev -- Yes --> I_try_interp[Try gpu_add_weighted
using PREVIOUS_FRAME_RESULT
and processed_frame]
I_try_interp --> J_success{Interpolation
success?}
J_success -- Yes --> K_clip[Clip to 0_255 and cast to uint8]
K_clip --> L_set_final[Set final_frame to interpolated result]
L_set_final --> M_update_prev[Set PREVIOUS_FRAME_RESULT to final_frame copy]
M_update_prev --> E_unlock
J_success -- No --> N_fallback[Set final_frame to processed_frame
and PREVIOUS_FRAME_RESULT to None]
N_fallback --> E_unlock
F_return --> Z_end[End]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new INTERPOLATION_LOCK wraps the entire interpolation computation; consider narrowing the locked section to just the reads/writes of PREVIOUS_FRAME_RESULT so that other threads don’t block on gpu_add_weighted and numpy work unnecessarily.
- Using a single global PREVIOUS_FRAME_RESULT and INTERPOLATION_LOCK will couple all concurrent video/image processing sessions; if multi-stream processing is expected, consider moving this state to a per-stream/context object instead of globals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new INTERPOLATION_LOCK wraps the entire interpolation computation; consider narrowing the locked section to just the reads/writes of PREVIOUS_FRAME_RESULT so that other threads don’t block on gpu_add_weighted and numpy work unnecessarily.
- Using a single global PREVIOUS_FRAME_RESULT and INTERPOLATION_LOCK will couple all concurrent video/image processing sessions; if multi-stream processing is expected, consider moving this state to a per-stream/context object instead of globals.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Lock only protects reads/writes of PREVIOUS_FRAME_RESULT. Expensive gpu_add_weighted and numpy computation now runs outside the lock.
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
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.
import loggingthat caused NameErrorSummary by Sourcery
Fix critical issues in face interpolation state handling, model loading, and global configuration, and update ignore rules.
Bug Fixes:
Enhancements:
Build: