Repository navigation
feat: Opus, MP4A-LATM and fragmented AAC audio (part of #14) - #52
Conversation
compot-dev
added
the
awaiting device test
PR Summary by QodoAdd Opus, MP4A-LATM, and fragmented AAC playback
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. G.726 cameras still have no audio
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| int pos = start; | ||
| while (pos < end) { | ||
| int len = 0; | ||
| int b; | ||
| do { | ||
| if (pos >= end) return units; | ||
| b = data[pos++] & 0xFF; |
There was a problem hiding this comment.
1. Latm streams fail to produce audio 🐞 Bug ≡ Correctness
splitLatm() reads whole bytes as payload lengths and forwards the following ranges as raw AAC, without parsing the bit-packed AudioMuxElement or its useSameStreamMux field. When a normal MP4A-LATM RTP payload arrives, the AAC decoder receives incorrectly sliced or still-framed data instead of an access unit.
Agent Prompt
## Issue description
The LATM splitter assumes byte-aligned length prefixes and passes LATM framing to a raw AAC decoder.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[432-448]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[117-122]
## Recommended Fix
Parse the supported AudioMuxElement layout at bit granularity, including useSameStreamMux and PayloadLengthInfo, and queue only extracted AAC access-unit bytes. Add a test using a framed AudioMuxElement.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Partly disagree, partly fixed in 36cbfbd. With out-of-band config (cpresent=0) the AudioMuxElement is called with muxConfigPresent = 0, so there is no useSameStreamMux bit, and with frameLengthType = 0 PayloadLengthInfo is a byte-aligned run of 0xFF bytes plus a final byte, directly followed by the AU. That is what splitLatm() parses. What was missing is the guarantee that the stream really uses this layout: latmToAsc() now validates the whole StreamMuxConfig and disables LATM for anything else (see finding 3).
| if (fragment != null) { | ||
| // Continuation: one AU header with the full AU size, then the next piece | ||
| int len = end - pos; | ||
| if (size != fragment.length || have + len > fragment.length) { | ||
| fragment = null; // does not belong to the AU being collected | ||
| } else { | ||
| System.arraycopy(data, pos, fragment, have, len); | ||
| have += len; | ||
| if (have == fragment.length) { |
There was a problem hiding this comment.
2. Lost packets can corrupt the next aac frame 🐞 Bug ≡ Correctness
AuAssembler.push() identifies a continuation solely by its declared size and accumulated byte count, without checking RTP sequence numbers. If a fragment is lost and a later fragmented access unit has the same declared size, its bytes can complete the stale buffer and be submitted as one corrupted frame.
Agent Prompt
## Issue description
The AAC assembler can join bytes from different access units after an RTP packet is lost.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[112-115]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[381-409]
## Recommended Fix
Pass the RTP sequence number to the assembler and discard a pending fragment when the next packet is not the expected successor. Add a packet-loss test with equal-sized access units.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 36cbfbd. AuAssembler.push() takes the RTP sequence number and drops the pending fragment when a packet is missing. Test: lossBetweenFragmentsDropsTheStaleAu (equal-sized AUs, packet 101 lost).
| r.bits(1); // allStreamsSameTimeFraming | ||
| r.bits(6); // numSubFrames | ||
| if (r.bits(4) != 0) return null; // numProgram + 1 must be 1 | ||
| if (r.bits(3) != 0) return null; // numLayer + 1 must be 1 |
There was a problem hiding this comment.
3. Unsupported latm layouts stay enabled 🐞 Bug ≡ Correctness
latmToAsc() discards allStreamsSameTimeFraming and numSubFrames, then returns before checking the remaining StreamMuxConfig fields, including frameLengthType. Configurations describing multiple subframes or a different framing mode therefore remain enabled and reach a splitter that has no handling for those layouts.
Agent Prompt
## Issue description
The LATM configuration parser enables layouts that the packet parser cannot interpret.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioConfig.java[144-163]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[432-448]
## Recommended Fix
Consume and validate the complete supported StreamMuxConfig, rejecting unsupported timing, subframe and frame-length modes rather than discarding their fields. Test that unsupported layouts disable LATM.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 36cbfbd. latmToAsc() now consumes and checks the full StreamMuxConfig: allStreamsSameTimeFraming = 1, numSubFrames = 0, one program and layer, GASpecificConfig without extension, frameLengthType = 0, otherDataPresent = 0, crcCheckPresent = 0. Test: latmRejectsLayoutsTheRtpPathCannotHandle.
| int freqIndex = ((asc[0] & 0x07) << 1) | ((asc[1] & 0x80) >> 7); | ||
| int channelConfig = (asc[1] & 0x78) >> 3; | ||
| if (freqIndex >= AAC_RATES.length || channelConfig < 1) return null; | ||
| return new int[] {AAC_RATES[freqIndex], Math.min(channelConfig, 2)}; |
There was a problem hiding this comment.
4. Multichannel latm audio is misplayed 🐞 Bug ≡ Correctness
ascInfo() collapses every channel configuration above two to stereo while latmToAsc() retains the original multichannel value in the decoder configuration. When such a stream decodes to more than two PCM channels, playback selects a mono channel mask for any channel count other than two and writes the multichannel bytes to that track.
Agent Prompt
## Issue description
LATM accepts multichannel configurations that the playback track cannot render correctly.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioConfig.java[155-172]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[253-258]
## Recommended Fix
Reject channel configurations above two until playback supports them, or preserve the real channel count and configure an appropriate output channel mask or downmix.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 36cbfbd. Channel configurations above 2 are rejected in both latmToAsc() and ascInfo() until playback supports more channels.
| int channelConfig = r.bits(4); | ||
| if (r.overrun() || objectType < 1 || objectType > 4 || channelConfig < 1 | ||
| || channelConfig > 7) { |
There was a problem hiding this comment.
5. Some latm profiles get conflicting setup 🐞 Bug ≡ Correctness
latmToAsc() accepts AAC object types one through four and preserves their type in the generated AudioSpecificConfig, but ensureAacDecoder() sets the AAC profile to LC unconditionally. A stream declaring AAC Main, SSR or LTP therefore reaches decoder configuration with a profile setting that conflicts with its supplied AudioSpecificConfig.
Agent Prompt
## Issue description
Accepted LATM AAC object types can conflict with the decoder's forced LC profile.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioConfig.java[151-163]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[532-538]
## Recommended Fix
Either accept only AAC-LC in LATM configuration or configure the decoder profile consistently with the object type encoded in the AudioSpecificConfig.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 36cbfbd. LATM is accepted only for AAC-LC (object type 2), matching the decoder profile.
c382a2f to
c9d0e73
Compare
92dfb14 to
42e0e17
Compare
36cbfbd to
44d1be2
Compare
Part of #14 (G.726 remains): - Opus (RFC 7587): one Opus packet per RTP packet, decoded by MediaCodec "audio/opus" (Android 5.0+) with an OpusHead csd-0 and codec delay / seek pre-roll csd-1/csd-2; output is 48 kHz. - MP4A-LATM (RFC 6416) with out-of-band config: the fmtp StreamMuxConfig is turned into an AudioSpecificConfig (audioMuxVersion 0, one program and layer), sample rate and channels come from it, and each AudioMuxElement is split by its PayloadLengthInfo. cpresent=1 and other layouts stay unsupported. - RFC 3640 AUs spanning several RTP packets are reassembled (AU-size in each fragment, RTP marker on the last); a fragment with missing pieces is dropped. - The AAC MediaCodec path is shared by AAC, LATM and Opus. - README (EN/RU/ZH): formats listed; only G.726 remains in the limitations. Tests: Opus SDP and header, StreamMuxConfig to ASC, LATM SDP, unsupported LATM layouts, PayloadLengthInfo splitting, fragmented AU reassembly, dropped incomplete fragment, whole AUs passing through.
- latmToAsc() parses the whole StreamMuxConfig and accepts only the layout the RTP path handles: same time framing, one subframe, one program and layer, AAC-LC, a standard sampling rate, mono or stereo, frameLengthType 0, no other data, no CRC. Anything else disables LATM instead of reaching the splitter or MediaCodec. - ascInfo() rejects more than two channels; finish() disables LATM when the ASC gives no valid rate/channels. - LatmAssembler joins an AudioMuxElement that spans several RTP packets (marker on the last) and drops an element with a missing piece. - AuAssembler tracks RTP sequence numbers: after a loss the pending AAC fragment is dropped, so bytes of two AUs are never joined.
42e0e17 to
6c7da5b
Compare
44d1be2 to
6f43336
Compare
|
/review |
| } else { | ||
| // MP4A-LATM, OPUS, G726, ... are not supported | ||
| // G.726 and others are not supported | ||
| codec = NONE; |
There was a problem hiding this comment.
1. G.726 cameras still have no audio 📎 Requirement gap ≡ Correctness
AudioConfig.applyRtpmap() continues to classify G.726 as unsupported and sets its codec to NONE. Any SDP advertising G.726 therefore reaches the existing unsupported-audio path instead of a decoder, leaving that camera without playback.
Agent Prompt
## Issue description
G.726 remains explicitly unsupported, so cameras that advertise G.726 audio are still dropped instead of being played. Implement a software G.726 decoder and connect it to the audio pipeline, or otherwise provide a working decoder path for supported G.726 variants.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/AudioConfig.java[64-66]
- android/app/src/main/java/com/openipc/decoder/AudioManager.java[149-173]
## Recommended Fix
Add G.726 codec recognition and SDP parameter handling, decode G.726 RTP payloads into PCM, and route the decoded samples through the existing audio output path. Add JVM coverage for codec selection and decoder input handling, and device-test playback with a G.726 camera.
ⓘ 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 6f43336 |
… and core-coder configs - A sequence gap right after a completed element may have lost the next element's start, so its continuation is no longer taken for a new element - Without markers the element buffer grew forever; it is capped at 64 KiB - frameLengthFlag and dependsOnCoreCoder are not carried by the 2-byte ASC, so such StreamMuxConfigs are refused instead of reaching the decoder with a wrong csd-0
|
Addressed in 68c8c25:
The 7 older LATM findings were fixed and answered earlier in this thread. /review |
|
Re LATM streams fail to produce audio: not a bug, intentional. Only |
compot-dev
removed
the
awaiting device test
feat: Opus, MP4A-LATM and fragmented AAC audio (part of #14)
1 participant
Part of #14. G.726 remains: Android has no G.726 decoder, so it would need a software implementation. That stays in #14.
audio/opus, available since Android 5.0.csd-0is an OpusHead header;csd-1/csd-2carry the codec delay and the seek pre-roll. Output is 48 kHzStreamMuxConfigfrom fmtpconfig=is converted into an AudioSpecificConfig (audioMuxVersion 0, one program, one layer). Sample rate and channel count are taken from it. Each AudioMuxElement is split by its PayloadLengthInfo.cpresent=1(config in-band) and other layouts are still not supported and are switched off cleanlyAAC, LATM and Opus share one MediaCodec path.
README in all three languages: the formats are listed, and only G.726 remains under "Known limitations".
Tests
AudioFormatsTest, 9 tests:103 JVM tests on this branch (#48 + #51 + #52), all pass.
assembleDebugandlintDebugpass.Device checklist