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

Create AudioRecord using reflection as a fallback #3862

Closed
wants to merge 3 commits into from

Conversation

yume-chan
Copy link
Contributor

@yume-chan yume-chan commented Mar 26, 2023

Fixes #3805

It duplicated all code from AudioRecord's constructor and AudioRecord.Builder.build() for both Android 11 and Android 12.

Android 13 uses AudioAttributes.Builder.replaceTags() instead of AudioAttributes.Builder.addTag(). The former may copy more fields from the source attributes, but not using it (in this PR) doesn't seem to matter.

It uses native reflections, but the code is much harder to read with that. Most methods/fields are only used once so performance shouldn't change even if joor was used. Jar package size grows from 52KB to 57KB (previous one using joor was 61KB).

The linter doesn't like it using private APIs, so a lint-baseline.xml file was created according to the docs. To update it, simply delete it and rebuild the project. Doesn't know why it can still access those APIs even if they are blocked, maybe Android Shell process doesn't have that restriction. Using joor won't cause those linting errors (and don't need the lint-baseline.xml file) maybe because the linter can't recognize the usage.

Tested (by using this build() method forcefully) on Android 11 and Android 13 emulator.

Expected to break in Android 14 since the addition of deviceId parameter.

@rom1v
Copy link
Collaborator

rom1v commented Mar 30, 2023

Thank you for your work 👍

It uses native reflections, but the code is much harder to read with that. Most methods/fields are only used once so performance shouldn't change even if joor was used. Jar package size grows from 52KB to 57KB (previous one using joor was 61KB).

Sure, but I want to avoid adding dependencies for only minor convenience gains. This adds complexity at other places (e.g. when the project is not built with gradle or to make offline builds), without mentioning maintenance, upgrade…

The linter doesn't like it using private APIs, so a lint-baseline.xml file was created according to the docs. To update it, simply delete it and rebuild the project. Doesn't know why it can still access those APIs even if they are blocked, maybe Android Shell process doesn't have that restriction. Using joor won't cause those linting errors (and don't need the lint-baseline.xml file) maybe because the linter can't recognize the usage.

Adding the following annotation seems sufficient:

@SuppressLint("SoonBlockedPrivateApi")
public class AudioRecordWrapper {

It duplicated all code from AudioRecord's constructor and AudioRecord.Builder.build() for both Android 11 and Android 12.

Thank you for taking care of all the difference for each version! That's a lot of painful work :)

IMO, we should minimize the duplicated code from AOSP, this is just a workaround with known values. For example, in the implementation of AudioRecordWrapper.build(), the tests relative to format or "privacy sensitive" are probably not necessary.

I think we don't even need a reimplementation of AudioRecord.Builder.build(), but just initialize an AudioRecord with the expected values, like you did here. Something like AudioRecordWorkarounds.createAudioRecord(MediaFormat format, int minBufferSize, …).

This should probably remove the need of most of AudioFormatWrapper and AudioAttributesWrapper (and the remaining methods, if any, could be inlined). What do you think?

Also, I noticed that this fallback code is executed on Android 11 when the screen is locked (even on non Vivo devices), IMO it should not (and it interacts with the retrying mechanism introduced by 02f4ff7). I don't know how we should detect that a failure is due to screen lock or because the AudioRecord itself is buggy/modified by the vendor on the device. Maybe we should limit this to vivo devices? In the past, I had so many problems introduced on other devices due to a workaround specific to meizu phones…

@yume-chan
Copy link
Contributor Author

I removed some checks, but I don't want to deviate it from AOSP too much, so others can read it more easily.

Maybe we should limit this to vivo devices?

Not sure how to do this. What's the Build.Brand value for those devices? vivo and iqoo?

rom1v pushed a commit that referenced this pull request May 24, 2023
Some devices (Vivo phones) fail to create an AudioRecord from an
AudioRecord.Builder (which throw a NullPointerException).

In that case, create an AudioRecord instance directly by reflection.

The AOSP version of AudioRecord constructor code can be found at:
 - Android 11 (R):
   <https://cs.android.com/android/platform/superproject/+/android-11.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=335;drc=64ed2ec38a511bbbd048985fe413268335e072f8>
 - Android 12 (S):
   <https://cs.android.com/android/platform/superproject/+/android-12.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=388;drc=2eebf929650e0d320a21f0d13677a27d7ab278e9>
 - Android 13 (T, functionally identical to Android 12):
   <https://cs.android.com/android/platform/superproject/+/android-13.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=382;drc=ed242da52f975a1dd18671afb346b18853d729f2>
 - Android 14 (U): Not released, but expected to change

PR #3862 <#3862>
Fixes #3805 <#3805>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented May 24, 2023

Thank you @yume-chan for your latest version 👍

I move your code to Workarounds.createAudioRecord(), and removed even more unnecessary parts: vivo_workaround. What do you think?

Here is a binary:

cc @A-viral-dev (#3805)


EDIT: I also moved the URLs pointing to AOSP AudioRecord implementation from code comments to the commit message. I could invent a reason, but the real one is that I did not manage to silent the linter errors about the lines greater than 150 chars due to the long URLs 😄

@Genymobile Genymobile deleted a comment from mamdouh911321 May 24, 2023
@rahaaatul
Copy link

Here is a binary:

I tried this and got this error:

INFO: Renderer: direct3d
INFO: Initial texture: 720x1600
WARN: Demuxer 'audio': stream explicitly disabled by the device
[server] ERROR: Exception on thread Thread[Thread-4,5,main]
java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.pm.PackageManager android.content.Context.getPackageManager()' on a null object reference
        at android.content.ContextWrapper.getPackageManager(ContextWrapper.java:102)
        at android.content.ContextWrapper.getPackageManager(ContextWrapper.java:102)
        at android.media.VivoAudioRecordImpl.getSignatures(VivoAudioRecordImpl.java:100)
        at android.media.VivoAudioRecordImpl.getSignInfo(VivoAudioRecordImpl.java:68)
        at android.media.VivoAudioRecordImpl.isSupportSubMixRecording(VivoAudioRecordImpl.java:133)
        at android.media.AudioRecord.<init>(AudioRecord.java:433)
        at android.media.AudioRecord$Builder.build(AudioRecord.java:798)
        at com.genymobile.scrcpy.AudioCapture.createAudioRecord(AudioCapture.java:58)
        at com.genymobile.scrcpy.AudioCapture.start(AudioCapture.java:90)
        at com.genymobile.scrcpy.AudioEncoder.encode(AudioEncoder.java:183)
        at com.genymobile.scrcpy.AudioEncoder.lambda$start$0$com-genymobile-scrcpy-AudioEncoder(AudioEncoder.java:120)
        at com.genymobile.scrcpy.AudioEncoder$$ExternalSyntheticLambda0.run(Unknown Source:2)
        at java.lang.Thread.run(Thread.java:923)```

@rom1v
Copy link
Collaborator

rom1v commented May 25, 2023

@rahaaatul Your stacktrace (line numbers) does not match the binary I posted, but matches v2.0, so you're probably not using the binary posted in the previous command.

How did you run it?

@rahaaatul
Copy link

@rahaaatul Your stacktrace (line numbers) does not match the binary I posted, but match v2.0, so you're probably not using the binary posted in the previous command.

How did you run it?

Oh shoot! My bad! I forgot that I added the released version in the PATH.

So after I downloaded your new binary, I extracted and opened the terminal and ran scrcpy there. Most likely it ran the other one in the PATH.

Sorry to bother you like that, I will try again. Please accept my apology.

@rahaaatul
Copy link

Okay, just tested it works fine. But the screen must be on first.

@rom1v
Copy link
Collaborator

rom1v commented May 25, 2023

But the screen must be on first.

Android 11?

@rahaaatul
Copy link

But the screen must be on first.

Android 11?

Yes

@yume-chan
Copy link
Contributor Author

I move your code to Workarounds.createAudioRecord(), and removed even more unnecessary parts: vivo_workaround. What do you think?

I think those are good simplifications. One thing I noticed is that the mChannelMask field wasn't being set in your code, but if it still works, maybe it's not used by Android.

rom1v pushed a commit that referenced this pull request May 25, 2023
Some devices (Vivo phones) fail to create an AudioRecord from an
AudioRecord.Builder (which throw a NullPointerException).

In that case, create an AudioRecord instance directly by reflection.

The AOSP version of AudioRecord constructor code can be found at:
 - Android 11 (R):
   <https://cs.android.com/android/platform/superproject/+/android-11.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=335;drc=64ed2ec38a511bbbd048985fe413268335e072f8>
 - Android 12 (S):
   <https://cs.android.com/android/platform/superproject/+/android-12.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=388;drc=2eebf929650e0d320a21f0d13677a27d7ab278e9>
 - Android 13 (T, functionally identical to Android 12):
   <https://cs.android.com/android/platform/superproject/+/android-13.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=382;drc=ed242da52f975a1dd18671afb346b18853d729f2>
 - Android 14 (U): Not released, but expected to change

PR #3862 <#3862>
Fixes #3805 <#3805>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented May 25, 2023

One thing I noticed is that the mChannelMask field wasn't being set in your code

Indeed, it is better to set it to the correct value (even if it works without). I updated the branch.

rom1v pushed a commit that referenced this pull request May 26, 2023
Some devices (Vivo phones) fail to create an AudioRecord from an
AudioRecord.Builder (which throws a NullPointerException).

In that case, create an AudioRecord instance directly by reflection.

The AOSP version of AudioRecord constructor code can be found at:
 - Android 11 (R):
   <https://cs.android.com/android/platform/superproject/+/android-11.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=335;drc=64ed2ec38a511bbbd048985fe413268335e072f8>
 - Android 12 (S):
   <https://cs.android.com/android/platform/superproject/+/android-12.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=388;drc=2eebf929650e0d320a21f0d13677a27d7ab278e9>
 - Android 13 (T, functionally identical to Android 12):
   <https://cs.android.com/android/platform/superproject/+/android-13.0.0_r1:frameworks/base/media/java/android/media/AudioRecord.java;l=382;drc=ed242da52f975a1dd18671afb346b18853d729f2>
 - Android 14 (U): Not released, but expected to change

PR #3862 <#3862>
Fixes #3805 <#3805>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented May 26, 2023

Merged into dev: cab3541

@WangShayne
Copy link

On my device, this doesn't seem to be a scrcpy bug,i dont't know. because I turned off selinux permissions, which is permissive. When I set "setenforce 1" in the shell again, it worked fine

@yume-chan yume-chan deleted the fix/vivo-audio-2 branch December 22, 2024 13:16
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.

4 participants