fix(engine): honor decoded sample count in Wave/MF ReadAudio at EOF by yuto-trd · Pull Request #1997 · b-editor/beutl

yuto-trd · 2026-06-23T16:17:42Z

Description

Unifies the MediaReader.ReadAudio contract across backends. The NAudio-backed readers (WaveReader, MFReader) discarded the float element count returned by ISampleProvider.Read and always returned a full requested-length Pcm with return true. Because Read never returns a negative value, the count >= 0 guard's false branch was effectively dead — so:

a read crossing end-of-stream reported success with a misleading full-length buffer, and
a read starting past EOF still succeeded instead of failing.

This PR makes both readers honor the actual decoded count: the provider's element count maps to frames via /2 (ToStereo() gives 2 floats per frame), so the Pcm is allocated at the decoded frame count and false is returned only when nothing was decoded. This matches the canonical FFmpegReaderProxy, which already builds its Pcm from the worker's reported sample count.

It also documents the contract:

MediaReader.ReadAudio — NumSamples is the actually-decoded count (may be < length near EOF), output is always stereo, false means nothing could be read, and callers must copy with Math.Min.
AudioStreamInfo.NumChannels — source metadata (display/thumbnails), intentionally differing from Pcm.NumChannels (always 2). No behavior change here per the chosen scope.

AVFReader's native path already zero-fills uncovered ranges (trailing silence), which satisfies the documented contract; reporting an actual short read there needs an extended C ABI + .dylib rebuild and is left as a follow-up (commented in code).

Affected areas

Beutl.Engine (rendering / scene / track)
Beutl.ProjectSystem (project / document persistence)
UI (Beutl.Editor, Beutl.Editor.Components, Beutl.Controls)
Beutl.Extensibility (plugin abstractions)
Beutl.NodeGraph (node editor)
Beutl.FFmpegIpc / Beutl.FFmpegWorker (media IPC boundary)
Beutl.Api (server API client)
Build / CI / docs only — also touches Beutl.Extensions.MediaFoundation and Beutl.Extensions.AVFoundation

Breaking changes

None. The canonical FFmpegReaderProxy already behaves this way, the only first-party consumer (SourceNode.Process) already copies with Math.Min(pcm.NumSamples, buffer.SampleCount) so a shorter Pcm is absorbed as trailing silence, and the change only shrinks an over-reported count to the truth. No public signature changes. Third-party MediaReader subclasses that return full-length zero-filled buffers remain valid under the documented <remarks> clause.

Test plan

tests/Beutl.UnitTests/Engine/Media/Wave/WaveReaderTests.cs (new) — cross-platform, drives a real WAV fixture written via NAudio.Wave.WaveFileWriter:
- in-range read → NumSamples == requested, samples match the written ramp
- read crossing EOF → NumSamples == actual (< requested)
- read starting past EOF → false, sound == null
Baseline-first: confirmed the EOF-crossing and past-EOF cases fail against unmodified code, then pass after the fix.
Regression: full Engine.Audio suite (200 tests) green; SourceNode/Composer unaffected.
MFReader: build-verified only (PlatformTarget=x64, not loadable on arm64 macOS); the fix is byte-for-byte symmetric with the unit-tested WaveReader change.

Fixed issues / References

Project board: b-editor/projects/9 ("Engine: review the audio read contract (fixed-length/EOF, channel count, A/V sync)")

Follow-ups

AVF native short-read reporting — extend beutl_avf_reader_read_audio C ABI with an out-sample-count, update the Swift readSamples + P/Invoke, rebuild the .dylib, add a macOS-gated test. Separate PR.
MF audio first-PTS / A-V sync — the review's third claim is MF-specific video/audio PTS alignment (MFDecoder._firstGapTimeStamp, video path only), out of scope for this contract task; leave to the existing MF follow-up item.
FFmpeg worker — confirmed no change needed; FFmpegReaderProxy is the reference. (Minor pre-existing note: ReadAudioChunked could spin if a non-final chunk short-reads — low priority.)

https://claude.ai/code/session_01UrSHLdLaod5Uih8VuoK4eJ

Summary by CodeRabbit

Documentation
- Added XML documentation clarifying audio channel handling and decode output specifications
- Added comments explaining native audio EOF behavior
Bug Fixes
- Improved audio frame counting accuracy in native decoders to properly handle end-of-stream scenarios
Tests
- Added comprehensive test suite for audio reading with bounds and EOF validation

The NAudio-backed readers (WaveReader, MFReader) discarded the float count returned by ISampleProvider.Read and always returned a full requested-length Pcm with `return true`. Because Read never returns a negative value, the `count >= 0` guard's false branch was effectively dead, so a read crossing end-of-stream reported success with a misleading full-length buffer, and a read starting past EOF still succeeded. Honor the actual decoded count instead: the provider's element count maps to frames via /2 (ToStereo gives 2 floats per frame), so allocate the Pcm at the decoded frame count and return false only when nothing was decoded. This matches the canonical FFmpegReaderProxy, which already builds its Pcm from the worker's reported sample count. Behavior-preserving for the only consumer: SourceNode.Process already copies with Math.Min(pcm.NumSamples, buffer.SampleCount), so a shorter Pcm is absorbed as trailing silence rather than over-read. No public signature changes. Also document the contract on MediaReader.ReadAudio and clarify that AudioStreamInfo.NumChannels (source metadata) intentionally differs from Pcm.NumChannels (always stereo). AVFReader's native zero-fill path already satisfies the documented contract; reporting an actual short read there needs an extended C ABI + dylib rebuild and is left as a follow-up. Refs: Project #9 "AI Review" item "Engine: review the audio read contract (fixed-length/EOF, channel count, A/V sync)" Claude-Session: https://claude.ai/code/session_01UrSHLdLaod5Uih8VuoK4eJ

coderabbitai · 2026-06-23T16:20:51Z

Warning

Review limit reached

@yuto-trd, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 9 minutes and 13 seconds. Learn how PR review limits work.

To continue reviewing without waiting, enable usage-based billing in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d001eb8a-709f-4993-bf8a-2a954b1f9c6e

📥 Commits

Reviewing files that changed from the base of the PR and between 741131e and 0405490.

📒 Files selected for processing (4)

src/Beutl.Engine/Media/Decoding/MediaReader.cs
src/Beutl.Engine/Media/Wave/WaveReader.cs
src/Beutl.Extensions.MediaFoundation/Decoding/MFReader.cs
tests/Beutl.UnitTests/Engine/Media/Wave/WaveReaderTests.cs

📝 Walkthrough

Walkthrough

Documents the ReadAudio stereo-output and EOF-truncation contract across AudioStreamInfo, MediaReader, and AVFReader. Fixes WaveReader.ReadAudio and MFReader.ReadAudioCore to derive the output PCM frame count from the actual bytes read by the sample provider instead of pre-computing it from length. Adds a WaveReaderTests suite covering in-range, EOF-crossing, and past-EOF read scenarios.

Changes

Audio Short-Read Contract

Layer / File(s)	Summary
Audio decode API contract documentation `src/Beutl.Engine/Media/Decoding/AudioStreamInfo.cs`, `src/Beutl.Engine/Media/Decoding/MediaReader.cs`, `src/Beutl.Extensions.AVFoundation/Decoding/AVFReader.cs`	XML docs added to `NumChannels` (source metadata, not buffer-size guidance) and `ReadAudio` (stereo output type, short-read semantics, silence contract); inline comment in `AVFReader.ReadAudio` explains native zero-fill behavior.
Short-read frame count fix in WaveReader and MFReader `src/Beutl.Engine/Media/Wave/WaveReader.cs`, `src/Beutl.Extensions.MediaFoundation/Decoding/MFReader.cs`	Both readers now allocate a `length * 2` float buffer, call `_provider.Read`, divide the returned element count by 2 to get actual frames, construct `Pcm<Stereo32BitFloat>` from that count, and return `false` when zero frames are read; the previous fixed-size pre-computation and `count >= 0` branching are removed.
WaveReader tests `tests/Beutl.UnitTests/Engine/Media/Wave/WaveReaderTests.cs`	New fixture generates a deterministic mono WAV; three tests assert correct `NumSamples` and stereo duplication for in-range reads, truncated `NumSamples` for EOF-crossing reads, and `false`/null output for past-EOF reads.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop, hop — the buffer knows
How many frames the sample shows!
No guessing lengths, we read what's there,
Short reads land softly, silence-fair.
The tests all pass, the WAV rings true —
One bunny cheers this tidy queue! 🎵

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: honoring decoded sample counts in Wave/MediaFoundation audio readers at end-of-file, which is the core behavioral change across multiple files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch yuto-trd/audio-read-contract

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

This PR aligns the MediaReader.ReadAudio behavior across backends by ensuring NAudio-based readers (WaveReader, MFReader) honor the actual decoded sample count near EOF instead of always returning a full requested-length Pcm. It also documents the intended contract and adds unit coverage for EOF edge cases.

Changes:

Update WaveReader and MFReader to size the returned Pcm from the provider’s decoded element count (frames = floats/2) and return false only when no frames were produced.
Document MediaReader.ReadAudio and clarify that AudioStreamInfo.NumChannels is source metadata and may differ from the decoded stereo output.
Add new WaveReader unit tests covering in-range reads, reads crossing EOF, and reads starting past EOF; annotate AVFoundation’s current zero-fill behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/Beutl.UnitTests/Engine/Media/Wave/WaveReaderTests.cs	Adds unit tests validating correct `ReadAudio` behavior at/near EOF for `WaveReader`.
src/Beutl.Engine/Media/Wave/WaveReader.cs	Fixes `WaveReader.ReadAudio` to respect actual decoded frame count.
src/Beutl.Extensions.MediaFoundation/Decoding/MFReader.cs	Applies the same decoded-frame-count fix to the Media Foundation reader.
src/Beutl.Extensions.AVFoundation/Decoding/AVFReader.cs	Documents AVF’s current zero-fill-at-EOF behavior relative to the contract.
src/Beutl.Engine/Media/Decoding/MediaReader.cs	Adds XML docs defining the `ReadAudio` contract for callers/backends.
src/Beutl.Engine/Media/Decoding/AudioStreamInfo.cs	Documents that `NumChannels` reflects source metadata, not the decoded stereo output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Address Copilot review on PR #1997. The EOF fix made Wave/MF return false for length == 0 (frames == 0), but the canonical FFmpeg/AVF backends — and the previous Wave/MF behavior — treat a zero-length request as a successful no-op returning an empty stereo Pcm. SoundSource can pass length == 0 via TimeSpan conversions, so special-case it before the short-read check to keep the contract consistent across backends. Also reword the ReadAudio docs: NumSamples is "the number present in the returned buffer" (not "actually decoded", which was inconsistent with the allowed zero-filled-tail shape), and the return contract now documents that length == 0 yields an empty buffer with true. Claude-Session: https://claude.ai/code/session_01UrSHLdLaod5Uih8VuoK4eJ

github-actions · 2026-06-23T16:30:26Z

No TODO comments were found.

github-actions · 2026-06-23T16:34:45Z

Package	Line Rate	Branch Rate	Complexity	Health
Beutl.Api	9%	4%	1171	➖
Beutl.Configuration	41%	22%	350	➖
Beutl.Controls	0%	0%	5513	➖
Beutl.Core	62%	56%	3069	➖
Beutl.Editor	78%	70%	1742	➖
Beutl.Editor.Components	1%	0%	8552	➖
Beutl.Embedding.MediaFoundation	6%	7%	1378	➖
Beutl.Engine	63%	52%	18118	➖
Beutl.Engine.SourceGenerators	59%	44%	540	➖
Beutl.Extensibility	38%	59%	112	➖
Beutl.Extensions.AVFoundation	5%	12%	246	➖
Beutl.Extensions.FFmpeg	5%	5%	860	➖
Beutl.FFmpegIpc	22%	27%	799	➖
Beutl.Language	7%	50%	1348	➖
Beutl.NodeGraph	24%	15%	2474	➖
Beutl.ProjectSystem	58%	43%	1061	➖
Beutl.Threading	100%	90%	137	✔
Beutl.Utilities	94%	87%	358	✔
Summary	35% (43350 / 124598)	30% (10795 / 35488)	47828	➖

Minimum allowed line rate is 0%

Copilot AI review requested due to automatic review settings June 23, 2026 16:17

Copilot started reviewing on behalf of yuto-trd June 23, 2026 16:18 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread src/Beutl.Engine/Media/Decoding/MediaReader.cs

Comment thread src/Beutl.Engine/Media/Wave/WaveReader.cs

Comment thread src/Beutl.Extensions.MediaFoundation/Decoding/MFReader.cs

Comment thread src/Beutl.Engine/Media/Decoding/MediaReader.cs Outdated

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(engine): honor decoded sample count in Wave/MF ReadAudio at EOF#1997

fix(engine): honor decoded sample count in Wave/MF ReadAudio at EOF#1997
yuto-trd wants to merge 2 commits into
mainfrom
yuto-trd/audio-read-contract

yuto-trd commented Jun 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

yuto-trd commented Jun 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Affected areas

Breaking changes

Test plan

Fixed issues / References

Follow-ups

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yuto-trd commented Jun 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading