Skip to content

Commit

Permalink
#3493 App freezes for 15 seconds when you press Next in UploadMediaDe…
Browse files Browse the repository at this point in the history
…tailsFragment - add apropriate schedulers and convert justs to fromCallable
  • Loading branch information
macgills committed Mar 12, 2020
1 parent d925e32 commit 6a2a456
Show file tree
Hide file tree
Showing 14 changed files with 124 additions and 157 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
* ask the UplaodModel for the image quality of the UploadItem
*
* @param uploadItem
* @param shouldValidateTitle
* @return
*/
public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {
return uploadModel.getImageQuality(uploadItem, shouldValidateTitle);
public Single<Integer> getImageQuality(UploadItem uploadItem) {
return uploadModel.getImageQuality(uploadItem);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
* query the RemoteDataSource for image quality
*
* @param uploadItem
* @param shouldValidateTitle
* @return
*/
public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {
return remoteDataSource.getImageQuality(uploadItem, shouldValidateTitle);
public Single<Integer> getImageQuality(UploadItem uploadItem) {
return remoteDataSource.getImageQuality(uploadItem);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
package fr.free.nrw.commons.upload;

import android.content.Context;

import org.apache.commons.lang3.StringUtils;

import javax.inject.Inject;
import javax.inject.Singleton;
import static fr.free.nrw.commons.utils.ImageUtils.EMPTY_TITLE;
import static fr.free.nrw.commons.utils.ImageUtils.FILE_NAME_EXISTS;
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;

import android.content.Context;
import fr.free.nrw.commons.media.MediaClient;
import fr.free.nrw.commons.nearby.Place;
import fr.free.nrw.commons.utils.ImageUtils;
import fr.free.nrw.commons.utils.ImageUtilsWrapper;
import io.reactivex.Single;
import io.reactivex.schedulers.Schedulers;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.apache.commons.lang3.StringUtils;
import timber.log.Timber;

import static fr.free.nrw.commons.utils.ImageUtils.EMPTY_TITLE;
import static fr.free.nrw.commons.utils.ImageUtils.FILE_NAME_EXISTS;
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;

/**
* Methods for pre-processing images to be uploaded
*/
Expand All @@ -41,38 +39,33 @@ public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,
this.mediaClient = mediaClient;
}

/**
* Check image quality before upload
* - checks duplicate image
* - checks dark image
* - checks geolocation for image
* - check for valid title
*/
Single<Integer> validateImage(UploadModel.UploadItem uploadItem, boolean checkTitle) {
int currentImageQuality = uploadItem.getImageQuality();
Timber.d("Current image quality is %d", currentImageQuality);
if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
return Single.just(ImageUtils.IMAGE_OK);
}
Timber.d("Checking the validity of image");
String filePath = uploadItem.getMediaUri().getPath();
Single<Integer> duplicateImage = checkDuplicateImage(filePath);
Single<Integer> wrongGeoLocation = checkImageGeoLocation(uploadItem.getPlace(), filePath);
Single<Integer> darkImage = checkDarkImage(filePath);
Single<Integer> itemTitle = checkTitle ? validateItemTitle(uploadItem) : Single.just(ImageUtils.IMAGE_OK);
Single<Integer> checkFBMD = checkFBMD(filePath);
Single<Integer> checkEXIF = checkEXIF(filePath);

Single<Integer> zipResult = Single.zip(duplicateImage, wrongGeoLocation, darkImage, itemTitle,
(duplicate, wrongGeo, dark, title) -> {
Timber.d("Result for duplicate: %d, geo: %d, dark: %d, title: %d", duplicate, wrongGeo, dark, title);
return duplicate | wrongGeo | dark | title;
});
return Single.zip(zipResult, checkFBMD , checkEXIF , (zip , fbmd , exif)->{
Timber.d("zip:" + zip + "fbmd:" + fbmd + "exif:" + exif);
return zip | fbmd | exif;
});
/**
* Check image quality before upload - checks duplicate image - checks dark image - checks
* geolocation for image - check for valid title
*/
Single<Integer> validateImage(UploadModel.UploadItem uploadItem) {
int currentImageQuality = uploadItem.getImageQuality();
Timber.d("Current image quality is %d", currentImageQuality);
if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
return Single.just(ImageUtils.IMAGE_OK);
}
Timber.d("Checking the validity of image");
String filePath = uploadItem.getMediaUri().getPath();

return Single.zip(
checkDuplicateImage(filePath),
checkImageGeoLocation(uploadItem.getPlace(), filePath),
checkDarkImage(filePath),
validateItemTitle(uploadItem),
checkFBMD(filePath),
checkEXIF(filePath),
(duplicateImage, wrongGeoLocation, darkImage, itemTitle, fbmd, exif) -> {
Timber.d("duplicate: %d, geo: %d, dark: %d, title: %d" + "fbmd:" + fbmd + "exif:" + exif,
duplicateImage, wrongGeoLocation, darkImage, itemTitle);
return duplicateImage | wrongGeoLocation | darkImage | itemTitle | fbmd | exif;
}
);
}

/**
* We want to discourage users from uploading images to Commons that were taken from Facebook.
Expand Down Expand Up @@ -113,7 +106,8 @@ private Single<Integer> validateItemTitle(UploadModel.UploadItem uploadItem) {
.map(doesFileExist -> {
Timber.d("Result for valid title is %s", doesFileExist);
return doesFileExist ? FILE_NAME_EXISTS : IMAGE_OK;
});
})
.subscribeOn(Schedulers.io());
}

/**
Expand All @@ -124,14 +118,14 @@ private Single<Integer> validateItemTitle(UploadModel.UploadItem uploadItem) {
*/
private Single<Integer> checkDuplicateImage(String filePath) {
Timber.d("Checking for duplicate image %s", filePath);
return Single.fromCallable(() ->
fileUtilsWrapper.getFileInputStream(filePath))
return Single.fromCallable(() -> fileUtilsWrapper.getFileInputStream(filePath))
.map(fileUtilsWrapper::getSHA1)
.flatMap(mediaClient::checkFileExistsUsingSha)
.map(b -> {
Timber.d("Result for duplicate image %s", b);
return b ? ImageUtils.IMAGE_DUPLICATE : ImageUtils.IMAGE_OK;
});
})
.subscribeOn(Schedulers.io());
}

/**
Expand Down Expand Up @@ -164,7 +158,8 @@ private Single<Integer> checkImageGeoLocation(Place place, String filePath) {
return Single.just(ImageUtils.IMAGE_OK);
}
return imageUtilsWrapper.checkImageGeolocationIsDifferent(geoLocation, place.getLocation());
});
})
.subscribeOn(Schedulers.io());
}
}

48 changes: 0 additions & 48 deletions app/src/main/java/fr/free/nrw/commons/upload/ReadFBMD.java

This file was deleted.

35 changes: 35 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/upload/ReadFBMD.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package fr.free.nrw.commons.upload

import fr.free.nrw.commons.utils.ImageUtils
import io.reactivex.Single
import io.reactivex.schedulers.Schedulers
import java.io.FileInputStream
import java.io.IOException
import javax.inject.Inject
import javax.inject.Singleton

/**
* We want to discourage users from uploading images to Commons that were taken from Facebook. This
* attempts to detect whether an image was downloaded from Facebook by heuristically searching for
* metadata that is specific to images that come from Facebook.
*/
@Singleton
class ReadFBMD @Inject constructor() {
fun processMetadata(path: String?) = Single.fromCallable {
try {
val fileStr = FileInputStream(path).use {
ByteArray(4096).apply { it.read(this) }.toString()
}
if (isFbmd(fileStr.indexOf("8BIM"), fileStr.indexOf("FBMD")))
return@fromCallable ImageUtils.FILE_FBMD
} catch (e: IOException) {
e.printStackTrace()
}
ImageUtils.IMAGE_OK
}.subscribeOn(Schedulers.io())

private fun isFbmd(psBlockOffset: Int, fbmdOffset: Int) =
psBlockOffset > 0 && fbmdOffset > 0 &&
fbmdOffset > psBlockOffset &&
fbmdOffset - psBlockOffset < 0x80
}
6 changes: 3 additions & 3 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile,
return Observable.just(getUploadItem(uploadableFile, place, source, similarImageInterface));
}

public Single<Integer> getImageQuality(UploadItem uploadItem, boolean checkTitle) {
return imageProcessingService.validateImage(uploadItem, checkTitle);
public Single<Integer> getImageQuality(UploadItem uploadItem) {
return imageProcessingService.validateImage(uploadItem);
}

private UploadItem getUploadItem(UploadableFile uploadableFile,
Expand Down Expand Up @@ -378,4 +378,4 @@ public int hashCode() {
}
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ private void showInfoAlert(int titleStringID, int messageStringId) {
@OnClick(R.id.btn_next)
public void onNextButtonClicked() {
uploadItem.setDescriptions(descriptionsAdapter.getDescriptions());
presenter.verifyImageQuality(uploadItem, true);
presenter.verifyImageQuality(uploadItem);
}

@OnClick(R.id.btn_previous)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ interface UserActionListener extends BasePresenter<View> {
void receiveImage(UploadableFile uploadableFile, @Contribution.FileSource String source,
Place place);

void verifyImageQuality(UploadItem uploadItem, boolean validateTitle);
void verifyImageQuality(UploadItem uploadItem);

void setUploadItem(int index, UploadItem uploadItem);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,14 @@ private void checkNearbyPlaces(UploadItem uploadItem) {
* asks the repository to verify image quality
*
* @param uploadItem
* @param validateTitle
*/
@Override
public void verifyImageQuality(UploadItem uploadItem, boolean validateTitle) {
public void verifyImageQuality(UploadItem uploadItem) {
view.showProgress(true);
Disposable imageQualityDisposable = repository
.getImageQuality(uploadItem, true)
.subscribeOn(ioScheduler)

compositeDisposable.add(
repository
.getImageQuality(uploadItem)
.observeOn(mainThreadScheduler)
.subscribe(imageResult -> {
view.showProgress(false);
Expand All @@ -129,9 +129,8 @@ public void verifyImageQuality(UploadItem uploadItem, boolean validateTitle) {
view.showMessage("" + throwable.getLocalizedMessage(),
R.color.color_error);
Timber.e(throwable, "Error occurred while handling image");
});

compositeDisposable.add(imageQualityDisposable);
})
);
}

/**
Expand Down
31 changes: 0 additions & 31 deletions app/src/main/java/fr/free/nrw/commons/utils/ImageUtilsWrapper.java

This file was deleted.

19 changes: 19 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/utils/ImageUtilsWrapper.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package fr.free.nrw.commons.utils

import fr.free.nrw.commons.location.LatLng
import io.reactivex.Single
import io.reactivex.schedulers.Schedulers
import javax.inject.Inject
import javax.inject.Singleton

@Singleton
class ImageUtilsWrapper @Inject constructor() {
fun checkIfImageIsTooDark(bitmapPath: String?) =
Single.fromCallable { ImageUtils.checkIfImageIsTooDark(bitmapPath) }
.subscribeOn(Schedulers.computation())

fun checkImageGeolocationIsDifferent(geolocationOfFileString: String?, latLng: LatLng?) =
Single.just(ImageUtils.checkImageGeolocationIsDifferent(geolocationOfFileString, latLng))
.subscribeOn(Schedulers.computation())
.map { isDifferent: Boolean -> if (isDifferent) ImageUtils.IMAGE_GEOLOCATION_DIFFERENT else ImageUtils.IMAGE_OK }
}
Loading

0 comments on commit 6a2a456

Please sign in to comment.