Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845773003:
Photo Picker Dialog: Add caching for the decoded images. (Closed)
DescriptionPhoto Picker Dialog: Add caching for the decoded images.
Two caches are used: one that caches images at display size and another
that caches images at very low resolution. The latter provides a backup
for the display-sized cache-misses, to help one not loose one's bearings
when scrolling up and down.
BUG=656015
Review-Url: https://codereview.chromium.org/2845773003
Cr-Commit-Position: refs/heads/master@{#470381}
Committed: https://chromium.googlesource.com/chromium/src/+/b6c7ed30b72bf250cc2f9d7954e0f44a5a9bcaf3
Patch Set 1 #
Total comments: 8
Patch Set 2 : Sync (no additional code change) #Patch Set 3 : Address feedback from Michael #Patch Set 4 : Address feedback from Michael #
Total comments: 13
Patch Set 5 : Address feedback from Theresa #
Total comments: 2
Patch Set 6 : Address feedback from Michael #
Total comments: 2
Patch Set 7 : Polish #
Total comments: 4
Patch Set 8 : Address feedback from Theresa #
Total comments: 2
Patch Set 9 : Remove dead var #
Messages
Total messages: 50 (33 generated)
Description was changed from ========== Add caching for the decoded images. Two caches are used: one that caches images at display size and another that caches images at very low resolution. The latter provides a backup for the display-sized cache-misses, to help one not loose one's bearings when scrolling up and down. BUG=656015 ========== to ========== Photo Picker Dialog: Add caching for the decoded images. Two caches are used: one that caches images at display size and another that caches images at very low resolution. The latter provides a backup for the display-sized cache-misses, to help one not loose one's bearings when scrolling up and down. BUG=656015 ==========
finnur@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Mind taking a first pass here? Note: Another CL is in progress which adds actual image decoding. Until then, these caches are caching placeholder images.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:49: mCategoryView.getHighResBitmaps().put(filePath, bitmap); The documentation for this param says it might be a placeholder image on failure. Should that be stored in the cache? https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:54: Bitmap lowres = BitmapUtils.scale(bitmap, 40, false); What's 40? Px? Dp? Maybe extract a documented constant. https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:68: private LruCache<String, Bitmap> mHighResBitmaps; What guarantee do we have that these caches do not survive for longer than needed? https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:130: mHighResBitmaps = new LruCache<String, Bitmap>(cacheSizeLarge) { Please ping memory-dev@chromium.org https://groups.google.com/a/chromium.org/forum/#!forum/memory-dev to discuss whether this use case merits hooking into ComponentCallbacks2.onTrimMemory https://developer.android.com/reference/android/content/ComponentCallbacks2.html
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
finnur@chromium.org changed reviewers: + twellington@chromium.org
Theresa, mind taking a look? https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:49: mCategoryView.getHighResBitmaps().put(filePath, bitmap); That changed with the latest sync. Decoding will not return a placeholder image on failure any longer (just a null bitmap). https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:54: Bitmap lowres = BitmapUtils.scale(bitmap, 40, false); On 2017/04/27 17:07:34, Michael van Ouwerkerk wrote: > What's 40? Px? Dp? Maybe extract a documented constant. Done. https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:68: private LruCache<String, Bitmap> mHighResBitmaps; This object is destroyed when the dialog goes away. https://codereview.chromium.org/2845773003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:130: mHighResBitmaps = new LruCache<String, Bitmap>(cacheSizeLarge) { Will do, thanks.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:19: private static final int GRAINY_THUMBNAIL_SIZE_PX = 40; 40px on an xxxhdpi is going to be a very different experience than 40px on an mdpi device. We should consider using dp rather than pixels. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:52: // Scaling the image down takes between 0-1 ms on average (Nexus 6 phone debug build). The Nexus 6 is a relatively good device. Did we take measurements on a slower (e.g. svelte or old KitKat) device? https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:53: Bitmap lowres = BitmapUtils.scale(bitmap, GRAINY_THUMBNAIL_SIZE_PX, false); Can this be done on an async background thread rather than the UI thread? https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:135: final int cacheSizeSmall = maxMemory / 8; // 1/8th of the available memory. How many bitmaps can the small cache hold vs the large? https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:138: protected int sizeOf(String key, Bitmap bitmap) { What does this override do?
Theresa, PTAL. Note: I realized at the end of current day here that I have yet to address the thread-safety of the cache -- I think. Can you advise whether I should use simple Lock constructs or if there's a better way? Like something akin to Collections.synchronizedMap(map), or something... https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:19: private static final int GRAINY_THUMBNAIL_SIZE_PX = 40; Good idea. Done. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:52: // Scaling the image down takes between 0-1 ms on average (Nexus 6 phone debug build). 0-2 ms on my Evercross Svelte device. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:53: Bitmap lowres = BitmapUtils.scale(bitmap, GRAINY_THUMBNAIL_SIZE_PX, false); On 2017/05/04 16:53:13, Theresa wrote: > Can this be done on an async background thread rather than the UI thread? Done. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:135: final int cacheSizeSmall = maxMemory / 8; // 1/8th of the available memory. On a Nexus 6 run on my test phone, this resulted in a cache of 128 MB for high-res images and just under 32 MB for low-res images. Each high-res image is 830 KB, so that cache can hold at most 157 images. Each low-res image is 6 KB, so that's 5461 images it can hold. On my Nexus 4, the large cache is 96 MB and 24 MB, respectively. Each high-res image is 234K, for a max of 420 images. Each low-res image is 2K, for a max of 12288 photos. On my svelte device, the large cache is 24 MB and 6 Mb, respectively. Each high-res image is 190K, for a max of 130 images. Each low-res image is 1K, for a max of 6144 photos. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:138: protected int sizeOf(String key, Bitmap bitmap) { It presents the size of the cache in KB, since that's a lot more useful metric than number of items.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2845773003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerWorkerTask.java (right): https://codereview.chromium.org/2845773003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerWorkerTask.java:16: class BitmapScalerWorkerTask extends AsyncTask<Void, Void, Bitmap> { nit: BitmapScalerTask seems cleaner. https://codereview.chromium.org/2845773003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerWorkerTask.java:28: mBitmap = bitmap; nit: you could make this the execution input param, to be received in doInBackGround so you get AsyncTask<Bitmap, Void, Bitmap>
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Patchset #6 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed. Theresa, PTAL.
Oh, forgot to mention. Thread synchronization is not required, as I first thought, since we don't touch the cache on another thread -- as Michael pointed out (privately).
https://codereview.chromium.org/2845773003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java (right): https://codereview.chromium.org/2845773003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java:31: * Enumerates (in the background) the image files on disk. Called on a non-UI thread nit: please update the description
https://codereview.chromium.org/2845773003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java (right): https://codereview.chromium.org/2845773003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java:31: * Enumerates (in the background) the image files on disk. Called on a non-UI thread On 2017/05/08 14:44:22, Michael van Ouwerkerk wrote: > nit: please update the description Done.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good overall! Just a question and a couple of nits. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:135: final int cacheSizeSmall = maxMemory / 8; // 1/8th of the available memory. On 2017/05/05 20:41:45, Finnur wrote: > On a Nexus 6 run on my test phone, this resulted in a cache of 128 MB for > high-res images and just under 32 MB for low-res images. > > Each high-res image is 830 KB, so that cache can hold at most 157 images. > Each low-res image is 6 KB, so that's 5461 images it can hold. > > On my Nexus 4, the large cache is 96 MB and 24 MB, respectively. > > Each high-res image is 234K, for a max of 420 images. > Each low-res image is 2K, for a max of 12288 photos. > > On my svelte device, the large cache is 24 MB and 6 Mb, respectively. > > Each high-res image is 190K, for a max of 130 images. > Each low-res image is 1K, for a max of 6144 photos. Thank you for the detailed information! https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:138: protected int sizeOf(String key, Bitmap bitmap) { On 2017/05/05 20:41:45, Finnur wrote: > It presents the size of the cache in KB, since that's a lot more useful metric > than number of items. I phrased my question poorly -- what is using this sizeOf() method? Is it just for debugging or does overriding the sizeOf() method change how the cache behaves? https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:20: private static final int GRAINY_THUMBNAIL_SIZE_DP = 12; Nit: Put this in dimens.xml and use Resources.getDimensionPixelSize() to retrieve the value in px. https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:96: // Scaling the image up takes between 3-4 ms on average (Nexus 6 phone debug build). nit: This type of information is likely to get out of date. I think it would be useful to include a bug # here that has a comment with the stats and relevant information (e.g. device type, OS version, Chrome version), so that we can look back a couple of years from now and know what analysis was done.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed. Theresa, PTAL. https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2845773003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:138: protected int sizeOf(String key, Bitmap bitmap) { It is for debugging. Removed. https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:20: private static final int GRAINY_THUMBNAIL_SIZE_DP = 12; On 2017/05/08 19:34:42, Theresa wrote: > Nit: Put this in dimens.xml and use Resources.getDimensionPixelSize() to > retrieve the value in px. Done. https://codereview.chromium.org/2845773003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:96: // Scaling the image up takes between 3-4 ms on average (Nexus 6 phone debug build). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2845773003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:29: private final float mDensity; This value isn't used anymore, right?
https://codereview.chromium.org/2845773003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2845773003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:29: private final float mDensity; Correct. Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2845773003/#ps200001 (title: "Remove dead var")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494342513413330,
"parent_rev": "57e78ad136f679baa8d97be295bac923f1c7521b", "commit_rev":
"b6c7ed30b72bf250cc2f9d7954e0f44a5a9bcaf3"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker Dialog: Add caching for the decoded images. Two caches are used: one that caches images at display size and another that caches images at very low resolution. The latter provides a backup for the display-sized cache-misses, to help one not loose one's bearings when scrolling up and down. BUG=656015 ========== to ========== Photo Picker Dialog: Add caching for the decoded images. Two caches are used: one that caches images at display size and another that caches images at very low resolution. The latter provides a backup for the display-sized cache-misses, to help one not loose one's bearings when scrolling up and down. BUG=656015 Review-Url: https://codereview.chromium.org/2845773003 Cr-Commit-Position: refs/heads/master@{#470381} Committed: https://chromium.googlesource.com/chromium/src/+/b6c7ed30b72bf250cc2f9d7954e0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b6c7ed30b72bf250cc2f9d7954e0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
