|
|
DescriptionPhoto Picker Dialog: Use sandboxed utility process for decoding images.
BUG=656015
Review-Url: https://codereview.chromium.org/2816733002
Cr-Original-Commit-Position: refs/heads/master@{#468209}
Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def65f1a8da7aa49
Review-Url: https://codereview.chromium.org/2816733002
Cr-Commit-Position: refs/heads/master@{#468923}
Committed: https://chromium.googlesource.com/chromium/src/+/ff4ca2efee2c251b4fded429984f4be90e1d43d5
Patch Set 1 #
Total comments: 58
Patch Set 2 : Sync #Patch Set 3 : Address comments from Theresa #Patch Set 4 : Change inSampleSize calculations #Patch Set 5 : Address comments from Yusuf #Patch Set 6 : Simplify decoder #
Total comments: 30
Patch Set 7 : Remove TODO (nullchecks) and fix findbugs #Patch Set 8 : Address feedback from Theresa #
Total comments: 10
Patch Set 9 : Address feedback from both #
Total comments: 10
Patch Set 10 : No code change, just sync'ing #Patch Set 11 : Address comments from Theresa #Messages
Total messages: 74 (43 generated)
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
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:54: } This is also in the other CL in flight (first one wins, I guess?) https://codereview.chromium.org/2816733002/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/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:259: PickerBitmap.PICTURE)); This code is for testing only and is going away anyway (when the async enumeration CL goes in).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
twellington@chromium.org changed reviewers: + yusufo@chromium.org
+yusufo@ for service creation/binding/communication https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:675: android:process=":decoder_service" /> This should have android:exported="false" so that external apps cannot access it. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:12: class BitmapUtils { nit: JavaDoc for the class https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:22: bitmap = cropToSquare(bitmap, width); I think that instead of manipulating the Bitmap here to ensure a minimum size and crop to a square, we can rely on the Android framework to do the scaling/centering for us. Android's ImageView has an scaleType property that controls how the image is scaled and positioned within the view. If the ImageView that contains the bitmaps is set to the desired size and the scaleType is set to centerCrop then I think you'll achieve the same visual affect with less programatic manipulation of the bitmaps. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:54: private static int calculateInSampleSize(BitmapFactory.Options options, int width, int height) { nit: this method can be private Also, since the width and height are the same can the two params be condensed to one "size" or "desiredSize" param? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / inSampleSize) >= width) { I think the math for calculating the inSampleSize can be simplified. If the image's real size is 4,000 x 4,000 and the desired size is 1,000, the inSampleSize should be 4. Looking at another example, if the image is 4,000 width x 6,000 height, based on the ensureMinSize() method, I think the inSampleSize should be 4. The decoded image will 1,000 x 1,5000 and the cropToSquare() method (or Android's ImageView scaleType=centerCrop) would chop off 250 from the top and bottom of the bitmap. I think this code will produce the correct inSampleSize: if (options.outHeight > size && options.outWidth > size) { inSampleSize = Math.min(options.outHeight, options.outWidth) / size; } https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:76: public static Bitmap scale(Bitmap bitmap, float scaleMaxSize, boolean filter) { Where is this method called from? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:44: static final String KEY_WIDTH = "width"; Since this represents width and height, could it be called "size" to cover both? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by We shouldn't be accessing methods that aren't publicly accessible. Is there a way to do this without reflection? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:179: bundle.putBoolean(KEY_SUCCESS, true); If there's an IOException, should we still be returning true here? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:118: // The callback the client wants us to use to report back when the service is ready. optional style nit: This could be re-written more concisely as "The callback used to notify the client when the service is ready." https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:181: for (DecoderServiceParams params : mRequests.values()) { Why does decodeImage() need to check if mRequests.size() == 1 before calling dispatchNextDecodeImageRequest() if this method loops through all of the requests? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:199: // The restricted utility process can't open the file to read the Is there Android documentation somewhere explaining this restriction? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:241: * @param filePath complete this JavaDoc https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:330: private Bitmap createPlaceholderBitmap(int width, int height) { Instead of creating a placeholder bitmap, can we set the background color on the ImageView holding the bitmap to grey? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:54: } On 2017/04/12 14:57:42, Finnur wrote: > This is also in the other CL in flight (first one wins, I guess?) Acknowledged. https://codereview.chromium.org/2816733002/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/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:259: PickerBitmap.PICTURE)); On 2017/04/12 14:57:42, Finnur wrote: > This code is for testing only and is going away anyway (when the async > enumeration CL goes in). Acknowledged.
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:124: case MSG_REGISTER_CLIENT: why do we need the two separate messages here? ie, why not do mClient = msg.reply inside MSG_DECODE_IMAGE? Note that they can still bind and not send anything, if we worry about initialization taking a long time. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:147: e.printStackTrace(); Log an error message with details if applicable, rather than a stack trace. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:174: e.printStackTrace(); see above. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:187: } catch (RemoteException e) { RemoteException can be a DeadObject or a TransactionTooLarge so maybe not make Jim worry for no good reason here. How big are the bitmaps we are sending here? For your own sake, you might want to have a separate TransactionTooLargeException catch here and record it just in case you will be curious whether these bitmaps are actually arriving to the other end. so two catches, one sets the client to null the other just records, maybe? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:191: if (imageFile != null) { one line https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:195: if (imagePfd != null) { one line https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:198: } catch (IOException e) { see above https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:201: } catch (Exception e) { but why would it crash? (You have all these other try/catch blocks inside that should capture related exceptions) StrictMode? https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:208: Message reply = Message.obtain(null, MSG_IMAGE_DECODED_REPLY); add a private send message below and avoid duplication of the whole try/catch here. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:212: } catch (RemoteException remoteException) { see above
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Back from Easter vacation... Thanks to both for reviewing. Note: This update addresses _only_ the comments made by Theresa (I didn't have time to look at Yusuf's comments today -- will look at that tomorrow). https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:675: android:process=":decoder_service" /> Excellent catch! Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:12: class BitmapUtils { On 2017/04/13 02:25:26, Theresa wrote: > nit: JavaDoc for the class Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:22: bitmap = cropToSquare(bitmap, width); That's a good point. I'll take a look at this tomorrow (ran out of time). https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:54: private static int calculateInSampleSize(BitmapFactory.Options options, int width, int height) { On 2017/04/13 02:25:26, Theresa wrote: > nit: this method can be private Not sure I understand. It already is... If you mean: Remove |static| then I'd point out that it is used from static content, so that produces a compile error... > Also, since the width and height are the same can the two params be condensed to > one "size" or "desiredSize" param? Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / inSampleSize) >= width) { The idea is to produce a photo that is no larger in both dimensions than |size|. Lets take a real life photo from my Nexus 6, which is 4160x3120 (the photo, not my Nexus 6). The real-estate space on that phone allows for thumbnails of size 461x461. inSampleSize = Math.min(options.outHeight, options.outWidth) / size; = Math.min(3120, 4160) / 461; = 3120 / 461 = 6 (integer division) inSampleSize is rounded down to the nearest power-of two, so this would be rounded to 4, I guess. The end result is a 1040x780 image, which is larger than the requested size (461x461 max). We want inSampleSize to hit a certain sweet-spot -- be as large as possible to minimize CPU time and memory usage in decoding and transferring, but not so large as to overly degrade the image quality. Therefore, I don't think we should use Androids View here to crop, because then we're decoding to a bigger image size than we can show and wasting both CPU and memory in the process. However, looking at this again, I see this function can be simplified a bit... https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:76: public static Bitmap scale(Bitmap bitmap, float scaleMaxSize, boolean filter) { Hmm... it isn't (yet)... Removed. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:44: static final String KEY_WIDTH = "width"; On 2017/04/13 02:25:27, Theresa wrote: > Since this represents width and height, could it be called "size" to cover both? Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by If by 'this' you mean 'obtain a FileDescriptor for a MemoryFile', then no -- I don't know of another way. I'd love to be proven wrong, but I've researched this a fair amount and not found a better answer. If by 'this' you mean 'transfer the data', then I'd say the answer is a bit trickier. As I see it, there are two ways of getting the data across sandboxed process boundaries. You can either pass a handle to a pre-existing copy of the bits (the FileDescriptor) or pass the bits themselves. I initially tried the latter, but found it wasteful and inefficient but the worst part is that that the IPC mechanism wasn't designed to transfer large amounts, such as bitmaps, as the data cap is very low. Bitmaps can easily brush up against the limit causing failures in decoding. Yes, it's possible to divide the image into smaller packets to avoid the transfer limit, but it is so much simpler, easier and efficient to just send over FileDescriptors and eliminate these extra copies as well as the errors caused by the transfer limits. Also, I should mention that getFileDescriptor is used on a code path to support pre-M-devices, so eventually it can be phased out. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:179: bundle.putBoolean(KEY_SUCCESS, true); Hmm... no, we should probably not do so... https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:118: // The callback the client wants us to use to report back when the service is ready. On 2017/04/13 02:25:27, Theresa wrote: > optional style nit: This could be re-written more concisely as "The callback > used to notify the client when the service is ready." Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:181: for (DecoderServiceParams params : mRequests.values()) { It doesn't loop through all (there's a break statement on line 183). We get a lot of requests in in the beginning. If we add one request and that's the only request in the queue, then we're the first to add requests and start the whole process. Otherwise, a request is being processed and the next one will be triggered once the first is done (until there are no more requests). https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:199: // The restricted utility process can't open the file to read the On 2017/04/13 02:25:27, Theresa wrote: > Is there Android documentation somewhere explaining this restriction? android:isolatedProcess If set to true, this service will run under a special process that is isolated from the rest of the system and has no permissions of its own. The only communication with it is through the Service API (binding and starting). - from: https://developer.android.com/guide/topics/manifest/service-element.html https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:241: * @param filePath On 2017/04/13 02:25:27, Theresa wrote: > complete this JavaDoc Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:330: private Bitmap createPlaceholderBitmap(int width, int height) { Yeah, that probably works. I'll take a look at that tomorrow also.
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / inSampleSize) >= width) { On 2017/04/18 17:21:14, Finnur wrote: > The idea is to produce a photo that is no larger in both dimensions than |size|. Shouldn't we ensure that the photo is at least |size| in both dimensions, while minimizing how much larger than |size| it is? > Lets take a real life photo from my Nexus 6, which is 4160x3120 (the photo, not > my Nexus 6). The real-estate space on that phone allows for thumbnails of size > 461x461. > > inSampleSize = Math.min(options.outHeight, options.outWidth) / size; > = Math.min(3120, 4160) / 461; > = 3120 / 461 > = 6 (integer division) > > inSampleSize is rounded down to the nearest power-of two, so this would be > rounded to 4, I guess. The end result is a 1040x780 image, which is larger than > the requested size (461x461 max). With the method in the latest patchset, the inSampleSize would actually end up being 16, right? while ((height / inSampleSize) >= maxSize || (width / inSampleSize) >= maxSize) { inSampleSize *= 2; } 4160 / 1 >= 461, inSampleSize = 2 4160 / 2 = 2080, inSampleSize = 4 4160 / 4 = 1040, inSampleSize = 8 4160 / 8 = 520, inSampleSize = 16 4160 / 16 = 260 -- DONE, image will be 195 x 260 Upscaling thumbnails generally doesn't look good. What's the sweet spot in your mind? In mine, it's at sample size 4, which ensures there is no up-scaling, but I could be talked into 8, which would be 390 x 520. There are some image sizes that are less-borderline where using a too-high sample size is going to result in upscaling, but that's a tradeoff we should consider. > We want inSampleSize to hit a certain sweet-spot -- be as large as possible to > minimize CPU time and memory usage in decoding and transferring, but not so > large as to overly degrade the image quality. Therefore, I don't think we should > use Androids View here to crop, because then we're decoding to a bigger image > size than we can show and wasting both CPU and memory in the process. I was only suggesting using Android views in place of ensureMinSize() and cropToSquare(). Using the Android View to crop/up-scale images that start out too small vs using a Bitmap crop/up-scale in this class doesn't affect the size of the image being decoded since the Bitmap scaling/cropping happens after we've decoded the image. I would expect that Bitmap operations are more costly than letting the Android View system layout the image (I don't have any data to back that up, so that assumption could be wrong), but I can see an argument for minimizing the number of bits being sent back in the response for the bitmap. > However, looking at this again, I see this function can be simplified a bit... https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by On 2017/04/18 17:21:14, Finnur wrote: > If by 'this' you mean 'obtain a FileDescriptor for a MemoryFile', then no -- I > don't know of another way. I'd love to be proven wrong, but I've researched this > a fair amount and not found a better answer. > > If by 'this' you mean 'transfer the data', then I'd say the answer is a bit > trickier. As I see it, there are two ways of getting the data across sandboxed > process boundaries. You can either pass a handle to a pre-existing copy of the > bits (the FileDescriptor) or pass the bits themselves. > > I initially tried the latter, but found it wasteful and inefficient but the > worst part is that that the IPC mechanism wasn't designed to transfer large > amounts, such as bitmaps, as the data cap is very low. Bitmaps can easily brush > up against the limit causing failures in decoding. Yes, it's possible to divide > the image into smaller packets to avoid the transfer limit, but it is so much > simpler, easier and efficient to just send over FileDescriptors and eliminate > these extra copies as well as the errors caused by the transfer limits. > > Also, I should mention that getFileDescriptor is used on a code path to support > pre-M-devices, so eventually it can be phased out. By "this" I mean a solution that doesn't use reflection to access hidden methods. Our policy is not to use reflection unless we have a clear path to remove it at some point (e.g. after a support library roll or after a new Android SDK becomes public). We need a solution, or at least a fallback, that doesn't rely on getFileDescriptor or createAshmemBitmap, since there's no guarantee that these methods exist. I haven't dug into how these methods are being used, but I can look more closely and try to recommend some alternatives if needed.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Yusuf is still off-the-hook; between reviews, meetings and trying (unsuccessfully) to get the View system to scale the images properly I wasn't able to show much progress here but wanted to get a round of comments in, at least (only one function changed). Theresa, PTAL. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / inSampleSize) >= width) { The sweet spot in my mind is the highest inSampleSize number we can use without noticing much degradation. But that's subjective, I guess. I'm OK with trying it this way (effectively changing the param from maxSize to minSize, so as to end with an inSampleSize of 4 in the case above). We can always revisit it, if memory constraints or speed of decoding are a problem. However, I'm having a problem getting the Android View system to work the way I want it. There's two problems. 1) If the image is _smaller_ than minSize, without ensureMinSize the View system does not seem to scale the image up to fill the available space provided (tried various combinations of xml params for the ImageView). I need all images, large and small to fully use up the square given. 2) If the image is _larger_ than the available space, the View system will correctly only show the portions of the image that fit within the boundary given. However, it doesn't actually crop the photo to fit, so the selection scale animation (that is supposed to make the image shrink) will make parts of the image (that were hidden) visible because the view system thinks -- hey, we've got all these extra bits we aren't showing, now is our chance! :) cropToSquare eliminates this by making sure that the image is always rectangular and fits exactly in the square given (and uses less memory while it lives on screen). https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by I would appreciate it if you could recommend some alternatives. I'd like to avoid copying the bits and would actually prefer to lobby the Android team to make this function public rather than resorting to a method that introduces a bitmap copy to get around it.
https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:47: // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by On 2017/04/19 16:01:35, Finnur wrote: > I would appreciate it if you could recommend some alternatives. I'd like to > avoid copying the bits and would actually prefer to lobby the Android team to > make this function public rather than resorting to a method that introduces a > bitmap copy to get around it. This is my understanding of the current process, am I missing anything? 1) After receiving a decode request from the client, we're using a ParcelFileDescriptor to decode a bitmap using a BitmapUtils method 2) On M+, creating an AshmemBitmap from the decoded bitmap and attaching it to the bundle. On pre-M, we copy the bitmap bytes to a buffer, writing the buffer bytes to a MemoryFile, creating a FileDescriptor then attaching the FileDescriptor to the bundle. 3) Sending the bundle to the client It's probably worthwhile to reach out to android chatty (ping me for the email if you don't have it) to ask for best practices in sharing large bitmaps between processes. Stepping away from shared memory, when you were passing the bits themselves, what process were you using? There are ways to compress the bytes (Bitmap.compress()) which may help with the size.
More or less, yes. When the decode request comes in, we open an inputFile, obtain its FileDescriptor and send that over (via ParcelFileDescriptor) to the sandboxed process. That process decodes the image straight from the FileDescriptor and puts the bytes read into a MemoryFile. Nothing controversial there. The function to obtain a FileDescriptor from a MemoryFile is obtained via Reflection and once I have the FileDescriptor I send that over (again, via ParcelFileDescriptor) to the parent process for reading and re-constructing the bitmap on the other side. In short, we are just passing FileDescriptors between the processes, thereby avoiding extra copies along the way. We use reflection to obtain a FileDescriptor for the MemoryFile and we use reflection for the function to create an ashmemBitmap straight out of the Bitmap object (for M+ devices). > It's probably worthwhile to reach out to android chatty Sounds good. I don't have the email. > Stepping away from shared memory, when you were passing the > bits themselves, what process were you using? There are ways > to compress the bytes (Bitmap.compress()) which may help with > the size. My initial attempt (just to get the thing working end-to-end) was to send the bits over using the IPC bundle, which is when I bumped into the size limit. I don't think compression will work, because the limit is very low -- 1MB? I forget -- but the raw image data to decode can be much larger.
This update addresses comments made by Yusuf only. Theresa is off the hook this time. :) https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:124: case MSG_REGISTER_CLIENT: Yeah, that's much better. Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:147: e.printStackTrace(); On 2017/04/13 19:05:25, Yusuf wrote: > Log an error message with details if applicable, rather than a stack trace. Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:174: e.printStackTrace(); On 2017/04/13 19:05:24, Yusuf wrote: > see above. Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:187: } catch (RemoteException e) { So... at the moment, we don't send _any_ bitmap data over the IPC channel. We write to a memory file and send its FileDescriptor over for reading on the other side. I don't expect any TransactionTooLargeExceptions. DeadObject... maybe. But I think logging and attempting to send a Sucess=false over is probably sufficient. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:191: if (imageFile != null) { On 2017/04/13 19:05:25, Yusuf wrote: > one line Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:195: if (imagePfd != null) { On 2017/04/13 19:05:25, Yusuf wrote: > one line Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:198: } catch (IOException e) { On 2017/04/13 19:05:24, Yusuf wrote: > see above Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:201: } catch (Exception e) { StrictMode? I don't know... :s My thinking was that this is a sandboxed utility process decoding untrusted content, so who knows what's going to happen? I'm willing to follow your guidance on how an unexpected error should be handled if/when it occurs. The code is not catching null pointer errors, for example. Not sure what else. I'd rather gracefully fail to decode the image if there's an unexpected error because it has two benefits: 1) I can notify the host so that it can delete the request and free up memory. 2) It is a much more pleasant UX than letting the process crash (user getting multiple "this process crashed" dialogs) But I'm open to suggestions. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:208: Message reply = Message.obtain(null, MSG_IMAGE_DECODED_REPLY); On 2017/04/13 19:05:25, Yusuf wrote: > add a private send message below and avoid duplication of the whole try/catch > here. Done. https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:212: } catch (RemoteException remoteException) { On 2017/04/13 19:05:25, Yusuf wrote: > see above Done.
Hi all, Sorry for the drive-by. :) I think you can achieve the goal by means that are both easier for you and also more amenable to the traditional IPC security review process. Rather than build up your own IPC, I'd consider the ThumbnailProvider: chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java which calls into native: ./chrome/browser/android/download/ui/thumbnail_provider.cc ./chrome/browser/android/download/ui/thumbnail_provider.h which uses an implementation that spawns a utility process: https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?q=ImageDe...
I added you to a doc, Chris. Basically, the latency of the TumbnailProvider kills the performance of the dialog. There's even been talk to switch over from the ThumbnailProvider completely and use this instead.
On 2017/04/21 18:55:47, Finnur wrote: > I added you to a doc, Chris. Basically, the latency of the TumbnailProvider > kills the performance of the dialog. There's even been talk to switch over from > the ThumbnailProvider completely and use this instead. ... on Android. But that's beyond the scope of this project.
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...
Updated. Both: PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
Thank you for getting rid of the reflection! https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:61: while ((halfHeight / inSampleSize) >= height || (halfWidth / inSampleSize) >= width) { On 2017/04/19 16:01:35, Finnur wrote: > The sweet spot in my mind is the highest inSampleSize number we can use without > noticing much degradation. But that's subjective, I guess. I'm OK with trying it > this way (effectively changing the param from maxSize to minSize, so as to end > with an inSampleSize of 4 in the case above). We can always revisit it, if > memory constraints or speed of decoding are a problem. > > However, I'm having a problem getting the Android View system to work the way I > want it. There's two problems. > > 1) If the image is _smaller_ than minSize, without ensureMinSize the View system > does not seem to scale the image up to fill the available space provided (tried > various combinations of xml params for the ImageView). I need all images, large > and small to fully use up the square given. That's a bit surprising since CENTER_CROP's documentation says "Scale the image uniformly (maintain the image's aspect ratio) so that both dimensions (width and height) of the image will be equal to or larger than the corresponding dimension of the view (minus padding)." But if you can't get it to work then scaling here is okay. > 2) If the image is _larger_ than the available space, the View system will > correctly only show the portions of the image that fit within the boundary > given. However, it doesn't actually crop the photo to fit, so the selection > scale animation (that is supposed to make the image shrink) will make parts of > the image (that were hidden) visible because the view system thinks -- hey, > we've got all these extra bits we aren't showing, now is our chance! :) > cropToSquare eliminates this by making sure that the image is always rectangular > and fits exactly in the square given (and uses less memory while it lives on > screen). Thank you for the explanation, that makes sense. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:23: public static Bitmap sizeBitmap(Bitmap bitmap, int width) { nit: this an be private Please change width to size here and everywhere else where width is being used for both width and height. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); I think that this should be a regular while loop. Currently the minimum sampling size returned is 2 because the condition is evaluated at the end of the loop. Also, earlier you mentioned that the inSampleSize is rounded down to the nearest power of 2. Could this be simplified to: if (options.outHeight > size && options.outWidth > size) { inSampleSize = Math.min(options.outHeight, options.outWidth) / size; } or would that change the calculation? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:104: return Bitmap.createBitmap(bitmap, x, y, size, size); When we do createScaledBitmap() and createBitmap() is Android re-using/recycling the bits in the previous bitmap, or do we end up creating 3 bitmaps, 2 of which are GC'ed at some unspecified point in the future? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:90: bitmap.recycle(); Are the bitmap pixels shared between processes? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:76: // Called when a connection to the Service has been lost. nit: use lowercase "service" for consistency with other comments? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:90: public int mWidth; Replace "width" with "size" here too https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:268: Bitmap bitmap = payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); Can you please document what's happening and why this is a good approach since the Android documentation is a bit lacking? I assume it's something along the lines of the android chatty response: "The most widely supported, easiest, and reasonably efficient method is to decode to an immutable bitmap and just return the bitmap over binder. It will internally memcpy itself to ashmem and then just send over the file descriptor. In the receiving process it will just leave the bitmap on ashmem[1] since it's immutable and carry on." https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:277: // older clients, we manage our own memory file. It's not clear by look at DecoderService that the returned bitmap is null in older versions of Android. What makes it null? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:319: private Bitmap createPlaceholderBitmap(int width, int height) { Bumping my comment from a previous pathchset: Instead of creating a placeholder bitmap, can we set the background color on the ImageView holding the bitmap to grey? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:329: * Ties up all the lose ends from the decoding request (communicates the results of the nit: s/lose/loose https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:68: // TODO(finnur): Use cached image, if available. Just checking that I remember correctly - the plan here is to introduce an LRU cache to avoid re-decoding images that have previously been decoded (when possible)? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:164: prepareBitmaps(); Can we prepare the list of bitmaps before the service is ready?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
Theresa and Yusuf, PTAL. > Thank you for getting rid of the reflection! Thank you for pressing the issue. :) https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:23: public static Bitmap sizeBitmap(Bitmap bitmap, int width) { On 2017/04/24 16:57:39, Theresa wrote: > nit: this an be private > > Please change width to size here and everywhere else where width is being used > for both width and height. Done. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); Yes, it would give wrong inSampleSize values (for an example, see my calculations above in this thread). I need exact multiples of 2. Note that the minimum returned in this function is 1, not 2 (I suspect you overlooked line 63). https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:104: return Bitmap.createBitmap(bitmap, x, y, size, size); It creates new bitmaps and the old will be recycled at some point. Willing to consider other options that are more efficient... but wondering if we can leave that for another CL? https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java:90: bitmap.recycle(); By passing the bitmap like this it is memcopy-ed to a shared buffer and a FileDescriptor for that buffer is passed over. The |bitmap| object is not needed after we've parceled. I've added a comment to clarify. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:76: // Called when a connection to the Service has been lost. On 2017/04/24 16:57:39, Theresa wrote: > nit: use lowercase "service" for consistency with other comments? Done. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:90: public int mWidth; On 2017/04/24 16:57:39, Theresa wrote: > Replace "width" with "size" here too Done. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:268: Bitmap bitmap = payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); Done (but in the other file). https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:277: // older clients, we manage our own memory file. Arg... forgot to delete this. Thanks! https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:319: private Bitmap createPlaceholderBitmap(int width, int height) { I'm just going to remove the placeholder image. I'm not sure we explicitly need to do more for bitmaps that fail to load as the current background is already gray. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:329: * Ties up all the lose ends from the decoding request (communicates the results of the On 2017/04/24 16:57:39, Theresa wrote: > nit: s/lose/loose Done. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:68: // TODO(finnur): Use cached image, if available. Yes, in an upcoming CL. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:164: prepareBitmaps(); On 2017/04/24 16:57:39, Theresa wrote: > Can we prepare the list of bitmaps before the service is ready? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
This is really shaping up well! I'm going to do another full pass later today or tomorrow morning. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); On 2017/04/25 15:38:00, Finnur wrote: > Yes, it would give wrong inSampleSize values (for an example, see my > calculations above in this thread). > > I need exact multiples of 2. Doesn't Android round down to the nearest power of 2? > Note that the minimum returned in this function is 1, not 2 (I suspect you > overlooked line 63). Yes, I did. Thank you. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:104: return Bitmap.createBitmap(bitmap, x, y, size, size); On 2017/04/25 15:38:00, Finnur wrote: > It creates new bitmaps and the old will be recycled at some point. > > Willing to consider other options that are more efficient... but wondering if we > can leave that for another CL? I think it's fine to leave for another CL. We're doing a number of things to try to limit the amount of memory used for the photo picker and creating extra bitmaps that are auto-GC'ed seems counter to that goal. We can look at Bitmap methods or take another look at how the Android View hierarchy for each bitmap row and try to get scaleType to work correctly. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:269: int size = payload.getInt(DecoderService.KEY_SIZE); Findbugs pointed out that this is a dead store.
https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:163: if (mRequests.size() == 1) { one line https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:211: if (pfd == null) { one line https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:240: * A class for handling communications from the server to us. s/server/service https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:277: Bitmap bitmap = payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); How about bitmap = !success ? null : payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); then you call closeRequest(host, filePath, bitmap); and you get rid of the above if
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...
Both: PTAL. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); I'll convert to your method for now, but I think we'll need to revisit this. > Doesn't Android round down to the nearest power of 2? Well, I think that's part of the problem. I wonder if the right approach is a combination of your approach and a rounding to the nearest multiple of 2. Lets take an extreme example to illustrate my case: A 110000x110000 photo on disk with the available space on screen being 500x500. Using your calculations, we'd get: inSampleSize = 110000 / 500 = 220. This would be rounded down to 128, which gives us an 859x859 photo (2.8 MB). If we instead round _up_ to the nearest multiple of two (256) then we get a 429x429 photo (700K). That's a significant difference on only one photo (faster decoding and much less memory used). The question is whether the efficiency gained is worth a slight up-scaling of the photos in some cases. I've not noticed a problem with that on my Nexus 6, but -- granted -- that's anecdotal. Hmm... Maybe I should get Chris to weigh in. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:104: return Bitmap.createBitmap(bitmap, x, y, size, size); Sounds good. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:163: if (mRequests.size() == 1) { On 2017/04/25 19:01:08, Yusuf wrote: > one line Done. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:211: if (pfd == null) { On 2017/04/25 19:01:08, Yusuf wrote: > one line Done. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:240: * A class for handling communications from the server to us. On 2017/04/25 19:01:08, Yusuf wrote: > s/server/service Done. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:269: int size = payload.getInt(DecoderService.KEY_SIZE); On 2017/04/25 17:42:59, Theresa wrote: > Findbugs pointed out that this is a dead store. Done. https://codereview.chromium.org/2816733002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:277: Bitmap bitmap = payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); On 2017/04/25 19:01:08, Yusuf wrote: > How about > > bitmap = !success ? null : > payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP); > then you call closeRequest(host, filePath, bitmap); > > and you get rid of the above if Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); On 2017/04/26 12:00:57, Finnur wrote: > I'll convert to your method for now, but I think we'll need to revisit this. > > > Doesn't Android round down to the nearest power of 2? > > Well, I think that's part of the problem. > > I wonder if the right approach is a combination of your approach and a rounding > to the nearest multiple of 2. > > Lets take an extreme example to illustrate my case: A 110000x110000 photo on > disk with the available space on screen being 500x500. > > Using your calculations, we'd get: inSampleSize = 110000 / 500 = 220. This > would be rounded down to 128, which gives us an 859x859 photo (2.8 MB). If we > instead round _up_ to the nearest multiple of two (256) then we get a 429x429 > photo (700K). That's a significant difference on only one photo (faster decoding > and much less memory used). > > The question is whether the efficiency gained is worth a slight up-scaling of > the photos in some cases. I've not noticed a problem with that on my Nexus 6, > but -- granted -- that's anecdotal. > > Hmm... Maybe I should get Chris to weigh in. The memory v quality trade-off is definitely something we'll want to evaluate. I think the quality will be subjective, but we should add metrics (follow-up CL) around how much memory on average is being used per thumbnail. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:25: bitmap = cropToSquare(bitmap, size); nit: can we add a TODO here to investigate options that require fewer bitmaps to be created? https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:31: * A class to communicate with the decoder service. nit: s/decoder service/{@link DecoderService}. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:170: for (DecoderServiceParams params : mRequests.values()) { Instead of a loop that terminates after one pass, it may be easier to quickly understand what's happening if we do something like this: if (mRequests.entrySet().iterator().hasNext()) { DecoderServiceParams params = mRequests.entrySet().iterator().next().getValue(); dispatchDecodeImageRequest(params.mFilPath, params.mSize); } Hopefully the iterator is already allocated and doing this (or the loop in the current patchset) doesn't create a new iterator each time we dispatch an image request. It would be interesting to heap profile and make sure only one is being allocated. If multiple are allocated, we should look at using a different data structure. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:198: Log.e(TAG, "Unable to obtain FileDescriptor: " + e); If we can't obtain a FileDescriptor, the decoder service won't be able to decode the image, right? Should we send a failed callback to the client and return early? https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:223: Log.e(TAG, "Communications failed (IO): " + e); If we run into exceptions sending a message to the service, what actions should we take to continue processing the queue of requests? Try to send the next request? Restart the service?
lgtm after twellington@'s comments are addressed.
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...
Thanks, Yusuf! Updated. Theresa: PTAL. https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:62: } while ((height / inSampleSize) >= minSize && (width / inSampleSize) >= minSize); On 2017/04/26 16:53:17, Theresa wrote: > On 2017/04/26 12:00:57, Finnur wrote: > > I'll convert to your method for now, but I think we'll need to revisit this. > > > > > Doesn't Android round down to the nearest power of 2? > > > > Well, I think that's part of the problem. > > > > I wonder if the right approach is a combination of your approach and a > rounding > > to the nearest multiple of 2. > > > > Lets take an extreme example to illustrate my case: A 110000x110000 photo on > > disk with the available space on screen being 500x500. > > > > Using your calculations, we'd get: inSampleSize = 110000 / 500 = 220. This > > would be rounded down to 128, which gives us an 859x859 photo (2.8 MB). If we > > instead round _up_ to the nearest multiple of two (256) then we get a 429x429 > > photo (700K). That's a significant difference on only one photo (faster > decoding > > and much less memory used). > > > > The question is whether the efficiency gained is worth a slight up-scaling of > > the photos in some cases. I've not noticed a problem with that on my Nexus 6, > > but -- granted -- that's anecdotal. > > > > Hmm... Maybe I should get Chris to weigh in. > > The memory v quality trade-off is definitely something we'll want to evaluate. I > think the quality will be subjective, but we should add metrics (follow-up CL) > around how much memory on average is being used per thumbnail. Acknowledged. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:25: bitmap = cropToSquare(bitmap, size); On 2017/04/26 16:53:17, Theresa wrote: > nit: can we add a TODO here to investigate options that require fewer bitmaps > to be created? Done. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:31: * A class to communicate with the decoder service. On 2017/04/26 16:53:17, Theresa wrote: > nit: s/decoder service/{@link DecoderService}. Done. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:170: for (DecoderServiceParams params : mRequests.values()) { Converted. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:198: Log.e(TAG, "Unable to obtain FileDescriptor: " + e); Good idea. Done. https://codereview.chromium.org/2816733002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:223: Log.e(TAG, "Communications failed (IO): " + e); At a minumum, we want to close the request. I think for now it is probably OK to try send the next request. We can look into restarting the service if it becomes more of an issue. Hmm... I do seem to recall seeing in my tests, though, that the service gets restarted if it crashes, for example. Anyway...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm !
Description was changed from ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 ========== to ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 ==========
finnur@chromium.org changed reviewers: + miguelg@chromium.org
Thanks! Miguel, mind doing an OWNERS check for android_chrome_strings.grd and AndroidManifest.xml?
finnur@chromium.org changed reviewers: + tedchoc@chromium.org - miguelg@chromium.org
Ted, I'm missing an OWNERS check for android_chrome_strings.grd and AndroidManifest.xml?
On 2017/04/28 10:43:07, Finnur OOO wrote: > Ted, I'm missing an OWNERS check for android_chrome_strings.grd and > AndroidManifest.xml? That was supposed to end in... would you mind? :)
On 2017/04/28 10:43:31, Finnur OOO wrote: > On 2017/04/28 10:43:07, Finnur OOO wrote: > > Ted, I'm missing an OWNERS check for android_chrome_strings.grd and > > AndroidManifest.xml? > > That was supposed to end in... would you mind? :) android_chrome_strings.grd, AndroidManifest.xml - lgtm
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2816733002/#ps240001 (title: "Address comments from Theresa")
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": 240001, "attempt_start_ts": 1493423170005170, "parent_rev": "cf2d176c85028484636d11d32b933e7bd3ad9a44", "commit_rev": "83c9f3f4faeb35a3a2607446def65f1a8da7aa49"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 ========== to ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/2853793003/ by aelias@chromium.org. The reason for reverting is: generate_orderfiles.py fails due to AndroidManifest.xml change (see http://crbug.com/717071 ) BUG=717071.
Message was sent while issue was closed.
Description was changed from ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6... ========== to ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6... ==========
The CQ bit was checked by finnur@chromium.org
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": 240001, "attempt_start_ts": 1493798422042330, "parent_rev": "baa3278ee9b306bb33907408d586fcb7fb3d5dab", "commit_rev": "ff4ca2efee2c251b4fded429984f4be90e1d43d5"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6... ========== to ========== Photo Picker Dialog: Use sandboxed utility process for decoding images. BUG=656015 Review-Url: https://codereview.chromium.org/2816733002 Cr-Original-Commit-Position: refs/heads/master@{#468209} Committed: https://chromium.googlesource.com/chromium/src/+/83c9f3f4faeb35a3a2607446def6... Review-Url: https://codereview.chromium.org/2816733002 Cr-Commit-Position: refs/heads/master@{#468923} Committed: https://chromium.googlesource.com/chromium/src/+/ff4ca2efee2c251b4fded429984f... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/ff4ca2efee2c251b4fded429984f... |