perf: merge packshot into single ffmpeg command with caching#131
Open
SecurityQQ wants to merge 1 commit intomainfrom
Open
perf: merge packshot into single ffmpeg command with caching#131SecurityQQ wants to merge 1 commit intomainfrom
SecurityQQ wants to merge 1 commit intomainfrom
Conversation
Reduces packshot rendering from 3 sequential Rendi round-trips (~77s) to 1 round-trip (~25-35s) by building a unified filter_complex that combines: - Background (fill-color or image with cover mode) - Logo overlay (scaled + positioned) - Title drawtext - Blinking CTA button animation (elastic scale/brightness pulse) - Final composite Key changes: - packshot.ts: New buildPackshotFilter() builds complete filter graph directly, bypassing editly(). Added deterministic cache via ctx.cache. - blinking-button.ts: Split into renderBlinkingButtonPngs() (Sharp only) and buildBlinkingButtonFilter() (returns filter_complex parts for merging). Legacy createBlinkingButton() preserved for backward compat. - layers.ts: Export escapeDrawText(), parseSize(), resolvePositionForOverlay() for reuse without duplicating editly logic. - New packshot.test.ts with 10 unit tests for helpers and filter builder. Resolves #128
Contributor
📝 WalkthroughWalkthroughrefactors packshot rendering from 3 sequential ffmpeg jobs to a unified single-pass filter graph, exports layer helpers for reuse, separates blinking-button concerns into pure png generation and filter-building functions, and adds caching to avoid redundant renders. Changes
Sequence Diagram(s)sequenceDiagram
participant old as old packshot flow
participant backend as ffmpeg backend
old->>backend: job 1: editly base (bg + title)
backend-->>old: base video
old->>backend: job 2: blinking button (pngs + filter)
backend-->>old: button video
old->>backend: job 3: overlay (composite)
backend-->>old: final output
note over old,backend: 3 sequential round-trips (~75s)
sequenceDiagram
participant new as new packshot flow
participant cache as cache layer
participant builder as filter builder
participant backend as ffmpeg backend
new->>cache: check cache key
alt cache hit
cache-->>new: cached output
else cache miss
new->>builder: compose unified filter_complex<br/>(bg + title + cta layers)
builder-->>new: single filter graph
new->>backend: single ffmpeg run
backend-->>new: final output
new->>cache: store result
end
note over new,backend: 1 round-trip (~25s) + zero intermediates
Estimated code review effort🎯 4 (complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/renderers/packshot.ts`:
- Around line 285-312: The drawtext block for the static CTA incorrectly uses
opts.staticCtaColor for fontcolor; change it to prefer a text-color prop (e.g.,
opts.staticCtaTextColor) and/or the shared cta text color (opts.ctaTextColor)
before falling back to "white" so text color aligns with the blinking CTA;
update the fontcolor expression in the drawtext filter (the code that builds the
drawtext string using opts.staticCtaText, opts.staticCtaColor, staticCtaPosition
and ctaOutLabel) to use opts.staticCtaTextColor ?? opts.ctaTextColor ?? "white"
instead of opts.staticCtaColor ?? "white".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ai-sdk/providers/editly/layers.tssrc/react/renderers/packshot.tssrc/react/renderers/packshot/blinking-button.tssrc/react/renderers/packshot/packshot.test.ts
Comment on lines
+285
to
+312
| if (opts.staticCtaText) { | ||
| const text = escapeDrawText(opts.staticCtaText); | ||
| const color = opts.staticCtaColor ?? "white"; | ||
|
|
||
| const maxFontSize = Math.round(Math.min(width, height) * 0.08); | ||
| const maxTextWidth = width * 0.9; | ||
| const fittedFontSize = Math.floor( | ||
| maxTextWidth / (opts.staticCtaText.length * 0.55), | ||
| ); | ||
| const fontSize = Math.max(16, Math.min(maxFontSize, fittedFontSize)); | ||
|
|
||
| let x = "(w-text_w)/2"; | ||
| let y = "(h-text_h)/2"; | ||
|
|
||
| const pos = resolvePosition(opts.staticCtaPosition ?? "bottom"); | ||
| if (typeof pos === "string") { | ||
| if (pos.includes("left")) x = "w*0.1"; | ||
| if (pos.includes("right")) x = "w*0.9-text_w"; | ||
| if (pos.includes("top")) y = "h*0.1"; | ||
| if (pos.includes("bottom")) y = "h*0.9-text_h"; | ||
| } | ||
|
|
||
| const ctaOutLabel = "cta_out"; | ||
| filters.push( | ||
| `[${currentLabel}]drawtext=text='${text}':fontsize=${fontSize}:fontcolor=${color}:x=${x}:y=${y}[${ctaOutLabel}]`, | ||
| ); | ||
| currentLabel = ctaOutLabel; | ||
| } |
Contributor
There was a problem hiding this comment.
static cta color might be using wrong prop
line 287 uses staticCtaColor ?? "white" as fontcolor, but looking at how it's called (lines 479-480), staticCtaColor gets props.ctaColor ?? "white".
however, ctaColor is the button background color (default #FF6B00 orange), not the text color. for the blinking version, ctaTextColor is used for text. shouldn't static cta use ctaTextColor for consistency?
suggested fix
- staticCtaColor:
- props.cta && !props.blinkCta ? (props.ctaColor ?? "white") : undefined,
+ staticCtaColor:
+ props.cta && !props.blinkCta ? (props.ctaTextColor ?? "#FFFFFF") : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/renderers/packshot.ts` around lines 285 - 312, The drawtext block
for the static CTA incorrectly uses opts.staticCtaColor for fontcolor; change it
to prefer a text-color prop (e.g., opts.staticCtaTextColor) and/or the shared
cta text color (opts.ctaTextColor) before falling back to "white" so text color
aligns with the blinking CTA; update the fontcolor expression in the drawtext
filter (the code that builds the drawtext string using opts.staticCtaText,
opts.staticCtaColor, staticCtaPosition and ctaOutLabel) to use
opts.staticCtaTextColor ?? opts.ctaTextColor ?? "white" instead of
opts.staticCtaColor ?? "white".
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
filter_complexctx.cache— identical packshots are instant on re-renderblinking-button.tsinto composable pieces:renderBlinkingButtonPngs()(Sharp only) +buildBlinkingButtonFilter()(filter graph parts)What changed
packshot.ts— Major rewritebuildPackshotFilter()constructs the complete filter graph directly, bypassingeditly()entirelybackend.run()call produces the final video (background + logo + title + blinking CTA in one command)computePackshotCacheKey()generates deterministic cache keys from all props + dimensionsctx.cacheas the rest of the SDKblinking-button.ts— Refactored into composable partsrenderBlinkingButtonPngs()— Sharp operations only (text + button SVG + glow), returns temp PNG pathsbuildBlinkingButtonFilter()— Returns filter_complex lines with configurable input indices for merging into a larger graphoscExpr,hexToRgb,even,getButtonYPosition, etc.)createBlinkingButton()preserved for backward compatlayers.ts— Minor exportsescapeDrawText(),parseSize(),resolvePositionForOverlay()now exported for reuseNew
packshot.test.tshexToRgb,even,oscExpr,getButtonYPosition) andbuildBlinkingButtonFilter()(structure, input indices, animation expressions, dimensions)Performance impact
Resolves #128