Repository navigation
raop_rtp: buffer "no data" packets so silence is not taken for loss - #597
danieldhaiti wants to merge 1 commit into
Conversation
|
Thank you for finding this issue. we would like to first reproduce it. If the client is an ipad, uxplay is in mirror mode, and a youtube with audio is playing in a browser (firefox) on the client, which is then stopped by going to a web page with no audio, audio packets change from packets carrying audio data, to 16 byte packets with the "no data " marker, but the seqnum sequence continues without a break.
|
|
@danieldhaiti you reported:
I looked at this some more. What I see is that (in contrast to your report), iOS does send a complete seqnum sequence, just switches to the 16 byte "no-data" packets during silence. But yes , this triggers a resend request.
your thoughts? (or do you really have an example where the ios client breaks the sequence? if so, please give the recipe for producing such a case) |
|
just tested. pushing the no data packet through the buffer fixed it.. I sane just one tru missed packet. I an extra check to not send resent requests it the previous packet was not data could also be added. |
During silence the sender switches to "no data" packets (12 bytes, or 16 with the 0x00 0x68 0x34 0x00 marker) whose seqnums continue the sequence. They were dropped before the buffer, so each of their seqnums looked like a lost packet: resend requests went out and the audio after the silence was held until the buffer overflowed. After the first sync, these packets now go through the buffer: raop_buffer_enqueue() stores them as entries without a payload (no decryption) and raop_buffer_dequeue() skips them, so they never reach audio_process. Before the first sync they are still dropped, so the start of the stream behaves as before. Co-Authored-By: Claude Opus 5.5 <[email protected]>
76d0dbd to
5629f8f
Compare
|
Thanks for testing it. You're right: the sequence is complete, and the gaps came from UxPlay dropping the no-data packets, not from iOS skipping seqnums. I've rewritten this PR with your approach and corrected the description:
I didn't add the "no resend if the previous packet was not data" check. The packet after the last no-data one is often the first audio after the silence (the start of a word), and with no-data packets in the buffer, a real loss during silence costs one resend request. One thing I couldn't settle: when the old code requested those seqnums, our notes from that test record the phone answering with "empty" replies. Those were presumably the 8-byte type 0x56 packets that UxPlay logs as "Received empty resent audio packet" and then drops. If that is how iOS answers for a no-data seqnum, a no-data packet that is really lost still holds the audio until the buffer overflows. I haven't captured those replies, so this PR leaves them alone. Tested with a standalone buffer test (fails on master, passes here, clean under ASan/LSan/UBSan) and a full build on Ubuntu 26.04 with no new warnings. I'll re-test with an iPhone on our appliance and report back. |
|
I had already made a very minimal change to fix this issue. It could be improved to not make a resend request if last received data before a missing packet was a 16-byte "no-data" packet with the marker, , but maybe that is not a big issue. Can you look at it and seem if anything else needs to be done? thanks! The fix is is in a new branch cloned from master, called "no_data" |
|
I tested One fix is needed. The dequeue check reads 4 bytes from every payload, so a 13–15 byte packet reads past its buffer (ASan: heap-buffer-overflow). Checking the size first fixes it: if (payload_size == 4 && memcmp(payload, no_data_marker, 4) == 0) {Optional:
I agree the extra resend check isn't needed. I'm closing #597 in favour of your branch. |
|
#define RAOP_PACKET_LEN 32768 so if the 12-byte no data packets are AirMyPC (old protocol), we can just add the 4-byte NO-DATA marker to them. Have you even seen a 12-byte empty packet which is not from AirMyPC? (The sync stuff will soon be completely redone after some testing is complete.) |
a seqnum sequence with gaps was generating resend requests for the "missing" non-data packets. fixes #597
|
No, never. Every no-data packet we captured (iPhone 17,2, iOS 26) is the 16-byte form with the marker, so padding the 12-byte form looks right. ec20312 works live: 27 no-data packets over pauses, 0 resend requests. Nothing more from us on this one. Separate bug found on the way: mirror audio stays silent after a YouTube session. Draft fix in #604. |
Problem
During silence the sender switches to "no data" packets (12 bytes, or 16 bytes with the
0x00 0x68 0x34 0x00marker), and their seqnums continue the sequence. They were dropped before the buffer, so each of their seqnums looked like a lost packet: resend requests went out, and the audio after the silence was held until the 256-entry buffer overflowed.The first version of this PR said the sender skips seqnums over silence. That was wrong. As @fduncanh found, the sequence is complete and the gaps came from dropping these packets. This version uses the approach suggested there.
Change
raop_buffer_enqueue(): a "no data" packet is stored as an entry with no payload and is not decrypted.raop_buffer_dequeue(): skips those entries, so they never reachaudio_process.raop_rtp_thread_udp(): after the first sync, "no data" packets go to the buffer instead of being dropped. Before the first sync they are still dropped, so the start of the stream behaves as before.Testing
raop_rtp_thread_udp()does after the first sync. Cases: