-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: time synchronised playback #296
base: master
Are you sure you want to change the base?
Conversation
I haven't yet had a chance to look through this properly (let alone test it), but still, some quick thoughts. Primarily: this is a really important feature that shairport has never had. Thanks for addressing it, and for the quality code. What is the significance of I suggest using the linear filtering bits for all the offset measurement and calculation stuff. There are also 2 things that could be done better than linear filtering:
Suggest also using uint32_t, uint64_t rather than traditional C int names. cport, tport are malloc()'d but never free()'d in the timing fns commit. Delay can probably be estimated for generic outputs by seeing how many samples they swallow before slowing down to realtime data consumption; that's why the original buffer fill tracking sets its target fill value after it starts to play.
The player_thread_func is getting a bit heavy, I think it's time to break it up. Finally, the app should probably report its audio latency in RTSP header, once everything is firmed up a bit. |
Thanks for taking time to look into this.
when you get around to trying the code, note that I've only tested with the Alsa sink for now. |
Because silence is all full of zeroes, you don't need to start playing it partway through? |
ok, now I get it. |
Hey, this looks promising! Unfortunately, it segfaults on two machines, (32-bit and 64-bit). I can't seem to figure out how to get debugging symbols compiled, so logs from gdb would be useless. Oh, and it failed to get compile on the 32-bit one with undefined reference to `clock_gettime', I worked around that by adding -lrt. |
@MohammadAG Building with |
Here's a log with a backtrace then, thanks. http://pastebin.com/raw.php?i=EPRNNmsp Edit: okay, I've debug into the code more, it seems like this issue happens when neither alas or pulse audio are there. |
Hi Mohammad, |
Hi, Yup, you're right (I guess you didn't see my edit above :p) Should it be doing that? I'm trying to find the right value for config.delay, just in case it's the reason this is happening. Another edit: 1750000 seems to be perfect. |
Fixed some more leaks/naming. Anyway, the code is a decent enough state to add more features like support for generic outputs and a decent regulation. I'll be working on that now. |
I would like to express my interest in the functionality. Originally I was going to offer to help but it looks like the learning curve on the audio areas are pretty steep and it will probably take me awhile to get into understanding how all of this works. I'm thinking the only way to get synchronous whole home playback when paired with AirFoil and matched to other apple and licenced airplay devices is with time synched vs. buffered playback. I have all of the airport express hardware to not care, but think that this project would really kick off in embedded applications if the time synchronized feature was available. I want to build an embedded device that has airplay/google cast/and bt 4.0 incoming playback support with automatic handoff between protocols. Then I want to make an iOS/Android app that will let you create custom "zones" with your embedded devices paired in different combos. IE: Living Room + Kitchen = Zone A and then it would create services for AirPlay/GoogleCast. Basically an Audio only version of the AirPort Express with other protocol support and Android NFC pairing for BT. Your project is a great foundation in making something like that exist. |
Been low on activity, but still getting closer to completion :-) still need:
@arodd Great ideas! If you want to help; just testing code is already helpful - for any code base 👍 |
@Nachtzuster Great work, thanks |
That's not a BSD issue, it's a Mac issue. clock_gettime() is POSIX.1 as of 1993, but on a Mac you have to use Mach calls - host_get_clock_service() for the SYSTEM_CLOCK (which is supposed to be monotonic). Yum. |
This is a nice piece of work and I'm really excited to see it coming through. I've only had a quick look at it and realise there are couple of parts which are still being tuned so I'd just add two comments at this stage:
I'd be happy to soak test the timing/sync algorithm when it's refined. I run multiple Pi based Shairports plus a couple of airport expresses in a domestic environment. |
Had another play with this tonight and it works quite well for me, plus when combined with my suggested refinements of resync and retransmission ( #307 / #310 ) I can get a good stable operation. Just a couple of issues came out on the Pi:
Thanks again for the good work. |
Moving on: all features are in place and working more or less properly now, still some loose end though... and I guess the old rate control stuff should be ripped out? Anyway:
@BewickPlace: Thanks! testing again would be great now :-) |
I've spent some time on this today, looking at general performance together with implementing my #307 #310 changes as required. Regarding the rate control, I found the algorithm was divergent on my Pi, with the extremes of the error growing over time. I thought I might try copying the existing biquad algorithm, but then realised this wasn't necessary as "sync_time" is proportional to the error and by altering the playback rate by simple linear function of this I achieved a very stable sync. By taking elements of #307 and #310 updates relevant to the new buffering approach I have now not only been able to get rapid auto re-sync after underrun, but also achieve good synchronisation between the impacted device and other devices that are playing. One other change I made was to force all synchronisation to start from a timestamp - the "Ouch!" warning - as I found this was a repeatable situation when selecting the shairport device (on iTunes) when itunes was already streaming to another device. The overall package is a very pleasing step forwards. I would offer up my changes but am unsure how to push them back to someone else's fork. Do let me know. |
Strike most of my comments from above. I've just discovered that you updated the files after I'd pulled down a close and started trying it out. I haven't looked closely at your changes but a couple look very similar. I'll take a closer look when I have time. |
Yes. Please. Amazing piece of work! |
I agree great work! Have looked at it tonight and my comments above re the Ouch warning and the rate control still stand. So I pulled the changes into a branch off my fork and reimplemented my upgrades albeit on your slightly changed approach to resyncing. There are also a couple of tweaks tightening the resend and buffer handling. You can see this on my sntp branch. I hope the changes are something you want to take on board. As I said the results are really pleasing. |
Looks like iTunes isn't sending sync along with the first packet for late joiners.... Hey! that does not comply with the unofficial spec. :-) The ratecontrol not working is probably to another constant type error, maybe Looking to borrow a Pi to debug, that might help too ;-) |
Thanks for your help on this. I've been using my build of this (which is Pi based) over the last week or so and it remains good. I've now scaled this up with multiple "rooms" (devices) playing simultaneously. This is generally fine but I do get the occasional drop out. This is caused by the Pi wifi adapter!! (any help on this gratefully received) but the seamless in-sync recovery now does a excellent job for me. I posted up a couple of tweaks to the advance packet handling yesterday. |
@plietar I've added mach timing support. Well, at least I tried to, zero experience with OSX... Thanks a lot. |
Hi, I wanted to report my experience. I'm running the code in an olinuxino board (iMX233 454Mhz, 64Mb) with 3.14.0-1-ARCH linux with the integrated sound card. Transmitting from iTunes in a mac. After setting asound.conf to dmix and 44100 I was getting great results (with the master branch) for the first audio track, but if it keep playing the following ones, It started to increase the number of (++++++++) messages. If I restarted the track, it came perfect again, so there was some kind of accumulative problem with the playback rate I imagine. After trying with Nachtzuster branch, the transmission goes wild from the start. Do you know what it may be causing it? |
Looks like you're having alsa buffer underruns, hmmm. Are you seeing the same if you lower the verbosity to -vv -v or even w/o logging? |
yes |
@mcantarutti can't reproduce the issue I'm afraid: the dmix sink work fine here. so it might be specific to you alsa setup. |
@abrasive things are spread out in 16 commits right now, I'm guessing this should be reduced to 5-6 aptly commented commits - or you want everything squashed down into one commit? After that is done, I'm at the bottom of my to-do list @BewickPlace should compile/work on a Pi out-of-the-box now. |
A condensed patchset would be ideal, if you have the time. Looking forward to merging this! |
Inspired by the above issue/posts and with a philosophy of less is more I've taken the opportunity to revise the rate matching algorithm to take advantage of the fact that we have the actual sample delta from full sync. The overall gambit was to reduce the number of corrections (stuffs +ve or -ve) we were making as I found on my Pi I was seeing 600+ corrections per million samples. This clearly contributes to the noise issue. Trying the @mikebrady fork, which has it's own matching algorithm I was seeing circa 180 corrections per million. Following some of the logic from @mikebrady and @yenchee1970 the revised algorithm uses the filtered sample error and determines required stuffs after applying a quadratic. Minor stuffs are ignored although this has only marginal benefit, but may assist with keeping bit perfect playback in a suitably robust environment. The impact on my Pi has been good, bringing corrections down to circa 120ppm or less, and with a sync error which typically stays within +/- 100 samples. These were taken over a wireless network with iTunes on windows as the source. For iPad/iPhone the deltas are significantly larger, but the improvement is comparable. I'd be interested to know how this performs on other platforms, please let me have your comments and thoughts. I've just pushed the changes back to my sntp fork. |
Good job! However, I just tried it, and unfortunately I couldn't hear any improvements. The ticking distortion is just as bad as it was before. (GUIless Debian Wheezy x64, Async USB soundcard). Do you want me to record the distortion or doesnt it matter? |
I just tested the soxr improvement. It works great except raspberry pi is not powerful enough to handle too many resample chunk at a second. We have two options to improve it. I add a SOXR parameter to change the soxr quality to speed up it. The library default is low quality. It is typically 128 chunks per second in an airplay stream. With quick quality, Raspberry pi is able to handle it without glitch. The other option is to limit number of resample per second. I change the number to 32. As long as the sender and receiver are not more than 32 samples apart which turns to be 725us, both stream will be in sync at last. It works very well here with both Windows with Airfoil and iTune or iPad air. Anyone who is interested on my patch can retrieve it under my fork. |
@mantheman could you identify the correction rate you are seeing and the spread of sync error - these are shown at debug level 1. |
does this help: http://tny.cz/bc11592b |
@mantheman thanks for posting that. I've since re-checked my build and can hear the distortion introduced even with minor amounts of stuffing. I've since built in the libsoxr changes (including a couple of small fixes) and this works extremely well. I use a Raspberry-Pi and as @yenchee1970 identified we have to run at a lower soxr quality setting. I find this both eliminates the distortion and is sufficient for the Pi to continue to stuff at a maximum rate of 126 adjusted frames every ntp sync period (ie every frame) when it needs to. I'm also re-tuning the rate matching algorithm as the new stuffing has some impact. @yenchee1970 was there a reason why you implemented the soxr using short to float rather than sticking with the short as per @Nachtzuster? Unfortunately the logfile itself is showing too much detail (level 2 or 3?) to determine the true stuffing performance on your platform. It only has one "Playback: corrections" debug statement which shows corrections (stuffs) @ 1766 ppm. These show up about every 20 seconds and need themselves to be averaged. Not to worry there is no necessity to look more closely at this. I'll push the changes back to my fork in a few days once I'm happy with them. |
Could you implement an option to choose the resample quality: You might want to take a look at how Squeezelite have done it: http://manpages.ubuntu.com/manpages/trusty/man1/squeezelite.1.html Heres the output with loglevel 1: http://pastebin.com/ErDQArVm |
@BewickPlace I merged the original work from @Nachtzuster where I believe the oneshot API takes float array only. To use interger instead, you have to use stream API. In the mean time, float shall give you better quality and dynamic range than all integer implementation unless it is changed to float in the library. If that is the case, it does not improve anything to use all short implentation. |
For those that are interested my fork is now up to date with both revised rate matching (lower) stuffs and libsoxr stuffing using @Nachtzuster's revised (short) implementation. I've included simple parameterisation for libsoxr Quality as per @yenchee1970. |
@BewickPlace I took a look at rate.h and I found all the calculation is done in double datatype while computing. Also, oneshot is able to deal with int16s. I modify my code to use that type directly, and remove float translation completely. It looks more clean now. |
I've been working on improving playback timing accuracy/stability. These last two commits do just that, but are still work-in-progress...
|
Dear @Nachtzuster,
What kind of additional information may help you with debugging? |
I'm unfortunately not enough of a coder to contribute, but I am looking forward to attempting to compile this to run on a TL-WR703n router running OpenWRT. |
While working on Shairport Sync – 2.1 is just out, with libsoxr support – I believe I came across errors in the handling of the packet buffer which might also be affecting your work. FWIW, I could not convince myself that the seq_diff and seq_order worked properly with respect to wraparound from 65535 to 0, so I wrote versions I could believe in – in player.c. |
@mikebrady With soxr enabled, I can still hear static noises the first 10 seconds, albeit much lower in strenght, After those 10 seconds soxr seems to work properly. It seems the heavy resyncing at the beginning is throwing the resampling a little bit off. If I remember correctly, I didn't hear any noise at all with @BewickPlace implementation of soxr. (But without soxr it has an unbearable amount of noise) Another thing is that the 1khz file is just audio, it shouldn't be trying to resync it at all unless to it drifts off really bad. It may or may not be possible to differentiate between video and just audio, but you guys know this better than me. |
Thanks for the report. You are right that resyncing is more "aggressive" in 2.1 — I was concentrating on allowing libsoxr to run without undue constraint due to processor load, and I removed the "throttling" code. I'll put something like it back, to limit the maximum rate of resyncing. The sync signals come from the source even when it's playing audio to one sink, so there will always be sync activity. |
Just pushed a slight update limiting interpolation to about 1 in 1000 frames (about one in three packets). For the life of me, I can't hear the fixed 1-2 second interval noise bursts you refer to. I'm testing on an iMac with iTunes over Ethernet to a Raspberry Pi driving a Topping TP30 and also an iPhone 5 with the Music app to the same Raspberry Pi. Here are representative log lines with soxr stuffing enabled:
|
This is my commandline: shairport -d -a This is a recording done with my iphone of the 1khz tone playing (played directly from safari ios) through my stereo. The Dac is an Ayre QB-9 with the 192khz xmos USB chip. This is with Soxr enabled : https://www.dropbox.com/s/ddpmw4h72uajnpp/Shairport%20Sync%202.1%20Soxr%20Commit%20601.m4a?dl=0 I also got a compiler warning: My system is a Debian Wheezy amd64. |
Thanks for that. As you say, the crackles on the 1kHz tone are a bit academic. I pushed a few tiny updates that reduce them a bit further, I think. Could you turn on -v and get a few stats from the log file? |
Sounds about the same. Shairport 2.0 corrects a littles less, but makes a little more noise when doing so, whereas 2.1 corrects a little all the time. That is my impression. Play connection from "xxx's iPad". (ipad 4 ios 7.1.2 safari browser playing http://www.realmofexcursion.com/audio/1khz.mp3) Play connection from "iTunes/12.0 (Macintosh; OS X 10.10)". (Norah Jones - Chasing Pirates alac) Btw can you fix the --on-start --on-stop options? It works with the official version, and also when I merged the on-start/stop/wait commit myself into shairport-sync 2.0. Of course I see why it doesn't work, and I could just make a script, but.. |
Thanks again. Nothing there jumping out – a lowish rate of correction, no untoward overcorrection. Regarding the –on-start stuff, maybe we should move the discussion to mikebrady#7? |
This converts buffered playback to time synchronised playback.
-adds a ntpsender and a ntpreceiver thread to measure sender time offset
-tries to synchronise audio at the sync packets by adjusting the playback rate
The code is not quite ready for show time:
Regulation is twitchy
There are audible glitches maybe due to the above?
Time related function should probably be moved to timing.[ch]
What to do with sinks that do not provide delay information?
Anyway, I'd like some feedback from interested parties.