-
Notifications
You must be signed in to change notification settings - Fork 383
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
refactor: Optimize screenshots preprocessing #812
Conversation
@KazuCocoa Could you please test it with real iPad? |
Will run a verification on my test devices tomorrow once I get to the office. |
Checked glance over a browser to
The current master did not have the error. Just a note:
I have a plan today, so my next look will be late |
thanks for checking it @KazuCocoa I did some optimisations. How do you record your video? I tried GStreamer with videoflip pipeline method set to |
I didn't record the video at that time. I just opened the stream URL on Chrome to go through refactored methods in order to compare before/after as a quick check. The orientation was both portrait and landscape at the beginning and changed the rotation in the middle. |
Sorry for not being too precise. This issue is only visible if the actual mjpeg stream is recorded to a video, like x264 or AVC one. Browsers properly handle EXIF tags and adjust shown images automatically. |
Oh, i see
|
I'll need to prepare the recording env on my local, so the video recording test will be late. At this time, I checked my previous comment's error in #812 (comment). I observed the wda process stop after 20 sec or more (at least in 1 min or less) when I opened the Hm, the error didn't have the stacktrace in the Xcode so my investigation will take more time for the process stop as well |
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.
The performance seems to be much better, the WDA process CPU threshold on my 13 Pro Max doesn't pass the 60% mark in the debugger and memory usage appears to be stable.
However, when I tested with ffmpeg it doesn't look like it solved the original problem (i.e. starting with portrait, going to landscape and then to portrait again - the orientation is incorrect).
For reference, the command I used is ffmpeg -f mjpeg -c:v mjpeg -i http://x.x.x.x:n test.mp4
Ok, I've finally made it work with the UIGraphicsImageRenderer |
Ran a check, unfortunately this approach looks to be too heavy resource-wise as well. When the device is in landscape it doesn't take long until it is stopped due to high CPU and memory utilization. Adding an autoreleasepool to |
thanks for confirming it @Dan-Maor I will only preserve the UIGraphicsImageRenderer-based transformation for regular PNG screenshots as these are not supposed to be taken so frequently |
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.
I've verified that the original recording behavior is preserved with the setting turned off and that the orientation fix is working with the setting turned on.
uti:UTTypeJPEG | ||
compressionQuality:recompressionQuality | ||
// iOS always returns screnshots in portrait orientation, but puts the real value into the metadata | ||
// Use it with care. See https://github.com/appium/WebDriverAgent/pull/812 |
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 can observe that videos generated from MJPEG streams supplied by WDA have rotation issues if device orientation is changed dynamically. No video issues are observed if screenshots orientation is adjusted properly according to the current implementation.