Repository navigation
test(engine): tone-map concurrency test passes on macOS - #6323
Merged
Merged
Conversation
On macOS the extractor checks once per process whether ffmpeg has zscale. The test ran that check through its counting shim, so it saw 11 ffmpeg runs instead of 8 cuts plus 2 probes and failed on every Mac with zscale. Take the check with the real ffmpeg before the shim counts.
Merged
3 of 4 tasks
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 0 important · 0 nits · head 4858ebd0
Test-only. isFfmpegFilterAvailable memoizes one process-wide filter list (not keyed by binary path), so warming it with the real ffmpeg before FFMPEG_PATH_ENV points at the shim keeps the -filters run out of the count on every platform. It never rejects, so a host without ffmpeg is unaffected. The count still catches a cache regression.
Scope
Read the diff, psnrFilterAvailability.ts and toneMapsWithZscale at head. CI was still running when I posted (CI, regression, CodeQL, Windows render verification); the required checks still gate the merge.
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
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.
What
videoFrameExtractor.concurrency.test.ts("finishes tone-mapped cuts of shared sources in the one slot, probing each source once") passes on macOS. It failed on cleanmainon every Mac whose ffmpeg haszscale, and is skipped or green on Linux CI.Why
The test counts every ffmpeg the extractor starts through a shim and expects 8 cuts plus one probe per source (10). On macOS, tone mapping first checks whether ffmpeg has
zscale(toneMapsWithZscale,packages/engine/src/services/videoFrameExtractor.ts:322-324). That check runsffmpeg -hide_banner -filtersonce per process and is cached (isFfmpegFilterAvailable,packages/engine/src/utils/psnrFilterAvailability.ts:33-38). In the test it ran through the shim, so the count was 11. On Linux the check is skipped, so CI never saw it.The extractor is right: the check is per process, not per cut, and it is not an extraction. The test now takes the check with the real ffmpeg before the shim starts counting, so the expectation stays 8 cuts plus 2 probes on every platform.
Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
On
mainon an Apple silicon Mac (its ffmpeg haszscale): fails 3 of 3 with 11 runs; the eleventh is-hide_banner -filters.With this change: the file passes 3 runs in a row.
Still guards the cache: with the filter check no longer memoized, the test fails with 18 runs.
Small on purpose: a lone test fix that makes local engine runs green on every Mac again; it shares nothing with any other open change.