OboeTester: Fix problem with phaseJitter=0 in DataPaths (#1978)

Set phase calculation as invalid when magnitude is low. Some tests has phaseJitter=0. Also some tests had very high phaseJitter and should have failed. So I lower the max allowable jitter.
google · Mar 13, 2024 · 79a44b2 · 79a44b2
1 parent c1d4eaf
commit 79a44b2
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 52 deletions.
diff --git a/apps/OboeTester/app/src/main/cpp/analyzer/BaseSineAnalyzer.h b/apps/OboeTester/app/src/main/cpp/analyzer/BaseSineAnalyzer.h
@@ -45,6 +45,10 @@ class BaseSineAnalyzer : public LoopbackProcessor {
         mScaledTolerance = mMagnitude * getTolerance();
     }
 
+    /**
+     *
+     * @return valid phase or kPhaseInvalid=-999
+     */
     double getPhaseOffset() {
         ALOGD("%s(), mPhaseOffset = %f\n", __func__, mPhaseOffset);
         return mPhaseOffset;
@@ -129,7 +133,18 @@ class BaseSineAnalyzer : public LoopbackProcessor {
         double cosMean = mCosAccumulator / mFramesAccumulated;
         double magnitude = 2.0 * sqrt((sinMean * sinMean) + (cosMean * cosMean));
         if (phasePtr != nullptr) {
-            double phase = atan2(cosMean, sinMean);
+            double phase;
+            if (magnitude < kMinValidMagnitude) {
+                phase = kPhaseInvalid;
+                ALOGD("%s() mag very low! sinMean = %7.5f, cosMean = %7.5f",
+                      __func__, sinMean, cosMean);
+            } else {
+                phase = atan2(cosMean, sinMean);
+                if (phase == 0.0) {
+                    ALOGD("%s() phase zero! sinMean = %7.5f, cosMean = %7.5f",
+                          __func__, sinMean, cosMean);
+                }
+            }
             *phasePtr = phase;
         }
         return magnitude;
@@ -153,9 +168,12 @@ class BaseSineAnalyzer : public LoopbackProcessor {
         if (mFramesAccumulated == mSinePeriod) {
             const double coefficient = 0.1;
             double magnitude = calculateMagnitudePhase(&mPhaseOffset);
-            ALOGD("%s(), mPhaseOffset = %f\n", __func__, mPhaseOffset);
-            // One pole averaging filter.
-            setMagnitude((mMagnitude * (1.0 - coefficient)) + (magnitude * coefficient));
+
+            ALOGD("%s(), phaseOffset = %f\n", __func__, mPhaseOffset);
+            if (mPhaseOffset != kPhaseInvalid) {
+                // One pole averaging filter.
+                setMagnitude((mMagnitude * (1.0 - coefficient)) + (magnitude * coefficient));
+            }
             resetAccumulator();
             return true;
         } else {
@@ -193,9 +211,19 @@ class BaseSineAnalyzer : public LoopbackProcessor {
     double  mPhaseIncrement = 0.0;
     double  mOutputPhase = 0.0;
     double  mOutputAmplitude = 0.75;
+    // This is the phase offset between the output sine wave and the recorded
+    // signal at the tuned frequency.
     // If this jumps around then we are probably just hearing noise.
+    // Noise can cause the magnitude to be high but mPhaseOffset will be pretty random.
+    // If we are tracking a sine wave then mPhaseOffset should be consistent.
     double  mPhaseOffset = 0.0;
+    // kPhaseInvalid indicates that the phase measurement cannot be used.
+    // We were seeing times when a magnitude of zero was causing atan2(s,c) to
+    // return a phase of zero, which looked valid to Java. This is a way of passing
+    // an error code back to Java as a single value to avoid race conditions.
+    static constexpr double kPhaseInvalid = -999.0;
     double  mMagnitude = 0.0;
+    static constexpr double kMinValidMagnitude = 2.0 / (1 << 16);
     int32_t mFramesAccumulated = 0;
     double  mSinAccumulator = 0.0;
     double  mCosAccumulator = 0.0;

diff --git a/apps/OboeTester/app/src/main/cpp/analyzer/DataPathAnalyzer.h b/apps/OboeTester/app/src/main/cpp/analyzer/DataPathAnalyzer.h
@@ -65,11 +65,13 @@ class DataPathAnalyzer : public BaseSineAnalyzer {
 
         if (transformSample(sample, mOutputPhase)) {
             // Analyze magnitude and phase on every period.
-            double diff = fabs(calculatePhaseError(mPhaseOffset, mPreviousPhaseOffset));
-            if (diff < mPhaseTolerance) {
-                mMaxMagnitude = std::max(mMagnitude, mMaxMagnitude);
+            if (mPhaseOffset != kPhaseInvalid) {
+                double diff = fabs(calculatePhaseError(mPhaseOffset, mPreviousPhaseOffset));
+                if (diff < mPhaseTolerance) {
+                    mMaxMagnitude = std::max(mMagnitude, mMaxMagnitude);
+                }
+                mPreviousPhaseOffset = mPhaseOffset;
             }
-            mPreviousPhaseOffset = mPhaseOffset;
         }
         return result;
     }

diff --git a/apps/OboeTester/app/src/main/cpp/analyzer/GlitchAnalyzer.h b/apps/OboeTester/app/src/main/cpp/analyzer/GlitchAnalyzer.h
@@ -183,17 +183,20 @@ class GlitchAnalyzer : public BaseSineAnalyzer {
                 mFramesAccumulated++;
                 // Must be a multiple of the period or the calculation will not be accurate.
                 if (mFramesAccumulated == mSinePeriod * PERIODS_NEEDED_FOR_LOCK) {
-                    setMagnitude(calculateMagnitudePhase(&mPhaseOffset));
-                    ALOGD("%s() mag = %f, mPhaseOffset = %f",
-                            __func__, mMagnitude, mPhaseOffset);
-                    if (mMagnitude > mThreshold) {
-                        if (fabs(mPhaseOffset) < kMaxPhaseError) {
-                            mState = STATE_LOCKED;
-                            mConsecutiveBadFrames = 0;
+                    double magnitude = calculateMagnitudePhase(&mPhaseOffset);
+                    if (mPhaseOffset != kPhaseInvalid) {
+                        setMagnitude(magnitude);
+                        ALOGD("%s() mag = %f, mPhaseOffset = %f",
+                              __func__, magnitude, mPhaseOffset);
+                        if (mMagnitude > mThreshold) {
+                            if (fabs(mPhaseOffset) < kMaxPhaseError) {
+                                mState = STATE_LOCKED;
+                                mConsecutiveBadFrames = 0;
 //                            ALOGD("%5d: switch to STATE_LOCKED", mFrameCounter);
+                            }
+                            // Adjust mInputPhase to match measured phase
+                            mInputPhase += mPhaseOffset;
                         }
-                        // Adjust mInputPhase to match measured phase
-                        mInputPhase += mPhaseOffset;
                     }
                     resetAccumulator();
                 }

diff --git a/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/BaseAutoGlitchActivity.java b/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/BaseAutoGlitchActivity.java
@@ -214,7 +214,6 @@ protected void log(String text) {
     protected void appendFailedSummary(String text) {
         mAutomatedTestRunner.appendFailedSummary(text);
     }
-
     protected void appendSummary(String text) {
         mAutomatedTestRunner.appendSummary(text);
     }

diff --git a/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/TestDataPathsActivity.java b/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/TestDataPathsActivity.java
@@ -19,11 +19,13 @@
 import static com.mobileer.oboetester.IntentBasedTestSupport.configureStreamsFromBundle;
 import static com.mobileer.oboetester.StreamConfiguration.convertChannelMaskToText;
 
+import android.app.Instrumentation;
 import android.media.AudioDeviceInfo;
 import android.media.AudioManager;
 import android.os.Build;
 import android.os.Bundle;
 import android.util.Log;
+import android.view.KeyEvent;
 import android.widget.CheckBox;
 import android.widget.RadioButton;
 import android.widget.RadioGroup;
@@ -88,11 +90,13 @@ public class TestDataPathsActivity  extends BaseAutoGlitchActivity {
 
     public static final int DURATION_SECONDS = 3;
     private final static double MIN_REQUIRED_MAGNITUDE = 0.001;
-    private final static double MAX_SINE_FREQUENCY = 1000.0;
+    private final static int MAX_SINE_FREQUENCY = 1000;
     private final static int TYPICAL_SAMPLE_RATE = 48000;
     private final static double FRAMES_PER_CYCLE = TYPICAL_SAMPLE_RATE / MAX_SINE_FREQUENCY;
     private final static double PHASE_PER_BIN = 2.0 * Math.PI / FRAMES_PER_CYCLE;
-    private final static double MAX_ALLOWED_JITTER = 2.0 * PHASE_PER_BIN;
+    private final static double MAX_ALLOWED_JITTER = 0.5 * PHASE_PER_BIN;
+    // This must match the value of kPhaseInvalid in BaseSineAnalyzer.h
+    private final static double PHASE_INVALID = -999.0;
     private final static String MAGNITUDE_FORMAT = "%7.5f";
 
     // These define the values returned by the Java API deviceInfo.getChannelMasks().
@@ -266,18 +270,20 @@ private void gatherData() {
             // Only look at the phase if we have a signal.
             if (mMagnitude >= MIN_REQUIRED_MAGNITUDE) {
                 double phase = getPhaseDataPaths();
-                // Wait for the analyzer to get a lock on the signal.
-                // Arbitrary number of phase measurements before we start measuring jitter.
-                final int kMinPhaseMeasurementsRequired = 4;
-                if (mPhaseCount >= kMinPhaseMeasurementsRequired) {
-                    double phaseError = Math.abs(calculatePhaseError(phase, mPhase));
-                    // collect average error
-                    mPhaseErrorSum += phaseError;
-                    mPhaseErrorCount++;
-                    Log.d(TAG, String.format(Locale.getDefault(), "phase = %7.4f, mPhase = %7.4f, phaseError = %7.4f, jitter = %7.4f",
-                            phase, mPhase, phaseError, getAveragePhaseError()));
+                if (phase != PHASE_INVALID) {
+                    // Wait for the analyzer to get a lock on the signal.
+                    // Arbitrary number of phase measurements before we start measuring jitter.
+                    final int kMinPhaseMeasurementsRequired = 4;
+                    if (mPhaseCount >= kMinPhaseMeasurementsRequired) {
+                        double phaseError = Math.abs(calculatePhaseError(phase, mPhase));
+                        // collect average error
+                        mPhaseErrorSum += phaseError;
+                        mPhaseErrorCount++;
+                        Log.d(TAG, String.format(Locale.getDefault(), "phase = %7.4f, mPhase = %7.4f, phaseError = %7.4f, jitter = %7.4f",
+                                phase, mPhase, phaseError, getAveragePhaseError()));
+                    }
+                    mPhase = phase;
                 }
-                mPhase = phase;
                 mPhaseCount++;
             }
         }
@@ -454,7 +460,8 @@ String getOneLineSummary() {
                 + " D=" + actualOutConfig.getDeviceId()
                 + ", ch=" + channelText(getOutputChannel(), actualOutConfig.getChannelCount())
                 + ", SR=" + actualOutConfig.getSampleRate()
-                + ", mag = " + getMagnitudeText(mMaxMagnitude);
+                + ", mag = " + getMagnitudeText(mMaxMagnitude)
+                + ", jitter = " + getJitterText();
     }
 
     @Override
@@ -533,18 +540,14 @@ int convertJavaInChannelMaskToNativeChannelMask(int javaChannelMask) {
     }
 
     void logOneLineSummary(TestResult testResult) {
-        logOneLineSummary(testResult, "");
-    }
-
-    void logOneLineSummary(TestResult testResult, String extra) {
         int result = testResult.result;
         String oneLineSummary;
         if (result == TEST_RESULT_SKIPPED) {
-            oneLineSummary = "#" + mAutomatedTestRunner.getTestCount() + extra + ", SKIP";
+            oneLineSummary = "#" + mAutomatedTestRunner.getTestCount() + ", SKIP";
         } else if (result == TEST_RESULT_FAILED) {
-            oneLineSummary = getOneLineSummary() + extra + ", FAIL";
+            oneLineSummary = getOneLineSummary() + ", FAIL";
         } else {
-            oneLineSummary = getOneLineSummary() + extra;
+            oneLineSummary = getOneLineSummary();
         }
         appendSummary(oneLineSummary + "\n");
     }
@@ -554,11 +557,6 @@ void logBoth(String text) {
         appendSummary(text + "\n");
     }
 
-    void logFailed(String text) {
-        log(text);
-        logAnalysis(text + "\n");
-    }
-
      private void testDeviceOutputInfo(AudioDeviceInfo outputDeviceInfo)  throws InterruptedException {
          AudioDeviceInfo inputDeviceInfo = findCompatibleInputDevice(outputDeviceInfo.getType());
          showDeviceInfo(outputDeviceInfo, inputDeviceInfo);

diff --git a/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/WaveformView.java b/apps/OboeTester/app/src/main/java/com/mobileer/oboetester/WaveformView.java
@@ -77,8 +77,9 @@ private void init() {
     protected void onSizeChanged(int w, int h, int oldw, int oldh) {
         mCurrentWidth = w;
         mCurrentHeight = h;
-        mOffsetY = 0.5f * h;
-        mScaleY = 0.0f - mOffsetY;
+        mOffsetY = 0.5f * h; // Center waveform vertically in the viewport.
+        // Scale down so that we can see the top of the waveforms if they are clipped.
+        mScaleY = -0.95f * mOffsetY; // Negate so positive values are on top.
     }
 
     public String getMessage() {
@@ -121,8 +122,8 @@ protected void onDraw(Canvas canvas) {
         float x0 = 0.0f;
         if (xScale < 1.0) {
             // Draw a vertical bar for multiple samples.
-            float ymin = mOffsetY;
-            float ymax = mOffsetY;
+            float ymin = mOffsetY; // vertical center
+            float ymax = mOffsetY; // vertical center
             for (int i = 0; i < mSampleCount; i++) {
                 float x1 = i * xScale;
                 if ((int) x0 != (int) x1) {

diff --git a/apps/OboeTester/app/src/main/res/layout/activity_data_paths.xml b/apps/OboeTester/app/src/main/res/layout/activity_data_paths.xml
@@ -16,18 +16,18 @@
         android:orientation="horizontal">
 
         <CheckBox
-            android:id="@+id/checkbox_paths_input_presets"
+            android:id="@+id/checkbox_paths_all_channels"
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
             android:checked="true"
-            android:text="InPre" />
+            android:text="AllCh" />
 
         <CheckBox
-            android:id="@+id/checkbox_paths_all_channels"
+            android:id="@+id/checkbox_paths_input_presets"
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
             android:checked="true"
-            android:text="AllCh" />
+            android:text="InPre" />
 
         <CheckBox
             android:id="@+id/checkbox_paths_all_sample_rates"