-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(media): bound the ffmpeg tool's child processes, inputs and scale targets #6989
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
f1726e1
fix(media): bound the ffmpeg tool's child processes, inputs and scale…
icecrasher321 627509f
fix(media): fit concat inside the shared area budget and stop input p…
icecrasher321 0dee35b
fix(media): floor the scaled concat axes so the area bound actually h…
icecrasher321 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** | ||
| * Execution bounds the ffmpeg tool enforces. | ||
| * | ||
| * These are mirrored into the Go tool catalog | ||
| * (`copilot/internal/tools/catalog/other/ffmpeg.go`), which is what lets the | ||
| * router reject an out-of-range argument structurally, before any storage read | ||
| * or child process. The model itself learns the limits from the parameter | ||
| * descriptions — copilot's `NormalizeToolParameters` drops every JSON Schema | ||
| * keyword outside its allowlist on the way to a provider, so the numbers are | ||
| * stated in prose there too. `ffmpeg-schema-parity.test.ts` fails when the two | ||
| * copies drift. | ||
| * | ||
| * `maxScalePixels` has no JSON Schema equivalent, so it lives in the parameter | ||
| * description on the Go side and is enforced here only. | ||
| */ | ||
| export const FFMPEG_LIMITS = { | ||
| /** Every input costs a full re-encode pass in `concat`, the only multi-input operation. */ | ||
| maxInputFiles: 20, | ||
| minScaleDimension: 16, | ||
| maxScaleDimension: 4096, | ||
| /** DCI 4K in either orientation — bounds the square frames the per-axis cap alone allows. */ | ||
| maxScalePixels: 4096 * 2304, | ||
| } as const |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { TOOL_RUNTIME_SCHEMAS } from '@/lib/copilot/generated/tool-schemas-v1' | ||
| import { FFMPEG_LIMITS } from '@/lib/media/ffmpeg-limits' | ||
|
|
||
| /** | ||
| * The ffmpeg bounds live twice: here, where the executor enforces them, and in | ||
| * the Go tool catalog, where they become the JSON Schema the model reads and | ||
| * Ajv checks at the router. Drift between the two is silent and user-visible — | ||
| * the model is told one ceiling and the transcode refuses at another — so pin | ||
| * the generated schema against the executor's own numbers. | ||
| * | ||
| * When this fails, change `ffmpeg.go` in the copilot repo and regenerate; do | ||
| * not edit the generated schema. | ||
| */ | ||
| interface SchemaNode { | ||
| properties?: Record<string, SchemaNode> | ||
| items?: SchemaNode | ||
| maxItems?: number | ||
| minimum?: number | ||
| maximum?: number | ||
| } | ||
|
|
||
| const ffmpegParameters = TOOL_RUNTIME_SCHEMAS.ffmpeg?.parameters as SchemaNode | undefined | ||
|
|
||
| describe('ffmpeg tool schema parity', () => { | ||
| it('declares the tool in the generated catalog', () => { | ||
| expect(ffmpegParameters?.properties).toBeDefined() | ||
| }) | ||
|
|
||
| it('caps inputs.files at the executor limit', () => { | ||
| expect(ffmpegParameters?.properties?.inputs?.properties?.files?.maxItems).toBe( | ||
| FFMPEG_LIMITS.maxInputFiles | ||
| ) | ||
| }) | ||
|
|
||
| it('bounds the scale dimensions at the executor limits', () => { | ||
| for (const axis of ['width', 'height'] as const) { | ||
| expect(ffmpegParameters?.properties?.[axis]?.minimum).toBe(FFMPEG_LIMITS.minScaleDimension) | ||
| expect(ffmpegParameters?.properties?.[axis]?.maximum).toBe(FFMPEG_LIMITS.maxScaleDimension) | ||
| } | ||
| }) | ||
|
|
||
| it('does not offer sandbox-only fields on a tool that runs in this process', () => { | ||
| const inputs = ffmpegParameters?.properties?.inputs?.properties | ||
| expect(Object.keys(inputs ?? {})).toEqual(['files']) | ||
| expect(inputs?.files?.items?.properties).toBeDefined() | ||
| expect(Object.keys(inputs?.files?.items?.properties ?? {})).toEqual(['path']) | ||
|
|
||
| const outputItem = ffmpegParameters?.properties?.outputs?.properties?.files?.items?.properties | ||
| expect(Object.keys(outputItem ?? {}).sort()).toEqual(['mimeType', 'mode', 'path']) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.