Fix and improve C tuple-to-array conversion in ipjsua Swift sample#4925
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Updates the iOS Swift ipjsua sample to access C fixed-size arrays (imported as Swift tuples) via direct memory access instead of Mirror, and fixes an out-of-bounds media iteration.
Changes:
- Replaced
Mirror-based tuple conversion with an unsafe-pointer-basedtupleToArray()that accepts an explicit element count. - Fixed media iteration to avoid reading one element past
media_cnt. - Switched video device name extraction to read the in-place
char[64]buffer.
| private func tupleToArray<Tuple, Value>( | ||
| _ tuple: Tuple, | ||
| count: any BinaryInteger, | ||
| as: Value.Type = Value.self |
There was a problem hiding this comment.
The as parameter is currently unused inside tupleToArray, which will trigger an “unused parameter” warning in Swift. If it’s only there for type inference, consider removing it and relying on the call-site type context, or rename it to _/use it explicitly to avoid warnings.
| as: Value.Type = Value.self | |
| _: Value.Type = Value.self |
There was a problem hiding this comment.
I don't see any “unused parameter” warning in Xcode.
| private func tupleToArray<Tuple, Value>( | ||
| _ tuple: Tuple, | ||
| count: any BinaryInteger, | ||
| as: Value.Type = Value.self | ||
| ) -> [Value] { | ||
| withUnsafePointer(to: tuple) { ptr in | ||
| let cnt = Int(count) | ||
| return ptr.withMemoryRebound(to: Value.self, capacity: cnt) { reboundPtr in | ||
| Array(UnsafeBufferPointer(start: reboundPtr, count: cnt)) |
There was a problem hiding this comment.
count: any BinaryInteger requires a newer Swift toolchain, but this Xcode project is configured with SWIFT_VERSION = 5.0, so this won’t compile as-is. Consider changing count to a concrete type (e.g., Int) or make it a generic parameter constrained to BinaryInteger (e.g., Count: BinaryInteger) and convert at call sites as needed.
There was a problem hiding this comment.
SWIFT_VERSION is unspecified in the project, so default SWIFT_VERSION = 5.0 is used. It is language mode, not the compiler version. The actual compiler shipping with modern (and not very modern) Xcode is Swift 5.9+/6.x and handles any syntax fine. Furthermore, using a protocol as a type parameter (existential) has worked since Swift 1 —any is just explicit syntax sugar added in Swift 5.6.
| let dev_name = withUnsafeBytes(of: infos[Int(i)].name) { ptr in | ||
| String(cString: ptr.baseAddress!.assumingMemoryBound(to: CChar.self)) |
There was a problem hiding this comment.
String(cString:) will continue reading memory until it finds a NUL terminator; pjmedia_vid_dev_info.name is a fixed char[64] and the header doesn’t explicitly guarantee it’s always NUL-terminated. To avoid a potential over-read, consider decoding with an explicit max length (e.g., find the first 0 byte within the 64-byte buffer and only decode up to that).
| let dev_name = withUnsafeBytes(of: infos[Int(i)].name) { ptr in | |
| String(cString: ptr.baseAddress!.assumingMemoryBound(to: CChar.self)) | |
| let dev_name = withUnsafeBytes(of: infos[Int(i)].name) { rawBuffer in | |
| let bytes = rawBuffer.bindMemory(to: UInt8.self) | |
| let nulIndex = bytes.firstIndex(of: 0) ?? bytes.endIndex | |
| return String(decoding: bytes[..<nulIndex], as: UTF8.self) |
There was a problem hiding this comment.
This makes sense.
So, I've changed to more verbose but safer approach:
let dev_name = withUnsafeBytes(of: infos[Int(i)].name) { buf in
/* Find the NUL terminator within the fixed buffer,
* defaulting to the full buffer size if none found.
*/
let len = buf.firstIndex(of: 0) ?? buf.count
return String(
bytes: UnsafeRawBufferPointer(rebasing: buf[0..<len]),
encoding: .utf8
)
}| pjsua_vid_enum_devs(&infos, &count); | ||
| for i in 0..<count { |
There was a problem hiding this comment.
The return value of pjsua_vid_enum_devs is ignored. If enumeration fails, count may be left unchanged and subsequent logic may run with stale values; consider checking the returned pj_status_t and only iterating when it’s PJ_SUCCESS (and/or set count = 0 on failure).
There was a problem hiding this comment.
pjsua_vid_enum_devs was ignored before the changes in in question.
As far as I understand, the for loop should be omitted entirely if enumerating video devices fails.
Changed to that.
…le app Replace Mirror-based tupleToArray() with direct memory reinterpretation using withUnsafePointer/withMemoryRebound, which is the idiomatic way to access C fixed-size arrays imported as tuples in Swift. Key changes: - tupleToArray() now takes an explicit count parameter, ensuring only valid elements are accessed (fixes off-by-one in media iteration where 0...media_cnt read one past the valid range) - Use withUnsafeBytes(of:) + bounded String(bytes:encoding:) for reading the device name (char[64]) safely in place, without relying on String(cString:) which would read past the buffer if no NUL terminator were present - Check pjsua_vid_enum_devs() return value before iterating devices
laconicman
force-pushed
the
imp/ipjsua-swift-tuple-to-array
branch
from
d8b2430 to
e71f452
Compare
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
Tested here and works without any issues
Replace
Mirror-basedtupleToArray()with direct memory reinterpretation usingwithUnsafePointer/withMemoryRebound, which is the idiomatic way to access C fixed-size arrays imported as tuples in Swift.Key changes:
tupleToArray()now takes an explicit count parameter, ensuring only valid elements are accessed0...media_cntread one past the valid rangewithUnsafeBytes(of:) + String(cString:)for reading the device name (char[64]) directly in place, since it is a null-terminated C string with no accompanying length field