Use PixelCopy API for capturing screenshots on API level 24+ #3008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markushi
requested review from
adinauer,
romtsn and
stefanosiano
as code owners
markushi
changed the title
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ca9895 | 364.31 ms | 460.46 ms | 96.15 ms |
| 93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
| 86f0096 | 368.63 ms | 446.92 ms | 78.29 ms |
| bc4be3b | 360.40 ms | 435.04 ms | 74.64 ms |
| 93a76ca | 391.54 ms | 475.65 ms | 84.11 ms |
| c7e2fbc | 372.00 ms | 461.71 ms | 89.71 ms |
| 99a51e2 | 405.11 ms | 479.65 ms | 74.54 ms |
| c7e2fbc | 393.98 ms | 478.24 ms | 84.27 ms |
| 93a76ca | 377.41 ms | 448.22 ms | 70.81 ms |
| 86f0096 | 381.33 ms | 476.58 ms | 95.25 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ca9895 | 1.72 MiB | 2.29 MiB | 576.51 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
| 86f0096 | 1.72 MiB | 2.29 MiB | 576.50 KiB |
| bc4be3b | 1.72 MiB | 2.29 MiB | 576.53 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
| c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
| 99a51e2 | 1.72 MiB | 2.29 MiB | 576.34 KiB |
| c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
| 86f0096 | 1.72 MiB | 2.29 MiB | 576.50 KiB |
Previous results on branch: feat/pixel-copy-api-for-screenshots
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 74e9aa1 | 409.14 ms | 494.29 ms | 85.15 ms |
| 30edd98 | 371.16 ms | 489.10 ms | 117.94 ms |
| 65c0ceb | 383.39 ms | 442.74 ms | 59.35 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 74e9aa1 | 1.72 MiB | 2.29 MiB | 577.53 KiB |
| 30edd98 | 1.72 MiB | 2.29 MiB | 576.85 KiB |
| 65c0ceb | 1.72 MiB | 2.29 MiB | 576.81 KiB |
…entry/sentry-java into feat/pixel-copy-api-for-screenshots
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
romtsn
reviewed
Oct 24, 2023
romtsn
reviewed
Oct 24, 2023
| @@ -38,13 +43,14 @@ public class ScreenshotUtils { | |||
|
|
|||
| if (!isActivityValid(activity, buildInfoProvider) | |||
| || activity.getWindow() == null | |||
| || activity.getWindow().getDecorView() == null | |||
| || activity.getWindow().getDecorView().getRootView() == null) { | |||
| || activity.getWindow().peekDecorView() == null | |||
There was a problem hiding this comment.
m: I'm wondering if we should rather memoize this into a variable, because the next time we call it, it might've already changed? especially if we are not on the main thread
romtsn
reviewed
Oct 24, 2023
| // Use Pixel Copy API on new devices, fallback to canvas rendering on older ones | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
|
|
||
| final HandlerThread thread = new HandlerThread("screenshot"); |
There was a problem hiding this comment.
Suggested change
| final HandlerThread thread = new HandlerThread("screenshot"); | |
| final HandlerThread thread = new HandlerThread("SentryScreenshot"); |
to make it more specific to our sdk
romtsn
reviewed
Oct 24, 2023
|
|
||
| success = | ||
| latch.await(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS) && copyResultSuccess.get(); | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
Suggested change
| } catch (InterruptedException e) { | |
| } catch (Throwable e) { |
I'd catch any throwable just to make sure
romtsn
reviewed
Oct 24, 2023
| success = | ||
| latch.await(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS) && copyResultSuccess.get(); | ||
| } catch (InterruptedException e) { | ||
| // ignored |
There was a problem hiding this comment.
Suggested change
| // ignored | |
| logger.log(SentryLevel.ERROR, "Taking screenshot failed (view.draw).", e); |
romtsn
reviewed
Oct 24, 2023
| final @NotNull CountDownLatch latch = new CountDownLatch(1); | ||
|
|
||
| // Use Pixel Copy API on new devices, fallback to canvas rendering on older ones | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { |
There was a problem hiding this comment.
l: We could use BuildInfoProvider here and test it. You could take a look at SentryGestureListenerClickTest where we completely mock windows and decor views, and there's also ShadowPixelCopy of robolectric that would be helpful
There was a problem hiding this comment.
Completely forget about that, I've added some test using ShadowPixelCopy
romtsn
approved these changes
Oct 24, 2023
…entry/sentry-java into feat/pixel-copy-api-for-screenshots
romtsn
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Description
Use PixelCopy on newer devices, as it allows us to capture screenshots on background threads as well.
Right now we don't seem to have any tests for
ScreenshotUtils.java, if you have an idea on how we could test this, I'm happy to add tests!💡 Motivation and Context
Fixes getsentry/sentry-react-native#3314
Closes #2916
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps