Fix binaural OBR crash when using oar_set_metadata_unit_to_process (#29)

[jingbo-marquis]

When oar_set_metadata_unit_to_process was called with a sub-frame size smaller than config.samples_per_channel for binaural rendering, OBR crashed because its internal buffer_size_per_channel (fixed at creation time) did not match the sub-frame input buffer size, triggering an ABSL_CHECK_EQ assertion failure in ObrImpl::Process.

Changes:

• Fix binaural OBR to always render the full frame using config.samples_per_channel, bypassing sub-frame position updates since OBR fixes buffer_size_per_channel at creation time (audio_elements_renderer.c)
• Add input validation in oar_set_metadata_unit_to_process to reject samples == 0 or samples > config.samples_per_channel (oar.c)
• Update API documentation to clarify that the samples parameter has no effect on binaural rendering (oar.h)
• Add test_metadata_unit_processing.c with 12 test cases covering binaural/stereo sub-frame rendering, invalid parameters, and crash isolation

Fixes #29

[yilun-zhangs]

LGTM

[trsonic]

Thanks @jingbo-marquis , I ran a review based on my context, and it seems that this PR introduces serious regression to the binaural object support path. The proposed tests are attached as a patch file (with txt extension) below the AI report.

Let me know what you think about this. Thanks.

Details below, roughly in order of severity.

Blocking

1. Binaural object positions no longer reach OBR

The new render() in src/renderer/audio_elements_renderer.c unconditionally calls lib->render and never applies queued position metadata. update_element_metadata still clones positions into ctx->rendering_metadata.positions, but the only consumer — audio_elements_renderer_sub_frames_apply_positions → lib->metadata_update — is no longer called, so obr_update_object_channel_position never fires. Objects stay at OBR's creation default (azimuth 0, elevation 0, distance 1.0), and metadatas_elapse ages the queued positions out unread. Any panning object that moved before this PR now renders frozen at front-center.

I'd suggest keeping the position-application core but calling it at frame granularity (one metadata_update per element before the full-frame render) rather than dropping it entirely. That keeps the fix for the sub-frame crash while preserving position rendering.

The test suite doesn't catch this because no binaural TC asserts a position effect — TC1/TC2/TC4/TC5/TC7 only assert non-silence, and a sine at default front-center is non-silent. TC12 (the only position-effect assertion) is stereo-only. A binaural analogue of TC12 — e.g. assert left/right energy asymmetry after a ±80° azimuth update — would have failed on this PR and would guard the fix going forward.

2. The #29 crash class is still reachable

add_data (around audio_elements_renderer.c:353-371) stores samples_per_channel from the first caller-supplied block without validating it against config.samples_per_channel — unlike audio_element_renderer.c:321-327, which rejects mismatched blocks. With config.samples_per_channel=960 and a 480-sample block, render passes 480 into obr_process, which hits ABSL_CHECK_EQ(480, 960) in ObrImpl::Process and aborts the host process. The comment on the new render() says it "always renders the full frame using config.samples_per_channel", but nothing enforces that. Mirroring audio_element_renderer's input validation (or returning ck_oar_error_inval from obr.c's _render) would close this off.

3. Heap buffer overflow in add_data on mismatched block sizes

Related, and pre-existing, but the fix now relies entirely on this function: the memcpy length uses the current block's samples_per_channel while the destination stride/capacity use the stored first-block size. Element A submits 480 samples (buffer sized channels_count*480), element B then submits 960 → the copy writes past the allocation. The realloc branch also resizes using the new block's size but never updates base.block.samples_per_channel, leaving stride and capacity inconsistent. Validating block sizes per point 2 would fix this too.

Tests / CI

• The new test binary is never executed. tests/examples/CMakeLists.txt builds test_metadata_unit_processing but there's no add_test, the cmake CI workflow's hardcoded smoke-test list (.github/workflows/ci-cmake.yml L43-46) isn't updated, and there's no cc_test in tests/examples/BUILD.bazel, so the Bazel //tests/examples/... wildcard doesn't pick it up either. As it stands a regression in oar_set_metadata_unit_to_process would pass CI green.
• Update calls' return values are ignored in the helpers (e.g. around test_metadata_unit_processing.c:116). If oar_update_audio_element_data starts failing, oar_render still returns 0 and is_output_non_silent's 1e-9f threshold can pass on uninitialized heap, so TCs report PASS with no audio delivered. Worth asserting on every API return.
• Windows child-spawn harness (test_metadata_unit_processing.c:873): the exe path passed to _spawnl is unquoted, so a path containing a space (e.g. C:\Program Files\...) breaks argv splitting — the child misses --child and recursively spawns the full suite. Separately, a test that returns -1 from child mode is indistinguishable from _spawnl's own -1 error return, so genuine failures get misreported as "spawn error". Quote the path and disambiguate via errno/a distinct sentinel exit code.

Nits

• audio_elements_renderer_sub_frames_apply_positions and audio_elements_renderer_private_object_based_elements_count become unreachable static functions after the render() change (-Wunused-function under -Wall, ~115 lines of dead logic). If the frame-granularity approach from point 1 is taken, most of this stays live; otherwise it should be deleted.
• add_object_element_without_position is a near-verbatim copy of add_object_element_and_data, differing only in the 7-line position block — a single helper with a flag would do. Both also fill only channel 0 of a buffer sized for input_channels, which becomes uninitialized-read territory if num_objects is ever bumped to the documented max of 2.
• NUM_TESTS derives from g_test_table, but g_test_names and the hand-written tc_results[0..11] summary block are coupled only by convention — appending a TC13 to one table without the others reads g_test_names out of bounds. Folding name + function into one table entry (and generating the summary from it) would make this safe.

Proposed test-suite extensions

The child-process isolation in run_test_in_child is a great fit for this bug class — a TC that hits an ABSL_CHECK abort shows up as CRASHED instead of killing the suite. Building on that, I'd propose the following additions. TC13–TC14 would currently fail against this PR (they pin the position regression from point 1), and TC15–TC16 would currently crash (they pin the still-open crash class from points 2–3) — so they'd land together with the corresponding fixes, and afterwards guard all of it.

TC13: binaural static position has an audible effect

Render a sine object at azimuth +80° in one oar_t instance and at −80° in a second instance, and assert the interaural asymmetry flips:

/* E_L / E_R computed as sum of |sample| per output channel */
assert(e_left_pos80 > e_right_pos80);   /* source on the left -> left ear louder */
assert(e_right_neg80 > e_left_neg80);   /* mirrored */

Comparing two renders against each other (rather than against an absolute ILD threshold) keeps it robust to HRIR details. This is the binaural analogue of TC12 and is the test that would have caught the regression in this PR: with positions never delivered to OBR, both instances render at front-center and both assertions fail.

TC14: binaural position update takes effect on the next frame

Render frame 1 with the object at +80°, call oar_update_audio_element_metadata with −80°, render frame 2, and assert the L/R energy balance flips between the frames. This pins the frame-granularity contract that the fixed render() should provide (one metadata_update per element before the full-frame render), independent of sub-frame processing — i.e. it verifies that position updates keep working across oar_render calls even though OBR's buffer_size_per_channel is fixed at creation.

TC15: input block smaller than config.samples_per_channel is rejected, not aborted

With config.samples_per_channel = 512, submit a 256-sample block via oar_update_audio_element_data and expect a nonzero error (e.g. ck_oar_error_inval) — mirroring the check audio_element_renderer.c already does — and then oar_render returning cleanly. Today this path reaches ABSL_CHECK_EQ in ObrImpl::Process and hard-aborts, which the harness would report as CRASHED; that's exactly the #29 crash class surviving through the data path, so this TC documents the intended contract and proves the fix.

TC16: mismatched block sizes across two elements are rejected

Add two object elements; submit a 256-sample block for element A, then a 512-sample block for element B, and expect the second update to be rejected. Today this is a heap buffer overflow in add_data (memcpy length from the new block, destination stride from the stored first-block size), so this TC only bites reliably under sanitizers — which is an argument for running the examples suite in an ASan job (see CI below).

Harness/CI retrofits (make the existing 12 TCs able to fail)

• Assert every API return value in the helpers (oar_update_audio_element_data, oar_update_audio_element_metadata). Right now a failing update still yields a PASS via the non-silence check on whatever the staging buffer contains.
• Actually run the suite in CI: add_test() in tests/examples/CMakeLists.txt (plus enable_testing() at the root if missing), add the binary to the smoke-test list in .github/workflows/ci-cmake.yml, and a cc_test in tests/examples/BUILD.bazel so the Bazel //tests/examples/... lane picks it up. An ASan variant (-fsanitize=address) of this suite would make TC16 deterministic.
• Merge the duplicated helpers into one add_object_element(oar, with_default_position, ...) and fill all input_channels (not just channel 0), so the helper stays valid if num_objects is ever raised.
• Fold names and functions into one table ({const char *name; test_fn_t fn;}) and generate the summary from a loop, so appending TC13+ can't desynchronize g_test_table, g_test_names, and the hand-written summary block.
• Windows spawn: quote the exe path passed to _spawnl and reserve a sentinel exit code for spawn failure so a test's own -1 isn't reported as "spawn error".

Patch with TC13–TC16, and what it shows

I've implemented TC13–TC16 on top of this PR — patch attached (GitHub renames attachments, so save it and apply with git am 0001-*.patch.txt or git apply). To run:

cmake -S . -B build -DOAR_BUILD_EXAMPLES=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build --parallel --target test_metadata_unit_processing
./build/tests/examples/test_metadata_unit_processing

Results with the patch applied on this PR's head vs. on current main (the same four helpers TC1–12 use, so main needs only the CMakeLists entry this PR already adds):

Test	main (pre-PR)	this PR
TC13 mirrored ±80°	PASS	FAIL — the two renders are bit-identical (diff rms = 0)
TC14 update between frames	PASS	FAIL — interaural balance never flips
TC15 undersized block	FAIL (block accepted)	CRASHED — SIGABRT in ObrImpl::Process
TC16 mismatched blocks	FAIL (both accepted)	FAIL (both accepted)
TC4/TC7 (this PR's #29 tests)	CRASHED (the original bug)	PASS

TC13/14 passing before this PR and failing after is the position regression from point 1 (and validates the tests' azimuth-sign expectations against the working path); TC15/16 failing on both sides shows the block-size validation gap from points 2–3 predates the PR and survives it. TC13's failure detail is worth quoting — the frozen front-center render still shows a ~2.6% interaural asymmetry from HRIR imperfection alone (E_L=126.6 vs E_R=123.3), which is why the primary assertion is "mirrored renders must differ" rather than an ILD threshold; ILD magnitude accuracy stays where it's already covered, in obr's own GetBroadbandILD unit tests.

Happy to open these as a follow-up PR instead if that's easier to pull from.
0001-Add-TC13-TC16-binaural-position-and-block-size-contr.patch.txt

[jingbo-marquis]

@trsonic
Thanks for the thorough review. All issues addressed in commits.

Changes

#	Issue	Fix
B1	Positions not reaching OBR	Added apply_frame_positions() — calls metadata_update at frame granularity before full-frame render
B2	#29 crash still reachable	add_data validates samples_per_channel; mismatched blocks rejected before reaching OBR
B3	Heap overflow	add_data adds channels validation; realloc branch confirmed required for multi-element growth, kept with clarifying comment
B4	Test not in CI	CMake add_test + Bazel cc_test
I3	API return values ignored	Merged duplicate helpers into add_object_element; all return values asserted; all channels filled
I5	Windows _spawnl path unquoted	argv[0] quoted + errno check to distinguish spawn failure from exit code 255
N1	Dead code	sub_frames_apply_positions deleted
N2	Test table coupling	Unified into single g_tests[] table; summary generated from loop
N3	Duplicate helper	Merged into add_object_element with add_position flag

New tests (TC13–TC17, all passing)

Test	Verifies
TC13	Mirrored ±80° renders differ + ILD follows azimuth sign
TC14	Position update takes effect on the next frame
TC15	Undersized block rejected
TC16	Mismatched block rejected
TC17	Multi-element + multi-object with independent positions