Improve extremely inefficient darkness-checking logic. (#2639)

* Improve extremely inefficient darkness-checking logic.

* Use ExifInterface from AndroidX instead of android.media.

* Fix false-positive detekt check.

* Fix false-positive detekt check.

app/build.gradle

@@ -88,6 +88,7 @@ dependencies {
     implementation "androidx.browser:browser:1.0.0"
     implementation "androidx.cardview:cardview:1.0.0"
     implementation 'androidx.constraintlayout:constraintlayout:1.1.3'
+    implementation "androidx.exifinterface:exifinterface:1.0.0"
 
     //swipe_layout
     implementation 'com.daimajia.swipelayout:library:1.2.0@aar'

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java

@@ -2,14 +2,10 @@ package fr.free.nrw.commons.upload;
 
 import android.annotation.SuppressLint;
 import android.content.ContentResolver;
-import android.media.ExifInterface;
 import android.net.Uri;
-import android.os.Build;
-import android.os.ParcelFileDescriptor;
 import androidx.annotation.NonNull;
 
 import java.io.File;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.List;
 
@@ -17,6 +13,7 @@ import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import androidx.exifinterface.media.ExifInterface;
 import fr.free.nrw.commons.caching.CacheController;
 import fr.free.nrw.commons.kvstore.JsonKvStore;
 import fr.free.nrw.commons.mwapi.CategoryApi;
@@ -96,22 +93,14 @@ public class FileProcessor implements SimilarImageDialogFragment.onResponse {
                 //Make sure the photos were taken within 20seconds
                 Timber.d("fild date:" + file.lastModified() + " time of creation" + timeOfCreation);
                 tempImageObj = null;//Temporary GPSExtractor to extract coords from these photos
-                ParcelFileDescriptor descriptor = null;
                 try {
-                    descriptor = contentResolver.openFileDescriptor(Uri.fromFile(file), "r");
+                    tempImageObj = new GPSExtractor(contentResolver.openInputStream(Uri.fromFile(file)));
-                } catch (FileNotFoundException e) {
+                } catch (Exception e) {
                     e.printStackTrace();
                 }
-                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+                if (tempImageObj != null) {
-                    if (descriptor != null) {
+                    tempImageObj = new GPSExtractor(file.getAbsolutePath());
-                        tempImageObj = new GPSExtractor(descriptor.getFileDescriptor());
-                    }
-                } else {
-                    if (filePath != null) {
-                        tempImageObj = new GPSExtractor(file.getAbsolutePath());
-                    }
                 }
-
                 if (tempImageObj != null) {
                     Timber.d("not null fild EXIF" + tempImageObj.imageCoordsExists + " coords" + tempImageObj.getCoords());
                     if (tempImageObj.getCoords() != null && tempImageObj.imageCoordsExists) {

app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java

@@ -2,7 +2,6 @@ package fr.free.nrw.commons.upload;
 
 import android.content.ContentResolver;
 import android.content.Context;
-import android.media.ExifInterface;
 import android.net.Uri;
 import android.webkit.MimeTypeMap;
 
@@ -17,6 +16,7 @@ import java.math.BigInteger;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 
+import androidx.exifinterface.media.ExifInterface;
 import timber.log.Timber;
 
 public class FileUtils {

app/src/main/java/fr/free/nrw/commons/upload/GPSExtractor.java

@@ -1,13 +1,12 @@
 package fr.free.nrw.commons.upload;
 
-import android.media.ExifInterface;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
-import androidx.annotation.RequiresApi;
 
-import java.io.FileDescriptor;
 import java.io.IOException;
+import java.io.InputStream;
 
+import androidx.exifinterface.media.ExifInterface;
 import timber.log.Timber;
 
 /**
@@ -33,17 +32,11 @@ class GPSExtractor {
 
     }
     /**
-     * Construct from the file descriptor of the image (only for API 24 or newer).
+     * Construct from a stream.
-     * @param fileDescriptor the file descriptor of the image
      */
-    @RequiresApi(24)
+    GPSExtractor(@NonNull InputStream stream) throws IOException {
-    GPSExtractor(@NonNull FileDescriptor fileDescriptor) {
+        ExifInterface exif = new ExifInterface(stream);
-        try {
+        processCoords(exif);
-            ExifInterface exif = new ExifInterface(fileDescriptor);
-            processCoords(exif);
-        } catch (IOException | IllegalArgumentException e) {
-            Timber.w(e);
-        }
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java

@@ -10,7 +10,6 @@ import javax.inject.Singleton;
 
 import fr.free.nrw.commons.mwapi.MediaWikiApi;
 import fr.free.nrw.commons.nearby.Place;
-import fr.free.nrw.commons.utils.BitmapRegionDecoderWrapper;
 import fr.free.nrw.commons.utils.ImageUtils;
 import fr.free.nrw.commons.utils.ImageUtilsWrapper;
 import fr.free.nrw.commons.utils.StringUtils;
@@ -30,7 +29,6 @@ import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;
 @Singleton
 public class ImageProcessingService {
     private final FileUtilsWrapper fileUtilsWrapper;
-    private final BitmapRegionDecoderWrapper bitmapRegionDecoderWrapper;
     private final ImageUtilsWrapper imageUtilsWrapper;
     private final MediaWikiApi mwApi;
     private final ReadFBMD readFBMD;
@@ -38,11 +36,9 @@ public class ImageProcessingService {
 
     @Inject
     public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,
-                                  BitmapRegionDecoderWrapper bitmapRegionDecoderWrapper,
                                   ImageUtilsWrapper imageUtilsWrapper,
                                   MediaWikiApi mwApi, ReadFBMD readFBMD, EXIFReader EXIFReader) {
         this.fileUtilsWrapper = fileUtilsWrapper;
-        this.bitmapRegionDecoderWrapper = bitmapRegionDecoderWrapper;
         this.imageUtilsWrapper = imageUtilsWrapper;
         this.mwApi = mwApi;
         this.readFBMD = readFBMD;
@@ -161,10 +157,7 @@ public class ImageProcessingService {
      */
     private Single<Integer> checkDarkImage(String filePath) {
         Timber.d("Checking for dark image %s", filePath);
-        return Single.fromCallable(() ->
+        return imageUtilsWrapper.checkIfImageIsTooDark(filePath);
-                fileUtilsWrapper.getFileInputStream(filePath))
-                .map(file -> bitmapRegionDecoderWrapper.newInstance(file, false))
-                .flatMap(imageUtilsWrapper::checkIfImageIsTooDark);
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/utils/BitmapRegionDecoderWrapper.java

@@ -1,22 +0,0 @@
-package fr.free.nrw.commons.utils;
-
-import android.graphics.BitmapRegionDecoder;
-
-import java.io.FileInputStream;
-import java.io.IOException;
-
-import javax.inject.Inject;
-import javax.inject.Singleton;
-
-@Singleton
-public class BitmapRegionDecoderWrapper {
-
-    @Inject
-    public BitmapRegionDecoderWrapper() {
-
-    }
-
-    public BitmapRegionDecoder newInstance(FileInputStream file, boolean isSharable) throws IOException {
-        return BitmapRegionDecoder.newInstance(file, isSharable);
-    }
-}

app/src/main/java/fr/free/nrw/commons/utils/ImageUtils.java

@@ -3,10 +3,10 @@ package fr.free.nrw.commons.utils;
 import android.app.WallpaperManager;
 import android.content.Context;
 import android.graphics.Bitmap;
-import android.graphics.BitmapRegionDecoder;
+import android.graphics.BitmapFactory;
 import android.graphics.Color;
-import android.graphics.Rect;
 import android.net.Uri;
+
 import androidx.annotation.IntDef;
 import androidx.annotation.Nullable;
 
@@ -24,6 +24,7 @@ import java.io.IOException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 
+import androidx.exifinterface.media.ExifInterface;
 import fr.free.nrw.commons.R;
 import fr.free.nrw.commons.location.LatLng;
 import timber.log.Timber;
@@ -87,33 +88,26 @@ public class ImageUtils {
     }
 
     /**
-     * @param bitmapRegionDecoder BitmapRegionDecoder for the image we wish to process
+     * @return IMAGE_OK if image is not too dark
-     * @return IMAGE_OK if image is neither dark nor blurry or if the input bitmapRegionDecoder provided is null
      * IMAGE_DARK if image is too dark
      */
-    static @Result
+    static @Result int checkIfImageIsTooDark(String imagePath) {
-    int checkIfImageIsTooDark(BitmapRegionDecoder bitmapRegionDecoder) {
+        long millis = System.currentTimeMillis();
-        if (bitmapRegionDecoder == null) {
+        try {
-            Timber.e("Expected bitmapRegionDecoder was null");
+            Bitmap bmp = new ExifInterface(imagePath).getThumbnailBitmap();
-            return IMAGE_OK;
+            if (bmp == null) {
+                bmp = BitmapFactory.decodeFile(imagePath);
+            }
+
+            if (checkIfImageIsDark(bmp)) {
+                return IMAGE_DARK;
+            }
+
+        } catch (Exception e) {
+            Timber.d(e, "Error while checking image darkness.");
+        } finally {
+            Timber.d("Checking image darkness took " + (System.currentTimeMillis() - millis) + " ms.");
         }
-
-        int loadImageHeight = bitmapRegionDecoder.getHeight();
-        int loadImageWidth = bitmapRegionDecoder.getWidth();
-
-        int checkImageTopPosition = 0;
-        int checkImageLeftPosition = 0;
-
-        Timber.v("left: " + checkImageLeftPosition + " right: " + loadImageWidth + " top: " + checkImageTopPosition + " bottom: " + loadImageHeight);
-
-        Rect rect = new Rect(checkImageLeftPosition,checkImageTopPosition, loadImageWidth, loadImageHeight);
-
-        Bitmap processBitmap = bitmapRegionDecoder.decodeRegion(rect,null);
-
-        if (checkIfImageIsDark(processBitmap)) {
-            return IMAGE_DARK;
-        }
-
         return IMAGE_OK;
     }
 
@@ -147,7 +141,6 @@ public class ImageUtils {
         int bitmapHeight = bitmap.getHeight();
 
         int allPixelsCount = bitmapWidth * bitmapHeight;
-        Timber.d("total %s", Integer.toString(allPixelsCount));
         int numberOfBrightPixels = 0;
         int numberOfMediumBrightnessPixels = 0;
         double brightPixelThreshold = 0.025 * allPixelsCount;

app/src/main/java/fr/free/nrw/commons/utils/ImageUtilsWrapper.java

@@ -1,7 +1,5 @@
 package fr.free.nrw.commons.utils;
 
-import android.graphics.BitmapRegionDecoder;
-
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
@@ -17,9 +15,8 @@ public class ImageUtilsWrapper {
 
     }
 
-    public Single<Integer> checkIfImageIsTooDark(BitmapRegionDecoder bitmapRegionDecoder) {
+    public Single<Integer> checkIfImageIsTooDark(String bitmapPath) {
-        int isImageDark = ImageUtils.checkIfImageIsTooDark(bitmapRegionDecoder);
+        return Single.just(ImageUtils.checkIfImageIsTooDark(bitmapPath))
-        return Single.just(isImageDark)
                 .subscribeOn(Schedulers.computation())
                 .observeOn(Schedulers.computation());
     }

app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt

@@ -1,11 +1,9 @@
 package fr.free.nrw.commons.upload
 
-import android.graphics.BitmapRegionDecoder
 import android.net.Uri
 import fr.free.nrw.commons.location.LatLng
 import fr.free.nrw.commons.mwapi.MediaWikiApi
 import fr.free.nrw.commons.nearby.Place
-import fr.free.nrw.commons.utils.BitmapRegionDecoderWrapper
 import fr.free.nrw.commons.utils.ImageUtils
 import fr.free.nrw.commons.utils.ImageUtilsWrapper
 import io.reactivex.Single
@@ -23,8 +21,6 @@ class u {
     @Mock
     internal var fileUtilsWrapper: FileUtilsWrapper? = null
     @Mock
-    internal var bitmapRegionDecoderWrapper: BitmapRegionDecoderWrapper? = null
-    @Mock
     internal var imageUtilsWrapper: ImageUtilsWrapper? = null
     @Mock
     internal var mwApi: MediaWikiApi? = null
@@ -68,9 +64,7 @@ class u {
         `when`(fileUtilsWrapper!!.getGeolocationOfFile(ArgumentMatchers.anyString()))
                 .thenReturn("latLng")
 
-        `when`(bitmapRegionDecoderWrapper!!.newInstance(any(FileInputStream::class.java), anyBoolean()))
+        `when`(imageUtilsWrapper?.checkIfImageIsTooDark(ArgumentMatchers.anyString()))
-                .thenReturn(mock(BitmapRegionDecoder::class.java))
-        `when`(imageUtilsWrapper!!.checkIfImageIsTooDark(any(BitmapRegionDecoder::class.java)))
                 .thenReturn(Single.just(ImageUtils.IMAGE_OK))
 
         `when`(imageUtilsWrapper!!.checkImageGeolocationIsDifferent(ArgumentMatchers.anyString(), any(LatLng::class.java)))
@@ -113,7 +107,7 @@ class u {
 
     @Test
     fun validateImageForDarkImage() {
-        `when`(imageUtilsWrapper!!.checkIfImageIsTooDark(any(BitmapRegionDecoder::class.java)))
+        `when`(imageUtilsWrapper?.checkIfImageIsTooDark(ArgumentMatchers.anyString()))
                 .thenReturn(Single.just(ImageUtils.IMAGE_DARK))
         val validateImage = imageProcessingService!!.validateImage(uploadItem, false)
         assertEquals(ImageUtils.IMAGE_DARK, validateImage.blockingGet())