[Android] Photo Picker: Remove deficiencies when flinging.

Removing an image decode from the queue should not also delete info
about which decode is currently in progress, since that causes the
queuing mechanism to think new requests should be sent for decoding.
This adversely affects flinging, because requests that had been queued
up (but are about to be deleted because the user has already flung past
them) would be sent for decoding when new entries are added.

This change also removes the Map of current requests, which may give
off the impression that we're switching from parallel to serial
decoding. However, that is not the case since it was serial to begin
with -- we currently only fire off one request at a time, so a Map is
not required to track what is in progress.

Bug: 1051925, 895776, 656015
Change-Id: Ieaef6d905148eb67d813de397d1582ca151aca79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2054249Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742626}

chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java

@@ -36,7 +36,6 @@ import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.Date;
 import java.util.Iterator;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.PriorityQueue;
 
@@ -193,8 +192,8 @@ public class DecoderServiceHost
     PriorityQueue<DecoderServiceParams> mPendingRequests =
             new PriorityQueue<>(/*initialCapacity=*/1, mRequestComparator);
 
-    // Map of file paths to processing decoding requests.
-    private LinkedHashMap<String, DecoderServiceParams> mProcessingRequests = new LinkedHashMap<>();
+    // The current processing request.
+    private DecoderServiceParams mProcessingRequest;
 
     // The callbacks used to notify the clients when the service is ready.
     List<ServiceReadyCallback> mCallbacks = new ArrayList<ServiceReadyCallback>();
@@ -261,7 +260,7 @@ public class DecoderServiceHost
             mPendingRequests.add(lowPriorityRequest);
         }
 
-        if (mProcessingRequests.size() == 0) dispatchNextDecodeRequest();
+        if (mProcessingRequest == null) dispatchNextDecodeRequest();
     }
 
     /**
@@ -282,21 +281,19 @@ public class DecoderServiceHost
      * Dispatches the next image/video for decoding (from the queue).
      */
     private void dispatchNextDecodeRequest() {
-        DecoderServiceParams params = getNextPending();
-        if (params != null) {
-            mProcessingRequests.put(params.mUri.getPath(), params);
-
-            params.mTimestamp = SystemClock.elapsedRealtime();
-            if (params.mFileType != PickerBitmap.TileTypes.VIDEO) {
-                dispatchDecodeImageRequest(params);
+        // A new decoding request should not be dispatched while something is already in progress.
+        assert mProcessingRequest == null;
+        mProcessingRequest = getNextPending();
+        if (mProcessingRequest != null) {
+            mProcessingRequest.mTimestamp = SystemClock.elapsedRealtime();
+            if (mProcessingRequest.mFileType != PickerBitmap.TileTypes.VIDEO) {
+                dispatchDecodeImageRequest(mProcessingRequest);
             } else {
-                dispatchDecodeVideoRequest(params, params.mFirstFrame);
+                dispatchDecodeVideoRequest(mProcessingRequest, mProcessingRequest.mFirstFrame);
             }
             return;
         }
 
-        if (mProcessingRequests.entrySet().iterator().hasNext()) return;
-
         int totalImageRequests =
                 mSuccessfulImageDecodes + mFailedImageDecodesRuntime + mFailedImageDecodesMemory;
         if (totalImageRequests > 0) {
@@ -429,48 +426,48 @@ public class DecoderServiceHost
      */
     public void closeRequest(String filePath, boolean isVideo, boolean fullWidth,
             @Nullable List<Bitmap> bitmaps, String videoDuration, long decodeTime, float ratio) {
-        DecoderServiceParams params = mProcessingRequests.get(filePath);
-        if (params != null) {
-            long endRpcCall = SystemClock.elapsedRealtime();
-            if (isVideo && bitmaps != null) {
-                if (bitmaps != null && bitmaps.size() > 1) {
-                    RecordHistogram.recordTimesHistogram(
-                            "Android.PhotoPicker.RequestProcessTimeAnimation",
-                            endRpcCall - params.mTimestamp);
-                } else {
-                    RecordHistogram.recordTimesHistogram(
-                            "Android.PhotoPicker.RequestProcessTimeThumbnail",
-                            endRpcCall - params.mTimestamp);
-                }
+        // If this assert triggers, it means that simultaneous requests have been sent for
+        // decoding, which should not happen.
+        assert mProcessingRequest.mUri.getPath().equals(filePath);
+        long endRpcCall = SystemClock.elapsedRealtime();
+        if (isVideo && bitmaps != null) {
+            if (bitmaps != null && bitmaps.size() > 1) {
+                RecordHistogram.recordTimesHistogram(
+                        "Android.PhotoPicker.RequestProcessTimeAnimation",
+                        endRpcCall - mProcessingRequest.mTimestamp);
             } else {
                 RecordHistogram.recordTimesHistogram(
-                        "Android.PhotoPicker.RequestProcessTime", endRpcCall - params.mTimestamp);
+                        "Android.PhotoPicker.RequestProcessTimeThumbnail",
+                        endRpcCall - mProcessingRequest.mTimestamp);
             }
+        } else {
+            RecordHistogram.recordTimesHistogram("Android.PhotoPicker.RequestProcessTime",
+                    endRpcCall - mProcessingRequest.mTimestamp);
+        }
+
+        mProcessingRequest.mCallback.imagesDecodedCallback(
+                filePath, isVideo, fullWidth, bitmaps, videoDuration, ratio);
 
-            params.mCallback.imagesDecodedCallback(
-                    filePath, isVideo, fullWidth, bitmaps, videoDuration, ratio);
-
-            if (decodeTime != -1 && bitmaps != null && bitmaps.get(0) != null) {
-                int sizeInKB = bitmaps.get(0).getByteCount() / ConversionUtils.BYTES_PER_KILOBYTE;
-                if (isVideo) {
-                    if (bitmaps.size() > 1) {
-                        RecordHistogram.recordTimesHistogram(
-                                "Android.PhotoPicker.VideoDecodeTimeAnimation", decodeTime);
-                    } else {
-                        RecordHistogram.recordTimesHistogram(
-                                "Android.PhotoPicker.VideoDecodeTimeThumbnail", decodeTime);
-                        RecordHistogram.recordCustomCountHistogram(
-                                "Android.PhotoPicker.VideoByteCount", sizeInKB, 1, 100000, 50);
-                    }
+        if (decodeTime != -1 && bitmaps != null && bitmaps.get(0) != null) {
+            int sizeInKB = bitmaps.get(0).getByteCount() / ConversionUtils.BYTES_PER_KILOBYTE;
+            if (isVideo) {
+                if (bitmaps.size() > 1) {
+                    RecordHistogram.recordTimesHistogram(
+                            "Android.PhotoPicker.VideoDecodeTimeAnimation", decodeTime);
                 } else {
                     RecordHistogram.recordTimesHistogram(
-                            "Android.PhotoPicker.ImageDecodeTime", decodeTime);
+                            "Android.PhotoPicker.VideoDecodeTimeThumbnail", decodeTime);
                     RecordHistogram.recordCustomCountHistogram(
-                            "Android.PhotoPicker.ImageByteCount", sizeInKB, 1, 100000, 50);
+                            "Android.PhotoPicker.VideoByteCount", sizeInKB, 1, 100000, 50);
                 }
+            } else {
+                RecordHistogram.recordTimesHistogram(
+                        "Android.PhotoPicker.ImageDecodeTime", decodeTime);
+                RecordHistogram.recordCustomCountHistogram(
+                        "Android.PhotoPicker.ImageByteCount", sizeInKB, 1, 100000, 50);
             }
-            mProcessingRequests.remove(filePath);
         }
+        mProcessingRequest = null;
 
         dispatchNextDecodeRequest();
     }
@@ -544,12 +541,13 @@ public class DecoderServiceHost
      * @param filePath The path to the image to cancel decoding.
      */
     public void cancelDecodeImage(String filePath) {
+        // It is important not to null out only pending requests and not mProcessingRequest, because
+        // it is used as a signal to see if the decoder is busy.
         Iterator it = mPendingRequests.iterator();
         while (it.hasNext()) {
             DecoderServiceParams param = (DecoderServiceParams) it.next();
             if (param.mUri.getPath().equals(filePath)) it.remove();
         }
-        mProcessingRequests.remove(filePath);
     }
 
     /** Sets a callback to use when the service is ready. For testing use only. */

chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHostTest.java

@@ -244,4 +244,41 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
 
         host.unbind(mContext);
     }
+
+    @Test
+    @LargeTest
+    public void testCancelation() throws Throwable {
+        DecoderServiceHost host = new DecoderServiceHost(this, mContext);
+        host.bind(mContext);
+        waitForDecoder();
+
+        String green = "green100x100.jpg";
+        String yellow = "yellow100x100.jpg";
+        String red = "red100x100.jpg";
+        String filePath = "chrome/test/data/android/photo_picker/";
+        String greenPath = UrlUtils.getIsolatedTestFilePath(filePath + green);
+        String yellowPath = UrlUtils.getIsolatedTestFilePath(filePath + yellow);
+        String redPath = UrlUtils.getIsolatedTestFilePath(filePath + red);
+
+        host.decodeImage(Uri.fromFile(new File(greenPath)), PickerBitmap.TileTypes.PICTURE, 10,
+                /*fullWidth=*/false, this);
+        host.decodeImage(Uri.fromFile(new File(yellowPath)), PickerBitmap.TileTypes.PICTURE, 10,
+                /*fullWidth=*/false, this);
+
+        // Now add and subsequently remove the request.
+        host.decodeImage(Uri.fromFile(new File(redPath)), PickerBitmap.TileTypes.PICTURE, 10,
+                /*fullWidth=*/false, this);
+        host.cancelDecodeImage(redPath);
+
+        // First decoding result should be the green image.
+        waitForThumbnailDecode();
+        Assert.assertEquals(greenPath, mLastDecodedPath);
+
+        // Next is the yellow image, and asserts in DecoderServiceHost (designed to catch when
+        // multiple simultaneous decoding requests are started) should not fire.
+        waitForThumbnailDecode();
+        Assert.assertEquals(yellowPath, mLastDecodedPath);
+
+        host.unbind(mContext);
+    }
 }