[pulseaudio-tickets] [Bug 42804] raop module does not work with shairport

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=42804

--- Comment #49 from Matthias <pulseaudio at niafc.de> ---
Hi Hajime,

sorry for keeping you up late with that patch. I would not have sent it if it
had not worked for me. Thank you for testing and (hopefully) fixing it!

>I tried your patch (on v4.0+raop at this moment), but unfortunately it did not work, in a sense that I still hear some sound jump when a packet retransmission happens.

I had a similar approach to yours regarding packet retransmission testing (i.
e. intentionally skipping packets in send_audio_packet()). When running this
version with retransmission disabled, result were exactly as you describe - the
remote device kept sending retransmit request. With retransmission enabled, the
device sent exactly one retransmit request (assuming that means the remote
device accepted the re-sent packet).
A possible explanation could be that my remote device accepts re-sent audio
packets at the streaming port (regardless of payload type(!)) if the sequence
number matches the one requested.

> 1. "payload type" of the retransmission packet should be 86 (0x56) [1]
> 2. retransmission packets should be sent to the control port [1], not to the
> streaming port
> 3. based on observation of a packet sequence from iPad to VSX-43, the
> retransmission packet seems to have a special structure. shairport's
> implementation for retransmission handling [2] supports this observation. 

Should have looked up [1] and [2] before sending this patch :/.

> struct retrans_pkt_hdr {
>     uint16_t rtphdr; /* = htons(0x80d6), where 0xd6 = 0x56(payload type) +
> 0x80(marker == on) */
>     uint8_t a; /* unknown; seems always 0x01 */
>     uint8_t b; /* unknown; seems some random number around 0x20~0x40  */
>     uint16_t orig_rtphdr; /* maybe; seems always htons(0x8060) */
>     uint16_t retrans_seq_num; /* in network byte order */
>     uint32_t retrans_ts; /* RTP timestamp in the original packet */
> };
> 
> I almost figured out the structure, but I couldn't figure out meanings in
> 'a' and 'b' members above.
> I'm not sure if this works for other devices, as I'm not 100% sure about the
> retransmission packet format.

I will test retransmission with Airtunes and my Minx Air. Maybe we can get some
information about the retransmission scheme from that.

> Besides the functionality issues above, I have a few questions regarding
> design.
> 1. in send_audio_packet(), it seems dangerous to use (retrans_seq_num > 0)
> as a flag, as the sequence number may wrap (1 in 65536).

Maybe a better way would be to add a flag parameter for retransmission and
another parameter for the sequence number (with c->seq as default).

> 2. I'm not sure if we really need to malloc buffer at resend_packets().
> Can't we just use the buffer stored in the pb? Since malloc may fail, I'd
> rather avoid calling malloc whenever possible.

I'm pretty sure there are better ways to do what I did :). As mentioned, my
experience in C (or anything without automatic memory management) boils down to
two weeks of patching.
Any feedback is welcome.

Will try your version with my device tonight and report back.

Greetings,

Matthias

>[1]: http://nto.github.io/AirPlay.html#audio-rtpstreams
>[2]: https://github.com/abrasive/shairport/blob/master/hairtunes.c#L476
>[3]: http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle/

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-bugs/attachments/20130904/e9d8f1b7/attachment.html>