aecdump updates and build alignment - #50
Merged
Merged
Conversation
The file input's accept filter only listed .pb and .aecdump, so .aecdump.binpb dumps were greyed out in the click-to-select dialog. Add both the compound extension and the plain .binpb suffix.
Picks up two upstream changes to Config: fields 6-8 (AECm) are now reserved rather than declared, and api_config_string (21) is added -- a stringified webrtc::AudioProcessing::Config that upstream documents as more likely to be up-to-date than the individual bool fields. Source: modules/audio_processing/debug.proto @ webrtc.googlesource.com/src. No decoder code reads Config yet, so this is generation-only for now.
protobufjs returns bytes fields as subarray views into the dump buffer at an arbitrary byteOffset, but Int16Array requires 2-byte alignment. Roughly half of int16 payloads therefore threw RangeError: start offset of Int16Array should be a multiple of 2 which escaped parseAecDump (the try/catch only covers Event.decode) and failed the whole file load. Copy to an aligned buffer when the offset is odd, mirroring what the float path already does. Also floor the frame count so a trailing partial frame drops a few samples instead of throwing on a fractional typed-array length. Verified against a synthetic int16 dump: 15 of 30 payloads land on odd offsets; the previous decoder throws, this one returns all three streams at the expected length with correct sample values.
audioBufferToWav downmixes sources with more than two channels to the first channel, but still wrote the source channel count into the header. The player then read mono samples as N-channel interleaved frames, so the track came out 1/N the length and N times too fast -- which on ChromeOS hardware with 3-4 mic channels looked like broken time alignment between tracks. Describe the data actually written instead. Verified with a 100 ms source: 3ch previously produced a 33.3 ms file and 4ch a 25.0 ms file; both now produce 100.0 ms mono. 1ch and 2ch output is unchanged.
Input and output were appended in independent branches, so any STREAM event carrying only one of them shifted that stream earlier by 10ms relative to the other, permanently, for the rest of the dump. Track the capture frame index and pad each stream with silence up to that frame's position before appending. Padding before the append (not after) also places a late-starting stream at its true offset instead of at t=0. A stream that never appears stays empty rather than becoming a full-length silent track. Frame size is sample_rate/100, matching unpack.cc. This addresses input-vs-output only. Reference-vs-capture alignment needs the shared-timeline work and is out of scope here. Verified on synthetic float dumps: with output missing from frames 2-4 the old decoder produced 7 frames of output against 10 of input; it now produces 10 with the audio at the right positions. With output starting at frame 5, its energy centroid moves from frame 2.54 to 7.54.
wavesurfer's drag path emits 'interaction' immediately but debounces the actual seekTo by 200ms while paused, so ws.getCurrentTime() inside the handler still returned the pre-drag position. Dragging one waveform therefore left the other tracks where they already were. The click path seeks before emitting, which is why only dragging was affected -- dragToSeek is enabled in the wavesurfer options. The event carries the correct target time (typed 'interaction: [newTime: number]'); use it. Verified in Chromium against a 30s synthetic dump: dragging the reference waveform to 70% previously left mic and out at 0s while ref moved to 21.01s; all three now land on 21.01s.
Adds tests/fixtures/make-dump.ts, which synthesizes aecdump files in the [int32 length][Event protobuf] framing that WebRTC's writer produces: int16 and deinterleaved-float variants, arbitrary rates and channel counts, and the ability to omit a stream from chosen capture frames. Payload sizes vary enough that roughly half the bytes fields land on an odd byteOffset, which is what exercises the decoder's alignment path. Also commits a 2s int16 fixture (~193KB) so browser tests can run without a generation step; regenerate with 'yarn fixtures:generate'. Output is byte-for-byte deterministic. Adds vitest for unit tests -- pinned to ^2 because vitest 4 requires Vite 6+ and would pull a second Vite alongside the project's Vite 5. Tests land in the next commit.
Unit tests (vitest, 'yarn test:unit'):
- decoder: unaligned int16 payloads, sample values through the
alignment copy, INIT format, input/output lockstep across dropped
frames, late-start positioning, streams that never appear, truncated
and empty input
- wav-helper: header must describe the data actually written, for 1-8
channels, plus the 16-bit and float32 paths
Browser tests (playwright, 'yarn test:dev'):
- track-sync.spec.ts: dragging and clicking a waveform move every
track to that position, using the 2s fixture
Checked that these fail without the fixes: reverting decoder.ts and
wav-helper.ts to 23e5556 fails 11 of the 18 unit tests, and restoring
the old 'interaction' handler fails the drag test while the click test
still passes -- the click path seeks before emitting, so only dragging
was ever broken.
Scopes playwright's testMatch to *.spec.ts so it no longer picks up the
vitest files, which its default testMatch would have matched.
'yarn test' ran playwright only, so the vitest suite was skipped by the default entry point. Split the granular scripts out (test:unit, test:e2e) and make 'test' run both, unit first so it fails fast. Wiring them up surfaced two problems: Playwright reuses whatever already answers on the preview port, so a foreign service on 8080 means the browser suite silently tests the wrong server -- here a local git server, which returned 403 for every asset. Make the port overridable via PREVIEW_PORT and pass --strictPort so a collision fails immediately instead of drifting to another port. 'tsc' in the build step emits to out-tsc/ and now compiles tests/ too, so vitest's path filter matched both the sources and their compiled copies and ran everything twice. Scope discovery with --dir. CI does not run either suite today (build only); wiring it up is a separate decision.
Padding runs before an append, so it needs a following chunk to trigger it. A stream whose last frames were missing therefore ended short: with output absent from frames 17-19 of a 20-frame dump, output came back 2720 samples against input's 3200. Unlike a mid-dump gap this misplaces no audio -- the stream simply stops early, and nothing follows it to be shifted. It shows up in rendering, where each track's duration is stretched to the container width, so a short tail skews that track's whole x-axis. Pad both streams to the capture timeline length after the parse loop. A stream that never appeared stays empty. This also makes the invariant total rather than partial: input and output are each exactly captureFrameCount frames, or empty -- which is what Phase 1's positioned tracks will assume. Reported in review; verified the two new tests fail without the change.
Adds MODULE.bazel on bzlmod -- Bazel 9 removed WORKSPACE entirely -- pinned to 9.2.0 via .bazelversion, plus REPO.bazel ignoring **/node_modules so package manager installs do not churn the build graph. //:site assembles the published tree with copy_to_directory, replacing the hardcoded cp list in the workflow. The five static demos each expose a filegroup. The two JavaScript subprojects follow in later commits. Verified byte-identical to the current CI output for the 34 static files: replicated the workflow's staging into a baseline tree and diffed it against bazel-bin/site with no differences. Also adds a root .gitignore for Bazel's convenience symlinks; the repo had none.
Adds aspect_rules_js and an npm_translate_lock for aecdump-viewer, plus npm_link_all_packages so //src/aecdump-viewer:node_modules is a Bazel-managed tree. Verified vite, vitest, lit, protobufjs, protobufjs-cli, typescript, wavesurfer.js and @playwright all link. rules_js reads pnpm-lock.yaml, so one is derived from yarn.lock with 'pnpm import' and committed alongside it. yarn.lock remains the developer-facing lockfile. Feeding yarn_lock to npm_translate_lock directly would enable update_pnpm_lock and let the repository rule write into the source tree during builds; reading a committed lock keeps fetches read-only, at the cost of regenerating it by hand when yarn.lock changes. See the follow-up note below. pnpm-workspace.yaml declares allowBuilds for esbuild and protobufjs, the only two dependencies with install lifecycle scripts. rules_js requires this to be explicit so nothing runs code at install time undeclared. Not yet wired: the proto codegen and vite bundle targets, and a guard against yarn.lock and pnpm-lock.yaml drifting apart.
rules_js resolves npm dependencies through pnpm regardless of which lockfile is
committed, so keeping yarn.lock alongside the generated pnpm-lock.yaml would
mean two lockfiles that can silently drift. Drop yarn.lock and let pnpm-lock.yaml
be the single source of truth; it was produced by 'pnpm import' from yarn.lock,
so resolved versions carry over unchanged.
Pins pnpm@10.34.5 via packageManager, switches the package.json scripts,
playwright.config.ts and CI to pnpm, and keeps CI working until Bazel takes over
the build in a later step.
Fixes an undeclared dependency this surfaced: the generated proto bindings do
'import Long = require("long")', but 'long' was never declared -- it only
resolved because yarn hoisted protobufjs's copy to the top level. pnpm's strict
layout does not, so the build failed with TS2307. Declared it explicitly.
Verified the pnpm-built bundle is byte-identical to the yarn-built baseline
(same content hash, index-C2bCgrdw.js), 20 unit tests pass, and Bazel still
links the tree including long.
aspect_rules_js pulls in aspect_tools_telemetry, which is enabled by default. It shells out to curl from a repository rule and POSTs to https://telemetry.aspect.build/ingest. The payload includes an organization name, a hash of the repository, and a salted hash of the username -- the local report.json already had the repo and user hashes populated after the first build. A public Google repository should not be sending build metadata to a third party without an explicit decision, so opt out in .bazelrc. --repo_env is required because the value is read by a repository rule. Note the lockfile only records ENV:ASPECT_TOOLS_TELEMETRY_TEST as a tracked variable, so the change does not necessarily invalidate an already-fetched repo; verified by forcing a re-fetch, after which report.json is {} with no payload envelope. The module also honours DO_NOT_TRACK. Also corrects the MODULE.bazel comment, which still described yarn.lock as the developer-facing lockfile after it had been deleted.
pbjs and pbts are modelled as two dependent actions, since pbts reads the JavaScript pbjs emits, and both mirror the package.json proto:generate script via chdir. The vite bundle declares the generated bindings as inputs. Verified against the current toolchain: debug.js and debug.d.ts are byte-identical to the npm script's output, and dist/ is byte-identical to the CI baseline down to the content hash (assets/index-C2bCgrdw.js). Removes the generated proto files from the working tree; they were gitignored build output that would collide with Bazel's declared outputs. Running 'pnpm run build' recreates them and will break 'bazel build' until the authoritative-build-path follow-up lands.
Applies the same decision taken for aecdump-viewer. rules_js resolves through pnpm regardless of which lockfile is committed, so keeping package-lock.json would mean either two lockfiles that can drift or a repository rule writing into the source tree during builds. pnpm-lock.yaml was produced by 'pnpm import' from package-lock.json, so resolved versions carry over unchanged. Pins pnpm@10.34.5 via packageManager and switches CI. Verified the pnpm-built dist is byte-identical to the npm-built baseline. pnpm-workspace.yaml allows install scripts for core-js-bundle and esbuild. husky is deliberately excluded: its hook only configures local git state, which is developer-machine setup rather than a build input. This leaves both subprojects on one package manager; the repo no longer mixes npm and Yarn.
Adds an npm_translate_lock and BUILD targets for ds-playground: tsc emits
out-tsc/, which index.html references and rollup bundles into dist/. The npm
script's rimraf is unnecessary in Bazel's sandbox, and cem analyze is dropped
per the plan -- it writes custom-elements.json to the project root, never to
dist/, so it has never been part of the published site.
Wires both bundles into //:site.
*** bazel build //:site is now byte-identical to the current CI output:
38 files, diff -r clean. This is the step 2 acceptance gate. ***
Two things the sandbox exposed that the old build masked:
- @material/web's .d.ts files import lit-html while declaring only lit, so its
types do not resolve under pnpm's strict layout. Hoisted that one package with
public_hoist_packages rather than declaring another package's missing
dependency in our own manifest.
- rollup-plugin-esbuild reads tsconfig.json to choose its transform target.
It was not declared as an input, so esbuild silently picked a different
default and downlevelled differently, changing the bundle by 52 bytes. tsc
output was identical throughout; only the bundle differed.
'bazel test //...' now runs both: the 20 vitest unit tests, and tsc as a typecheck-only gate. Both are test targets rather than build targets, since 'bazel test //...' does not select plain build targets. The typecheck uses --noEmit, which the package.json build does not: it runs tsc purely as a gate but still writes JavaScript nobody consumes into out-tsc/ -- the same stray output that made vitest run every test twice during Phase 0. Hoists @types/node for aecdump-viewer. undici-types, a transitive dependency of @types/node pulled in by vitest, carries /// <reference types="node" /> which does not resolve under pnpm's strict layout; the local typecheck does not hit this, so it is a layout artifact rather than a real type error. //:site remains byte-identical to the CI baseline.
@types/node was a genuine undeclared direct dependency, not the layout artifact the previous commit described. Our own TypeScript imports node builtins -- node:url and node:path in tests/track-sync.spec.ts, node:fs in tests/fixtures/generate.ts -- so the types are required directly and were only resolving transitively through vitest. Declared as a dev dependency at ^22 to match the Node the toolchain actually provides (v22.22.0), rather than the transitive @types/node@25.9.1 that was typing a Node 22 runtime against Node 25 APIs. With it declared, the public_hoist_packages workaround for undici-types is unnecessary and is removed; the typecheck passes without it. Also narrows the unit target, which declared every TypeScript file under tests/ including the Playwright specs. Verified: changing an e2e spec now reruns only the typecheck, whose tsconfig legitimately covers all TypeScript, while the unit target stays cached. //:site remains byte-identical to the CI baseline.
Completes step 4. 'bazel test //...' now runs all three suites: unit, typecheck, and the browser tests. Pins @playwright/test to exactly 1.56.0 in package.json, not just the lock, so a future lock regeneration cannot silently cross the 1.57 Chrome-for-Testing boundary that rules_playwright 0.5.3 cannot generate URLs for. browsers.json is vendored from playwright-core so the browser revision (1194) is a committed input rather than an unhashed unpkg fetch. **rules_playwright 0.5.3 works on Bazel 9**, which its BCR presubmit does not cover -- this was the acceptance check for the whole approach, so the 1.50.1 fallback is not needed. Replaces the package-manager script in playwright.config.ts with a plain Node preview server, so the tests serve the bundle Bazel declared rather than whatever dist/ happens to be in the source tree, and work in a test action where no package manager exists. Three integration problems, each diagnosed by bisecting against a direct CLI run in the same directory: - rules_js's Node fs patch and Playwright's module loader hooks each resolve a spec by a different path, yielding two TestType instances and 'did not expect test.describe() to be called here'. Fixed with patch_node_fs = False. - Runfiles link each file individually and Playwright's scanner does not follow symlinked spec files, so collection found nothing. Fixed by materializing the specs and fixtures as one copy_to_directory artifact, whose contents are real files. - Bazel's sandbox stages files the same way, so discovery fails there too. The target is tagged no-sandbox, as upstream's own example does for its browser tests. Also notes untidy state found along the way: after the downgrade, stale 1.60.0 links survived in bazel-bin alongside 1.56.0 and a 'bazel clean' was needed to clear them. They did NOT cause the failure -- the error was unchanged after the clean, and only patch_node_fs = False altered it. The lockfile never referenced 1.60.0. //:site remains byte-identical to the CI baseline.
The e2e target runs unsandboxed, so Node resolves packages through runfiles symlinks into the shared bazel-bin tree and can read files that are not declared inputs. Bazel was memoising the verdict as though the inputs were complete: a result influenced by ambient output-tree state could be reused indefinitely. Tag the target external, which forces unconditional execution. Note that 'no-cache' does NOT do this, which is worth recording because it is the obvious-looking choice. Bazel has two distinct layers: action-output caching, which no-cache governs, and test-result caching, which produces the '(cached) PASSED' annotation and is governed by --cache_test_results and the external tag. Measured: with +no-cache the second run still reported '(cached) PASSED'. So did +external injected via --modify_execution_info, because tags like external are resolved at analysis time and never consult execution info -- injecting an execution requirement is not equivalent to setting a tag. Verified: three consecutive runs of the e2e target all execute, while unit and typecheck stay cached, so the change is scoped to the one unsound target. Costs 3.5s per invocation. //:site remains byte-identical. This is a mitigation, not a fix. It removes the memoisation hazard; it does not make the inputs complete. Staging specs into TEST_TMPDIR so the sandbox can be re-enabled is the next step, and the tag should stay until Node no longer canonicalises package symlinks out of the sandbox.
Replaces the per-subproject install and build steps with 'bazel test //...' and 'bazel build //:site'. Bazel supplies the Node toolchain, npm dependencies and Playwright browsers, so there is no setup-node, package-manager install or playwright install step -- which was the point of the migration. The tests now run on pull requests; previously they ran nowhere. setup-bazel's caches all default to false, so they are enabled explicitly. The upload and gh-pages deploy jobs are unchanged. Only the way out/ is produced changes: bazel-bin is a symlink the artifact upload does not follow, so the tree is staged with cp -RL. Bazel's outputs are mode 555 and the deploy job commits this tree, so modes are normalised to 644/755 -- without that every deployed file would flip to executable and the first deploy would diff the whole tree. Verified by running the exact sequence locally: the staged artifact is 38 files, byte-identical to the pre-migration CI output, with file modes matching too.
Replaces the cp -RL plus two chmod passes with 'bazel run //:stage_site', using write_source_files from aspect_bazel_lib -- already a dependency, so no new module. It is used here as a directory-copy primitive rather than for its usual round-tripping purpose, so diff_test and the destination-exists check are off: out/ is build output, not something checked in. Three measured improvements over the shell it replaces: - Writes 644/755 directly, so the post-hoc chmod normalisation is gone. The deploy job commits this tree, and Bazel's own outputs are 555, so that compensation was load-bearing -- now it is simply not needed. - Does not reach into bazel-bin, a convenience symlink that --symlink_prefix can move or suppress. - Removes files that are no longer part of the site. cp left them; verified by planting a stale file and re-running. Considered rules_pkg's pkg_install for the same job. write_source_files wins on handling directory outputs natively -- both bundles are tree artifacts, which is the fiddly part of pkg_files -- and on adding no dependency. Verified end to end: the staged artifact is 38 files, content and modes identical to the pre-migration CI output.
Pins all four action references to commit SHAs, keeping the version as a trailing comment so the intent stays readable and Dependabot can still update them. This was the reported failure: unpinned-uses is a mandatory check under a blanket pin-to-hash policy. Fixing only that would have left CI red, because the gate also fails on any Medium finding and three more were present, all pre-existing: - artipacked: actions/checkout persists the GITHUB_TOKEN into .git/config, which is reachable from build output and uploaded artifacts. The build job never pushes and the deploy job supplies its own token, so persist-credentials: false costs nothing. - excessive-permissions (x2): the workflow declared no permissions, so both jobs ran with broad defaults. Deny at the top level and opt in per job -- contents: read for build, and deploy keeps the contents: write it already had. Verified with zizmor 1.29.0 locally: 'No findings to report', exit 0, where the same run previously reported four errors plus three warnings. The build pipeline is unaffected -- tests pass and the staged artifact is still byte-identical. Note actions/checkout is pinned at v2, which is old enough to be worth bumping on its own merits; left alone here to keep this change to the security fix.
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.
No description provided.