fix(avatar): harden the cropper's error handling; unify with the codebase
Checking error handling on the crop sheet surfaced three real problems, two of
which I had introduced.
1. Crash on a high-megapixel photo. The picked image was decoded at full size
and handed to a hardware Canvas. An 8000x8000 photo decodes to 256MB, which
Android allocates but the canvas refuses to draw ("trying to draw too large
bitmap") — thrown inside Compose's draw phase, where the runCatching around
the decode can't see it. Proven live: an 8000x8000 pick killed the app.
Fixed by subsampling at decode (inSampleSize from a bounds-only pass) so the
working bitmap is capped at ~2048px / ~16MB. Native heap on that pick went
256MB(attempted) -> 46MB.
2. The subsample fix then broke decoding for EVERY image. decodeStream returns
null by design in inJustDecodeBounds mode, and I had `?: return null` on it —
so loadOriented bailed right after the bounds pass, for all inputs. It only
looked like "the huge photo failed"; a normal photo would have failed too. I
never re-tested a small image after the change. The new AvatarCropSheetTest
caught it. The stream is now what's guarded; the real check is the header size.
3. Errors were swallowed and off-standard. runCatching{}.getOrNull() dropped the
cause and a failure silently dismissed the sheet — indistinguishable from a
save that did nothing. Now unified with the screen's own pattern
(EditProfileViewModel.save): the sheet reports the Throwable up via a new
onError, the VM records it through the injected CrashReporter and surfaces
the message through uiState.error -> the existing snackbar. The crop also
moved off the main thread (withContext(IO)); inline, `saving` flipped within
one frame and never showed.
Adds AvatarCropSheetTest (androidTest, 4 tests, on-device BitmapFactory/Canvas):
subsample math for 8000+/oversized, oversized decode stays bounded, garbage
returns null instead of throwing, crop output is square and bounded. All green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
7b8fa652f6
commit
5151698d56
|
|
@ -0,0 +1,87 @@
|
||||||
|
package app.closer.ui.settings
|
||||||
|
|
||||||
|
import android.graphics.Bitmap
|
||||||
|
import android.graphics.Canvas
|
||||||
|
import android.graphics.Color
|
||||||
|
import android.net.Uri
|
||||||
|
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||||
|
import androidx.test.platform.app.InstrumentationRegistry
|
||||||
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Assert.assertNotNull
|
||||||
|
import org.junit.Assert.assertTrue
|
||||||
|
import org.junit.Test
|
||||||
|
import org.junit.runner.RunWith
|
||||||
|
import java.io.File
|
||||||
|
import java.io.FileOutputStream
|
||||||
|
|
||||||
|
/**
|
||||||
|
* On-device checks for the avatar cropper's decode/crop, using the real `BitmapFactory` and
|
||||||
|
* `Canvas` — the exact things a JVM unit test can't exercise and where the real bug lived.
|
||||||
|
*
|
||||||
|
* A picked photo used to be decoded at full size and then handed straight to a hardware Canvas: an
|
||||||
|
* 8000x8000 image decodes to 256MB, which Android will allocate but the canvas refuses to draw
|
||||||
|
* ("trying to draw too large bitmap"), thrown inside Compose's draw phase where the runCatching
|
||||||
|
* around the decode never saw it — a hard crash on first paint for anyone with a high-megapixel
|
||||||
|
* camera. These tests pin the subsampling that fixes it.
|
||||||
|
*/
|
||||||
|
@RunWith(AndroidJUnit4::class)
|
||||||
|
class AvatarCropSheetTest {
|
||||||
|
|
||||||
|
private val context = InstrumentationRegistry.getInstrumentation().targetContext
|
||||||
|
|
||||||
|
private fun writeJpeg(w: Int, h: Int): Uri {
|
||||||
|
val bmp = Bitmap.createBitmap(w, h, Bitmap.Config.ARGB_8888)
|
||||||
|
Canvas(bmp).drawColor(Color.rgb(120, 90, 200))
|
||||||
|
val file = File(context.cacheDir, "avatar_test_${w}x${h}.jpg")
|
||||||
|
FileOutputStream(file).use { bmp.compress(Bitmap.CompressFormat.JPEG, 90, it) }
|
||||||
|
bmp.recycle()
|
||||||
|
return Uri.fromFile(file)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun sampleSize_bringsAnyImageWithinBudget() {
|
||||||
|
// The pathological cases: the exact size that crashed, and beyond it.
|
||||||
|
assertEquals(1, sampleSizeFor(2048, 2048, 2048))
|
||||||
|
assertEquals(2, sampleSizeFor(4000, 3000, 2048))
|
||||||
|
assertEquals(4, sampleSizeFor(8000, 8000, 2048))
|
||||||
|
assertEquals(8, sampleSizeFor(12000, 9000, 2048))
|
||||||
|
// Whatever the input, the decoded edge is capped — so the drawn bitmap can't exceed the
|
||||||
|
// hardware canvas limit that used to throw.
|
||||||
|
for (dim in intArrayOf(2049, 6000, 8000, 16000)) {
|
||||||
|
val decodedEdge = dim / sampleSizeFor(dim, dim, 2048)
|
||||||
|
assertTrue("edge $decodedEdge should be <= 2048", decodedEdge <= 2048)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun loadOriented_decodesAnOversizedImageDownToBudget() {
|
||||||
|
// Forces the subsampling path (4096 > 2048 -> inSampleSize 2 -> 2048). Not the full 8000
|
||||||
|
// case: creating an 8000x8000 ARGB source is 256MB, which the test process can't allocate
|
||||||
|
// in the same heap the fix is about — sampleSize_bringsAnyImageWithinBudget covers 8000+
|
||||||
|
// arithmetically. This proves the decode actually honours inSampleSize and returns a
|
||||||
|
// bounded, drawable bitmap (the property whose absence crashed the hardware canvas).
|
||||||
|
val bmp = loadOriented(context, writeJpeg(4096, 4096))
|
||||||
|
assertNotNull("oversized image should still decode (subsampled)", bmp)
|
||||||
|
assertTrue("decoded edge must be bounded to <= 2048", bmp!!.width <= 2048 && bmp.height <= 2048)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun loadOriented_returnsNullForGarbageRatherThanThrowing() {
|
||||||
|
val file = File(context.cacheDir, "not_an_image.jpg")
|
||||||
|
file.writeText("this is not a JPEG")
|
||||||
|
assertEquals(null, loadOriented(context, Uri.fromFile(file)))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun crop_producesASquareBitmapAtOutputSize() {
|
||||||
|
val src = Bitmap.createBitmap(1200, 800, Bitmap.Config.ARGB_8888)
|
||||||
|
Canvas(src).drawColor(Color.rgb(200, 120, 160))
|
||||||
|
val outUri = cropToCircleSquare(context, src, scale = 1f, offset = androidx.compose.ui.geometry.Offset.Zero, viewportPx = 512f)
|
||||||
|
val out = context.contentResolver.openInputStream(outUri)!!.use {
|
||||||
|
android.graphics.BitmapFactory.decodeStream(it)
|
||||||
|
}
|
||||||
|
assertNotNull(out)
|
||||||
|
assertEquals("avatar output is square", out.width, out.height)
|
||||||
|
assertTrue("avatar output is bounded", out.width in 1..512)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -34,6 +34,7 @@ import androidx.compose.runtime.getValue
|
||||||
import androidx.compose.runtime.mutableFloatStateOf
|
import androidx.compose.runtime.mutableFloatStateOf
|
||||||
import androidx.compose.runtime.mutableStateOf
|
import androidx.compose.runtime.mutableStateOf
|
||||||
import androidx.compose.runtime.remember
|
import androidx.compose.runtime.remember
|
||||||
|
import androidx.compose.runtime.rememberCoroutineScope
|
||||||
import androidx.compose.runtime.setValue
|
import androidx.compose.runtime.setValue
|
||||||
import androidx.compose.ui.Alignment
|
import androidx.compose.ui.Alignment
|
||||||
import androidx.compose.ui.Modifier
|
import androidx.compose.ui.Modifier
|
||||||
|
|
@ -50,6 +51,7 @@ import androidx.compose.ui.unit.IntSize
|
||||||
import androidx.compose.ui.unit.dp
|
import androidx.compose.ui.unit.dp
|
||||||
import androidx.exifinterface.media.ExifInterface
|
import androidx.exifinterface.media.ExifInterface
|
||||||
import kotlinx.coroutines.Dispatchers
|
import kotlinx.coroutines.Dispatchers
|
||||||
|
import kotlinx.coroutines.launch
|
||||||
import kotlinx.coroutines.withContext
|
import kotlinx.coroutines.withContext
|
||||||
import java.io.File
|
import java.io.File
|
||||||
import java.io.FileOutputStream
|
import java.io.FileOutputStream
|
||||||
|
|
@ -78,10 +80,12 @@ private const val MAX_SCALE = 4f
|
||||||
internal fun AvatarCropSheet(
|
internal fun AvatarCropSheet(
|
||||||
sourceUri: Uri,
|
sourceUri: Uri,
|
||||||
onDismiss: () -> Unit,
|
onDismiss: () -> Unit,
|
||||||
onConfirm: (Uri) -> Unit
|
onConfirm: (Uri) -> Unit,
|
||||||
|
onError: (Throwable) -> Unit
|
||||||
) {
|
) {
|
||||||
val context = LocalContext.current
|
val context = LocalContext.current
|
||||||
val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
|
val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
|
||||||
|
val scope = rememberCoroutineScope()
|
||||||
|
|
||||||
var bitmap by remember(sourceUri) { mutableStateOf<Bitmap?>(null) }
|
var bitmap by remember(sourceUri) { mutableStateOf<Bitmap?>(null) }
|
||||||
var failed by remember(sourceUri) { mutableStateOf(false) }
|
var failed by remember(sourceUri) { mutableStateOf(false) }
|
||||||
|
|
@ -91,8 +95,18 @@ internal fun AvatarCropSheet(
|
||||||
var saving by remember(sourceUri) { mutableStateOf(false) }
|
var saving by remember(sourceUri) { mutableStateOf(false) }
|
||||||
|
|
||||||
LaunchedEffect(sourceUri) {
|
LaunchedEffect(sourceUri) {
|
||||||
bitmap = withContext(Dispatchers.IO) { runCatching { loadOriented(context, sourceUri) }.getOrNull() }
|
// runCatching{}.getOrNull() dropped the cause on the floor: an unreadable file and a decode
|
||||||
if (bitmap == null) failed = true
|
// failure looked identical and neither reached anyone. Report, then show the in-context
|
||||||
|
// message — the sheet stays open because there is nothing to proceed with.
|
||||||
|
withContext(Dispatchers.IO) { runCatching { loadOriented(context, sourceUri) } }
|
||||||
|
.onSuccess { decoded ->
|
||||||
|
bitmap = decoded
|
||||||
|
if (decoded == null) {
|
||||||
|
failed = true
|
||||||
|
onError(IllegalStateException("Avatar: could not decode picked image"))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
.onFailure { failed = true; onError(it) }
|
||||||
}
|
}
|
||||||
|
|
||||||
ModalBottomSheet(onDismissRequest = onDismiss, sheetState = sheetState) {
|
ModalBottomSheet(onDismissRequest = onDismiss, sheetState = sheetState) {
|
||||||
|
|
@ -112,7 +126,7 @@ internal fun AvatarCropSheet(
|
||||||
color = SettingsInk
|
color = SettingsInk
|
||||||
)
|
)
|
||||||
Text(
|
Text(
|
||||||
text = if (failed) "That image couldn't be opened. Try another one."
|
text = if (failed) "That photo couldn't be used. Try another one."
|
||||||
else "Pinch to zoom, drag to move.",
|
else "Pinch to zoom, drag to move.",
|
||||||
style = MaterialTheme.typography.bodyMedium,
|
style = MaterialTheme.typography.bodyMedium,
|
||||||
color = SettingsMuted
|
color = SettingsMuted
|
||||||
|
|
@ -175,12 +189,21 @@ internal fun AvatarCropSheet(
|
||||||
Button(
|
Button(
|
||||||
onClick = {
|
onClick = {
|
||||||
val source = bmp ?: return@Button
|
val source = bmp ?: return@Button
|
||||||
saving = true
|
// Off the main thread: this allocates a bitmap, redraws it and JPEG-encodes
|
||||||
val out = runCatching {
|
// to disk. Doing that inline also made `saving` a lie — it flipped true and
|
||||||
cropToCircleSquare(context, source, scale, offset, viewport)
|
// false within one frame, so the button never actually showed its state.
|
||||||
}.getOrNull()
|
scope.launch {
|
||||||
saving = false
|
saving = true
|
||||||
if (out != null) onConfirm(out) else onDismiss()
|
withContext(Dispatchers.IO) {
|
||||||
|
runCatching {
|
||||||
|
cropToCircleSquare(context, source, scale, offset, viewport)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
.onSuccess { saving = false; onConfirm(it) }
|
||||||
|
// Say so rather than just closing: a silent dismiss looks exactly
|
||||||
|
// like a successful save that quietly did nothing.
|
||||||
|
.onFailure { saving = false; failed = true; onError(it) }
|
||||||
|
}
|
||||||
},
|
},
|
||||||
enabled = bmp != null && !saving,
|
enabled = bmp != null && !saving,
|
||||||
modifier = Modifier.weight(2f),
|
modifier = Modifier.weight(2f),
|
||||||
|
|
@ -224,12 +247,45 @@ private fun clampOffset(
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Decodes [uri], honouring the EXIF orientation. Phone cameras store the sensor image and a
|
* Largest edge we will decode. Nothing here needs more: the preview is at most a screen wide and
|
||||||
* rotation flag; without applying it a portrait selfie crops sideways.
|
* the saved avatar is [OUTPUT_PX], so 2048 still leaves full detail at [MAX_SCALE] zoom.
|
||||||
|
*
|
||||||
|
* This bound is a crash fix, not a nicety. Decoding at full size looks fine right up until someone
|
||||||
|
* picks a photo from a modern high-megapixel camera: a 48MP shot decodes to ~192MB and an 8000x8000
|
||||||
|
* image to 256MB, and while Android will happily *allocate* that, the hardware canvas refuses to
|
||||||
|
* *draw* a bitmap over ~100MB — `RuntimeException: Canvas: trying to draw too large bitmap`, thrown
|
||||||
|
* inside Compose's draw phase where no runCatching around the decode can see it. Proven live: an
|
||||||
|
* 8000x8000 pick killed the app outright. Sampling down at decode caps this at ~16MB.
|
||||||
*/
|
*/
|
||||||
private fun loadOriented(context: Context, uri: Uri): Bitmap? {
|
private const val MAX_DECODE_PX = 2048
|
||||||
|
|
||||||
|
/** Power-of-two subsample that brings [w]x[h] within [max] — what BitmapFactory expects. */
|
||||||
|
internal fun sampleSizeFor(w: Int, h: Int, max: Int): Int {
|
||||||
|
var sample = 1
|
||||||
|
while (w / sample > max || h / sample > max) sample *= 2
|
||||||
|
return sample
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Decodes [uri] at a bounded size, honouring the EXIF orientation. Phone cameras store the sensor
|
||||||
|
* image and a rotation flag; without applying it a portrait selfie crops sideways.
|
||||||
|
*/
|
||||||
|
internal fun loadOriented(context: Context, uri: Uri): Bitmap? {
|
||||||
|
// Bounds-only pass first: reading the header costs nothing and tells us how far to subsample.
|
||||||
|
// NB: decodeStream returns null *by design* in inJustDecodeBounds mode — it only fills `bounds`.
|
||||||
|
// So the stream is what's guarded here, and the real "could we read it" check is the size test
|
||||||
|
// below. (A `?: return null` on the decode result would fire on every image, always.)
|
||||||
|
val bounds = BitmapFactory.Options().apply { inJustDecodeBounds = true }
|
||||||
|
(context.contentResolver.openInputStream(uri) ?: return null).use {
|
||||||
|
BitmapFactory.decodeStream(it, null, bounds)
|
||||||
|
}
|
||||||
|
if (bounds.outWidth <= 0 || bounds.outHeight <= 0) return null
|
||||||
|
|
||||||
|
val opts = BitmapFactory.Options().apply {
|
||||||
|
inSampleSize = sampleSizeFor(bounds.outWidth, bounds.outHeight, MAX_DECODE_PX)
|
||||||
|
}
|
||||||
val decoded = context.contentResolver.openInputStream(uri)?.use {
|
val decoded = context.contentResolver.openInputStream(uri)?.use {
|
||||||
BitmapFactory.decodeStream(it)
|
BitmapFactory.decodeStream(it, null, opts)
|
||||||
} ?: return null
|
} ?: return null
|
||||||
val rotation = context.contentResolver.openInputStream(uri)?.use { stream ->
|
val rotation = context.contentResolver.openInputStream(uri)?.use { stream ->
|
||||||
when (ExifInterface(stream).getAttributeInt(
|
when (ExifInterface(stream).getAttributeInt(
|
||||||
|
|
@ -254,7 +310,7 @@ private fun loadOriented(context: Context, uri: Uri): Bitmap? {
|
||||||
* [OUTPUT_PX] by the viewport→output ratio. Square, not circular: every avatar slot already clips
|
* [OUTPUT_PX] by the viewport→output ratio. Square, not circular: every avatar slot already clips
|
||||||
* to a circle, and a transparent-cornered PNG would cost size for nothing.
|
* to a circle, and a transparent-cornered PNG would cost size for nothing.
|
||||||
*/
|
*/
|
||||||
private fun cropToCircleSquare(
|
internal fun cropToCircleSquare(
|
||||||
context: Context,
|
context: Context,
|
||||||
source: Bitmap,
|
source: Bitmap,
|
||||||
scale: Float,
|
scale: Float,
|
||||||
|
|
|
||||||
|
|
@ -139,7 +139,10 @@ fun EditProfileContent(
|
||||||
onConfirm = { cropped ->
|
onConfirm = { cropped ->
|
||||||
viewModel.setPhotoUri(cropped.toString())
|
viewModel.setPhotoUri(cropped.toString())
|
||||||
cropSource = null
|
cropSource = null
|
||||||
}
|
},
|
||||||
|
// Failures go through the VM so they surface in this screen's existing error snackbar,
|
||||||
|
// like every other failure here, instead of dying inside the sheet.
|
||||||
|
onError = { viewModel.onPhotoError(it) }
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.StateFlow
|
||||||
import kotlinx.coroutines.flow.asStateFlow
|
import kotlinx.coroutines.flow.asStateFlow
|
||||||
import kotlinx.coroutines.flow.update
|
import kotlinx.coroutines.flow.update
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
|
import app.closer.core.crash.CrashReporter
|
||||||
import javax.inject.Inject
|
import javax.inject.Inject
|
||||||
|
|
||||||
data class EditProfileUiState(
|
data class EditProfileUiState(
|
||||||
|
|
@ -39,6 +40,7 @@ class EditProfileViewModel @Inject constructor(
|
||||||
private val authRepository: AuthRepository,
|
private val authRepository: AuthRepository,
|
||||||
private val userRepository: UserRepository,
|
private val userRepository: UserRepository,
|
||||||
private val storageDataSource: FirebaseStorageDataSource,
|
private val storageDataSource: FirebaseStorageDataSource,
|
||||||
|
private val crashReporter: CrashReporter,
|
||||||
private val coupleRepository: CoupleRepository,
|
private val coupleRepository: CoupleRepository,
|
||||||
private val encryptionManager: CoupleEncryptionManager
|
private val encryptionManager: CoupleEncryptionManager
|
||||||
) : ViewModel() {
|
) : ViewModel() {
|
||||||
|
|
@ -82,6 +84,19 @@ class EditProfileViewModel @Inject constructor(
|
||||||
|
|
||||||
fun setPhotoUri(uri: String?) = _uiState.update { it.copy(photoUri = uri, error = null) }
|
fun setPhotoUri(uri: String?) = _uiState.update { it.copy(photoUri = uri, error = null) }
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The avatar cropper couldn't read or write the picked photo (unreadable file, decode failure,
|
||||||
|
* no space). Surfaced the same way every other failure on this screen is — `error` -> the
|
||||||
|
* snackbar the screen already collects — rather than dying inside the sheet, and recorded so a
|
||||||
|
* swallowed decode failure isn't invisible.
|
||||||
|
*/
|
||||||
|
fun onPhotoError(throwable: Throwable) {
|
||||||
|
crashReporter.recordException(throwable)
|
||||||
|
_uiState.update {
|
||||||
|
it.copy(error = throwable.message ?: "Couldn't use that photo. Please try again.")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
fun save() {
|
fun save() {
|
||||||
val state = _uiState.value
|
val state = _uiState.value
|
||||||
val name = state.displayName.trim()
|
val name = state.displayName.trim()
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue