From 3f4511b36126bcc56feeab77fe82303ed5052acd Mon Sep 17 00:00:00 2001 From: iflyhere <57563846+iflyhere@users.noreply.github.com> Date: Sat, 10 Oct 2026 17:34:21 +0200 Subject: [PATCH 1/2] Harden the audio decoder against bad packets and a closed stream Audio was checked end to end on a Quest 3 against an OpenIPC camera with audio switched on in majestic (audio.enabled true, Opus): the camera's microphone, Opus in the RTP stream over wfb, decoded here and played in the headset. It works. Reading the decoder on the way turned up six defects, most of them reachable from radio input. - The packet copy wrote into a fixed 250-byte array with no length check. That does not even take malformed input: 20 ms of PCM at 8 kHz is already 320 bytes, so majestic's pcm codec can write past the end with perfectly ordinary packets. The buffer holds 1500 bytes now and anything larger is dropped - dropped, not truncated, because a cut Opus packet decodes to noise rather than failing. - The payload was read from a fixed offset of 12 with no check. A shorter packet gave a negative size; opus_packet_get_nb_frames() then returned an error that was not checked; and the product sized a variable-length array on the stack. Decoding goes into a fixed buffer for the longest packet Opus allows - 120 ms, 5760 samples at 48 kHz - and the header length is worked out from the packet, CSRCs, extension and padding included. - RTPPacket reads its header straight out of the buffer and its length assert is commented out, so a runt packet was an out-of-bounds read with an underflowed payload size handed to both the video parser and the audio decoder. Every RTP packet enters through onNewRTPData(), which now drops anything shorter than a header; the payload size is clamped in RTPPacket as well. - The stream could be written to after it was closed. VideoActivity.onPause() calls VideoPlayer.stop(), which closes the stream, before stopAudio() has stopped the decode thread, so that thread could still write to it. stopAudio() also left the pointer set, so the destructor closed the same stream a second time. The stream is behind its own mutex now and cleared when closed. - The Opus decoder comes from opus_decoder_create(), which allocates with malloc, and was freed with delete. It is destroyed with opus_decoder_destroy() now. A start after stop also created a second decoder without freeing the first - and onResume() is such a start, after every pause. - Packets were decoded with the queue lock held, so enqueueAudio() on the receive thread - which carries the video too - waited out every decode. The lock is released before decoding. initAudio() checked none of the calls it made and reported success regardless; failures are logged and reported now. --- app/videonative/src/main/cpp/AudioDecoder.cpp | 182 +++++++++++++----- app/videonative/src/main/cpp/AudioDecoder.h | 32 ++- app/videonative/src/main/cpp/VideoPlayer.cpp | 9 + app/videonative/src/main/cpp/parser/RTP.hpp | 4 +- 4 files changed, 177 insertions(+), 50 deletions(-) diff --git a/app/videonative/src/main/cpp/AudioDecoder.cpp b/app/videonative/src/main/cpp/AudioDecoder.cpp index 668c948e..fbc53c45 100644 --- a/app/videonative/src/main/cpp/AudioDecoder.cpp +++ b/app/videonative/src/main/cpp/AudioDecoder.cpp @@ -7,9 +7,6 @@ #define TAG "pixelpilot" -#define SAMPLE_RATE 48000 -#define CHANNELS 1 - AudioDecoder::AudioDecoder() { initAudio(); @@ -18,13 +15,27 @@ AudioDecoder::AudioDecoder() AudioDecoder::~AudioDecoder() { stopAudioProcessing(); - delete pOpusDecoder; - AAudioStream_requestStop(m_stream); - AAudioStream_close(m_stream); + { + std::lock_guard lock(m_mtxStream); + closeOutputLocked(); + } + // opus_decoder_create() allocates with malloc, so this is the only correct way to free it; + // the delete that used to be here was undefined behaviour. + if (pOpusDecoder != nullptr) + { + opus_decoder_destroy(pOpusDecoder); + pOpusDecoder = nullptr; + } } void AudioDecoder::enqueueAudio(const uint8_t* data, const std::size_t data_length) { + // Dropped rather than truncated: a cut Opus packet does not fail to decode, it decodes to + // noise. + if (data_length == 0 || data_length > AudioUDPPacket::kMaxLen) + { + return; + } { std::lock_guard lock(m_mtxQueue); m_audioQueue.push(AudioUDPPacket(data, data_length)); @@ -43,70 +54,153 @@ void AudioDecoder::processAudioQueue() { break; } - if (!m_audioQueue.empty()) - { - AudioUDPPacket audioPkt = m_audioQueue.front(); - onNewAudioData(audioPkt.data, audioPkt.len); - m_audioQueue.pop(); - lock.unlock(); - } + AudioUDPPacket audioPkt = m_audioQueue.front(); + m_audioQueue.pop(); + // Decoded with the queue released. enqueueAudio() is called on the receive thread, which + // carries the video as well, and it used to wait for every decode to finish. + lock.unlock(); + onNewAudioData(audioPkt.data, audioPkt.len); } } void AudioDecoder::initAudio() { __android_log_print(ANDROID_LOG_DEBUG, TAG, "initAudio"); - int error; - pOpusDecoder = opus_decoder_create(SAMPLE_RATE, CHANNELS, &error); - // Create a stream m_builder - AAudio_createStreamBuilder(&m_builder); + // Only once: start after stop() comes back through here, and creating a second decoder + // leaked the first. + if (pOpusDecoder == nullptr) + { + int error = OPUS_OK; + pOpusDecoder = opus_decoder_create(kSampleRate, kChannels, &error); + if (error != OPUS_OK || pOpusDecoder == nullptr) + { + __android_log_print(ANDROID_LOG_ERROR, TAG, "opus decoder could not be created: %s", + opus_strerror(error)); + pOpusDecoder = nullptr; + isInit = false; + return; + } + } + std::lock_guard lock(m_mtxStream); + isInit = openOutputLocked(); +} - // Set the stream format - AAudioStreamBuilder_setFormat(m_builder, AAUDIO_FORMAT_PCM_I16); - AAudioStreamBuilder_setChannelCount(m_builder, CHANNELS); // Mono - AAudioStreamBuilder_setSampleRate(m_builder, SAMPLE_RATE); // 8000 Hz +bool AudioDecoder::openOutputLocked() +{ + AAudioStreamBuilder* builder = nullptr; + aaudio_result_t result = AAudio_createStreamBuilder(&builder); + if (result != AAUDIO_OK) + { + __android_log_print(ANDROID_LOG_ERROR, TAG, "audio stream builder failed: %s", + AAudio_convertResultToText(result)); + return false; + } - AAudioStreamBuilder_setBufferCapacityInFrames(m_builder, BUFFER_CAPACITY_IN_FRAMES); + // The camera may encode at 8 kHz; that does not matter here, because an Opus decoder + // renders any Opus stream at whichever of its rates it was created for. + AAudioStreamBuilder_setFormat(builder, AAUDIO_FORMAT_PCM_I16); + AAudioStreamBuilder_setChannelCount(builder, kChannels); + AAudioStreamBuilder_setSampleRate(builder, kSampleRate); + AAudioStreamBuilder_setBufferCapacityInFrames(builder, BUFFER_CAPACITY_IN_FRAMES); - // Open the stream - AAudioStreamBuilder_openStream(m_builder, &m_stream); - // Clean up the m_builder - AAudioStreamBuilder_delete(m_builder); + result = AAudioStreamBuilder_openStream(builder, &m_stream); + AAudioStreamBuilder_delete(builder); + if (result != AAUDIO_OK) + { + __android_log_print(ANDROID_LOG_ERROR, TAG, "audio stream could not be opened: %s", + AAudio_convertResultToText(result)); + m_stream = nullptr; + return false; + } - AAudioStream_requestStart(m_stream); + result = AAudioStream_requestStart(m_stream); + if (result != AAUDIO_OK) + { + __android_log_print(ANDROID_LOG_ERROR, TAG, "audio stream would not start: %s", + AAudio_convertResultToText(result)); + AAudioStream_close(m_stream); + m_stream = nullptr; + return false; + } + return true; +} - isInit = true; +void AudioDecoder::closeOutputLocked() +{ + if (m_stream != nullptr) + { + AAudioStream_requestStop(m_stream); + AAudioStream_close(m_stream); + // Cleared, so nothing writes to a closed stream and the destructor cannot close it a + // second time - which is what stopAudio() followed by destruction used to do. + m_stream = nullptr; + } } void AudioDecoder::stopAudio() { __android_log_print(ANDROID_LOG_DEBUG, TAG, "stopAudio"); - AAudioStream_requestStop(m_stream); - AAudioStream_close(m_stream); + std::lock_guard lock(m_mtxStream); + closeOutputLocked(); isInit = false; } void AudioDecoder::onNewAudioData(const uint8_t* data, const std::size_t data_length) { - const int rtp_header_size = 12; - const uint8_t* opus_payload = data + rtp_header_size; - int opus_payload_size = data_length - rtp_header_size; - - int frame_size = opus_packet_get_samples_per_frame(opus_payload, SAMPLE_RATE); - int nb_frames = opus_packet_get_nb_frames(opus_payload, opus_payload_size); + // Everything below is read from the radio, so every length is checked before it is used. + // The payload used to be taken from a fixed offset of 12 with no check at all: a packet + // shorter than that gave a negative size, which then sized a stack array. + constexpr std::size_t kFixedHeader = 12; + if (data_length < kFixedHeader) + { + return; + } - // Decode the frame - int pcm_size = frame_size * nb_frames * CHANNELS; - if (pOpusDecoder && m_stream) + // The real header length: CSRCs and an extension both lengthen it, and reading from a + // fixed offset would decode the end of the header as audio. + std::size_t header = kFixedHeader + 4 * (data[0] & 0x0F); + if (data[0] & 0x10) + { + if (data_length < header + 4) + { + return; + } + const std::size_t words = (static_cast(data[header + 2]) << 8) | data[header + 3]; + header += 4 + 4 * words; + } + if (data_length <= header) { - opus_int16 pcm[pcm_size]; - int decoded_samples = opus_decode(pOpusDecoder, opus_payload, opus_payload_size, pcm, pcm_size, 0); + return; + } - if (decoded_samples < 0) + std::size_t payload = data_length - header; + if (data[0] & 0x20) + { + // Padding, with its own length in the last byte. + const uint8_t padding = data[data_length - 1]; + if (padding >= payload) { return; } - // Process the decoded PCM data - AAudioStream_write(m_stream, pcm, decoded_samples, 0); + payload -= padding; + } + + if (pOpusDecoder == nullptr) + { + return; + } + // Into a fixed buffer sized for the longest packet Opus allows, rather than one sized from + // what the packet claims about itself. + const int decoded = opus_decode(pOpusDecoder, data + header, static_cast(payload), m_pcm, + kMaxFrameSamples, 0); + if (decoded <= 0) + { + return; + } + + std::lock_guard lock(m_mtxStream); + if (m_stream != nullptr) + { + AAudioStream_write(m_stream, m_pcm, decoded, 0); } } diff --git a/app/videonative/src/main/cpp/AudioDecoder.h b/app/videonative/src/main/cpp/AudioDecoder.h index 60ba1837..f12c7118 100644 --- a/app/videonative/src/main/cpp/AudioDecoder.h +++ b/app/videonative/src/main/cpp/AudioDecoder.h @@ -6,6 +6,7 @@ #define PIXELPILOT_AUDIODECODER_H #include #include +#include #include #include #include @@ -14,18 +15,31 @@ typedef struct _AudioUDPPacket { - _AudioUDPPacket(const uint8_t* _data, size_t _len) + // Room for any RTP packet the link delivers. It was 250 bytes with nothing checking the + // length, so a perfectly ordinary packet - PCM at 8 kHz is already 320 bytes per 20 ms - + // was written past the end of the array. enqueueAudio() drops anything larger. + static constexpr size_t kMaxLen = 1500; + + _AudioUDPPacket(const uint8_t* _data, size_t _len) : len(_len <= kMaxLen ? _len : 0) { - memcpy(data, _data, _len); - len = _len; + if (len > 0) + { + memcpy(data, _data, len); + } }; - uint8_t data[250]; + uint8_t data[kMaxLen]; size_t len; } AudioUDPPacket; class AudioDecoder { public: + static constexpr int kSampleRate = 48000; + static constexpr int kChannels = 1; + // The longest an Opus packet can be is 120 ms, which at 48 kHz is 5760 samples per + // channel - so a buffer this size holds any packet opus_decode() will accept. + static constexpr int kMaxFrameSamples = 5760; + AudioDecoder(); ~AudioDecoder(); @@ -56,6 +70,9 @@ class AudioDecoder private: void onNewAudioData(const uint8_t* data, const std::size_t data_length); + // Both expect m_mtxStream to be held. + bool openOutputLocked(); + void closeOutputLocked(); private: const int BUFFER_CAPACITY_IN_FRAMES = 4096; @@ -64,8 +81,13 @@ class AudioDecoder std::condition_variable m_cvQueue; bool stopAudioFlag = false; std::thread m_audioThread; - AAudioStreamBuilder* m_builder = nullptr; + // Guards m_stream. The decode thread writes to it while stopAudio() closes it from another + // thread - and VideoActivity.onPause() calls VideoPlayer.stop(), which closes the stream, + // before stopAudio() has stopped that thread. + std::mutex m_mtxStream; AAudioStream* m_stream = nullptr; OpusDecoder* pOpusDecoder = nullptr; + // Only ever touched by the decode thread. + opus_int16 m_pcm[kMaxFrameSamples * kChannels]; }; #endif // PIXELPILOT_AUDIODECODER_H diff --git a/app/videonative/src/main/cpp/VideoPlayer.cpp b/app/videonative/src/main/cpp/VideoPlayer.cpp index 832383f7..b4382d40 100644 --- a/app/videonative/src/main/cpp/VideoPlayer.cpp +++ b/app/videonative/src/main/cpp/VideoPlayer.cpp @@ -109,6 +109,15 @@ void VideoPlayer::processQueue() // Not yet parsed bit stream (e.g. raw h264 or rtp data) void VideoPlayer::onNewRTPData(const uint8_t* data, const std::size_t data_length) { + // Every RTP packet from the link comes through here, and RTPPacket reads its header + // straight out of the buffer - its own length check is commented out - so a packet + // shorter than a header was an out-of-bounds read, with an underflowed payload size + // handed on to the video parser and the audio decoder. + if (data_length < sizeof(rtp_header_t)) + { + return; + } + // Parse the RTP packet const RTP::RTPPacket rtpPacket(data, data_length); uint16_t idx = rtpPacket.header.getSequence(); diff --git a/app/videonative/src/main/cpp/parser/RTP.hpp b/app/videonative/src/main/cpp/parser/RTP.hpp index 66e942b5..b49cdd7c 100644 --- a/app/videonative/src/main/cpp/parser/RTP.hpp +++ b/app/videonative/src/main/cpp/parser/RTP.hpp @@ -199,7 +199,9 @@ class RTPPacket RTPPacket(const uint8_t* rtp_data, const size_t data_length) : header(*((rtp_header_t*) rtp_data)), rtpPayload(&rtp_data[sizeof(rtp_header_t)]), - rtpPayloadSize(data_length - sizeof(rtp_header_t)) + // Clamped rather than left to wrap: VideoPlayer::onNewRTPData() drops anything + // shorter than a header, but a size_t that underflows is not a mistake to make twice. + rtpPayloadSize(data_length >= sizeof(rtp_header_t) ? data_length - sizeof(rtp_header_t) : 0) { // assert(data_length >= sizeof(rtp_header_t)); // r.n we don't support padding From 9011e998b650b81711f144219a9efcf8b0bb8b84 Mon Sep 17 00:00:00 2001 From: iflyhere <57563846+iflyhere@users.noreply.github.com> Date: Sat, 10 Oct 2026 18:18:48 +0200 Subject: [PATCH 2/2] Bound the audio queue and size packets to what the receivers deliver Two points from review, both fair. Decoding now runs with the queue lock released, which was the point - the receive thread, which carries the video too, no longer waits out every decode. But that lock was also, by accident, the only thing holding the receive thread back, and the queue itself has never had a limit. If the decoder ever fell behind, packets would pile up without bound. The queue holds at most 50 packets now, about a second of 20 ms frames, and when it is full the oldest goes: for live audio a packet played late is only latency. The 1500-byte limit was arbitrary. The receivers hand over up to 65507 bytes (UdpReceiver's UDP_PACKET_MAX_SIZE; UdsReceiver stops at 3700), so a packet the link could legitimately deliver was being dropped. Each packet is stored at its own size now, and only something longer than either receiver can produce is refused. --- app/videonative/src/main/cpp/AudioDecoder.cpp | 13 +++++++-- app/videonative/src/main/cpp/AudioDecoder.h | 29 ++++++++++--------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/app/videonative/src/main/cpp/AudioDecoder.cpp b/app/videonative/src/main/cpp/AudioDecoder.cpp index fbc53c45..9a302513 100644 --- a/app/videonative/src/main/cpp/AudioDecoder.cpp +++ b/app/videonative/src/main/cpp/AudioDecoder.cpp @@ -31,13 +31,20 @@ AudioDecoder::~AudioDecoder() void AudioDecoder::enqueueAudio(const uint8_t* data, const std::size_t data_length) { // Dropped rather than truncated: a cut Opus packet does not fail to decode, it decodes to - // noise. + // noise. Nothing either receiver delivers is this long; see AudioUDPPacket::kMaxLen. if (data_length == 0 || data_length > AudioUDPPacket::kMaxLen) { return; } { std::lock_guard lock(m_mtxQueue); + // Bounded, because decoding runs without this lock now and nothing else holds the + // receive thread back. If the decoder ever falls behind, the oldest packet goes: + // played late it would only be latency. + if (m_audioQueue.size() >= kMaxQueued) + { + m_audioQueue.pop(); + } m_audioQueue.push(AudioUDPPacket(data, data_length)); } m_cvQueue.notify_one(); @@ -54,12 +61,12 @@ void AudioDecoder::processAudioQueue() { break; } - AudioUDPPacket audioPkt = m_audioQueue.front(); + AudioUDPPacket audioPkt = std::move(m_audioQueue.front()); m_audioQueue.pop(); // Decoded with the queue released. enqueueAudio() is called on the receive thread, which // carries the video as well, and it used to wait for every decode to finish. lock.unlock(); - onNewAudioData(audioPkt.data, audioPkt.len); + onNewAudioData(audioPkt.data.data(), audioPkt.data.size()); } } diff --git a/app/videonative/src/main/cpp/AudioDecoder.h b/app/videonative/src/main/cpp/AudioDecoder.h index f12c7118..9235b24b 100644 --- a/app/videonative/src/main/cpp/AudioDecoder.h +++ b/app/videonative/src/main/cpp/AudioDecoder.h @@ -6,29 +6,27 @@ #define PIXELPILOT_AUDIODECODER_H #include #include -#include #include #include #include #include +#include #include "libs/include/opus.h" typedef struct _AudioUDPPacket { - // Room for any RTP packet the link delivers. It was 250 bytes with nothing checking the - // length, so a perfectly ordinary packet - PCM at 8 kHz is already 320 bytes per 20 ms - - // was written past the end of the array. enqueueAudio() drops anything larger. - static constexpr size_t kMaxLen = 1500; + // Sized to the packet. It used to be a fixed 250-byte array filled with no length check, + // so an ordinary packet - 20 ms of PCM at 8 kHz is already 320 bytes - was written past + // its end. + // + // The limit is the largest datagram either receiver can hand over: UdpReceiver's + // UDP_PACKET_MAX_SIZE, 65507 (UdsReceiver stops at 3700). Anything the link can deliver + // therefore fits, and enqueueAudio() only refuses what could not have come from it. + static constexpr size_t kMaxLen = 65507; - _AudioUDPPacket(const uint8_t* _data, size_t _len) : len(_len <= kMaxLen ? _len : 0) - { - if (len > 0) - { - memcpy(data, _data, len); - } - }; - uint8_t data[kMaxLen]; - size_t len; + _AudioUDPPacket(const uint8_t* _data, size_t _len) : data(_data, _data + _len) {} + + std::vector data; } AudioUDPPacket; class AudioDecoder @@ -39,6 +37,9 @@ class AudioDecoder // The longest an Opus packet can be is 120 ms, which at 48 kHz is 5760 samples per // channel - so a buffer this size holds any packet opus_decode() will accept. static constexpr int kMaxFrameSamples = 5760; + // Packets waiting to be decoded, at most. About a second of 20 ms frames: more than any + // decoder should ever fall behind, and for live audio an older packet is only latency. + static constexpr size_t kMaxQueued = 50; AudioDecoder(); ~AudioDecoder();