Move copyPixelsFromBuffer to a background thread.
#49414
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
0xZOne
force-pushed
the
task/off_ui_thread
branch
15 times, most recently
from
a604b1d to
4839cc3
Compare
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
|
This PR is missing a detailed description of the changes and benchmarks associated with. |
0xZOne
force-pushed
the
task/off_ui_thread
branch
from
4839cc3 to
e2cab5f
Compare
I'm not quite clear on how to include a benchmark. Would you mind offering some guidance? Thanks~ |
0xZOne
force-pushed
the
task/off_ui_thread
branch
6 times, most recently
from
c5ea4c4 to
ec6e55c
Compare
0xZOne
force-pushed
the
task/off_ui_thread
branch
2 times, most recently
from
029cf4e to
406c227
Compare
Since your making this change for performance reasons. I suggest you write a small program that allows you to measure/quantify how much time you save on the main UI thread? That is, a program that calls convertImageToBitmap with a bunch of different image sizes and measures how long it takes to complete. That will give us an understanding of how impactful this change is. |
0xZOne
force-pushed
the
task/off_ui_thread
branch
from
406c227 to
de72fc0
Compare
| final HardwareBuffer buffer = image.getHardwareBuffer(); | ||
| bitmap = Bitmap.wrapHardwareBuffer(buffer, ColorSpace.get(ColorSpace.Named.SRGB)); |
There was a problem hiding this comment.
This should be super cheap right?
We're then paying the cost of synchronization/extra threading which will slow things down.
| ByteBuffer buffer = imagePlane.getBuffer(); | ||
| buffer.rewind(); | ||
| currentBitmap.copyPixelsFromBuffer(buffer); | ||
| bitmap.copyPixelsFromBuffer(buffer); |
There was a problem hiding this comment.
This is the only piece that would benefit things, but without a benchmark and/or tracing to show that this actually helps things in real applications we're just adding overhead for newer devices.
It might make sense to add some tracing around this call so we could look at existing benchmarks/applications and see how long it's really taking. I'm not 100% convinced that this will benefit too much from parallelization though, because Android devices tend to have slower memory access, and making more threads do more memory related work will probably be bad in some cases.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
We'd need to see some traces that show this actually helps things, ideally on both an API 29+ phone and an API <29 phone.
Are there any tests in the devicelab suite that already cover this scenario?
Is this helping the application(s) you're working on? Even if you can't share the full application can you share some traces?
| assertFalse(imageView.acquireLatestImage()); | ||
|
|
||
| // Simulate the next frame available. | ||
| imageView.onImageAvailable(mockReader); | ||
| assertTrue(imageView.acquireLatestImage()); |
There was a problem hiding this comment.
Although this test does cover the new behavior, I'm not sure that we really want the new behavior.
…142399) After #100990, we should use `initExpensiveAndroidView` for Android Hybrid Composition mode instead of `initSurfaceAndroidView`. `initSurfaceAndroidView` attempts to use `TLHC` when possible. In cases where that is not supported, it falls back to using Hybrid Composition. flutter/engine#49414
|
We don't think this patch is safe to land, and lacking the benchmarks or evidence that it is faster and correct we don't plan to continue reviewing this. |
4 participants
Move the time-consuming
copyPixelsFromBufferfunction to a background thread to avoid blocking the UI thread. At the same time, remove the unnecessarycurrentImagemember.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.