feat(pcm)!: align one-shot decode with the sibling io.Reader contract - #55
Conversation
Fold three related changes into one PR. 1. One-shot decode contract (issue #50). The sibling pcm packages (go-flac, go-aac, go-opus) share DecodeInterleaved(io.Reader) returning ([]byte, StreamInfo, error), plus DecodeInterleavedLimit, ErrDecodeLimit and DefaultMaxDecodedBytes, so a caller can dispatch decode across codecs on one signature. go-wav diverged: its one-shot took a []byte and returned (StreamInfo, []byte, error). The byte-slice zero-copy fast path is kept, renamed to DecodeInterleavedBytes and reordered to samples-first for package consistency. New DecodeInterleaved(io.Reader) and DecodeInterleavedLimit(io.Reader, maxBytes) are added, bounding the in-memory decode at DefaultMaxDecodedBytes (1 GiB) with a wrapped ErrDecodeLimit, mirroring go-flac. BREAKING CHANGE: the exported DecodeInterleaved signature and return order change. The old byte-slice behaviour is available under DecodeInterleavedBytes. Every un-updated caller is a compile error (the swapped return types cannot be assigned), never a silent bug. 2. bext free-text CR/LF (issue #54). Field recorders pack CRLF-separated key=value metadata into the bext Description, which the read path returns verbatim but the write path rejected, so a decoded recorder bext could not be re-encoded. Description, Originator and OriginatorReference now accept CR and LF, matching CodingHistory; every other control byte, and anything outside ASCII, is still refused, so fixed-width field boundaries cannot be corrupted. 3. Add codecov.yml making project and patch coverage informational, matching the sibling repos, so small diffs with defensive error branches do not fail the advisory patch check. Closes #50, #54.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🟡 Changes recommended
A couple of newly introduced messages/comments are misleading (decode-limit error op name and bext string-field documentation), and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns go-wav/pcm’s one-shot decode API with sibling codec packages by introducing an io.Reader-based DecodeInterleaved (plus a bounded DecodeInterleavedLimit), while keeping the existing zero-copy byte-slice fast path under DecodeInterleavedBytes. It also relaxes bext free-text validation to allow CR/LF for recorder interoperability and adds a Codecov configuration to make coverage checks informational.
Changes:
- Add
DecodeInterleaved(io.Reader)andDecodeInterleavedLimit(io.Reader, maxBytes)withDefaultMaxDecodedBytesandErrDecodeLimit. - Rename the old byte-slice one-shot decode to
DecodeInterleavedBytes([]byte)and align return order to([]byte, StreamInfo, error). - Allow CR/LF in bext free-text fields for field-recorder round-trip, and add Codecov informational coverage config.
File summaries
| File | Description |
|---|---|
| README.md | Updates docs for new one-shot decode entrypoints and bext CR/LF behavior. |
| pcm/fuzz_test.go | Switches fuzz coverage to DecodeInterleavedBytes and updates messages/ordering. |
| pcm/example_test.go | Updates the existing example and adds a new reader-based decode example. |
| pcm/encoder_test.go | Adjusts tests for the renamed/swap-ordered byte-slice decode helper. |
| pcm/doc.go | Updates package docs to describe the new reader-based contract and the byte-slice fast path. |
| pcm/decode_oneshot.go | Implements DecodeInterleaved, DecodeInterleavedLimit, decode-size limiting, and renames old API to DecodeInterleavedBytes. |
| pcm/decode_oneshot_test.go | Updates one-shot decode tests to target DecodeInterleavedBytes and new return order. |
| pcm/decode_oneshot_reader_test.go | Adds new tests covering reader-based one-shot decode parity and decode limits. |
| pcm/companded_test.go | Updates companded tests for the renamed/swap-ordered byte-slice decode helper. |
| pcm/bext.go | Relaxes bext free-text validation to accept CR/LF and updates comments/validation helpers. |
| pcm/bext_test.go | Extends validation tests for CR/LF acceptance and round-trip behavior. |
| pcm/bext_decoder_test.go | Adds round-trip test for recorder-style CRLF Description through decode/re-encode. |
| pcm/bench_test.go | Renames/updates benchmarks and adds a reader-based benchmark. |
| codecov.yml | Marks project and patch coverage statuses informational. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Every string field below is written as ASCII that also admits CR and LF (see | ||
| // [Bext.Description]). The three fixed-width fields are NUL-padded to their wire | ||
| // width, and Config.validate rejects a value that does not fit rather than | ||
| // truncating it; CodingHistory is the exception, the variable-length tail | ||
| // appended raw after the fixed body. UMID is binary and written verbatim; the |
There was a problem hiding this comment.
Fixed in b782f0e. Narrowed the wording to the free-text fields: OriginationDate and OriginationTime are string fields but take the fixed date/time forms checkDateTime enforces, so they do not accept CR or LF.
| if c.max > 0 && c.n > c.max-len(p) { | ||
| return 0, fmt.Errorf( | ||
| "go-wav/pcm: DecodeInterleaved: %w: output would exceed %d bytes", | ||
| ErrDecodeLimit, c.max) | ||
| } |
There was a problem hiding this comment.
Leaving this one as is. The cappedWriter is shared by DecodeInterleaved and DecodeInterleavedLimit (the former delegates to the latter), and it deliberately mirrors the same message the sibling packages go-flac and go-aac emit from their identical shared writer, which is the family consistency this PR is about. The wrapped ErrDecodeLimit sentinel is preserved, so errors.Is matches regardless of entry point. If the shared label is worth changing I'd rather do it across all three packages at once than diverge here.
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (14)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe PR adds reader-based one-shot PCM decoding with configurable output limits, renames the byte-slice decoder, updates callers, and permits CR/LF in Bext text fields with recorder round-trip coverage. ChangesPCM decoding and Bext interoperability
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The reader-based decode API, output limits, byte-slice migration, and Bext CR/LF interoperability changes are ready to merge. Sequence Diagram(s)sequenceDiagram
participant Caller
participant DecodeInterleaved
participant Decoder
participant cappedWriter
Caller->>DecodeInterleaved: pass io.Reader and optional limit
DecodeInterleaved->>Decoder: decode stream blocks
Decoder->>cappedWriter: write decoded samples
cappedWriter-->>DecodeInterleaved: enforce max decoded bytes
DecodeInterleaved-->>Caller: return samples, StreamInfo, error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
OriginationDate and OriginationTime are string fields but take the fixed date/time forms checkDateTime enforces, so they do not admit CR and LF; the previous wording implied every string field does. Addresses a PR review note.
Summary
Three related changes folded into one PR.
One-shot decode contract (#50). The sibling pcm packages (go-flac, go-aac, go-opus) share
DecodeInterleaved(io.Reader) ([]byte, StreamInfo, error)plusDecodeInterleavedLimit,ErrDecodeLimitandDefaultMaxDecodedBytes, so a caller can dispatch decode across codecs on one signature. go-wav diverged: its one-shot took a[]byteand returned(StreamInfo, []byte, error). This adds the shared contract and keeps the byte-slice zero-copy fast path under a new name.DecodeInterleaved(r io.Reader, opts ...Option) ([]byte, wav.StreamInfo, error)is new and matches the siblings. It reads the whole stream into memory, bounds it atDefaultMaxDecodedBytes(1 GiB), and returns a wrappedErrDecodeLimitpast that.DecodeInterleavedLimit(r io.Reader, maxBytes int, opts ...Option)is new and takes a caller-chosen ceiling;maxBytes <= 0is unbounded.DecodeInterleavedBytes(b []byte, opts ...Option) ([]byte, wav.StreamInfo, error)is the formerDecodeInterleaved: the zero-copy fast path, kept, with its return order swapped to samples-first so the package is internally consistent.Breaking change. The exported
DecodeInterleavedchanges signature and return order. The old byte-slice behaviour lives on underDecodeInterleavedBytes. Every un-updated caller becomes a compile error, because the swapped return types ([]bytevswav.StreamInfo) cannot be assigned, so the break is loud rather than a silent runtime bug. This lands on av1.0.0-tagged module, so it needs a release decision: either a/v2module path (the semver-correct route) or an explicitly documented pre-adoption break, givenv1.0.0is only days old and has no known dependents. No in-repo caller relies on the old surface.bext free-text CR/LF (#54). Field recorders pack CRLF-separated
key=valuemetadata into the bextDescription. The read path returns those bytes verbatim, but the write path rejected them, so a decoded recorder bext could not be re-encoded.Description,OriginatorandOriginatorReferencenow accept CR and LF, matchingCodingHistory. Every other control byte, and anything outside ASCII, is still refused, so fixed-width field boundaries cannot be corrupted on round-trip.codecov.yml. Adds a
codecov.ymlmarking project and patch coverage informational, matching the sibling repos, so small diffs with defensive error branches do not fail the advisory patch check.Related Issues
Closes #50
Closes #54
Test Plan
go test -race ./...green; package coverage 97.0%go vet ./...,golangci-lint run ./..., andgofmtcleanFuzzDecode,FuzzParseHeader) greenWithConvertTo;ErrDecodeLimitsingle-block and multi-block boundaries; unbounded andWithIgnoreLengthpaths; non-seekable readerSummary by CodeRabbit
New Features
bexttext fields while continuing to reject other invalid characters.bextvalues during decoding and re-encoding.Documentation