Skip to content
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

Fixed 3 issues around AudioClockClient.AdjustedPosition #584

Merged
merged 2 commits into from
Jan 31, 2020

Conversation

goddogthedoggod
Copy link
Contributor

As I want to determine the exact playback position using WasapiOut, I had a look at the source code of AudioClockClient.AdjustedPosition and found that there were two points I did not understand:

  1. How the position returned by audioClockClientInterface.GetPosition was supposed to relate to a byte offset.
  2. Why there was a division by 100 in the calculation of qposDiff.

A third issue emerged while testing the fix to the pevious two: AdjustedPosition must not be used when playback is not playing.

Regarding 1.: The documentation specifies that the units of this device position are undefined, but that it can always be converted into seconds through division by the clock frequency. I realized that this position was being equated with the byte offset in the stream or, in other words, there was an assumption that the clock frequency always equals the number of bytes per second.
On the one hand, I testet this assumption on 5 output devices all by different manufacturers and found it to be true each time. On the other hand, I googled a few other open source projects that use WASAPI and found that none of them repeated this assumption, instead always dividing the position by the clock frequency.
Therefore I concluded that it would be prudent for NAudio to also perform this conversion from device position units to bytes and implemented it in WasapiOut.GetPosition (using outputFormat.AverageBytesPerSecond). Additionally, I changed mentions of the word "bytes" in the implementation of AudioClockClient.AdjustedPosition to clarify the distinction.

Regarding 2.: I checked the math of the implementation of AudioClockClient.AdjustedPosition and (substituting 10000000 for TimeSpan.TicksPerSecond) arrived at Frequency = (posDiff / deltaT) * 100, where posDiff is the renamed variable bytes (see above) and deltaT = (qposNow - qpos) / 10000000, i.e. deltaT is time passed according to the performance counter in seconds. Intuitively, Frequency is the number of device positions advanced during 1 second and, as it is constant, can be calculated from the number of device positions advanced during any time interval devided by that time interval or, in other words, should equal (posDiff / deltaT). This shows that the calculation was off by a factor of 100.
Therefore I removed the division by that 100 from the initialization of qposDiff.
I also took the liberty of inlining the variable byteLatency, thereby eliminating a division where it wasn't needed (when Stopwatch.IsHighResolution is false) and the name for a variable that has to do with neither bytes nor latency (AFAICT, as it's actually a conversion factor). Note that this affects the rounding error: previously byteLatency was rounded down, resulting in possibly increasing the value of bytes (aka. posDiff), now the rounding error in posDiff is smaller but will never increase its value. Conversely, rounding posDiff up might be preferrable as time continues passing while AdjustedPosition is being calculated.

Regarding testing 1.: With an output device that doesn't use bytes per second as the clock frequency, WasapiOut.GetPosition should have been off by a significant factor. However, I have not encountered such a device yet, such a device may not even exist. Therefore I do not see a practical way to (re)produce issue 1 and test that it's now fixed. I did compile and check with my test app that GetPosition still reports bytes, though.

Regarding testing 2.: The effect of issue 2 on the result of AdjustedPosition depends on how much time passes between qpos and qposNow. I suspect it would have been negligible except in cases where GetPosition returned false 5 times anyway. Example: for a 32-Bit, 48kHz, stereo stream, a qposDiff of at least 209 (20.9µs) is needed for the error to amount to 1 frame; with 11kHz mono, that doesn't even amount to 1 byte. Maybe in a test with intermittent bursts of high system stress involving high-priority events (as per the documentation), qposDiff might intermittently increase enough to make an uneven progression of WasapiOut.GetPosition measurable. However, since I discovered and corrected this issue by checking the relatively simple math of the source code, rather than e.g. by noticing something odd while using NAudio, I think that testing for this error should be unnecessary. I did compile and check that it still runs with my test app, though.

Issue 3: While testing, I paused playback while continuing to poll the result of WasapiOut.GetPosition. I noticed the value fluctuating by up to 2 bytes on my system. The reason was that AudioClockClient.AdjustedPosition continues to adjust the position as if playback is ongoing even when it is paused, coupled with the adjustment now being 100 times stronger. (Evidently, qposDiff stays less than 79 on my system.) I fixed this issue by letting WasapiOut.GetPosition use AudioClockClient.GetPosition instead of AdjustedPosition when playback is paused.
I decline creating a unit test for this issue as I don't have any experience with NUnit. I hope my explanation of this simple fix is convincing enough on its own. It would probably not be too difficult for someone to create a test for this, e.g. one that starts and pauses playback of a stream with at least 8 bytes per frame (higher is better) and then monitors WasapiOut.GetPosition for a second to detect the absence of fluctuations, while also monitoring AudioClockClient.AdjustedPosition to detect the presence of the fluctuations.

Sorry for the inclusion of the change in global.json. This is my first time using Git (or any distributed VCS) and I did not find a way to ignore this file when committing my changes. (I followed this tutorial. Pointers are welcome.) This change was needed on my system in order to be able to compile. As I have no experience with building UWP Apps or SDK-style projects, I elected to not figure out how to make the project compatible with all (future) versions of MSBuild.Sdk.Extras. (Pointers are, again, welcome.)

…n by 100.

Added conversion of result of AdjustedPosition from device position units into bytes to WasapiOut.GetPosition.
@markheath markheath merged commit f3c2a15 into naudio:master Jan 31, 2020
@goddogthedoggod goddogthedoggod deleted the fix-AdjustedPosition branch February 3, 2020 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants