optimize unnecessary copy #573
Conversation
theomonnom
commented
Feb 10, 2026
theomonnom
commented
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if isinstance(data, memoryview): | ||
| data = data.obj # type: ignore[assignment] |
There was a problem hiding this comment.
🔴 AudioFrame.init using memoryview.obj loses slice offset and length information
When a sliced memoryview is passed to AudioFrame.__init__, data.obj returns the full underlying buffer object, discarding the slice's offset and length. This causes the frame to reference the wrong region of memory.
Root Cause and Impact
The old code did data = memoryview(data).cast("B") which preserved slice information — a sliced memoryview would remain sliced. The new code at line 53 does data = data.obj which always returns the original underlying buffer object, regardless of any slicing.
For example:
ba = bytearray(1000)
mv = memoryview(ba)[100:200] # 100 bytes starting at offset 100
# Old: memoryview(mv).cast("B") → len=100, points to ba[100:200] ✓
# New: mv.obj → ba itself, len=1000, points to ba[0:1000] ✗This causes two problems:
- Length validation is wrong:
len(data)returns the full buffer length, so the size check at line 58 may pass when the actual slice is too small. - Wrong memory address in FFI calls:
_proto_info()ataudio_frame.py:103callsget_address(memoryview(self._data))which returns the address of the start of the full buffer, not the slice offset. The FFI layer then reads/writes the wrong memory region.
Impact: Silent data corruption or out-of-bounds memory access when AudioFrame is constructed from a sliced memoryview.
Prompt for agents
In audio_frame.py lines 52-53, instead of unconditionally unwrapping memoryview.obj, you need to preserve the data view correctly. The safest approach is to convert the memoryview to bytes or bytearray while preserving the slice boundaries. For example, replace:
if isinstance(data, memoryview):
data = data.obj
with something like:
if isinstance(data, memoryview):
if data.readonly:
data = bytes(data)
else:
data = bytearray(data)
Alternatively, if the goal is zero-copy, you could check whether the memoryview is contiguous and non-sliced (data.contiguous and data.nbytes == len(data.obj)) before using data.obj, and fall back to copying otherwise. But this adds complexity. The simplest correct fix is to only use data.obj when the memoryview covers the entire underlying object.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| n = len(data) // ctypes.sizeof(ctypes.c_int16) | ||
| self._data = (ctypes.c_int16 * n).from_buffer_copy(data) | ||
| self._data = data |
There was a problem hiding this comment.
🔴 AudioFrame with immutable bytes data breaks in-place modification by APM
When AudioFrame is constructed with bytes data, the FFI layer in apm.py process_stream/process_reverse_stream writes to immutable memory, causing undefined behavior.
Root Cause and Impact
Previously, AudioFrame.__init__ always created a mutable copy via (ctypes.c_int16 * n).from_buffer_copy(data), so self._data was always a writable ctypes array. The new code stores the original data object directly (self._data = data at audio_frame.py:67), which can be immutable bytes.
The process_stream method in apm.py:51-55 gets a pointer to the frame's data and passes it to the FFI layer which modifies the audio in-place:
bdata = data.data.cast("b") # read-only memoryview if _data is bytes
req.apm_process_stream.data_ptr = get_address(memoryview(bdata))The get_address function at _utils.py:50-53 obtains the address of the bytes object via ctypes.c_char_p, and the FFI writes to that address. Writing to bytes memory is undefined behavior in CPython — it bypasses immutability guarantees and can corrupt string interning, hash caches, or cause segfaults.
This is confirmed by the existing test at tests/rtc/test_apm.py:61-76 which creates AudioFrames from bytes (via .tobytes()) and then calls apm.process_stream() on them, expecting in-place modification.
Impact: Silent data corruption (processed audio not actually written back) or crashes when using AudioProcessingModule with bytes-backed AudioFrames. The test test_audio_processing would produce incorrect output.
Prompt for agents
The core issue is that AudioFrame now stores bytes data directly without copying, but some consumers (apm.py process_stream/process_reverse_stream) need to modify the data in-place via FFI. There are several possible fixes:
1. In AudioFrame.__init__, always ensure self._data is mutable: if isinstance(data, bytes), convert to bytearray(data). This preserves the zero-copy optimization for bytearray inputs while ensuring mutability.
2. Alternatively, in apm.py process_stream/process_reverse_stream, check if the frame data is writable and make a mutable copy if needed before passing to FFI, then write the result back.
3. Or document that AudioFrame data must be mutable (bytearray) for APM processing and raise an error in process_stream if the data is read-only.
Option 1 is the simplest and maintains backward compatibility. In audio_frame.py around line 67, change:
self._data = data
to:
self._data = data if isinstance(data, bytearray) else bytearray(data)
This still avoids the copy for bytearray inputs (the common internal path) while ensuring bytes inputs get converted to mutable storage.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def get_address(data) -> int: | ||
| if isinstance(data, memoryview): | ||
| if not data.readonly: | ||
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | ||
| data = data.obj | ||
| if isinstance(data, bytearray): | ||
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | ||
| if isinstance(data, bytes): | ||
| addr = ctypes.cast(ctypes.c_char_p(data), ctypes.c_void_p).value | ||
| assert addr is not None | ||
| return addr | ||
| raise TypeError(f"expected bytes, bytearray, or memoryview, got {type(data)}") |
There was a problem hiding this comment.
🔴 get_address falls through to TypeError for read-only memoryview backed by non-bytes/bytearray object
When get_address receives a read-only memoryview whose .obj is not bytes or bytearray (e.g., a memoryview of another memoryview, or a read-only buffer from a third-party library), it raises TypeError instead of returning the correct address.
Root Cause
At _utils.py:44-47, when a read-only memoryview is passed, the code does data = data.obj and then checks only for bytearray and bytes. If .obj is any other type (e.g., another memoryview, a numpy array, or a ctypes array), the function falls through to the raise TypeError at line 54.
For example, memoryview(memoryview(some_bytes)) creates a memoryview whose .obj is the inner memoryview, not the bytes object. This would cause:
get_address(memoryview(memoryview(b"hello")))
# data.readonly = True → data = data.obj (inner memoryview)
# not bytearray, not bytes → TypeErrorThis can be triggered indirectly through apm.py:55 where get_address(memoryview(bdata)) wraps an already-memoryview bdata in another memoryview, or through audio_resampler.py:102.
Impact: TypeError raised when processing audio through APM or resampler with certain data types.
| def get_address(data) -> int: | |
| if isinstance(data, memoryview): | |
| if not data.readonly: | |
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | |
| data = data.obj | |
| if isinstance(data, bytearray): | |
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | |
| if isinstance(data, bytes): | |
| addr = ctypes.cast(ctypes.c_char_p(data), ctypes.c_void_p).value | |
| assert addr is not None | |
| return addr | |
| raise TypeError(f"expected bytes, bytearray, or memoryview, got {type(data)}") | |
| def get_address(data) -> int: | |
| if isinstance(data, memoryview): | |
| if not data.readonly: | |
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | |
| obj = data.obj | |
| if isinstance(obj, (bytes, bytearray)): | |
| pass # fall through to handle obj below | |
| else: | |
| # For other backing types, convert to bytes to get address | |
| addr = ctypes.cast(ctypes.c_char_p(bytes(data)), ctypes.c_void_p).value | |
| assert addr is not None | |
| return addr | |
| data = obj | |
| if isinstance(data, bytearray): | |
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | |
| if isinstance(data, bytes): | |
| addr = ctypes.cast(ctypes.c_char_p(data), ctypes.c_void_p).value | |
| assert addr is not None | |
| return addr | |
| raise TypeError(f"expected bytes, bytearray, or memoryview, got {type(data)}") | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(data, memoryview): | ||
| if not data.readonly: | ||
| return ctypes.addressof(ctypes.c_char.from_buffer(data)) | ||
| data = data.obj |
There was a problem hiding this comment.
🔴 get_address loses slice offset for readonly sliced memoryviews via .obj unwrapping
When get_address receives a readonly sliced memoryview, data = data.obj on line 47 returns the full underlying buffer, discarding the slice offset. The returned address points to the start of the full buffer, not the start of the slice.
Root Cause and Impact
The get_address function handles readonly memoryviews by falling through to data = data.obj, which returns the original underlying object (e.g., a bytes object). For a sliced readonly memoryview like memoryview(b'hello world')[2:6], .obj returns the full b'hello world', and the address returned is of the start of b'hello world', not of index 2.
This affects VideoFrame._proto_info() at livekit-rtc/livekit/rtc/video_frame.py:113 and VideoFrame.get_plane() at line 147, where get_address(self.data) is called with memoryview(self._data). If a user constructs a VideoFrame with a sliced readonly memoryview (e.g., VideoFrame(..., data=memoryview(some_bytes)[offset:])), the FFI layer receives the wrong data pointer, leading to incorrect video frame processing or memory access issues.
The writable (non-readonly) path correctly handles slices via ctypes.c_char.from_buffer(data), which respects the memoryview's buffer protocol including offset.
Prompt for agents
In livekit-rtc/livekit/rtc/_utils.py line 47, `data = data.obj` discards the slice offset for readonly memoryviews. To fix this, you need to handle readonly sliced memoryviews properly. One approach: convert the readonly memoryview to bytes first via `data = bytes(data)` which preserves the slice content. Alternatively, use `ctypes.c_char.from_buffer_copy(data)` to get the address of a copy, though this adds a copy. The cleanest fix may be to convert to bytes: replace line 47 with `data = bytes(data)`, which correctly captures just the sliced region.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
