Repository navigation
Conversation
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.
PR Summary by QodoHarden RTP audio decoding and stream shutdown
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Old audio plays after playback resumes
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
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.
| lock.unlock(); | ||
| onNewAudioData(audioPkt.data.data(), audioPkt.data.size()); |
There was a problem hiding this comment.
1. Old audio plays after playback resumes 🐞 Bug ≡ Correctness
processAudioQueue() now releases the queue lock while decoding, allowing packets to accumulate, but stopAudioProcessing() leaves those packets in m_audioQueue. If playback pauses while decoding falls behind, the restarted worker consumes the retained packets before new audio, potentially playing up to 50 old packets after resume.
Agent Prompt
## Issue description
Audio queued during a decode can survive a pause and play after resume.
## Fix Focus Areas
- app/videonative/src/main/cpp/AudioDecoder.h[56-69]
- app/videonative/src/main/cpp/AudioDecoder.cpp[61-69]
## Recommended Fix
After stopping and joining the decode worker, clear the pending audio queue under its mutex before a new worker can start. Preserve the existing synchronization around enqueue and dequeue.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9011e99 |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
This is a good set of fixes and I want it in. The six defects are real: the 250-byte fixed copy, the negative payload size feeding a VLA, runt RTP packets reaching the parser, the write-after-close on the AAudio stream, delete on a malloc'd Opus decoder, and decoding under the queue lock. The replacements are correct on my read: the RTP header walk checks header + 4 before reading the extension length, padding is validated against the payload, m_pcm is sized for the longest legal Opus packet, the queue is bounded with oldest-drop, and m_mtxStream covers both the decode-thread write and the close. The follow-ups to Qodo's two findings in 9011e99 are right as well.
One blocking item, mechanical: the clang-format job fails on this branch. app/videonative has its own .clang-format, and clang-format 18.1.8 wants 19 lines changed in AudioDecoder.cpp, 8 in AudioDecoder.h, 7 in VideoPlayer.cpp. pip install clang-format==18.1.8 && scripts/format_sources.sh and push; the rest of CI (host test, debug APK, emulator smoke) is already green on the reopened run.
Minor, take or leave: kMaxQueued = 50 at 20 ms frames is a second of audio; fine for live playback, just worth a comment that it is deliberate latency bounding, which you have.
This branch has not been deployed
2 participants
I checked audio end to end on a Quest 3 against an OpenIPC camera (SSC338Q, OpenIPC 2.6) with
audio switched on in majestic: the camera's microphone, Opus in the RTP stream over wfb-ng,
decoded here and played in the headset. It works. Reading
AudioDecoderon the way turned upsix defects, most of them reachable from radio input.
The decoder
The packet copy overflowed on ordinary packets.
AudioUDPPacketcopied into a fixed250-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
pcmcodec can write past the end with perfectlyordinary packets. Each packet is stored at its own size now, up to the 65507 bytes the UDP
receiver can hand over, and only something longer is refused — refused, not truncated, because a
cut Opus packet decodes to noise rather than failing.
A short packet sized a stack array from a negative number. The payload was read from a fixed
offset of 12 with no check. A shorter packet made the size negative,
opus_packet_get_nb_frames()then returned an error that was not checked, and the product sizeda 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 itself, CSRCs, extension and padding included.
Runt packets reached the parser.
RTPPacketreads its header straight out of the buffer,and its length assert is commented out, so a packet shorter than a header 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
RTPPacketas well.The stream could be written to after it was closed.
VideoActivity.onPause()callsVideoPlayer.stop(), which closes the stream, beforestopAudio()has stopped the decodethread — so that thread could still write to it.
stopAudio()also left the pointer set, so thedestructor closed the same stream a second time. The stream is behind its own mutex now and
cleared when closed.
The Opus decoder was freed with
delete. It comes fromopus_decoder_create(), whichallocates with
malloc; it is released withopus_decoder_destroy()now. A start after a stopalso created a second decoder without freeing the first — and
onResume()is such a start,after every pause.
The receive thread waited for every decode. Packets were decoded with the queue lock held, so
enqueueAudio()— called on the receive thread, which carries the video too — waited out eachdecode. The lock is released before decoding. That lock was also, by accident, the only thing
holding the receive thread back, so the queue is bounded now: at most 50 packets, about a second,
oldest dropped first.
initAudio()checked none of the calls it made and reported success regardless; failures arelogged and reported now.
Testing
On a Quest 3 with the camera above: audio and video play on this branch, and the same process
went through several pause and resume cycles — each one closes the stream in
onPause()andopens a new one in
onResume(), which is where the leaked decoder and the write to a closedstream used to happen — with nothing in the crash buffer. That build also carried #126;
the two touch different files.
The packet-handling fixes were found by reading the code and are bounds checks; I have not
driven the decoder with crafted packets.