audio: use size_t for circular-buffer bytes counts - #11208
softwarecki wants to merge 3 commits into
Conversation
…lpers Change cir_buf_bytes_without_wrap() and cir_buf_bytes_without_wrap_rewind() to return size_t instead of int. Both now use uintptr_t for the pointer arithmetic. Correct doxygen comments of cir_buf_bytes_without_wrap(), cir_buf_wrap() and source_cir_buf_wrap(). Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
Change the frames parameter, return type, and the internal byte/frame/ sample counters of vol_zc_get_s16/s24/s32 (and the vol_zc_func typedef) to size_t, and make the channel count unsigned. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Signed pointer-distance arithmetic remains in circular-buffer helpers and can overflow on valid 32-bit address-boundary cases.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates circular-buffer byte accounting and volume zero-crossing interfaces to use unsigned, pointer-sized types, with related assertion and documentation fixes.
Changes:
- Uses
size_tanduintptr_tfor byte calculations. - Updates volume zero-crossing counters and callback types.
- Corrects pointer assertions and Doxygen documentation.
File summaries
| File | Summary |
|---|---|
src/include/sof/audio/audio_stream.h |
Updates stream pointer assertions and byte-distance handling. |
src/include/module/audio/audio_stream.h |
Updates circular-buffer helpers and documentation. |
src/audio/volume/volume.h |
Updates the zero-crossing callback signature. |
src/audio/volume/volume.c |
Updates zero-crossing counters and channel typing. |
Review details
Suppressed comments (3)
src/include/module/audio/audio_stream.h:205
cir_buf_wrap()is used for both source/read and sink/write pointers (for example, the output pointer insrc/audio/component.c:311), so describingptras a read pointer is inaccurate for this generic helper. Please document it as a read-or-write pointer, as the previous API wording did.
* Verifies the pointer and performs rollover when reaching the end of the circular buffer.
* @param ptr Read pointer that may have moved past the buffer end.
src/include/sof/audio/audio_stream.h:824
- The rewind assertion is now unsigned, but
to_beginis still computed by subtractingintptr_taddresses and storing the result inint. A valid range crossing0x80000000therefore passes the assertion while the subtraction can overflow and return an incorrect rewind distance. Use unsigned pointer-sized arithmetic and propagate a suitable non-negative type.
assert((uintptr_t)ptr >= (uintptr_t)source->addr);
int to_begin = (intptr_t)ptr - (intptr_t)source->addr;
return to_begin;
src/include/sof/audio/audio_stream.h:841
- These checks now accept addresses on either side of
0x80000000, but the rewind results below still convert the addresses tointptr_tbefore subtracting. A valid rewind that crosses that sign boundary can therefore invoke signed-overflow arithmetic and return an invalid pointer. Keep the pointer-distance calculations unsigned (or use pointer arithmetic) throughout this helper.
assert((uintptr_t)wptr >= (uintptr_t)source->addr);
assert((uintptr_t)source->end_addr > (uintptr_t)wptr);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| int remaining_samples = frames * channels; | ||
| size_t bytes, nmax, i, n; | ||
| unsigned int j; | ||
| size_t remaining_samples = frames * channels; |
There was a problem hiding this comment.
hm, would all modules now be expected to change remaining_samples in all their processing methods to size_t? it would be a rather big change
There was a problem hiding this comment.
Ultimately, I'd like all byte, sample, and frame counts to use size_t for consistency and some unsigned type for channels count. That said, I'm not planning a project-wide conversion right now.
I'm only introducing these changes in modules that are already being touched, such as during sink/source API conversions, or when they naturally come up as part of other work (for example, when updating helper definitions). Over time, this should allow us to move towards a consistent use of size_t without requiring a large-scale refactoring effort.
| int remaining_samples = frames * channels; | ||
| size_t bytes, nmax, i, n; | ||
| unsigned int j; | ||
| size_t remaining_samples = frames * channels; |
There was a problem hiding this comment.
in general I like this change, it is very close to what types I'd also use for channel counters and byte numbers (perhaps not for frame / sample counts though). But we have many modules that follow a roughly the same pattern - int for channel counts etc. Should all be changed now? If yes, maybe it should be a dedicated effort - establish types for all these common variables and change them throughout the code base
There was a problem hiding this comment.
For frame and sample counts, I also prefer size_t. For 8-bit formats, the byte count is identical to the sample count, so using the same type throughout feels natural.
I agree it would be beneficial to establish a set of preferred types and gradually converge towards them. However, I don't think this is important enough to justify a project-wide conversion right now. Also, I'm not a fan of introducing dedicated typedefs such as channels_no_t - I'd rather use standard types directly.
Switch the pointer bound-check assertions from intptr_t to uintptr_t casts. A signed comparison is incorrect for addresses crossing the 0x80000000 boundary and can silently invert the assert. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
f2fc0a1 to
279949f
Compare
Change
cir_buf_bytes_without_wrap()andcir_buf_bytes_without_wrap_rewind()to returnsize_tinstead ofint. Both now useuintptr_tfor the pointer arithmetic. Correct doxygen comments ofcir_buf_bytes_without_wrap(),cir_buf_wrap()andsource_cir_buf_wrap().In the volume module change the frames parameter, return type, and the internal byte/frame/sample counters of
vol_zc_get_s16/s24/s32(and thevol_zc_functypedef) tosize_t, and make the channel count unsigned.Switch the pointer bound-check assertions from
intptr_ttouintptr_tcasts. A signed comparison is incorrect for addresses crossing the 0x80000000 boundary and can silently invert the assert.