diff --git a/apps/multiplatform/DESKTOP_ANIMATED_IMAGES_PLAN.md b/apps/multiplatform/DESKTOP_ANIMATED_IMAGES_PLAN.md new file mode 100644 index 0000000000..4d5bc1017f --- /dev/null +++ b/apps/multiplatform/DESKTOP_ANIMATED_IMAGES_PLAN.md @@ -0,0 +1,82 @@ +# Animated images on desktop + +## The problem + +`SimpleAndAnimatedImageView` on desktop drew a single `BitmapPainter` and carried the marker +`// LALAL make it animated too`. Android decodes animations with coil, iOS with SwiftyGif, and desktop showed +the first frame and stopped. `ImageFullScreenView.kt` carried a matching marker over the image branch. + +## Why this shape + +**Skia's `Codec`, which skiko already puts on the desktop classpath.** No new dependency. It decodes both GIF +and animated WebP, reports per-frame durations and repeat counts, and supports random access into frames. + +**Not `components-animatedimage`** (already declared in `common/build.gradle.kts`, unused). Its `animate()` +ignores the result of `allocPixels` and decodes inside composition. A 35-byte GIF declaring 65535x65535 asks +for a 17GB raster; `allocPixels` returns false, and the following `readPixels` throws +`IllegalArgumentException` from inside the composition — a remote crash from anyone who can send a file. It +also decodes on the UI thread, measured at ~11ms per frame for a 1244x554 animation. + +## Bounds + +Everything below is decoded from bytes somebody else composed, so each bound answers a specific crafted +input, and anything outside them keeps showing the still image the chat already renders. Animation degrades +to a picture, never to an error, and failures are never alerted — an alert per malformed file would itself +let a sender disrupt the app. + +| Bound | What it answers | +| --- | --- | +| raster measured in bytes, sides multiplied as `Long` | `65535 * 65535` overflows `Int` to a negative number and would pass a naive budget check | +| per-side cap, independent of the raster bound | 65535x32 is only 2.1MP and would otherwise animate with a 65535-pixel scanline | +| bytes per pixel read from the codec | the file chooses its colour type; the budget must not assume four bytes | +| file size checked before the bytes are copied natively | Skia copies the encoded bytes and scans them to count frames | +| magic-byte prefilter (`GIF8`, `RIFF....WEBP`) | photos are most of what a chat holds and none are animations; they never reach a second decoder | +| `allocPixels` result honoured | it reports failure by returning false, and reading into an unallocated bitmap throws | +| frame duration floor, and 100ms substituted for "no delay" | a 4.6MB GIF can hold 200 000 zero-delay frames | +| single exception boundary around every native call | the frame count and repeat count are read from the file too | + +Long frame delays are honoured rather than clamped — they are the author's, and they cost nothing. + +## Cost, and the optimisations that were rejected + +Measured on a 1244x554 GIF and its WebP equivalent: + +| | work/frame | CPU while playing | +| --- | --- | --- | +| typical GIF | 2.80 ms | 1.7% of one core | +| animated WebP | 4.19 ms | 2.5% of one core | +| 2000x891 GIF | 180.6 ms | 100% of one core | + +Typical animations are close to free; the whole cost problem is the pathological tail. So an animation whose +frames take more than 100ms to decode, twice in a row, stops and keeps the still. Two in a row because this +is wall time: a single frame can overrun by being descheduled, and a busy machine should not turn a cheap +animation into a still. + +Two optimisations were measured and **rejected**: + +- **Decoding at display size.** Scaled decode is supported at arbitrary sizes, but it costs CPU rather than + saving it: 2000x891 goes from 177.8ms to 300.3ms per frame (+68%) to save 59% of the raster — and it only + engages on the files that are already the most expensive. +- **Half-depth pixels.** Skia refuses `RGB_565` and `ARGB_4444` for GIF outright. It works only for opaque + WebP, at +11% decode for -50% raster, which does not justify a format-specific path. + +What was kept: decoding is confined to two threads of the shared pool, so untrusted decode work cannot starve +the coroutines that deliver messages; and frames are only decoded while they can be seen — not while the app +sits in the tray, and not while the image is behind the privacy blur, where each frame would otherwise be +decoded, uploaded and then blurred away again for nobody. + +## Verification + +- 20 000 fuzzed mutations (bit flips, truncations, header corruption) over a real corpus plus crafted hostile + files: no exception escapes the structure, no hangs. +- Frames advance, per-frame delays are read correctly, and the loop wraps back to frame 0 after a full cycle + with byte-identical pixels. +- An oversized animation is refused by the bounds and still renders through the existing still-image path. +- Unit tests cover the bounds as arithmetic; skiko's native library is not on the test runtime classpath. + +## Deliberately not in this change + +- **WebP still images do not decode on desktop at all.** `getLoadedImage` uses ImageIO, which has no WebP + reader, so a received `.webp` never reaches this code and picking one to send is dropped. Separate fix. +- **The decode raster is left to the collector.** Releasing it explicitly needs to know which thread Compose + Desktop draws on, and skiko uses a different redrawer per platform; guessing risks a use-after-free. diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/AnimatedImage.desktop.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/AnimatedImage.desktop.kt new file mode 100644 index 0000000000..c9c0f69e1a --- /dev/null +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/AnimatedImage.desktop.kt @@ -0,0 +1,184 @@ +package chat.simplex.common.platform + +import androidx.compose.runtime.* +import androidx.compose.ui.graphics.ImageBitmap +import androidx.compose.ui.graphics.asComposeImageBitmap +import chat.simplex.common.simplexWindowState +import kotlinx.coroutines.* +import kotlinx.coroutines.flow.first +import org.jetbrains.skia.AnimationFrameInfo +import org.jetbrains.skia.Bitmap +import org.jetbrains.skia.Codec +import org.jetbrains.skia.Data + +// The bytes decoded here come from whoever sent the file, so each bound below answers a specific crafted +// input rather than estimating what is reasonable. Whatever falls outside the bounds keeps showing the still +// image the chat already renders: animation degrades to a picture, never to an error. Failures are logged and +// never alerted - an alert per malformed file would itself let a sender disrupt the app. + +// Largest animation we hold frames for: one reused raster of 1920x1920 at four bytes a pixel is ~15MB, and +// several can be on screen at once. A crafted file can ask for far more - a 35-byte GIF declaring +// 65535x65535 asks for 17GB - and it also chooses the colour type, so this is measured in bytes rather than +// assuming four of them per pixel. +private const val MAX_ANIMATED_RASTER_BYTES: Long = 1920L * 1920 * 4 +// Neither side may exceed this, independently of the raster bound: 65535x32 is only 2.1MP and would otherwise +// be animated with a 65535-pixel scanline. Wide enough to leave banner-shaped animations playing. +private const val MAX_ANIMATED_SIDE = 4096 +// Skia copies the encoded bytes into native memory and scans them to count frames, so the file size bounds +// both. Comfortably above real animations - the largest in this repository is 1.5MB. +private const val MAX_ANIMATED_FILE_SIZE = 32 * 1024 * 1024 +// A frame may declare no delay at all: a 4.6MB GIF can hold 200 000 zero-delay frames, which would decode +// flat out for as long as it stayed on screen. Browsers substitute 100ms for an unspecified delay, and the +// floor bounds the rate for the rest. Long delays are left alone - they are the author's, and they cost +// nothing to honour. +private const val DEFAULT_FRAME_DURATION_MS = 100L +private const val MIN_FRAME_DURATION_MS = 20L +// An animation whose frames cost more than this to decode is left as a still. A 1244x554 animation decodes a +// frame in under 3ms and spends under 2% of a core playing; a 2000x891 one takes 180ms a frame, which is a +// whole core held to show about five frames a second. The still is the better picture and costs nothing. +private const val MAX_FRAME_DECODE_MS = 100L + +/** + * The frame of [data] to draw right now, or [still] when [data] is not an animation, falls outside the bounds + * above, or fails to decode. Decoding runs off the UI thread; the animation stops when the caller leaves. + */ +@Composable +fun rememberAnimatedImage(data: ByteArray, still: ImageBitmap, blurred: State? = null): State { + // The state and the decoding are keyed alike on purpose: were the state replaced without the decoding + // restarting, frames would keep being written into a state nobody reads. The blur is deliberately not a + // key - it changes as the mouse moves, and it pauses the animation rather than starting it over. + val frame = remember(data, still) { mutableStateOf(still) } + LaunchedEffect(data, still) { + withContext(animationDecoder) { + val codec = animatableCodec(data) ?: return@withContext + try { + playFrames(codec, blurred) { frame.value = it } + } finally { + codec.close() + } + } + } + return frame +} + +// Decoding is CPU work on somebody else's data, so it is confined to a small share of the shared pool: +// several large animations on screen must not starve the coroutines that deliver messages. +@OptIn(ExperimentalCoroutinesApi::class) +private val animationDecoder = Dispatchers.Default.limitedParallelism(2) + +// A codec for data worth animating, or null. Null covers "not an animation" and "outside the bounds" alike, +// because the caller responds to both the same way - by keeping the still image. +private fun animatableCodec(data: ByteArray): Codec? { + // Photos are almost all of what a chat holds and none of them are animations, so they leave here without + // their bytes ever being copied into native memory or handed to a second decoder. + if (!looksAnimatable(data) || data.size > MAX_ANIMATED_FILE_SIZE) return null + var codec: Codec? = null + try { + // Skia keeps its own reference to the encoded bytes, so this copy is released the moment the codec has + // taken it, rather than left to the collector: it is a native buffer of up to the file size behind a + // small Java object, which is not something the collector has much reason to reclaim. Frames still + // decode afterwards - checked against a codec whose Data had already been freed. + val encoded = Data.makeFromBytes(data) + codec = try { + Codec.makeFromData(encoded) + } finally { + encoded.close() + } + val info = codec.imageInfo + // Dimensions are read from the header, while counting frames scans the file, so the frames are only + // counted once the dimensions are known to be sane + if (rasterWithinBounds(info.width, info.height, info.bytesPerPixel) && codec.frameCount > 1) return codec + } catch (e: Throwable) { + // Not the stack trace: this runs on data from other people, at the rate they can send it + Log.e(TAG, "Unable to read animated image: $e") + } + codec?.close() + return null +} + +// The two container formats this app treats as animated, recognised by their own bytes rather than by the +// name the sender chose for the file. +internal fun looksAnimatable(data: ByteArray): Boolean = + data.startsWith("GIF8") || (data.startsWith("RIFF") && data.startsWith("WEBP", offset = 8)) + +private fun ByteArray.startsWith(ascii: String, offset: Int = 0): Boolean { + if (size < offset + ascii.length) return false + return ascii.indices.all { this[offset + it] == ascii[it].code.toByte() } +} + +// Whether a frame of this size is one we will hold in memory, as plain numbers so that the bounds can be +// checked on their own - they are the part that has to be right about a file someone else composed. +internal fun rasterWithinBounds(width: Int, height: Int, bytesPerPixel: Int): Boolean { + if (width !in 1..MAX_ANIMATED_SIDE || height !in 1..MAX_ANIMATED_SIDE) return false + // A colour type claiming no bytes per pixel would make any raster look free + if (bytesPerPixel < 1) return false + // The sides are bounded before they are multiplied, so that the product cannot wrap: 65535 * 65535 already + // overflows Int to a negative number, and an unbounded three-way product can overflow Long as well. + return width.toLong() * height * bytesPerPixel <= MAX_ANIMATED_RASTER_BYTES +} + +private suspend fun playFrames(codec: Codec, blurred: State?, showFrame: (ImageBitmap) -> Unit) { + val bitmap = Bitmap() + try { + // allocPixels reports failure by returning false rather than throwing, and reading a frame into an + // unallocated bitmap throws, so the result is checked instead of assumed. + if (!bitmap.allocPixels(codec.imageInfo)) return + // Reading the frame count is also what makes getFrameInfo below work at all: in skiko 0.9.4 the + // single-frame accessor reads past its own buffer until the count has been taken (or the whole frames + // array, which costs an object per frame). The loop is bounded by the count, so the order holds. + // Never loop over fewer frames than an animation has, either: the loop only suspends inside the range, + // so a frameless codec would spin a core forever and could not even be cancelled, and a single-frame one + // would re-decode the same picture for as long as it was on screen. + val frameCount = codec.frameCount + if (frameCount < 2) return + var loopsLeft = codec.repetitionCount // negative repeats forever + var slowFrames = 0 + while (true) { + for (i in 0 until frameCount) { + awaitFramesAreSeen(blurred) + val startedDecoding = System.nanoTime() + codec.readPixels(bitmap, i) + // Two in a row, because this is wall time: one frame can overrun simply by being descheduled, and a + // busy machine should not turn a cheap animation into a still. Expensive ones overrun every frame. + if (System.nanoTime() - startedDecoding > MAX_FRAME_DECODE_MS * 1_000_000) slowFrames++ else slowFrames = 0 + if (slowFrames >= 2) { + Log.d(TAG, "Animation too expensive to decode, keeping the still image") + return + } + // A new wrapper around the same raster, so the state sees a change - as the video surface does. + // The bitmap itself is deliberately never closed: the wrapper handed to Compose points at its + // pixels, and freeing them while a frame may still be drawn would be a use-after-free. + showFrame(bitmap.asComposeImageBitmap()) + // Frame info is read one frame at a time: reading the whole array costs 200 000 objects (~24MB) for + // a 4.6MB file. The wait is not shortened by the time decoding took - playing a little slower than + // authored is better than a sleep that can shrink to nothing. + delay(frameDuration(codec.getFrameInfo(i))) + } + if (loopsLeft == 0) return + if (loopsLeft > 0) loopsLeft-- + } + } catch (e: CancellationException) { + throw e // the view went away, which is not a decoding failure + } catch (e: Throwable) { + // Every call above reads a file composed by somebody else, and the frame count and repeat count are read + // from it too. A failure in any of them ends the animation on the last frame that decoded, instead of + // reaching the composition as a crash. + Log.e(TAG, "Unable to play animated image: $e") + } +} + +// Frames are only decoded while there is somebody to see them. The app is built to live in the tray and +// composition survives being hidden there; and a blurred image is only revealed while the mouse is over it, +// so the rest of the time each frame would be decoded, uploaded and then blurred away again for nobody. +private suspend fun awaitFramesAreSeen(blurred: State?) { + if (framesAreSeen(blurred)) return + snapshotFlow { framesAreSeen(blurred) }.first { it } +} + +private fun framesAreSeen(blurred: State?): Boolean = + simplexWindowState.windowVisible.value && blurred?.value != true + +private fun frameDuration(info: AnimationFrameInfo): Long { + val declared = info.duration.toLong() + return if (declared <= 0) DEFAULT_FRAME_DURATION_MS else declared.coerceAtLeast(MIN_FRAME_DURATION_MS) +} diff --git a/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/AnimatedImageBoundsTest.kt b/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/AnimatedImageBoundsTest.kt new file mode 100644 index 0000000000..11ef5bc07d --- /dev/null +++ b/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/AnimatedImageBoundsTest.kt @@ -0,0 +1,95 @@ +package chat.simplex.app + +import chat.simplex.common.platform.looksAnimatable +import chat.simplex.common.platform.rasterWithinBounds +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +// Bounds an animated image must satisfy before the desktop chat holds decoded frames for it. They are checked +// here as arithmetic, without a decoder: skiko's native library is not on the test runtime classpath, and +// these numbers are the part that has to be right about a file composed by someone else. +// Not covered here, and verified by hand against Skia instead: that a crafted file reports the dimensions it +// declares, and that a real animation reports more than one frame. +class AnimatedImageBoundsTest { + private val BYTES_PER_PIXEL = 4 // what a GIF or WebP decodes to + + @Test + fun testOrdinaryAnimationIsWithinBounds() { + assertTrue(rasterWithinBounds(64, 64, BYTES_PER_PIXEL)) + assertTrue(rasterWithinBounds(1244, 554, BYTES_PER_PIXEL)) + } + + @Test + fun testHugeDeclaredDimensionsAreRejected() { + // 65535x65535 is a 17GB raster, and a GIF declaring it fits in 35 bytes. Rejected by the side bound + // before anything is multiplied; the wrapping products are covered separately below. + assertFalse(rasterWithinBounds(65535, 65535, BYTES_PER_PIXEL)) + } + + @Test + fun testDimensionsOverRasterBudgetAreRejected() { + // Plausible-looking, but one raster of this size is ~64MB and a chat shows several at once + assertFalse(rasterWithinBounds(4000, 4000, BYTES_PER_PIXEL)) + } + + @Test + fun testAspectRatioIsBoundedOnEachSideSeparately() { + // Only 2.1MP, so the raster bound alone would animate this with a 65535-pixel scanline + assertFalse(rasterWithinBounds(65535, 32, BYTES_PER_PIXEL)) + assertFalse(rasterWithinBounds(32, 65535, BYTES_PER_PIXEL)) + // A banner-shaped animation is wide but sane on both counts, and keeps playing + assertTrue(rasterWithinBounds(3000, 500, BYTES_PER_PIXEL)) + assertTrue(rasterWithinBounds(4096, 900, BYTES_PER_PIXEL)) + } + + @Test + fun testBudgetBoundariesAreExact() { + assertTrue(rasterWithinBounds(1920, 1920, BYTES_PER_PIXEL)) + assertFalse(rasterWithinBounds(1921, 1920, BYTES_PER_PIXEL)) + assertFalse(rasterWithinBounds(4097, 100, BYTES_PER_PIXEL)) + } + + @Test + fun testEmptyDimensionsAreRejected() { + assertFalse(rasterWithinBounds(0, 64, BYTES_PER_PIXEL)) + assertFalse(rasterWithinBounds(64, 0, BYTES_PER_PIXEL)) + assertFalse(rasterWithinBounds(-1, 64, BYTES_PER_PIXEL)) + } + + @Test + fun testWiderColourTypesCountAgainstTheSameBudget() { + // The file chooses its colour type, so the bound counts bytes rather than assuming four per pixel: the + // 1920x1920 that fits at four bytes is twice the raster at eight + assertFalse(rasterWithinBounds(1920, 1920, 8)) + assertTrue(rasterWithinBounds(1357, 1357, 8)) + // A colour type claiming no bytes per pixel would otherwise make any raster look free + assertFalse(rasterWithinBounds(4096, 4096, 0)) + } + + @Test + fun testAnimatableContainersAreRecognised() { + assertTrue(looksAnimatable("GIF89a...".toByteArray())) + assertTrue(looksAnimatable("GIF87a...".toByteArray())) + assertTrue(looksAnimatable("RIFF????WEBPVP8X".toByteArray())) + } + + @Test + fun testPhotosNeverReachTheAnimationDecoder() { + assertFalse(looksAnimatable(bytes(0x89, 'P'.code, 'N'.code, 'G'.code, 0x0D, 0x0A, 0x1A, 0x0A))) + assertFalse(looksAnimatable(bytes(0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 0x4A, 0x46))) + // A RIFF container that is not WebP - a wave file, say - is not an animation either + assertFalse(looksAnimatable("RIFF????WAVEfmt ".toByteArray())) + } + + @Test + fun testShortDataIsRejectedWithoutReadingPastTheEnd() { + assertFalse(looksAnimatable(ByteArray(0))) + assertFalse(looksAnimatable("GIF".toByteArray())) + // Long enough for the RIFF tag, too short for the format that follows it + assertFalse(looksAnimatable("RIFF".toByteArray())) + assertFalse(looksAnimatable("RIFF1234WEB".toByteArray())) + } + + private fun bytes(vararg values: Int): ByteArray = values.map { it.toByte() }.toByteArray() +}