-
-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Implement internal audio/video recording for devices >= Android Q #88
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
Conversation
|
|
|
addressing appium/appium#13177 |
|
It works flawlessly as far as I tested it on my android device with latest appium, note: io.appium.settings apk which includes this commit must be built and installed and above 3 adb commands need to be issued first, here is sample test script including recording https://gist.github.com/sirmordred/4c505ddce96f7513bfbe49d38ef041be |
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.
Thank you.
Is there any reason why you did not add this feature in https://github.com/appium/appium-uiautomator2-server , for example, as a mobile: command? https://github.com/appium/appium-uiautomator2-driver
(kind of a basic question as my out of curiosity)
[updated]
ah, this was for the below comment in the linked issue?
Which means one can simply install any sound recording app, control it using UIA2 to start/stop recording process and be happy about it %)
| // adb shell appops set io.appium.settings PROJECT_MEDIA allow | ||
|
|
||
| // TO START SCREENRECORD: | ||
| // adb shell am start -n "io.appium.settings/io.appium.settings.Settings" -a io.appium.settings.recording.ACTION_START --es "recording_filename" "abc.mp4" |
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.
Should this be start activity? I wonder if this feature can be via broadcast. (I haven't dug in the recording feature, so I might miss something)
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.
i tried the initiating recording with broadcast but since i had to trigger startActivityForResult() method supplied with mediaprojection intent object (see https://github.com/sirmordred/io.appium.settings/blob/master/app/src/main/java/io/appium/settings/Settings.java#L108) i had to use Settings activity
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.
but i will try to dig more on this
|
AFAIK app sound would only be recorded if the particular app allows that. Also, does the |
|
thank you so much for your review and feedbacks, i'll address them asap i hope, im doing this in my free time and i'll be on vacation for a while, after that i will continue to work on it and try to address your questions etc. thanks |
app/src/main/java/io/appium/settings/recorder/RecorderConstant.java
Outdated
Show resolved
Hide resolved
app/src/main/java/io/appium/settings/recorder/RecorderConstant.java
Outdated
Show resolved
Hide resolved
app/src/main/java/io/appium/settings/recorder/RecorderService.java
Outdated
Show resolved
Hide resolved
|
okay while i was testing recording in different emulators (even if it worked perfect on real devices, Huawei Android 10 and Samsung Android 12) it crashed on emulators (media encoder was crashing while configuring it with given width/height resolutions) and i realized that this is because media codecs and its capabilities as well as device display resolutions vary device-to-device very much and the core problem is media encoder/decoder (encoder in our case) is not GUARANTEED to encode/decode in given any resolutions (width/height) in the past, we were giving raw display width/height to media encoder https://github.com/sirmordred/io.appium.settings/blob/203ef2dd68231f69d368a3a64a1909f614f3aaa8/app/src/main/java/io/appium/settings/recorder/RecorderThread.java#L372 of course we were clamping it, but clamp was not not working reliably, even if we clamp width/height against media encoder's capabilities, it was resulting values like 1080x1823 (some devices have notch or virtual back/home button fields some not) and media encoder was crashing with these type of values, so i deep dive into media encoder and its capabilities, i saw that media encoder can support up to 5 different resolution modes, i got that assumption from CTS tests, see https://github.com/aosp/cts/blob/08e720f8ee9a1170d995b03b748528098f22e101/tests/tests/media/src/android/media/cts/VideoEncoderTest.java#L1346 and above, since all released android phones MUST pass CTS tests, we can safely assume that media encoder can support these resolution modes, so i added these resolution modes and helper functions to detect supported max resolution on-device, and media encoder is now works without crashing as far as i tested it on both real-devices and emulators 👍 i also added user-selectable-resolution-mode argument to making it dynamic (didnt test different resolutions yet, only tested max supported resolution on device, but will test it ASAP 👍 EDIT: i tried and it works great #88 (comment) ) other changes:
|
|
Voilaa 💯 i tested different resolution modes and it works perfect 👍 and resulting output videos are really smaller in size (as expected) we now support different resolutions 👍 |
|
AFAIK the AVC video encoder used here supports pretty wide set of resolutions. One of main requirements to these resolutions is, though, that both width and height must be divisible by two. You could also use https://stackoverflow.com/questions/44902334/how-to-get-max-resolution-of-mediacodec-before-configuring to check if the particular width, height and frame rate are supported by the selected codec instead of hardcoding the limits. Usually one would only provide the scale value to keep the original aspect ratio rather than both width and height |
|
In iOS, for example, we set the scale value to 2, which means the video resolution is by default half of the original device resolution |
i thought the same but in android's AVC/H264 codec, i saw and experienced supported resolutions are not really that much wide, for example try the following code on any high-end phone or low-end phone (it doesn't matter) it will print "yes" for all resolutions from 176x144 to 1920x1080 range but it will print "no" for the last one -native 2K resolution 2560x1440- even though it is divisible by 2, so we are much likely limited to these resolutions i guess |
|
Okay i have done everything in my mind/which i noted 👍 also tried on the following devices and i can confirm its working good without problem 👍
EDIT: lastly i tried it on Pixel 2 Phone Emulator running upcoming Android 13, and it worked fine too without problem 👍 |
README.md
Outdated
|
|
||
| ### Arguments (see above start command as an example for giving arguments) | ||
| - filename (Mandatory) - You can type recording video file name as you want, but recording currently supports only "mp4" format so your filename must end with ".mp4" | ||
| - priority (Optional) - Default value: 2 which means recording thread priority is Maximum however if you face performance drops during testing with recording enabled, you can reduce recording priority to 1 - Normal or 0 - Minimum |
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.
usually when we are talking about priorities then lower value means higher priority. Wouldn't the current notation be confusing?
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.
maybe it even makes more sence to provide priority as a string (e.g. high/normal/low)
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.
good idea 👍 done, i changed it now to string (high/normal/low) sirmordred@a61a988
README.md
Outdated
| - filename (Mandatory) - You can type recording video file name as you want, but recording currently supports only "mp4" format so your filename must end with ".mp4" | ||
| - priority (Optional) - Default value: 2 which means recording thread priority is Maximum however if you face performance drops during testing with recording enabled, you can reduce recording priority to 1 - Normal or 0 - Minimum | ||
| - max_duration_sec (Optional) (in seconds) - Default value: 900 seconds which means maximum allowed duration is 15 minute, you can increase it if your test takes longer than that | ||
| - resolution_mode (Optional) - Default value: 5 which means default resolution for recording is Full HD 1920x1080 however you can change it to following resolutions: 4 - HD (1280x720), 3 - 480P (720x480), 2 - QVGA (320x240), 1 - QCIF (176x144) |
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.
I would rather prefer to explicitly provide resolution string
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.
you mean we should enforce resolution arg as Mandatory ? or passing resolution arg as string (full_hd, hd, 480p etc.) instead of integer?
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.
pass a string instead of integer, e.g. 640x480 or 1920x1080
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.
okay done sirmordred@bc9ba91 reason i couldn't use switch-case in here sirmordred@bc9ba91#diff-f3d89bcfbd433c431567a1fc5306d1e60589ae7abfc5add09b4e0bbcb87cbe66R256 is that it doesn't support android.util.Size object 👍 also shortened argument name resolution_mode to resolution 👍
| .toAbsolutePath() | ||
| .toString(); | ||
|
|
||
| recordingRotation = RecorderUtil.getDeviceRotationInDegree(getApplicationContext()); |
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.
Just an idea for the future:
I assume the main issue with runtime rotation is the fact we cannot change the video resolution dynamically in runtime. So it makes sence to setup the maximum video resolution as (max(W, H) x max(W, H)), which could potentially fit both orientations. Of course, this should be optional.
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.
you mean values like 1920x1920 or 4096x4096 right? yeah that is good idea and can potentially fit both orientations as you said 👍 noted, will try to sum up and write as comment all future-directions after the PR 👍
| - priority (Optional) - Default value: 2 which means recording thread priority is Maximum however if you face performance drops during testing with recording enabled, you can reduce recording priority to 1 - Normal or 0 - Minimum | ||
| - max_duration_sec (Optional) (in seconds) - Default value: 900 seconds which means maximum allowed duration is 15 minute, you can increase it if your test takes longer than that | ||
| - resolution_mode (Optional) - Default value: 5 which means default resolution for recording is Full HD 1920x1080 however you can change it to following resolutions: 4 - HD (1280x720), 3 - 480P (720x480), 2 - QVGA (320x240), 1 - QCIF (176x144) | ||
|
|
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.
Ideas for the future:
It would be nice to have FPS value configurable
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.
+1 yeah, noted, will try to sum up and write as comment all future-directions after the PR 👍
app/src/main/java/io/appium/settings/recorder/RecorderConstant.java
Outdated
Show resolved
Hide resolved
app/src/main/java/io/appium/settings/recorder/RecorderConstant.java
Outdated
Show resolved
Hide resolved
mykola-mokhnach
left a comment
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.
had some minor comments though
|
thank you so much for the review @mykola-mokhnach 👍 after resolving last feedback #88 (comment) and receive LGTM from you 👍 i'll squash all commits into single one (not doing this for now because of information loss of commits) and sign EasyCLA 👍 |
Signed-off-by: sirmordred <[email protected]>
|
Okay i squashed all changes into single commit but individual commit history can be seen through here (i opened another branch) https://github.com/sirmordred/io.appium.settings/tree/recording_feature and also signed EasyCLA and after all changes, i double-checked that it runs without problem on my real test phones 👍 |
|
FUTURE DIRECTIONS:
|
# [4.1.0](v4.0.10...v4.1.0) (2022-06-17) ### Features * Implement internal audio/video recording for devices >= Android Q ([#88](#88)) ([8b854ae](8b854ae))
|
🎉 This PR is included in version 4.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…Android.Q
REQUIRED STEPS to activate screenrecord feature:
TO START SCREENRECORD:
TO STOP SCREENRECORD:
TO PULL RECORDED MP4 FILE: