-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve audio recording configuration #2752
Conversation
From Android 10, an apps with privacy sensitive audio sources have higher priority https://developer.android.com/media/platform/sharing-audio-input#behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple general comments -
-
Can we add a Jira link and repro steps in the PR description to help anyone coming across this PR in future.
-
We also discussed setting up the audio config callback and pause recording on our side if we get `isClientSilenced() as true to handle situations in which user navigates away from CC recording and start a new recording. Curious if there is a reason so that we decided to not implement it ?
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { | ||
recorder.setAudioSource(MediaRecorder.AudioSource.VOICE_COMMUNICATION); | ||
} else { | ||
recorder.setAudioSource(MediaRecorder.AudioSource.MIC); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test that the recording with source set to VOICE_COMMUNICATION
are playable in browser (instead of download) on HQ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tested it and it plays normally.
|
I would think that we should include it as it seems to be the most fullproof solution to have empty recordings (callback works irrespective of Android version ?) |
From Android 7.1. But the tricky part is in assessing when the recording has gone silent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For versions prior to 10, I was considering using getMaxAmplitude() but I wasn't able to thoroughly test its reliability. Any thoughts?
I think the only way to check will be to test it out. Have you been facing difficulties testing ?
The testing was successful. The tricky part is in having certainty that 0 (zero) amplitude always means that the recording has gone silent and not that maybe that there is no sound to record, i.e. people stopped talking and there is no background noise. |
Are you getting a 0 output from the method in this case ? |
Not necessarily, which gives me 80% confidence that this option should work fine, so happy to push this out and have QA confirm that that's the case. Agree? |
Sounds good to me. |
|
||
try { | ||
Thread.sleep(500); | ||
} catch (InterruptedException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel right to me. If we don't think recorder.getMaxAmplitude()
, lets not use it at all instead of hacking something ambiguous here. I would suggest that we only do a one time check on recorder.getMaxAmplitude()
and treat it as reliable unless we find otherwise
da57e51
to
93e8e8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
@damagatchi retest this please |
Summary
This PR was motivated by an issue with the Audio Recording feature in which the resulting recording had no sound. The issue was replicated by recording audio with CommCare and another voice recorder app at the same time, it was noticed that navigating away from CommCare would cause the recording to go silent. A few options were entertained as potential solutions, but ultimately taking advantage of some recording configuration elements introduced in the latest versions of Android, proved to be sufficient to ensure that CommCare audio recording feature is handled with higher priority by the system.
While it was possible to replicate the issue, it's important to note that the actual conditions in which the issue occurred could not be established as access to the list of apps in the affected devices were not possible.
The actual replication steps are:
Ticket: https://dimagi.atlassian.net/browse/SAAS-15417
Product Description
There are a couple change visible to the user:
Safety Assurance
Safety story
These changes were successfully tested on Android 9, 10, 11 and 12.
QA Note: Test for bug fix and add a test in QA plan to test for simultaneous recordings.