refactor(face-swapper): extract pair-building helpers, reduce nesting#1685
Open
laurigates wants to merge 2 commits intohacksider:mainfrom
Open
refactor(face-swapper): extract pair-building helpers, reduce nesting#1685laurigates wants to merge 2 commits intohacksider:mainfrom
laurigates wants to merge 2 commits intohacksider:mainfrom
Conversation
Add FACE_CONFIDENCE_THRESHOLD, DETECTION_INTERVAL, DETECTION_CACHE_SIZE, FPS_CAP, and MOUTH_FEATHER_RADIUS constants to replace scattered magic numbers throughout the codebase. Note: MAP_LOCK is handled separately in PR hacksider#1655 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors face-swap pair selection by extracting dedicated helpers for file-based and live modes, centralizing detection timing constants in globals, and tightening synchronization around mapping structures. Sequence diagram for live face-swap pair building with _build_pairs_livesequenceDiagram
participant Caller as process_frame_v2
participant FS as _build_pairs_live
participant GF as get_many_faces
participant G as modules_globals
participant FC as find_closest_centroid
Caller->>FS: _build_pairs_live(processed_frame)
FS->>GF: get_many_faces(processed_frame)
GF-->>FS: detected_faces
FS->>FS: check detected_faces is not empty
FS->>G: acquire MAP_LOCK
FS->>G: simple_map = getattr(simple_map)
FS->>G: release MAP_LOCK
alt many_faces enabled
FS->>G: source_face = default_source_face()
FS->>FS: for each target_face in detected_faces
FS->>FS: append (source_face, target_face)
else simple_map configured
FS->>FS: source_faces = simple_map.source_faces
FS->>FS: target_embeddings = simple_map.target_embeddings
alt len(detected_faces) <= len(target_embeddings)
loop for each detected_face
FS->>FS: ensure detected_face.normed_embedding
FS->>FC: find_closest_centroid(target_embeddings, detected_face.normed_embedding)
FC-->>FS: closest_idx
FS->>FS: append (source_faces[closest_idx], detected_face)
end
else len(detected_faces) > len(target_embeddings)
FS->>FS: build detected_embeddings and detected_faces_with_embedding
loop for i, target_embedding in target_embeddings
FS->>FC: find_closest_centroid(detected_embeddings, target_embedding)
FC-->>FS: closest_idx
FS->>FS: append (source_faces[i], detected_faces_with_embedding[closest_idx])
end
end
else no mapping configured
FS->>G: source_face = default_source_face()
FS->>GF: get_one_face(processed_frame)
GF-->>FS: target_face
FS->>FS: if source_face and target_face append pair
end
FS-->>Caller: source_target_pairs
Class diagram for refactored face_swapper helpers and globals constantsclassDiagram
class ModulesGlobals {
<<module>>
+float FACE_CONFIDENCE_THRESHOLD
+float DETECTION_INTERVAL
+int DETECTION_CACHE_SIZE
+int FPS_CAP
+int MOUTH_FEATHER_RADIUS
+Lock MAP_LOCK
+str target_path
+bool many_faces
+list source_target_map
+dict simple_map
}
class FaceSwapperModule {
<<module>>
+deque FRAME_CACHE
+dict FACE_DETECTION_CACHE
+float LAST_DETECTION_TIME
+int FRAME_SKIP_COUNTER
+bool ADAPTIVE_QUALITY
+get_faces_optimized(frame: Frame, use_cache: bool) List~Face~
+process_frame(source_face: Face, temp_frame: Frame) Frame
+process_frame_v2(temp_frame: Frame, temp_frame_path: str) Frame
+_build_pairs_from_file_map(temp_frame_path: str) list
+_build_pairs_live(processed_frame: Frame) list
}
class FaceDetectionHelpers {
<<functions>>
+get_many_faces(frame: Frame) list
+get_one_face(frame: Frame) Face
+find_closest_centroid(centroids: list, embedding: Any) tuple
+default_source_face() Face
+is_image(path: str) bool
+is_video(path: str) bool
}
ModulesGlobals <.. FaceSwapperModule : uses
FaceDetectionHelpers <.. FaceSwapperModule : calls
class get_faces_optimized {
+frame: Frame
+use_cache: bool
+return List~Face~
}
FaceSwapperModule o-- get_faces_optimized
class _build_pairs_from_file_map {
+temp_frame_path: str
+return list
}
class _build_pairs_live {
+processed_frame: Frame
+return list
}
FaceSwapperModule o-- _build_pairs_from_file_map
FaceSwapperModule o-- _build_pairs_live
class process_frame_v2 {
+temp_frame: Frame
+temp_frame_path: str
+return Frame
}
FaceSwapperModule o-- process_frame_v2
ModulesGlobals --> _build_pairs_from_file_map : provides_maps
ModulesGlobals --> _build_pairs_live : provides_maps
FaceDetectionHelpers --> _build_pairs_live : detection_and_matching
FaceDetectionHelpers --> _build_pairs_from_file_map : file_type_checks
File-Level Changes
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:
- In
_build_pairs_from_file_map, the image/video handling logic is almost duplicated between themany_facesand non-many_facesbranches; consider extracting the shared inner loops into a small helper to avoid divergence between the two paths over time. - In the live-stream fallback path, the previous implementation called
get_one_face(processed_frame, detected_faces)to reuse already-detected faces, while the new_build_pairs_livecallsget_one_face(processed_frame)without passingdetected_faces; it may be worth checking whether this reintroduces extra detection work or changes behavior and, if so, preserving the prior call pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_build_pairs_from_file_map`, the image/video handling logic is almost duplicated between the `many_faces` and non-`many_faces` branches; consider extracting the shared inner loops into a small helper to avoid divergence between the two paths over time.
- In the live-stream fallback path, the previous implementation called `get_one_face(processed_frame, detected_faces)` to reuse already-detected faces, while the new `_build_pairs_live` calls `get_one_face(processed_frame)` without passing `detected_faces`; it may be worth checking whether this reintroduces extra detection work or changes behavior and, if so, preserving the prior call pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…nstants - Extract _build_pairs_from_file_map and _build_pairs_live from process_frame_v2, reducing nesting from 7 levels to 1 - Remove local DETECTION_INTERVAL; use modules.globals.DETECTION_INTERVAL - Both helpers snapshot maps under MAP_LOCK before iterating Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
0a22b62 to
c754e76
Compare
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
1 participant
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.
Summary
process_frame_v2into focused helpers:_build_pairs_simple()— single-source mode (no map)_build_pairs_mapped()— map-faces mode (image/video)_build_pairs_live()— live/webcam mode with mapUSE_GLOBAL_CONSTANTflag for source face caching (from stacked PR refactor(globals): add named constants for magic numbers #1683)Behavioral note: the helpers return empty pairs lists instead of early-returning from the
parent function, but the effect is identical — no swaps are performed when no pairs are found.
Stacked on #1683 (adds
USE_GLOBAL_CONSTANTto globals).Test plan
Generated with Claude Code