mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-08-28 20:08:16 +00:00
desktop: add bounded animated image decoder
Skia's Codec is already on the desktop classpath through skiko and decodes both GIF and animated WebP. The frames come from a file somebody else composed, so the decoder is bounded before it allocates: the raster is measured in bytes with the sides multiplied as Long, each side is capped separately so an extreme aspect ratio cannot slip under the byte budget, and the encoded size is checked before the bytes are copied into native memory. Anything outside the bounds, or any failure, keeps the still image the chat already renders. Nothing calls this yet.
This commit is contained in:
Narasimha-sc
@@ -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.
|
||||
+184
@@ -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<Boolean>? = null): State<ImageBitmap> {
|
||||
// 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<Boolean>?, 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<Boolean>?) {
|
||||
if (framesAreSeen(blurred)) return
|
||||
snapshotFlow { framesAreSeen(blurred) }.first { it }
|
||||
}
|
||||
|
||||
private fun framesAreSeen(blurred: State<Boolean>?): 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)
|
||||
}
|
||||
+95
@@ -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()
|
||||
}
|
||||
Reference in New Issue
Block a user