-
-
Notifications
You must be signed in to change notification settings - Fork 10
Let a caller hand in the face map, and stop a lost crop looking deliberate #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,27 @@ def _parse_json_transcript(raw_text): | |
| return data.get("words", []), data.get("segments", []), data | ||
|
|
||
|
|
||
| def _json_file_arg(raw: str | None, name: str): | ||
| """JSON given inline or named as a file, or nothing. | ||
|
|
||
| Both, because the two callers differ: keyframes are a handful of numbers | ||
| and have always been passed inline, while a face map is a per-second record | ||
| of every face in the video, which an argument list will not carry. Missing | ||
| or malformed loses the framing hint, never the run. | ||
| """ | ||
| if not raw: | ||
| return None | ||
| text = raw.strip() | ||
| try: | ||
| if text.startswith("{") or text.startswith("["): | ||
| return json.loads(text) | ||
| with open(text, "r", encoding="utf-8") as handle: | ||
| return json.load(handle) | ||
| except (OSError, ValueError) as exc: | ||
| print(f" Warning: {name} could not be read ({exc}); ignoring it", file=sys.stderr) | ||
| return None | ||
|
|
||
|
|
||
| def _cached_face_map(video_path: str): | ||
| """Face maps are keyed by video content, not by transcript, so an imported | ||
| transcript can still borrow the map from an earlier run on the same file.""" | ||
|
|
@@ -470,6 +491,22 @@ def cmd_studio(args): | |
| "--caption-scale", str(getattr(args, "caption_scale", 1.0)), | ||
| "--crop", args.crop, | ||
| "--format", getattr(args, "format", None) or "vertical", | ||
| ] | ||
| # Where the faces are, and where somebody put the frame by hand. Both are | ||
| # what the crop needs and neither could be handed to it before, so a caller | ||
| # that already knew — the cloud worker scans the window before it renders — | ||
| # had no way to say so and watched `speaker` fall through to a letterbox. | ||
| # A path is made absolute because the script runs from elsewhere; JSON | ||
| # given inline is handed straight through. | ||
| def _pass_through(value): | ||
| text = str(value).strip() | ||
| return text if text.startswith(("{", "[")) else os.path.abspath(text) | ||
|
|
||
| if getattr(args, "face_map", None): | ||
| cmd += ["--face-map", _pass_through(args.face_map)] | ||
| if getattr(args, "crop_keyframes", None): | ||
| cmd += ["--crop-keyframes", _pass_through(args.crop_keyframes)] | ||
| cmd += [ | ||
| "--logo-position", getattr(args, "logo_position", "top-left"), | ||
| "--logo-scale", str(getattr(args, "logo_scale", 1.0)), | ||
| "--intro-seconds", str(args.intro_seconds), | ||
|
|
@@ -979,6 +1016,45 @@ def _transcribe_progress(pct, msg): | |
| # Extract face_map before result gets overwritten in clip loop | ||
| face_map = result.get("face_map") | ||
|
|
||
| # A map handed in on the command line wins over one the transcriber found, | ||
| # because the caller who bothered to pass it scanned this video rather than | ||
| # inferring the layout from who was speaking. It is also the only map there | ||
| # is when the transcript came from an engine that does not diarize, which is | ||
| # the case where `speaker` framing used to fall through to a letterbox. | ||
| given = _json_file_arg(getattr(args, "face_map", None), "--face-map") | ||
| if given: | ||
| face_map = given | ||
| print(" Using the face map passed in (speaker framing preserved)") | ||
|
|
||
| # Still nothing, and the crop about to run needs one. Scanning costs a | ||
| # minute on a long episode and buys back every clip in it: without a map, | ||
| # `speaker` and `face` skip every rung that could place a frame and land on | ||
| # the whole wide source letterboxed into the cut. | ||
| if not face_map and config.get("crop_strategy") in ("face", "speaker", "speaker-hardcut"): | ||
| try: | ||
| from services.face_analysis import analyze_faces | ||
| print(" No face map yet; scanning the episode for faces") | ||
| # The last word's end, because the scan only uses this to decide | ||
| # how many frames to sample; it reads the real frame count off the | ||
| # file itself and spreads the samples across all of it. | ||
| spoken = max((float(w.get("end") or 0) for w in words), default=0.0) | ||
| found = analyze_faces( | ||
| video_path, | ||
| [{"speaker": w["speaker"], "start": w["start"], "end": w["end"]} | ||
| for w in words if w.get("speaker")], | ||
| spoken, | ||
| progress_callback=lambda p, m: None, | ||
| ) | ||
| except Exception as exc: | ||
| print(f" Face scan failed ({type(exc).__name__}: {exc}); " | ||
| "the crop will fall back", file=sys.stderr) | ||
| found = None | ||
| if found and found.get("clusters"): | ||
| face_map = found | ||
| print(f" Found {len(found['clusters'])} face position(s)") | ||
| else: | ||
| print(" No faces found; the crop will fall back") | ||
|
|
||
| # Check speaker data availability (needed for smart cropping) | ||
| speakers_in_words = set(w.get("speaker") for w in words if w.get("speaker")) | ||
| diarization_warning = result.get("diarization_warning") | ||
|
|
@@ -4399,7 +4475,12 @@ def main(): | |
| help="Caption placement (default: follows the chosen style)") | ||
| proc.add_argument("--caption-scale", type=float, choices=[0.7, 0.75, 0.8, 0.85, 0.9, 0.95, 1.0, 1.05, 1.1, 1.15, 1.2, 1.25, 1.3, 1.35, 1.4, 1.45, 1.5], | ||
| help="Caption size multiplier (default: 1)") | ||
| proc.add_argument("--crop", choices=["center", "face", "speaker", "speaker-hardcut"]) | ||
| proc.add_argument("--crop", choices=["center", "face", "speaker", "speaker-hardcut", "manual"]) | ||
| proc.add_argument("--face-map", dest="face_map", default=None, | ||
| help="Where the faces sit in this video, as JSON. Skips detection and lets " | ||
| "speaker framing work on a transcript that carries no speaker labels.") | ||
| proc.add_argument("--crop-keyframes", dest="crop_keyframes", default=None, | ||
| help="Hand-placed crop positions, as JSON. Used by --crop manual.") | ||
|
Comment on lines
+4449
to
+4454
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Update the custom help for the new crop inputs.
🤖 Prompt for AI Agents
Comment on lines
+4453
to
+4454
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Forward The parser accepts this option, but 🤖 Prompt for AI Agents |
||
| proc.add_argument("--format", choices=["vertical", "horizontal", "square"], help="Output aspect ratio (default: vertical)") | ||
| proc.add_argument("--profile", choices=["podcast", "party", "action"], help="Detection profile: podcast (transcript-first, default), party/action (laughter/energy highlights)") | ||
| proc.add_argument("--logo", help="Logo image (asset name or path)") | ||
|
|
@@ -4485,7 +4566,12 @@ def main(): | |
| studio.add_argument("--caption-style", choices=["hormozi", "karaoke", "subtle", "branded"], default="hormozi") | ||
| studio.add_argument("--caption-position", choices=["auto", "upper", "center", "lower"], default="auto") | ||
| studio.add_argument("--caption-scale", type=float, default=1.0) | ||
| studio.add_argument("--crop", choices=["center", "face", "speaker", "speaker-hardcut"], default="face") | ||
| studio.add_argument("--crop", choices=["center", "face", "speaker", "speaker-hardcut", "manual"], default="face") | ||
| studio.add_argument("--face-map", dest="face_map", default=None, | ||
| help="Where the faces sit in this video, as JSON. Skips detection and lets " | ||
| "speaker framing work on a transcript that carries no speaker labels.") | ||
| studio.add_argument("--crop-keyframes", dest="crop_keyframes", default=None, | ||
| help="Hand-placed crop positions, as JSON. Used by --crop manual.") | ||
| studio.add_argument("--format", choices=["vertical", "horizontal", "square"], default="vertical", | ||
| help="Output aspect ratio (default: vertical)") | ||
| studio.add_argument("--template", help="Cut in a saved look (podcli Pro). Name or id.") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,11 +161,82 @@ def _json_arg(raw, name): | |
| return None | ||
|
|
||
|
|
||
| def _json_file(raw, name): | ||
| """JSON given inline or named as a file, or nothing. | ||
|
|
||
| Both, because the two callers differ: keyframes are a handful of numbers | ||
| and have always been passed inline, while a face map is a per-second record | ||
| of every face in the video and an argument list will not carry one. Telling | ||
| them apart by looking is cheaper than a second flag. | ||
| """ | ||
| if not raw: | ||
| return None | ||
| text = raw.strip() | ||
| if text.startswith("{") or text.startswith("["): | ||
| return _json_arg(text, name) | ||
| try: | ||
| with open(text, "r", encoding="utf-8") as handle: | ||
| return json.load(handle) | ||
| except (OSError, ValueError) as exc: | ||
| print(f" Warning: {name} could not be read ({exc}); ignoring it", | ||
| file=sys.stderr, flush=True) | ||
| return None | ||
|
|
||
|
|
||
| # The crops that cannot place a frame without knowing where the faces are. | ||
| _WANTS_FACES = ("face", "speaker", "speaker-hardcut") | ||
|
|
||
|
|
||
| def _face_map_for(video, crop, start, end): | ||
| """Where the faces are, scanned now, when the crop needs it and nobody said. | ||
|
|
||
| A face map used to arrive only from the diarizing transcriber, so a cut | ||
| made against a whisper transcript or an imported one had none, and every | ||
| rung of the crop ladder that needs one was skipped. What the caller got | ||
| instead was the whole wide frame letterboxed into the cut, reported as a | ||
| centre crop and looking deliberate. | ||
|
|
||
| Scanning here costs a few seconds on the window actually being cut, which | ||
| is the trade every one of those clips would have taken. | ||
| """ | ||
| if crop not in _WANTS_FACES: | ||
| return None | ||
| try: | ||
| from services.face_analysis import analyze_faces | ||
| except ImportError: | ||
| return None | ||
|
|
||
| # The whole file's length rather than the fragment's: the scan spreads its | ||
| # samples across the file it is given, and sizing the count to a 40-second | ||
| # window would take twenty samples across an hour-long episode. | ||
| duration = _probe_duration(video) or max(0.0, (end or 0) - (start or 0)) | ||
| if duration <= 0: | ||
| return None | ||
|
|
||
| print(" [fragment] no face map for this cut; scanning it", flush=True) | ||
| try: | ||
| # No speaker segments: those come from diarization, and the whole point | ||
| # of scanning here is that there was none. Clusters and the split | ||
| # screen are still found, which is what a crop needs to place a frame. | ||
| found = analyze_faces(video, [], duration) | ||
| except Exception as exc: | ||
| print(f" Warning: face scan failed ({type(exc).__name__}: {exc}); " | ||
| "the crop will fall back", file=sys.stderr, flush=True) | ||
| return None | ||
|
|
||
| if not found or not found.get("clusters"): | ||
| print(" [fragment] no faces found in this cut", flush=True) | ||
| return None | ||
| print(f" [fragment] found {len(found['clusters'])} face position(s)" | ||
| f"{' , split screen' if found.get('is_split_screen') else ''}", flush=True) | ||
| return found | ||
|
|
||
|
|
||
| def _render_fragment(video, start, end, words, style, crop, title, out_dir, fmt="vertical", | ||
| logo=None, name_card=None, motion=None, caption_position="auto", | ||
| caption_scale=1.0, logo_position="top-left", logo_scale=1.0, | ||
| topic=None, progress=None, cards=None, brand=None, font_family=None, | ||
| captions=True): | ||
| captions=True, face_map=None, crop_keyframes=None): | ||
| """Render the fragment with face-crop + captions via the existing engine.""" | ||
| from services.clip_generator import generate_clip | ||
| print(f" [fragment] rendering {start:.1f}s–{end:.1f}s ({style}, crop={crop}, {fmt})", flush=True) | ||
|
|
@@ -175,6 +246,7 @@ def _render_fragment(video, start, end, words, style, crop, title, out_dir, fmt= | |
| caption_font_scale=round(caption_scale * 100), | ||
| logo_position=logo_position, logo_scale=logo_scale, | ||
| crop_strategy=crop, format=fmt, | ||
| face_map=face_map, crop_keyframes=crop_keyframes, | ||
| transcript_words=words, title=title, output_dir=out_dir, | ||
| logo_path=logo, name_card=name_card, motion=motion, | ||
| topic=topic, progress=progress, cards=cards, brand=brand, font_family=font_family, | ||
|
|
@@ -296,6 +368,11 @@ def main(): | |
| ap.add_argument("--progress", action="store_true", | ||
| help="Draw how much of the clip is left along the bottom edge") | ||
| ap.add_argument("--progress-color", default=None) | ||
| ap.add_argument("--face-map", dest="face_map", default=None, | ||
| help="Path to a JSON face map for this video. Lets speaker framing work " | ||
| "on a transcript that carries no speaker labels.") | ||
| ap.add_argument("--crop-keyframes", dest="crop_keyframes", default=None, | ||
| help="Path to hand-placed crop positions as JSON. Used by --crop manual.") | ||
|
Comment on lines
+325
to
+326
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Add
🤖 Prompt for AI Agents |
||
| ap.add_argument("--cards", default=None, | ||
| help="On-screen cards as JSON, each with kind/start/end") | ||
| ap.add_argument("--brand", default=None, | ||
|
|
@@ -391,6 +468,10 @@ def media_path(value: str | None, kind: str) -> str | None: | |
| print(" Warning: --motion is not valid JSON; using each style's own motion", | ||
| flush=True) | ||
|
|
||
| face_map = _json_file(args.face_map, "--face-map") | ||
| if face_map is None: | ||
| face_map = _face_map_for(video, args.crop, start, end) | ||
|
|
||
| fragment = _render_fragment( | ||
| video, start, end, words, args.caption_style, args.crop, "fragment", out_dir, | ||
| fmt=args.format, | ||
|
|
@@ -410,6 +491,8 @@ def media_path(value: str | None, kind: str) -> str | None: | |
| brand=_json_arg(args.brand, "--brand"), | ||
| font_family=args.font_family, | ||
| captions=not args.no_captions, | ||
| face_map=face_map, | ||
| crop_keyframes=_json_file(args.crop_keyframes, "--crop-keyframes"), | ||
| ) | ||
|
|
||
| platforms = [p.strip() for p in platforms_str.split(",") if p.strip()] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge nmbrthirteen/podcli /tmp/coderabbit-repo-knowledge/nmbrthirteen-podcli-1e36e803Length of output: 1444
🏁 Script executed:
Repository: nmbrthirteen/podcli
Length of output: 22784
🏁 Script executed:
Repository: nmbrthirteen/podcli
Length of output: 23091
🏁 Script executed:
Repository: nmbrthirteen/podcli
Length of output: 7198
Validate JSON shapes in both input helpers.
A truthy list or scalar passed as
--face-mapreaches_sane_speaker_mappings, which calls.getand raisesAttributeError. Require a JSON object inbackend/cli.pyandbackend/clip_studio.py.A non-list or malformed entry passed as
--crop-keyframesreaches_manual_crop_x_expr, which indexesk["t"]andk["x_pct"]and can raise. Require a list of objects with the required keyframe fields inbackend/clip_studio.py.🧰 Tools
🪛 ast-grep (0.45.2)
[warning] 100-100: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(text, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
📍 Affects 2 files
backend/cli.py#L99-L102(this comment)backend/clip_studio.py#L175-L179🤖 Prompt for AI Agents
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate face-map JSON at both CLI boundaries.
Both helpers accept any syntactically valid JSON. A truthy non-object reaches
_sane_speaker_mappings(), which calls.get()and can abort rendering withAttributeError.backend/cli.py#L99-L102: require--face-mapto decode to a JSON object before assigningface_map.backend/clip_studio.py#L175-L179: apply the same validation before forwardingface_maptogenerate_clip().🧰 Tools
🪛 ast-grep (0.45.2)
[warning] 100-100: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(text, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
📍 Affects 2 files
backend/cli.py#L99-L102(this comment)backend/clip_studio.py#L175-L179🤖 Prompt for AI Agents