From f3354d1aaec1ab175cc2c28721448863d3f33923 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Thu, 20 Jun 2024 13:18:58 +0200 Subject: [PATCH] refactor: Improve IO performance and simplify code with Okio (#769) `ImageDownsizer.downsizeImage()`: - Remove the return value, it was ignored - Throw `FileNotFoundException` when `openInputStream` returns null `ImageDownsizer.getImageOrientation()`: - Throw `FileNotFoundException` when `openInputStream` returns null `MediaUploader.prepareMedia()`: - Copy URI contents using Okio buffers / source / sink `UriExtensions`: - Rename from `IOUtils` - Implement `Uri.copyToFile()` using Okio buffers / source / sink - Replace `ProgressRequestBody()` with `Uri.asRequestBody()` using Okio buffers / source / sink `DraftHelper.copyToFolder()` - Use Okio buffers / source / sink `CompositeWithOpaqueBackground` - Use constants `SIZE_BYTES` and `CHARSET` instead of magic values - Use `Objects.hash` when hashing multiple objects Based on work by Christophe Beyls in - https://github.com/tuskyapp/Tusky/pull/4366 - https://github.com/tuskyapp/Tusky/pull/4372 --- app/build.gradle.kts | 1 + app/lint-baseline.xml | 4 +- .../components/compose/ImageDownsizer.kt | 56 +++++++------ .../components/compose/MediaUploader.kt | 57 ++++++------- .../pachli/components/drafts/DraftHelper.kt | 9 +- .../app/pachli/network/ProgressRequestBody.kt | 54 ------------ .../util/CompositeWithOpaqueBackground.kt | 11 +-- app/src/main/java/app/pachli/util/IOUtils.kt | 50 ----------- .../main/java/app/pachli/util/MediaUtils.kt | 35 +------- .../java/app/pachli/util/UriExtensions.kt | 83 +++++++++++++++++++ core/designsystem/lint-baseline.xml | 4 +- gradle/libs.versions.toml | 2 + 12 files changed, 159 insertions(+), 207 deletions(-) delete mode 100644 app/src/main/java/app/pachli/network/ProgressRequestBody.kt delete mode 100644 app/src/main/java/app/pachli/util/IOUtils.kt create mode 100644 app/src/main/java/app/pachli/util/UriExtensions.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index b457274a7..3df8ce3fa 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -151,6 +151,7 @@ dependencies { implementation(libs.bundles.retrofit) implementation(libs.bundles.okhttp) + implementation(libs.okio) implementation(libs.conscrypt.android) diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index 0384fed8c..debef57f9 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -105,11 +105,11 @@ + options.inJustDecodeBounds = true + BitmapFactory.decodeStream(input, null, options) + } + // Get EXIF data, for orientation info. val orientation = getImageOrientation(uri, contentResolver) + /* Unfortunately, there isn't a determined worst case compression ratio for image * formats. So, the only way to tell if they're too big is to compress them and * test, and keep trying at smaller sizes. The initial estimate should be good for @@ -58,27 +59,20 @@ fun downsizeImage( * sure it gets downsized to below the limit. */ var scaledImageSize = 1024 do { - val outputStream = try { - FileOutputStream(tempFile) - } catch (e: FileNotFoundException) { - return false - } - val decodeBitmapInputStream = try { - contentResolver.openInputStream(uri) - } catch (e: FileNotFoundException) { - return false - } + val outputStream = FileOutputStream(tempFile) + val decodeBitmapInputStream = contentResolver.openInputStream(uri) + ?: throw FileNotFoundException("openInputStream returned null") options.inSampleSize = calculateInSampleSize(options, scaledImageSize, scaledImageSize) options.inJustDecodeBounds = false val scaledBitmap = decodeBitmapInputStream.use { BitmapFactory.decodeStream(it, null, options) - } ?: return false + } ?: return val reorientedBitmap = reorientBitmap(scaledBitmap, orientation) if (reorientedBitmap == null) { scaledBitmap.recycle() - return false + return } /* Retain transparency if there is any by encoding as png */ val format: CompressFormat = if (!reorientedBitmap.hasAlpha()) { @@ -90,6 +84,20 @@ fun downsizeImage( reorientedBitmap.recycle() scaledImageSize /= 2 } while (tempFile.length() > sizeLimit) - - return true +} + +/** + * @return The EXIF orientation of the image at the local [uri]. + * @throws FileNotFoundException if [uri] could not be opened. + */ +private fun getImageOrientation(uri: Uri, contentResolver: ContentResolver): Int { + val inputStream = contentResolver.openInputStream(uri) + ?: throw FileNotFoundException("openInputStream returned null") + + return inputStream.use { input -> + ExifInterface(input).getAttributeInt( + ExifInterface.TAG_ORIENTATION, + ExifInterface.ORIENTATION_NORMAL, + ) + } } diff --git a/app/src/main/java/app/pachli/components/compose/MediaUploader.kt b/app/src/main/java/app/pachli/components/compose/MediaUploader.kt index 920987ea6..6a53d33c0 100644 --- a/app/src/main/java/app/pachli/components/compose/MediaUploader.kt +++ b/app/src/main/java/app/pachli/components/compose/MediaUploader.kt @@ -16,7 +16,6 @@ package app.pachli.components.compose -import android.annotation.SuppressLint import android.content.ContentResolver import android.content.Context import android.media.MediaMetadataRetriever @@ -33,14 +32,12 @@ import app.pachli.core.common.string.randomAlphanumericString import app.pachli.core.data.model.InstanceInfo import app.pachli.core.network.model.MediaUploadApi import app.pachli.core.ui.extensions.getErrorString -import app.pachli.network.ProgressRequestBody import app.pachli.util.MEDIA_SIZE_UNKNOWN +import app.pachli.util.asRequestBody import app.pachli.util.getImageSquarePixels import app.pachli.util.getMediaSize import dagger.hilt.android.qualifiers.ApplicationContext import java.io.File -import java.io.FileInputStream -import java.io.FileOutputStream import java.io.IOException import javax.inject.Inject import javax.inject.Singleton @@ -60,6 +57,9 @@ import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.shareIn import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.MultipartBody +import okio.buffer +import okio.sink +import okio.source import retrofit2.HttpException import timber.log.Timber @@ -167,22 +167,20 @@ class MediaUploader @Inject constructor( val suffix = "." + MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType ?: "tmp") - contentResolver.openInputStream(inUri).use { input -> + contentResolver.openInputStream(inUri)?.source()?.buffer().use { input -> if (input == null) { Timber.w("Media input is null") uri = inUri return@use } val file = File.createTempFile("randomTemp1", suffix, context.cacheDir) - FileOutputStream(file.absoluteFile).use { out -> - input.copyTo(out) - uri = FileProvider.getUriForFile( - context, - BuildConfig.APPLICATION_ID + ".fileprovider", - file, - ) - mediaSize = getMediaSize(contentResolver, uri) - } + file.absoluteFile.sink().buffer().use { it.writeAll(input) } + uri = FileProvider.getUriForFile( + context, + BuildConfig.APPLICATION_ID + ".fileprovider", + file, + ) + mediaSize = getMediaSize(contentResolver, uri) } } ContentResolver.SCHEME_FILE -> { @@ -195,17 +193,16 @@ class MediaUploader @Inject constructor( val suffix = inputFile.name.substringAfterLast('.', "tmp") mimeType = MimeTypeMap.getSingleton().getMimeTypeFromExtension(suffix) val file = File.createTempFile("randomTemp1", ".$suffix", context.cacheDir) - val input = FileInputStream(inputFile) - FileOutputStream(file.absoluteFile).use { out -> - input.copyTo(out) - uri = FileProvider.getUriForFile( - context, - BuildConfig.APPLICATION_ID + ".fileprovider", - file, - ) - mediaSize = getMediaSize(contentResolver, uri) + inputFile.source().buffer().use { input -> + file.absoluteFile.sink().buffer().use { it.writeAll(input) } } + uri = FileProvider.getUriForFile( + context, + BuildConfig.APPLICATION_ID + ".fileprovider", + file, + ) + mediaSize = getMediaSize(contentResolver, uri) } else -> { Timber.w("Unknown uri scheme %s", uri) @@ -268,24 +265,20 @@ class MediaUploader @Inject constructor( } val map = MimeTypeMap.getSingleton() val fileExtension = map.getExtensionFromMimeType(mimeType) - val filename = "%s_%s_%s.%s".format( + val filename = "%s_%d_%s.%s".format( context.getString(R.string.app_name), - System.currentTimeMillis().toString(), + System.currentTimeMillis(), randomAlphanumericString(10), fileExtension, ) - // `stream` is closed in ProgressRequestBody.writeTo - @SuppressLint("recycle") - val stream = contentResolver.openInputStream(media.uri) - if (mimeType == null) mimeType = "multipart/form-data" var lastProgress = -1 - val fileBody = ProgressRequestBody( - stream!!, + val fileBody = media.uri.asRequestBody( + contentResolver, + requireNotNull(mimeType.toMediaTypeOrNull()) { "Invalid Content Type" }, media.mediaSize, - mimeType.toMediaTypeOrNull()!!, ) { percentage -> if (percentage != lastProgress) { trySend(UploadEvent.ProgressEvent(percentage)) diff --git a/app/src/main/java/app/pachli/components/drafts/DraftHelper.kt b/app/src/main/java/app/pachli/components/drafts/DraftHelper.kt index 126e720c2..d2b46ae3f 100644 --- a/app/src/main/java/app/pachli/components/drafts/DraftHelper.kt +++ b/app/src/main/java/app/pachli/components/drafts/DraftHelper.kt @@ -182,15 +182,10 @@ class DraftHelper @Inject constructor( // saving redrafted media try { val request = Request.Builder().url(toString()).build() - val response = okHttpClient.newCall(request).execute() - val sink = file.sink().buffer() - - response.body?.source()?.use { input -> - sink.use { output -> - output.writeAll(input) - } + response.body?.source()?.buffer?.use { input -> + file.sink().buffer().use { it.writeAll(input) } } } catch (ex: IOException) { Timber.w(ex, "failed to save media") diff --git a/app/src/main/java/app/pachli/network/ProgressRequestBody.kt b/app/src/main/java/app/pachli/network/ProgressRequestBody.kt deleted file mode 100644 index e42fd4e57..000000000 --- a/app/src/main/java/app/pachli/network/ProgressRequestBody.kt +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright 2023 Pachli Association - * - * This file is a part of Pachli. - * - * This program is free software; you can redistribute it and/or modify it under the terms of the - * GNU General Public License as published by the Free Software Foundation; either version 3 of the - * License, or (at your option) any later version. - * - * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even - * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General - * Public License for more details. - * - * You should have received a copy of the GNU General Public License along with Pachli; if not, - * see . - */ - -package app.pachli.network - -import java.io.IOException -import java.io.InputStream -import okhttp3.MediaType -import okhttp3.RequestBody -import okio.BufferedSink - -class ProgressRequestBody( - private val content: InputStream, - private val contentLength: Long, - private val mediaType: MediaType, - private val uploadListener: (Int) -> Unit, -) : RequestBody() { - override fun contentType(): MediaType { - return mediaType - } - - @Throws(IOException::class) - override fun writeTo(sink: BufferedSink) { - val buffer = ByteArray(DEFAULT_BUFFER_SIZE) - var uploaded: Long = 0 - content.use { content -> - var read: Int - while (content.read(buffer).also { read = it } != -1) { - uploadListener((100 * uploaded / contentLength).toInt()) - uploaded += read.toLong() - sink.write(buffer, 0, read) - } - uploadListener((100 * uploaded / contentLength).toInt()) - } - } - - companion object { - private const val DEFAULT_BUFFER_SIZE = 2048 - } -} diff --git a/app/src/main/java/app/pachli/util/CompositeWithOpaqueBackground.kt b/app/src/main/java/app/pachli/util/CompositeWithOpaqueBackground.kt index 8f5dbc33b..5c13b5b6e 100644 --- a/app/src/main/java/app/pachli/util/CompositeWithOpaqueBackground.kt +++ b/app/src/main/java/app/pachli/util/CompositeWithOpaqueBackground.kt @@ -25,12 +25,12 @@ import android.graphics.ColorMatrixColorFilter import android.graphics.Paint import android.graphics.Shader import androidx.annotation.ColorInt +import com.bumptech.glide.load.Key import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool import com.bumptech.glide.load.resource.bitmap.BitmapTransformation -import com.bumptech.glide.util.Util import java.nio.ByteBuffer -import java.nio.charset.Charset import java.security.MessageDigest +import java.util.Objects /** * Set an opaque background behind the non-transparent areas of a bitmap. @@ -58,10 +58,11 @@ class CompositeWithOpaqueBackground(@ColorInt val backgroundColor: Int) : Bitmap return false } - override fun hashCode() = Util.hashCode(ID.hashCode(), backgroundColor.hashCode()) + override fun hashCode() = Objects.hash(ID, backgroundColor) + override fun updateDiskCacheKey(messageDigest: MessageDigest) { messageDigest.update(ID_BYTES) - messageDigest.update(ByteBuffer.allocate(4).putInt(backgroundColor.hashCode()).array()) + messageDigest.update(ByteBuffer.allocate(Int.SIZE_BYTES).putInt(backgroundColor).array()) } override fun transform( @@ -109,7 +110,7 @@ class CompositeWithOpaqueBackground(@ColorInt val backgroundColor: Int) : Bitmap companion object { private val ID = CompositeWithOpaqueBackground::class.qualifiedName!! - private val ID_BYTES = ID.toByteArray(Charset.forName("UTF-8")) + private val ID_BYTES = ID.toByteArray(Key.CHARSET) /** Paint with a color filter that converts 8bpp alpha images to a 1bpp mask */ private val EXTRACT_MASK_PAINT = Paint().apply { diff --git a/app/src/main/java/app/pachli/util/IOUtils.kt b/app/src/main/java/app/pachli/util/IOUtils.kt deleted file mode 100644 index ac4fc48c2..000000000 --- a/app/src/main/java/app/pachli/util/IOUtils.kt +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright 2023 Pachli Association - * - * This file is a part of Pachli. - * - * This program is free software; you can redistribute it and/or modify it under the terms of the - * GNU General Public License as published by the Free Software Foundation; either version 3 of the - * License, or (at your option) any later version. - * - * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even - * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General - * Public License for more details. - * - * You should have received a copy of the GNU General Public License along with Pachli; if not, - * see . - */ - -package app.pachli.util - -import android.content.ContentResolver -import android.net.Uri -import java.io.File -import java.io.FileNotFoundException -import java.io.FileOutputStream - -private const val DEFAULT_BLOCKSIZE = 16384 - -fun Uri.copyToFile( - contentResolver: ContentResolver, - file: File, -): Boolean { - try { - contentResolver.openInputStream(this).use { from -> - from ?: return false - - FileOutputStream(file).use { to -> - val chunk = ByteArray(DEFAULT_BLOCKSIZE) - while (true) { - val bytes = from.read(chunk, 0, chunk.size) - if (bytes < 0) break - to.write(chunk, 0, bytes) - } - } - } - } catch (e: FileNotFoundException) { - return false - } - - return true -} diff --git a/app/src/main/java/app/pachli/util/MediaUtils.kt b/app/src/main/java/app/pachli/util/MediaUtils.kt index 928ac34e2..6a8e5321e 100644 --- a/app/src/main/java/app/pachli/util/MediaUtils.kt +++ b/app/src/main/java/app/pachli/util/MediaUtils.kt @@ -25,11 +25,9 @@ import android.net.Uri import android.provider.OpenableColumns import androidx.exifinterface.media.ExifInterface import java.io.FileNotFoundException -import java.io.IOException import java.text.SimpleDateFormat import java.util.Date import java.util.Locale -import timber.log.Timber /** * Helper methods for obtaining and resizing media files @@ -45,9 +43,7 @@ const val MEDIA_SIZE_UNKNOWN = -1L * @return the size of the media in bytes or {@link MediaUtils#MEDIA_SIZE_UNKNOWN} */ fun getMediaSize(contentResolver: ContentResolver, uri: Uri?): Long { - if (uri == null) { - return MEDIA_SIZE_UNKNOWN - } + uri ?: return MEDIA_SIZE_UNKNOWN var mediaSize = MEDIA_SIZE_UNKNOWN val cursor: Cursor? @@ -66,14 +62,14 @@ fun getMediaSize(contentResolver: ContentResolver, uri: Uri?): Long { } @Throws(FileNotFoundException::class) -fun getImageSquarePixels(contentResolver: ContentResolver, uri: Uri): Long { +fun getImageSquarePixels(contentResolver: ContentResolver, uri: Uri): Int { val options = BitmapFactory.Options() contentResolver.openInputStream(uri).use { input -> options.inJustDecodeBounds = true BitmapFactory.decodeStream(input, null, options) } - return (options.outWidth * options.outHeight).toLong() + return options.outWidth * options.outHeight } fun calculateInSampleSize(options: BitmapFactory.Options, reqWidth: Int, reqHeight: Int): Int { @@ -119,9 +115,7 @@ fun reorientBitmap(bitmap: Bitmap?, orientation: Int): Bitmap? { else -> return bitmap } - if (bitmap == null) { - return null - } + bitmap ?: return null return try { val result = Bitmap.createBitmap( @@ -142,27 +136,6 @@ fun reorientBitmap(bitmap: Bitmap?, orientation: Int): Bitmap? { } } -fun getImageOrientation(uri: Uri, contentResolver: ContentResolver): Int { - val inputStream = try { - contentResolver.openInputStream(uri) - } catch (e: FileNotFoundException) { - Timber.w(e) - return ExifInterface.ORIENTATION_UNDEFINED - } - inputStream ?: return ExifInterface.ORIENTATION_UNDEFINED - - return inputStream.use { - val exifInterface = try { - ExifInterface(it) - } catch (e: IOException) { - Timber.w(e) - return@use ExifInterface.ORIENTATION_UNDEFINED - } - - exifInterface.getAttributeInt(ExifInterface.TAG_ORIENTATION, ExifInterface.ORIENTATION_NORMAL) - } -} - fun getTemporaryMediaFilename(extension: String): String { return "${MEDIA_TEMP_PREFIX}_${SimpleDateFormat("yyyyMMdd_HHmmss", Locale.US).format(Date())}.$extension" } diff --git a/app/src/main/java/app/pachli/util/UriExtensions.kt b/app/src/main/java/app/pachli/util/UriExtensions.kt new file mode 100644 index 000000000..dd6d5efde --- /dev/null +++ b/app/src/main/java/app/pachli/util/UriExtensions.kt @@ -0,0 +1,83 @@ +/* + * Copyright 2023 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.util + +import android.content.ContentResolver +import android.net.Uri +import java.io.File +import java.io.IOException +import okhttp3.MediaType +import okhttp3.RequestBody +import okio.Buffer +import okio.BufferedSink +import okio.FileNotFoundException +import okio.buffer +import okio.sink +import okio.source + +fun Uri.copyToFile(contentResolver: ContentResolver, file: File): Boolean { + return try { + val inputStream = contentResolver.openInputStream(this) ?: return false + inputStream.source().buffer().use { source -> + file.sink().buffer().use { it.writeAll(source) } + } + true + } catch (e: IOException) { + false + } +} + +// Aligned with okio.Segment.SIZE (internal) for better performance +private const val DEFAULT_CHUNK_SIZE = 8192L + +fun interface UploadCallback { + fun onProgressUpdate(percentage: Int) +} + +fun Uri.asRequestBody( + contentResolver: ContentResolver, + contentType: MediaType? = null, + contentLength: Long = -1L, + uploadListener: UploadCallback? = null, +): RequestBody { + return object : RequestBody() { + override fun contentType(): MediaType? = contentType + + override fun contentLength(): Long = contentLength + + override fun writeTo(sink: BufferedSink) { + val buffer = Buffer() + var uploaded: Long = 0 + val inputStream = contentResolver.openInputStream(this@asRequestBody) + ?: throw FileNotFoundException("Unavailable ContentProvider") + + inputStream.source().use { source -> + while (true) { + val read = source.read(buffer, DEFAULT_CHUNK_SIZE) + if (read == -1L) { + break + } + sink.write(buffer, read) + uploaded += read + uploadListener?.let { if (contentLength > 0L) it.onProgressUpdate((100L * uploaded / contentLength).toInt()) } + } + uploadListener?.onProgressUpdate(100) + } + } + } +} diff --git a/core/designsystem/lint-baseline.xml b/core/designsystem/lint-baseline.xml index d61c63df9..7fb106162 100644 --- a/core/designsystem/lint-baseline.xml +++ b/core/designsystem/lint-baseline.xml @@ -8,7 +8,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"> @@ -19,7 +19,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"> diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index c2edda8a7..8ff7ef49a 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -61,6 +61,7 @@ moshi = "1.15.1" moshix = "0.27.1" networkresult-calladapter = "1.0.0" okhttp = "4.12.0" +okio = "3.9.0" quadrant = "1.9.1" retrofit = "2.11.0" robolectric = "4.12.2" @@ -203,6 +204,7 @@ networkresult-calladapter = { module = "at.connyduck:networkresult-calladapter", okhttp-core = { module = "com.squareup.okhttp3:okhttp", version.ref = "okhttp" } okhttp-logging-interceptor = { module = "com.squareup.okhttp3:logging-interceptor", version.ref = "okhttp" } okhttp-tls = { module = "com.squareup.okhttp3:okhttp-tls", version.ref = "okhttp" } +okio = { module = "com.squareup.okio:okio", version.ref = "okio" } retrofit-converter-moshi = { module = "com.squareup.retrofit2:converter-moshi", version.ref = "retrofit" } retrofit-core = { module = "com.squareup.retrofit2:retrofit", version.ref = "retrofit" } robolectric = { module = "org.robolectric:robolectric", version.ref = "robolectric" }