Skip to content

fix: resolve 4 bugs found via code audit - #1753

Open
sjhddh wants to merge 1 commit into
hacksider:mainfrom
sjhddh:fix/multiple-bug-fixes
Open

fix: resolve 4 bugs found via code audit#1753
sjhddh wants to merge 1 commit into
hacksider:mainfrom
sjhddh:fix/multiple-bug-fixes

Conversation

@sjhddh

@sjhddh sjhddh commented Apr 12, 2026

Copy link
Copy Markdown

Summary

Fixes four bugs discovered during a code audit of the face swapper and UI modules:

1. Missing logging import (face_swapper.py)

  • logging.error() is called in pre_check() but logging was never imported
  • Causes NameError when model directory creation fails

2. Model filename mismatch (face_swapper.py)

  • pre_check() downloads inswapper_128.onnx (FP32)
  • get_face_swapper() loads inswapper_128.onnx (FP32)
  • But pre_start() checks for inswapper_128_fp16.onnx — a file that is never downloaded
  • This causes pre_start() to always fail with "Model not found"

3. VideoCapture resource leak (ui.py)

  • render_video_preview() only calls capture.release() on the failure path
  • When a frame is successfully read, the function returns without releasing the capture
  • Leaks file handles on every video preview render

4. get_one_face() called with wrong arity (face_swapper.py)

  • Line 530 passes two arguments (processed_frame, detected_faces) but get_one_face() only accepts one
  • Causes TypeError at runtime in the map_faces fallback path
  • Fixed by selecting from the already-detected faces directly, which also avoids redundant face detection

Test plan

  • Verify pre_check() handles directory creation errors without NameError
  • Verify pre_start() finds the correct model file after pre_check() downloads it
  • Verify video preview doesn't leak file handles (check with lsof during preview)
  • Verify map_faces fallback path works without TypeError

🤖 Generated with Claude Code

Summary by Sourcery

Fix bugs in the face swapper model handling and face selection logic, and ensure video preview resources are correctly released.

Bug Fixes:

  • Ensure face swapper startup checks for the same ONNX model file that is downloaded during pre-check.
  • Fix the face swap fallback path by selecting from already-detected faces instead of calling a helper with an invalid signature.
  • Release video capture resources on successful video preview rendering to avoid leaking file handles.

@sourcery-ai

sourcery-ai Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Fixes four audited bugs in the face swapper and UI modules: corrects the face swapper model path and a wrong-arity face selection call, and ensures proper logging import and video capture cleanup in the UI preview flow.

Sequence diagram for face swapper model pre_check and pre_start flow

sequenceDiagram
    participant App
    participant FaceSwapper as face_swapper_module
    participant FileSystem

    App->>FaceSwapper: pre_check()
    FaceSwapper->>FileSystem: ensure models_dir exists
    alt models_dir creation fails
        FileSystem-->>FaceSwapper: OSError
        FaceSwapper->>FaceSwapper: logging.error(error)
        FaceSwapper-->>App: False
    else models_dir ok
        FileSystem-->>FaceSwapper: ok
        FaceSwapper-->>App: True
    end

    App->>FaceSwapper: pre_start()
    FaceSwapper->>FaceSwapper: model_path = join(models_dir, inswapper_128.onnx)
    FaceSwapper->>FileSystem: exists(model_path)
    alt model file exists
        FileSystem-->>FaceSwapper: True
        FaceSwapper-->>App: True
    else model file missing
        FileSystem-->>FaceSwapper: False
        FaceSwapper->>FaceSwapper: update_status(Model not found: inswapper_128.onnx)
        FaceSwapper-->>App: False
    end
Loading

Sequence diagram for render_video_preview VideoCapture lifecycle

sequenceDiagram
    actor User
    participant UI as ui_module
    participant CV2 as cv2_VideoCapture

    User->>UI: render_video_preview(video_path, size)
    UI->>CV2: VideoCapture(video_path)
    UI->>CV2: read()
    alt frame read successful
        CV2-->>UI: ret=True, frame
        UI->>UI: gpu_cvt_color(frame)
        UI->>UI: Image.fromarray(...)
        alt size provided
            UI->>UI: ImageOps.fit(image, size, LANCZOS)
        end
        UI->>CV2: release()
        UI-->>User: CTkImage
    else frame read failed
        CV2-->>UI: ret=False, None
        UI->>CV2: release()
        UI->>UI: cv2.destroyAllWindows()
        UI-->>User: None or error image
    end
Loading

File-Level Changes

Change Details Files
Align face swapper pre_start() model path with the model actually downloaded in pre_check().
  • Change pre_start() to look for inswapper_128.onnx instead of the non-downloaded inswapper_128_fp16.onnx
  • Keep failure messaging and existence check logic the same so behavior only changes in which file is required
modules/processors/frame/face_swapper.py
Fix incorrect use of get_one_face() in the map_faces fallback path by reusing already-detected faces directly.
  • Replace the call to get_one_face(processed_frame, detected_faces) with a direct selection of a target face from detected_faces using the smallest x-coordinate (left-most face)
  • Guard face selection with a truthy detected_faces check and only append source/target pairs when both faces are present
modules/processors/frame/face_swapper.py
Ensure video preview releases VideoCapture on both success and failure paths to avoid resource leaks.
  • Call capture.release() immediately before returning the successfully created CTkImage in render_video_preview()
  • Keep the existing release and cv2.destroyAllWindows() calls on the failure path so all paths clean up resources
modules/ui.py

Possibly linked issues

  • #(not specified): PR fixes the inswapper filename mismatch in face_swapper.py that causes the model load failure described in issue.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • In render_video_preview, consider wrapping cv2.VideoCapture usage in a context manager or a try/finally block so the capture is reliably released even if an exception occurs between opening the stream and returning the image.
  • The fallback face selection now uses min(detected_faces, key=lambda x: x.bbox[0]); if get_one_face encodes the canonical selection heuristic, it may be better to reuse or refactor that logic so face-selection behavior stays consistent in one place and does not rely on assumptions about bbox.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `render_video_preview`, consider wrapping `cv2.VideoCapture` usage in a context manager or a try/finally block so the capture is reliably released even if an exception occurs between opening the stream and returning the image.
- The fallback face selection now uses `min(detected_faces, key=lambda x: x.bbox[0])`; if `get_one_face` encodes the canonical selection heuristic, it may be better to reuse or refactor that logic so face-selection behavior stays consistent in one place and does not rely on assumptions about `bbox`.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

1. Add missing `import logging` in face_swapper.py (NameError on dir creation failure)
2. Fix model filename mismatch: pre_start() checked for inswapper_128_fp16.onnx but pre_check() downloads inswapper_128.onnx
3. Fix VideoCapture resource leak in ui.py render_video_preview() — release on success path
4. Fix get_one_face() wrong arity in map_faces fallback — use already-detected faces directly

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sjhddh
sjhddh force-pushed the fix/multiple-bug-fixes branch from 1c49cff to 2a80029 Compare June 3, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant