[Android] Photo Picker: Prioritize still image decoding ahead of videos.

Still images are quite fast to decode, in comparison to videos, which
need to be decoded in two phases (first frame and then rest). We should
not hold up still image decoding while video are being processed. This
will make the picker dialog more responsive with mixed content.

Bug: 1029056, 895776
Change-Id: Ida535a273a9aef50fc6f567b0e3300b3299fe769
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031029
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Auto-Submit: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740289}

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

@@ -33,8 +33,12 @@ import org.chromium.content_public.browser.UiThreadTaskTraits;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 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;
 
 /**
  * A class to communicate with the {@link DecoderService}.
@@ -127,41 +131,67 @@ public class DecoderServiceHost
      */
     private static class DecoderServiceParams {
         // The URI for the file containing the bitmap to decode.
-        public Uri mUri;
+        final Uri mUri;
 
         // The requested width of the bitmap, once decoded.
-        public int mWidth;
+        final int mWidth;
 
         // Whether this is image is taking up the full width of the screen.
-        public boolean mFullWidth;
+        final boolean mFullWidth;
 
         // The type of media being decoded.
         @PickerBitmap.TileTypes
-        int mFileType;
+        final int mFileType;
+
+        // Whether this is a request to decode only the first frame of a video. This field is
+        // ignored for non-videos.
+        final boolean mFirstFrame;
+
+        // When the request was added.
+        final Date mCreateTime;
 
         // The callback to use to communicate the results of the decoding.
-        ImagesDecodedCallback mCallback;
+        final ImagesDecodedCallback mCallback;
 
         // The timestamp for when the request was sent for decoding.
         long mTimestamp;
 
         public DecoderServiceParams(Uri uri, int width, boolean fullWidth,
-                @PickerBitmap.TileTypes int fileType, ImagesDecodedCallback callback) {
+                @PickerBitmap.TileTypes int fileType, boolean firstFrame,
+                ImagesDecodedCallback callback) {
             mUri = uri;
             mWidth = width;
             mFullWidth = fullWidth;
             mFileType = fileType;
+            mFirstFrame = firstFrame;
+            mCreateTime = new Date();
             mCallback = callback;
         }
     }
 
-    // Map of file paths to pending decoding requests of high priority.
-    private LinkedHashMap<String, DecoderServiceParams> mHighPriorityRequests =
-            new LinkedHashMap<>();
+    // A request comparator that assign priority in the following way:
+    // Top priority: Still images (TileType.PICTURES).
+    // Medium priority: Videos (decoding of first frame only).
+    // Low priority: Videos (multiple frames for animating).
+    private Comparator<DecoderServiceParams> mRequestComparator = (r1, r2) -> {
+        if (r1.mFileType != r2.mFileType) {
+            return r1.mFileType - r2.mFileType;
+        }
+
+        // If they are both video types, then first frame decoding has priority.
+        if (r1.mFileType == PickerBitmap.TileTypes.VIDEO && r1.mFirstFrame != r2.mFirstFrame) {
+            return r1.mFirstFrame ? -1 : 1;
+        }
+
+        // The two requests share the same file type, or are identical video requests (both
+        // requesting first frame or both requesting additional frames) so they can be considered
+        // equal. Go with first in first out.
+        return r1.mCreateTime.compareTo(r2.mCreateTime);
+    };
 
-    // Map of file paths to pending decoding requests of low priority.
-    private LinkedHashMap<String, DecoderServiceParams> mLowPriorityRequests =
-            new LinkedHashMap<>();
+    // A queue of pending requests.
+    PriorityQueue<DecoderServiceParams> mPendingRequests =
+            new PriorityQueue<>(/*initialCapacity=*/1, mRequestComparator);
 
     // Map of file paths to processing decoding requests.
     private LinkedHashMap<String, DecoderServiceParams> mProcessingRequests = new LinkedHashMap<>();
@@ -219,55 +249,40 @@ public class DecoderServiceHost
      */
     public void decodeImage(Uri uri, @PickerBitmap.TileTypes int fileType, int width,
             boolean fullWidth, ImagesDecodedCallback callback) {
-        DecoderServiceParams params =
-                new DecoderServiceParams(uri, width, fullWidth, fileType, callback);
-        mHighPriorityRequests.put(uri.getPath(), params);
-        if (mProcessingRequests.size() == 0) dispatchNextDecodeRequest();
-    }
-
-    /**
-     * Fetches the next high-priority decoding request from the queue and removes it from the queue.
-     * If that request is a video decoding request, a request for decoding additional frames is
-     * added to the low-priority queue.
-     * @return Next high-priority request pending.
-     */
-    private DecoderServiceParams getNextHighPriority() {
-        assert mHighPriorityRequests.size() > 0;
-        DecoderServiceParams params = mHighPriorityRequests.entrySet().iterator().next().getValue();
-        mHighPriorityRequests.remove(params.mUri.getPath());
+        DecoderServiceParams params = new DecoderServiceParams(
+                uri, width, fullWidth, fileType, /*firstFrame=*/true, callback);
+        mPendingRequests.add(params);
         if (params.mFileType == PickerBitmap.TileTypes.VIDEO) {
-            // High-priority decoding requests for videos are requests for first frames (see
-            // dispatchDecodeVideoRequest). Adding another low-priority request is a request for
-            // decoding the rest of the frames.
-            DecoderServiceParams lowPriorityRequest = new DecoderServiceParams(params.mUri,
-                    params.mWidth, params.mFullWidth, params.mFileType, params.mCallback);
-            mLowPriorityRequests.put(params.mUri.getPath(), lowPriorityRequest);
+            // Decoding requests for videos are requests for first frames only. Add another
+            // low-priority request for decoding the rest of the frames.
+            DecoderServiceParams lowPriorityRequest =
+                    new DecoderServiceParams(params.mUri, params.mWidth, params.mFullWidth,
+                            params.mFileType, /*firstFrame=*/false, params.mCallback);
+            mPendingRequests.add(lowPriorityRequest);
         }
-        return params;
+
+        if (mProcessingRequests.size() == 0) dispatchNextDecodeRequest();
     }
 
     /**
-     * Fetches the next low-priority decoding request from the queue and removes it from the queue.
-     * @return Next low-priority request pending, or null. Null can be returned in two scenarios:
-     *         If no requests remain or if the only request remaining is a low-priority request
-     *         where it's high-priority counterpart is still being processed.
+     * Fetches the next decoding request from the queue (and removes it from the queue). Note: Still
+     * images are preferred over videos (if available) because they are both faster to complete
+     * decoding and (arguably) also more likely to be shared by the user.
+     * @return Next pending request (of highest priority).
      */
-    private DecoderServiceParams getNextLowPriority() {
-        for (DecoderServiceParams request : mLowPriorityRequests.values()) {
-            String filePath = request.mUri.getPath();
-            if (mProcessingRequests.get(filePath) != null) continue;
-            mLowPriorityRequests.remove(filePath);
-            return request;
+    private DecoderServiceParams getNextPending() {
+        if (mPendingRequests.isEmpty()) {
+            return null;
         }
-        return null;
+
+        return mPendingRequests.remove();
     }
 
     /**
      * Dispatches the next image/video for decoding (from the queue).
      */
     private void dispatchNextDecodeRequest() {
-        boolean highPriority = mHighPriorityRequests.entrySet().iterator().hasNext();
-        DecoderServiceParams params = highPriority ? getNextHighPriority() : getNextLowPriority();
+        DecoderServiceParams params = getNextPending();
         if (params != null) {
             mProcessingRequests.put(params.mUri.getPath(), params);
 
@@ -275,7 +290,7 @@ public class DecoderServiceHost
             if (params.mFileType != PickerBitmap.TileTypes.VIDEO) {
                 dispatchDecodeImageRequest(params);
             } else {
-                dispatchDecodeVideoRequest(params, highPriority);
+                dispatchDecodeVideoRequest(params, params.mFirstFrame);
             }
             return;
         }
@@ -463,14 +478,14 @@ public class DecoderServiceHost
     /**
      * Communicates with the utility process to decode a single video.
      * @param params The information about the decoding request.
-     * @param highPriority True if the decoding request is a high-priority request.
+     * @param firstFrame True if the decoding request is for the first frame only.
      */
-    private void dispatchDecodeVideoRequest(DecoderServiceParams params, boolean highPriority) {
+    private void dispatchDecodeVideoRequest(DecoderServiceParams params, boolean firstFrame) {
         // Videos are decoded by the system (on N+) using a restricted helper process, so
         // there's no need to use our custom sandboxed process.
         assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.N;
 
-        int frames = highPriority ? 1 : 10;
+        int frames = firstFrame ? 1 : 10;
         int intervalMs = 2000;
         mWorkerTask = new DecodeVideoTask(this, mContentResolver, params.mUri, params.mWidth,
                 params.mFullWidth, frames, intervalMs);
@@ -529,8 +544,11 @@ public class DecoderServiceHost
      * @param filePath The path to the image to cancel decoding.
      */
     public void cancelDecodeImage(String filePath) {
-        mHighPriorityRequests.remove(filePath);
-        mLowPriorityRequests.remove(filePath);
+        Iterator it = mPendingRequests.iterator();
+        while (it.hasNext()) {
+            DecoderServiceParams param = (DecoderServiceParams) it.next();
+            if (param.mUri.getPath().equals(filePath)) it.remove();
+        }
         mProcessingRequests.remove(filePath);
     }
 

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

@@ -19,7 +19,9 @@ import java.util.Date;
  * A class to keep track of the meta data associated with a an image in the photo picker.
  */
 public class PickerBitmap implements Comparable<PickerBitmap> {
-    // The possible types of tiles involved in the viewer.
+    // The possible types of tiles involved in the viewer. Note that the values for PICTURE and
+    // VIDEO matter, because they are used to prioritize still images over videos in the priority
+    // queue in PickerCategoryView.
     @IntDef({TileTypes.PICTURE, TileTypes.CAMERA, TileTypes.GALLERY, TileTypes.VIDEO})
     @Retention(RetentionPolicy.SOURCE)
     public @interface TileTypes {
@@ -46,6 +48,10 @@ public class PickerBitmap implements Comparable<PickerBitmap> {
      * @param type The type of tile involved.
      */
     public PickerBitmap(Uri uri, long lastModified, @TileTypes int type) {
+        // PICTURE must have a lower value than VIDEO, in order for the priority queue in
+        // PickerCategoryView to prioritize still images ahead of video.
+        assert TileTypes.PICTURE < TileTypes.VIDEO;
+
         mUri = uri;
         mLastModified = lastModified;
         mType = type;

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

@@ -107,13 +107,13 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.bind(mContext);
         waitForDecoder();
 
-        String fileName1 = "noogler.mp4";
-        String fileName2 = "noogler2.mp4";
-        String fileName3 = "blue100x100.jpg";
+        String video1 = "noogler.mp4";
+        String video2 = "noogler2.mp4";
+        String jpg1 = "blue100x100.jpg";
         String filePath = "chrome/test/data/android/photo_picker/";
-        File file1 = new File(UrlUtils.getIsolatedTestFilePath(filePath + fileName1));
-        File file2 = new File(UrlUtils.getIsolatedTestFilePath(filePath + fileName2));
-        File file3 = new File(UrlUtils.getIsolatedTestFilePath(filePath + fileName3));
+        File file1 = new File(UrlUtils.getIsolatedTestFilePath(filePath + video1));
+        File file2 = new File(UrlUtils.getIsolatedTestFilePath(filePath + video2));
+        File file3 = new File(UrlUtils.getIsolatedTestFilePath(filePath + jpg1));
 
         host.decodeImage(
                 Uri.fromFile(file1), PickerBitmap.TileTypes.VIDEO, 10, /*fullWidth=*/false, this);
@@ -122,38 +122,42 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.decodeImage(
                 Uri.fromFile(file3), PickerBitmap.TileTypes.PICTURE, 10, /*fullWidth=*/false, this);
 
-        // First decoding result should be first frame only of video 1.
+        // First decoding result should be first frame of video 1. Even though still images take
+        // priority over video decoding, video 1 will be the only item in the queue when the first
+        // decoding request is kicked off (as a result of calling decodeImage).
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
 
-        // Second decoding result is first frame of video 2, because that's higher priority than the
-        // rest of video 1.
+        // When the decoder is finished with the first frame of video 1, there will be two new
+        // requests available for processing. Video2 was added first, but that will be skipped in
+        // favor of the still image, so the jpg is expected to be decoded next.
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName2));
-        Assert.assertEquals(true, mLastIsVideo);
-        Assert.assertEquals("0:00", mLastVideoDuration);
+        Assert.assertTrue(mLastDecodedPath.contains(jpg1));
+        Assert.assertEquals(false, mLastIsVideo);
+        Assert.assertEquals(null, mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
 
-        // Third in line should be the jpg file.
+        // Third decoding result is first frame of video 2, because that's higher priority than the
+        // rest of video 1.
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName3));
-        Assert.assertEquals(false, mLastIsVideo);
-        Assert.assertEquals(null, mLastVideoDuration);
+        Assert.assertTrue(mLastDecodedPath.contains(video2));
+        Assert.assertEquals(true, mLastIsVideo);
+        Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
 
         // Remaining frames of video 1.
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(10, mLastFrameCount);
 
         // Remaining frames of video 2.
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName2));
+        Assert.assertTrue(mLastDecodedPath.contains(video2));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(10, mLastFrameCount);
@@ -168,17 +172,17 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.bind(mContext);
         waitForDecoder();
 
-        String fileName1 = "noogler.mp4"; // 1920 x 1080 video.
-        String fileName2 = "blue100x100.jpg";
+        String video1 = "noogler.mp4"; // 1920 x 1080 video.
+        String jpg1 = "blue100x100.jpg";
         String filePath = "chrome/test/data/android/photo_picker/";
-        File file1 = new File(UrlUtils.getIsolatedTestFilePath(filePath + fileName1));
-        File file2 = new File(UrlUtils.getIsolatedTestFilePath(filePath + fileName2));
+        File file1 = new File(UrlUtils.getIsolatedTestFilePath(filePath + video1));
+        File file2 = new File(UrlUtils.getIsolatedTestFilePath(filePath + jpg1));
 
         // Thumbnail photo. 100 x 100 -> 10 x 10.
         host.decodeImage(
                 Uri.fromFile(file2), PickerBitmap.TileTypes.PICTURE, 10, /*fullWidth=*/false, this);
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName2));
+        Assert.assertTrue(mLastDecodedPath.contains(jpg1));
         Assert.assertEquals(false, mLastIsVideo);
         Assert.assertEquals(null, mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
@@ -190,7 +194,7 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.decodeImage(
                 Uri.fromFile(file2), PickerBitmap.TileTypes.PICTURE, 200, /*fullWidth=*/true, this);
         waitForThumbnailDecode();
-        Assert.assertTrue(mLastDecodedPath.contains(fileName2));
+        Assert.assertTrue(mLastDecodedPath.contains(jpg1));
         Assert.assertEquals(false, mLastIsVideo);
         Assert.assertEquals(null, mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
@@ -202,7 +206,7 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.decodeImage(
                 Uri.fromFile(file1), PickerBitmap.TileTypes.VIDEO, 10, /*fullWidth=*/false, this);
         waitForThumbnailDecode(); // Initial frame.
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
@@ -210,7 +214,7 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         Assert.assertEquals(10, mLastInitialFrame.getWidth());
         Assert.assertEquals(10, mLastInitialFrame.getHeight());
         waitForThumbnailDecode(); // Rest of frames.
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(10, mLastFrameCount);
@@ -222,7 +226,7 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         host.decodeImage(
                 Uri.fromFile(file1), PickerBitmap.TileTypes.VIDEO, 2000, /*fullWidth=*/true, this);
         waitForThumbnailDecode(); // Initial frame.
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(1, mLastFrameCount);
@@ -230,7 +234,7 @@ public class DecoderServiceHostTest implements DecoderServiceHost.ServiceReadyCa
         Assert.assertEquals(2000, mLastInitialFrame.getWidth());
         Assert.assertEquals(1125, mLastInitialFrame.getHeight());
         waitForThumbnailDecode(); // Rest of frames.
-        Assert.assertTrue(mLastDecodedPath.contains(fileName1));
+        Assert.assertTrue(mLastDecodedPath.contains(video1));
         Assert.assertEquals(true, mLastIsVideo);
         Assert.assertEquals("0:00", mLastVideoDuration);
         Assert.assertEquals(10, mLastFrameCount);