docs(video): refine spec with verified file-type detection and simplified converter
Verified exact file-type v22 magic-byte detection for all 5 formats against real fixture buffers (no normalizeFormat aliases needed), and simplified the converter's resolution-scaling gate to match audio.js's validation-boundary pattern (app.js range-checks, converter trusts the value). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,8 +31,6 @@ const CODEC_ARGS = {
|
|||||||
avi: ['-c:v', 'mpeg4', '-c:a', 'libmp3lame'],
|
avi: ['-c:v', 'mpeg4', '-c:a', 'libmp3lame'],
|
||||||
};
|
};
|
||||||
|
|
||||||
const RESOLUTION_HEIGHTS = [480, 720, 1080];
|
|
||||||
|
|
||||||
export function registerVideoConverters() {
|
export function registerVideoConverters() {
|
||||||
for (const sourceFormat of VIDEO_FORMATS) {
|
for (const sourceFormat of VIDEO_FORMATS) {
|
||||||
for (const targetFormat of VIDEO_FORMATS) {
|
for (const targetFormat of VIDEO_FORMATS) {
|
||||||
@@ -44,7 +42,7 @@ export function registerVideoConverters() {
|
|||||||
convert: async (inputPath, outputPath, { quality, timeoutMs } = {}) => {
|
convert: async (inputPath, outputPath, { quality, timeoutMs } = {}) => {
|
||||||
const ffmpegPath = process.env.FFMPEG_PATH || 'ffmpeg';
|
const ffmpegPath = process.env.FFMPEG_PATH || 'ffmpeg';
|
||||||
const args = ['-y', '-i', inputPath, ...CODEC_ARGS[targetFormat]];
|
const args = ['-y', '-i', inputPath, ...CODEC_ARGS[targetFormat]];
|
||||||
if (quality != null && RESOLUTION_HEIGHTS.includes(quality)) {
|
if (quality != null) {
|
||||||
args.push('-vf', `scale=-2:${quality}`);
|
args.push('-vf', `scale=-2:${quality}`);
|
||||||
}
|
}
|
||||||
args.push(outputPath);
|
args.push(outputPath);
|
||||||
@@ -56,7 +54,7 @@ export function registerVideoConverters() {
|
|||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
This mirrors `audio.js` exactly: a fixed per-target codec map, a flat `register` loop, `quality` passed straight through from the job row with no extra config threading.
|
This mirrors `audio.js`'s overall shape (fixed per-target codec map, a flat `register` loop, `quality` passed straight through from the job row with no extra config threading), with one simplification: `audio.js` gates its `-b:a` push on `BITRATE_CAPABLE_FORMATS.includes(targetFormat)` because bitrate doesn't apply to lossless targets (`wav`, `flac`); resolution scaling applies uniformly to every video target, so no analogous per-target gate is needed here — `quality != null` alone is sufficient, since `app.js`'s `isValidQuality` is what restricts the actual value set (`480`/`720`/`1080`) before the converter ever runs, same division of responsibility the audio family already uses (the converter trusts the value; `app.js` is the only place that range-checks it).
|
||||||
|
|
||||||
### Codec choice per target container (fixed, no user choice — confirmed with the user)
|
### Codec choice per target container (fixed, no user choice — confirmed with the user)
|
||||||
|
|
||||||
@@ -114,7 +112,17 @@ avi: 'video/x-msvideo',
|
|||||||
mkv: 'video/x-matroska',
|
mkv: 'video/x-matroska',
|
||||||
```
|
```
|
||||||
|
|
||||||
`resolveInputFormat`/`normalizeFormat`: relies on `file-type`'s existing magic-byte sniffing (no `UNDETECTABLE_*` fallback expected to be needed, since all five containers have well-defined signatures). The exact `ext` string `file-type` v22 returns for `.mov` and `.mkv` inputs is **not yet independently verified against real fixtures** — flagging this explicitly (same as the archive spec did for its own `file-type` behavior) so the implementation plan verifies it with real sample files during TDD, adding a `normalizeFormat` alias only if a mismatch is found (e.g. if `mov` sniffs as `qt` rather than `mov`).
|
`resolveInputFormat`/`normalizeFormat`: verified directly against the installed `file-type` v22 source (`source/index.js`, `source/detectors/ebml.js`) and confirmed with hand-crafted minimal fixture buffers run through `fileTypeFromBuffer`:
|
||||||
|
|
||||||
|
| format | detection | `ext` returned | `mime` returned |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `mp4` | `ftyp` box at offset 4 (ISO base media) | `mp4` | `video/mp4` |
|
||||||
|
| `mov` | `free`/`mdat`/`moov`/`wide` atom at offset 4, or `ftyp` with brand `qt` | `mov` | `video/quicktime` |
|
||||||
|
| `avi` | `RIFF` + `AVI ` at offset 8 | `avi` | `video/vnd.avi` |
|
||||||
|
| `webm` | EBML DocType `webm` | `webm` | `video/webm` |
|
||||||
|
| `mkv` | EBML DocType `matroska` | `mkv` | `video/matroska` |
|
||||||
|
|
||||||
|
All five `ext` values are identical to their declared format name, so **no `normalizeFormat` alias is needed** for any of them (unlike `tar.gz`/`tar.bz2`/`tar.7z` in the archive family). Note `file-type`'s detected `mime` for `avi` (`video/vnd.avi`) and `mkv` (`video/matroska`) differ from this feature's own `OUTPUT_MIME_TYPES` values (`video/x-msvideo`, `video/x-matroska`) — this is not a bug: the detected mime is only ever stored as `inputMimeType` (informational), while `OUTPUT_MIME_TYPES` independently drives the download `Content-Type` header; the two are never compared to each other anywhere in the codebase (confirmed by reading `app.js`/`worker.js`/`mime.js` — `resolveInputFormat`'s `valid` check compares `ext`, never `mime`).
|
||||||
|
|
||||||
## Registration wiring
|
## Registration wiring
|
||||||
|
|
||||||
@@ -134,9 +142,21 @@ No DB/schema changes anywhere in this feature.
|
|||||||
|
|
||||||
## Testing
|
## Testing
|
||||||
|
|
||||||
`test/converters/video.test.js`, same convention as `test/converters/audio.test.js`: small real video fixtures (need to source or generate tiny sample clips — e.g. generate a few-frame test clip with ffmpeg itself as a fixture-creation step, since there's no existing video fixture in the repo). One representative conversion test per target codec path (`mp4 -> webm`, `mp4 -> avi`, `webm -> mkv`, etc. — not all 20 pairs), plus a resolution-scaling test asserting the output's height matches the requested `480`/`720`/`1080` value (via `ffprobe` or a lightweight video-metadata read), plus a same-format-pair rejection test (`resolve('mp4', 'mp4')` returns `null`).
|
`test/converters/video.test.js`, same convention as `test/converters/audio.test.js` — confirmed by reading that file: it mocks `node:child_process`'s `execFile` entirely (`vi.mock('node:child_process', ...)`) and asserts on the exact args array passed, rather than invoking a real ffmpeg binary or using real media fixtures. `ffmpeg` is not installed on this dev machine (confirmed: `ffmpeg -version` → command not found), so this mocked approach is also the only one that works locally, not just the established convention. Tests: registration (all 20 pairs registered, all 5 same-format pairs return `null` via `resolve`), codec args per target (one assertion per row of the codec table), `-vf scale=-2:<height>` appended only when `quality` is one of `480`/`720`/`1080` and omitted otherwise, `FFMPEG_PATH` env var respected, `timeoutMs` forwarded, and a rejected `execFile` callback propagating as a rejected promise.
|
||||||
|
|
||||||
`test/mime.test.js`: cases for the 5 new MIME types, plus `resolveInputFormat` cases for `.mov`/`.mkv` fixtures (the two formats whose `file-type` `ext` output isn't yet independently confirmed).
|
`test/mime.test.js`: cases for the 5 new `OUTPUT_MIME_TYPES` entries, plus `resolveInputFormat` cases for all 5 formats using hand-crafted minimal magic-byte buffers (verified against the installed `file-type` v22 via a throwaway script — exact bytes, written directly in the test with `fs.writeFile` then deleted, same pattern the existing `mime.test.js` already uses for `.txt`/`.md`):
|
||||||
|
```js
|
||||||
|
// mp4: [box size][ftyp][isom]
|
||||||
|
Buffer.concat([Buffer.from([0, 0, 0, 0x18]), Buffer.from('ftyp'), Buffer.from('isom')])
|
||||||
|
// mov: [box size][moov]
|
||||||
|
Buffer.concat([Buffer.from([0, 0, 0, 0x08]), Buffer.from('moov')])
|
||||||
|
// avi: RIFF + size(4, arbitrary) + 'AVI '
|
||||||
|
Buffer.concat([Buffer.from('RIFF'), Buffer.from([0, 0, 0, 0]), Buffer.from('AVI ')])
|
||||||
|
// webm: EBML id(4) + len(1)=0x81 + DocType id(2)=0x42,0x82 + len(1)=0x84 + 'webm'
|
||||||
|
Buffer.concat([Buffer.from([0x1a, 0x45, 0xdf, 0xa3, 0x81, 0x42, 0x82, 0x84]), Buffer.from('webm')])
|
||||||
|
// mkv: same but DocType payload 'matroska' (8 bytes), len byte 0x88
|
||||||
|
Buffer.concat([Buffer.from([0x1a, 0x45, 0xdf, 0xa3, 0x81, 0x42, 0x82, 0x88]), Buffer.from('matroska')])
|
||||||
|
```
|
||||||
|
|
||||||
`test/api/jobs.test.js`: valid resolution values accepted, out-of-range resolution values rejected, `family: 'video'` recorded on the created job.
|
`test/api/jobs.test.js`: valid resolution values accepted, out-of-range resolution values rejected, `family: 'video'` recorded on the created job.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user