Fix test criteria to fix paqa_latency.c - #1111
Conversation
|
After discussion with Ross, the plan is to:
|
deb402c to
fbecb61
Compare
I added that test. But we are seeing failures where the latency hits the maximum and then keeps going up! also |
|
NEXT STEP: confirm that rounding error is the problem. investigate source of rounding error. consider revising the spec I think that the test is valid with respect to the current spec. I think the failing tests reflect a bug in the code in the sense that the test results indicate a violation of the current spec for Lines 570 to 580 in 5d01510 I suspect the issue is that there is rounding down when the spec says to always round up. This could easily be a bug in PA/CoreAudio. It would be good to know whether the test also fails with other host APIs. We are undecided whether rounding down within some epsilon should be in specification . It would be user friendly to allow for +/- half (or 1/4 or 0.1) sample period of slop to allow for incorrect rounding in floating point calculations. I definitely think that the actual latency should be reported as accurately as possible, so the code should not "pretend" to have honoured the suggested latency when it actually rounded down, even a little bit. |
|
Suggestions for improving the test:
|
|
Print the delta value between final and suggested latency with scientific notation. |
RossBencina
left a comment
There was a problem hiding this comment.
Phil and I agreed to merge this as a failing test once the following two improvements are made:
- Print in seconds (next to sample rate) 1/sampleRate
- Print suggested, final delta
Just expect monotonic increases, and not much higher latency than suggested. Fixes #742
|
I made the suggested changes. Now I get this: See attachment for the full log: qa_latency_PR1111_20160716.txt |
|
TODO Could add a test for more serious failures, "smoke". Call this monotonic test paqa_latency_monotonic.c Suggest 0.0, get >= defaultMinLatency. |
|
I think test could be merged now, but I agree that making the output easier to interpret would be helpful. We also discussed splitting out a smoke test:
|
This allows the rest of paqa_latency.c to pass.
|
I pulled out the monotonic test that was failing. |
| @@ -0,0 +1,244 @@ | |||
| /** @file paqa_latency.c | |||
| @@ -0,0 +1,244 @@ | |||
| /** @file paqa_latency.c | |||
| @ingroup qa_src | |||
| @brief Test latency estimates. | |||
There was a problem hiding this comment.
Describe monotonic test.
| @author Phil Burk <philburk@softsynth.com> | ||
| */ | ||
| /* | ||
| * $Id: patest_sine.c 1368 2008-03-01 00:38:27Z rossb $ |
|
I need to run autoconf to add the new test. |
RossBencina
left a comment
There was a problem hiding this comment.
Looks good. I've made some suggestions to improve readability.
|
|
||
| if( highLatency < 0.001 ) | ||
| { | ||
| numLoops = 2; /* Just test 0 and high. Don't divide by 0. */ |
There was a problem hiding this comment.
Comment is a bit cryptic. It might be clearer if you assign both values of numLoops here, something like:
numLoops = (highLatency < 0.001)
? 2 /* test suggestedLatency 0 and highLatency. note numLoops must be > 0 to avoid divzero */
: 11;
Buy why 11?
| PaStreamParameters streamParameters; | ||
| const PaStreamInfo* streamInfo; | ||
| double lowLatency; | ||
| double highLatency; |
There was a problem hiding this comment.
I'm not a fan of "final". It doesn't carry enough meaning, especially given that is exactly the quantity being tested by this test. Suggest rename finalLatency to reportedLatency or to streamInfoLatency, I have a slight preference for reportedLatency but if you want to keep it short could go with 'infoLatency' or 'streamLatency'. I think we already agreed that "actual latency" is not the right concept here.
| static int paqaCheckMultipleSuggested( PaDeviceIndex deviceIndex, int isInput ) | ||
| { | ||
| int i; | ||
| int numLoops = 11; |
There was a problem hiding this comment.
Why 11? move initialization above loop see comment below.
| printf(INDENT "suggested[%2d] = %8.6f", i, streamParameters.suggestedLatency ); | ||
|
|
||
| err = Pa_OpenStream( | ||
| &stream, |
There was a problem hiding this comment.
check style guide for indent policy. i think 2 tabstops right of block
|
|
||
| #define INDENT " " | ||
| /*******************************************************************/ | ||
| static int paqaCheckMultipleSuggested( PaDeviceIndex deviceIndex, int isInput ) |
There was a problem hiding this comment.
?rename paqaCheckStreamLatencyIsMonotonic or paqaCheckStreamMonotonicLatency
| /* If we are not at maximum then we should be rounding up. */ | ||
| if (atMaximumLatency == 0) { | ||
| QA_ASSERT_TRUE("Latency should be >= suggestedLatency", | ||
| finalLatency >= streamParameters.suggestedLatency); | ||
| } else { | ||
| QA_ASSERT_TRUE("Latency should be == detectedMaximumLatency", | ||
| finalLatency == detectedMaximumLatency); | ||
| } |
There was a problem hiding this comment.
| /* If we are not at maximum then we should be rounding up. */ | |
| if (atMaximumLatency == 0) { | |
| QA_ASSERT_TRUE("Latency should be >= suggestedLatency", | |
| finalLatency >= streamParameters.suggestedLatency); | |
| } else { | |
| QA_ASSERT_TRUE("Latency should be == detectedMaximumLatency", | |
| finalLatency == detectedMaximumLatency); | |
| } | |
| if (atMaximumLatency == 0) { | |
| /* Below the maximum, stream should always round latency up. */ | |
| QA_ASSERT_TRUE("Latency should be >= suggestedLatency", | |
| finalLatency >= streamParameters.suggestedLatency); | |
| } else { | |
| /* Once the maximum has been reached, reported latency should remain constant. */ | |
| QA_ASSERT_TRUE("Latency should be == detectedMaximumLatency", | |
| finalLatency == detectedMaximumLatency); | |
| } |
| // Get the latency from the streamInfo now because it will be invalid after the | ||
| // stream is closed. |
There was a problem hiding this comment.
C++ comments (seems weird having a mixture in this file)
| finalLatency >= previousLatency); | ||
| QA_ASSERT_TRUE("Latency should be > 0.0", finalLatency > 0.0); | ||
| if (atMaximumLatency == 0) { | ||
| /* If we get a lower value then we must be clipping at max latency. */ |
There was a problem hiding this comment.
| /* If we get a lower value then we must be clipping at max latency. */ | |
| /* When not yet at the maximum, interpret a reported stream latency | |
| that is less than the suggested latency as clipping at max. */ |
| QA_ASSERT_TRUE("Latency should be monotonically non-decreasing with suggested latency.", | ||
| finalLatency >= previousLatency); | ||
| QA_ASSERT_TRUE("Latency should be > 0.0", finalLatency > 0.0); |
There was a problem hiding this comment.
Put sanity check first.
| QA_ASSERT_TRUE("Latency should be monotonically non-decreasing with suggested latency.", | |
| finalLatency >= previousLatency); | |
| QA_ASSERT_TRUE("Latency should be > 0.0", finalLatency > 0.0); | |
| QA_ASSERT_TRUE("Latency should be > 0.0", finalLatency > 0.0); | |
| QA_ASSERT_TRUE("Latency should be monotonically non-decreasing with increasing suggested latency.", | |
| finalLatency >= previousLatency); |
| QA_ASSERT_TRUE("Latency should be monotonically non-decreasing with suggested latency.", | ||
| finalLatency >= previousLatency); | ||
| QA_ASSERT_TRUE("Latency should be > 0.0", finalLatency > 0.0); | ||
| if (atMaximumLatency == 0) { |
There was a problem hiding this comment.
Add explanatory comment:
| if (atMaximumLatency == 0) { | |
| /* The state machine below checks that the reported stream latency | |
| * remains at or above the suggested latency until the point at which | |
| * the reported stream latency clamps at its maximum. | |
| * | |
| * latency ^ . | |
| * value | ___._______ | |
| * | /. | |
| * | _- '. . . . suggested latency | |
| * | / . _______ reported ("final") latency | |
| * | -. | |
| * '---------------------> | |
| * 0 loop iteration | |
| */ | |
| if (atMaximumLatency == 0) { |
It was failing because the latency was too low.
Now we allow lower latency.
Fixes #742