diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/other/videoplayer/SkiaBitmapVideoSurface.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/other/videoplayer/SkiaBitmapVideoSurface.kt index f5bba2d344..26748b8425 100644 --- a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/other/videoplayer/SkiaBitmapVideoSurface.kt +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/other/videoplayer/SkiaBitmapVideoSurface.kt @@ -8,6 +8,7 @@ import org.jetbrains.skia.Bitmap import org.jetbrains.skia.ColorAlphaType import org.jetbrains.skia.ColorType import org.jetbrains.skia.ImageInfo +import uk.co.caprica.vlcj.media.VideoOrientation import uk.co.caprica.vlcj.player.base.MediaPlayer import uk.co.caprica.vlcj.player.embedded.videosurface.CallbackVideoSurface import uk.co.caprica.vlcj.player.embedded.videosurface.VideoSurface @@ -22,10 +23,42 @@ import javax.swing.SwingUtilities // https://github.com/JetBrains/compose-multiplatform/pull/3336/files internal class SkiaBitmapVideoSurface : VideoSurface(VideoSurfaceAdapters.getVideoSurfaceAdapter()) { + private companion object { + // A received file declares its own size, and vlc allocates the buffer we ask for here (and we copy + // it into a java array of the same size), so an unbounded request is an out-of-memory from a message. + // Above the budget the picture is scaled down keeping its aspect - 4096x4096 of RV32 is 64 MB + const val MAX_BUFFER_PIXELS = 4096L * 4096L + val transposedOrientations = setOf( + VideoOrientation.LEFT_TOP, + VideoOrientation.LEFT_BOTTOM, + VideoOrientation.RIGHT_TOP, + VideoOrientation.RIGHT_BOTTOM, + ) + + // Keeps the aspect, never returns a side below 1, and keeps width * height * 4 inside an Int + fun boundedSize(width: Int, height: Int): Pair { + val w = width.coerceAtLeast(1) + val h = height.coerceAtLeast(1) + val pixels = w.toLong() * h.toLong() + if (pixels <= MAX_BUFFER_PIXELS) return w to h + val scale = kotlin.math.sqrt(MAX_BUFFER_PIXELS.toDouble() / pixels.toDouble()) + var sw = (w * scale).toInt().coerceAtLeast(1) + var sh = (h * scale).toInt().coerceAtLeast(1) + // Scaling both sides assumes both shrink; a side pinned at 1 only shrinks the area linearly, + // so a 2_000_000_000 x 1 declaration would still get 45 times the budget. Divide the budget + // by the pinned side instead + if (sw.toLong() * sh.toLong() > MAX_BUFFER_PIXELS) { + if (sw >= sh) sw = (MAX_BUFFER_PIXELS / sh).toInt() else sh = (MAX_BUFFER_PIXELS / sw).toInt() + } + return sw to sh + } + } + private val videoSurface = SkiaBitmapVideoSurface() @Volatile private var mediaPlayer: MediaPlayer? = null private lateinit var imageInfo: ImageInfo private lateinit var frameBytes: ByteArray + @Volatile private var allocated = false private val skiaBitmap: Bitmap = Bitmap() private val composeBitmap = mutableStateOf(null) @@ -49,19 +82,37 @@ internal class SkiaBitmapVideoSurface : VideoSurface(VideoSurfaceAdapters.getVid val tracks = player?.media()?.info()?.videoTracks() val playingTrack = player?.video()?.track() val track = tracks?.firstOrNull { it.id() == playingTrack } ?: tracks?.singleOrNull() - this.sourceWidth = track?.width()?.takeIf { it > 0 } ?: sourceWidth - this.sourceHeight = track?.height()?.takeIf { it > 0 } ?: sourceHeight - return RV32BufferFormat(this.sourceWidth, this.sourceHeight) + // Both track sides or neither: one side from the track and the other from the padded size libvlc + // passed never described the same picture, and transposing such a pair compounds the mismatch + val trackW = track?.width() ?: 0 + val trackH = track?.height() ?: 0 + val useTrack = trackW > 0 && trackH > 0 + val width = if (useTrack) trackW else sourceWidth + val height = if (useTrack) trackH else sourceHeight + // The track carries the size before rotation, but vlc rotates the picture before it reaches + // this buffer, so for the transposed orientations the picture arrives with the sides swapped. + // Only when the track's own sides are used: the size libvlc passed is already rotated + val transposed = useTrack && (track?.orientation() in transposedOrientations) + val orientedWidth = if (transposed) height else width + val orientedHeight = if (transposed) width else height + val (w, h) = boundedSize(orientedWidth, orientedHeight) + this.sourceWidth = w + this.sourceHeight = h + return RV32BufferFormat(w, h) } override fun allocatedBuffers(buffers: Array) { - frameBytes = buffers[0].run { ByteArray(remaining()).also(::get) } + // rewind first, as in display: remaining() on an already-read buffer would size this short + frameBytes = buffers[0].run { rewind(); ByteArray(remaining()).also(::get) } imageInfo = ImageInfo( sourceWidth, sourceHeight, ColorType.BGRA_8888, ColorAlphaType.PREMUL, ) + // Last, and volatile: vlc calls this on its own thread while display reads imageInfo and + // frameBytes on the event thread, and this write is what publishes them to it + this@SkiaBitmapVideoSurface.allocated = true } } @@ -71,11 +122,35 @@ internal class SkiaBitmapVideoSurface : VideoSurface(VideoSurfaceAdapters.getVid nativeBuffers: Array, bufferFormat: BufferFormat, ) { + // The native buffer belongs to vlc and is only guaranteed to exist for the duration of this + // callback, so everything that touches it has to happen here, on vlc's thread - deferred code + // would read through a pointer vlc may have freed on a format change. Only the copy is done + // here; skia and compose are event-thread objects and get the private copy + if (!this@SkiaBitmapVideoSurface.allocated) return + val info = imageInfo + // imageInfo comes from the format that was last allocated and this frame from the format it was + // rendered with; they differ across a renegotiation, and the pixels would be read with the + // wrong stride, so display only what matches + if (bufferFormat.width != info.width || bufferFormat.height != info.height) return + val rowBytes = info.width.toLong() * 4 + val needed = rowBytes * info.height + val buffer = nativeBuffers[0] + // rewind first: the same buffer is reused for every frame, so its position is at the end of + // the previous read and remaining() would be 0 + buffer.rewind() + // Capture the array: a renegotiation replaces the field with one of another size before the + // deferred install runs, and info's geometry must be read against the array it was copied into. + // The next frame's copy can overwrite it while the install reads - a torn frame at worst, since + // the geometry checks above hold for both frames of the same format + val bytes = frameBytes + if (needed > bytes.size || buffer.remaining().toLong() < needed) return + buffer.get(bytes, 0, needed.toInt()) SwingUtilities.invokeLater { - nativeBuffers[0].rewind() - nativeBuffers[0].get(frameBytes) - skiaBitmap.installPixels(imageInfo, frameBytes, bufferFormat.width * 4) - composeBitmap.value = skiaBitmap.asComposeImageBitmap() + // installPixels reports whether skia took the pixels; publishing the bitmap when it did not + // would hand compose a bitmap with no pixels behind it + if (skiaBitmap.installPixels(info, bytes, rowBytes.toInt())) { + composeBitmap.value = skiaBitmap.asComposeImageBitmap() + } } } } diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/VideoPlayer.desktop.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/VideoPlayer.desktop.kt index 768d2f421d..d9375f5d79 100644 --- a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/VideoPlayer.desktop.kt +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/platform/VideoPlayer.desktop.kt @@ -11,7 +11,6 @@ import uk.co.caprica.vlcj.media.Media import uk.co.caprica.vlcj.media.MediaEventAdapter import uk.co.caprica.vlcj.media.MediaParsedStatus import uk.co.caprica.vlcj.media.ParseFlag -import uk.co.caprica.vlcj.media.VideoOrientation import uk.co.caprica.vlcj.player.base.* import uk.co.caprica.vlcj.player.component.CallbackMediaPlayerComponent import uk.co.caprica.vlcj.player.component.EmbeddedMediaPlayerComponent @@ -232,7 +231,18 @@ actual class VideoPlayer actual constructor( player.media().startPaused(uri.toFile().absolutePath) val snap = withTimeoutOrNull(1500L) { while (surface.bitmap.value == null) delay(50) - surface.bitmap.value!!.toAwtImage() + // The render callback installs pixels into the surface bitmap on the event thread, so read it + // there too - converting off that thread races a resize on format renegotiation and segfaults + // inside skia while reading pixels of the previous, smaller buffer + val holder = java.util.concurrent.atomic.AtomicReference(null) + // invokeAndWait rethrows whatever the conversion threw, wrapped, and the callers of this have + // no handler; a frame that cannot be converted is a missing preview, not a failed send + try { + javax.swing.SwingUtilities.invokeAndWait { holder.set(surface.bitmap.value?.toAwtImage()) } + } catch (e: Exception) { + Log.e(TAG, "getBitmapFromVideo snapshot failed: ${e.stackTraceToString()}") + } + holder.get() } val orientation = player.media().info().videoTracks().firstOrNull()?.orientation() if (orientation == null) { @@ -242,17 +252,9 @@ actual class VideoPlayer actual constructor( return@withContext VideoPlayerInterface.PreviewAndDuration(preview = defaultPreview, timestamp = 0L, duration = 0L) } - val preview: ImageBitmap? = when (orientation) { - VideoOrientation.TOP_LEFT -> snap - VideoOrientation.TOP_RIGHT -> snap?.flip(false, true) - VideoOrientation.BOTTOM_LEFT -> snap?.flip(true, false) - VideoOrientation.BOTTOM_RIGHT -> snap?.rotate(180.0) - VideoOrientation.LEFT_TOP -> snap /* Transposed */ - VideoOrientation.LEFT_BOTTOM -> snap?.rotate(-90.0) - VideoOrientation.RIGHT_TOP -> snap?.rotate(90.0) - VideoOrientation.RIGHT_BOTTOM -> snap /* Anti-transposed */ - else -> snap - }?.toComposeImageBitmap() + // vlc applies the display matrix before the frame reaches the video surface, so the snapshot + // arrives upright; orienting it again here would undo that + val preview: ImageBitmap? = snap?.toComposeImageBitmap() val duration = player.duration.toLong() player.stop() putHelperPlayer(mediaComponent) diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/CIVideoView.desktop.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/CIVideoView.desktop.kt index bdfbf6863f..d60790b694 100644 --- a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/CIVideoView.desktop.kt +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/CIVideoView.desktop.kt @@ -4,6 +4,7 @@ import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.* import androidx.compose.runtime.* import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.ContentScale import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalWindowInfo import androidx.compose.ui.unit.Dp @@ -14,6 +15,8 @@ import java.awt.Window @Composable actual fun PlayerView(player: VideoPlayer, width: Dp, onClick: () -> Unit, onLongClick: () -> Unit, stop: () -> Unit) { Box { + // The preview this replaces while playing is drawn with FillWidth, so a video smaller than the + // item width has to grow the same way here - Fit would leave it at its own size in the middle SurfaceFromPlayer(player, Modifier .width(width) @@ -21,7 +24,8 @@ actual fun PlayerView(player: VideoPlayer, width: Dp, onClick: () -> Unit, onLon onLongClick = onLongClick, onClick = { if (player.player.isPlaying) stop() else onClick() } ) - .onRightClick(onLongClick) + .onRightClick(onLongClick), + contentScale = ContentScale.FillWidth ) } } diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/ImageFullScreenView.desktop.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/ImageFullScreenView.desktop.kt index 583aa8f52f..9fb4afbf7d 100644 --- a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/ImageFullScreenView.desktop.kt +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/chat/item/ImageFullScreenView.desktop.kt @@ -43,7 +43,7 @@ actual fun FullScreenVideoView(player: VideoPlayer, modifier: Modifier, close: ( } @Composable -fun BoxScope.SurfaceFromPlayer(player: VideoPlayer, modifier: Modifier) { +fun BoxScope.SurfaceFromPlayer(player: VideoPlayer, modifier: Modifier, contentScale: ContentScale = ContentScale.Fit) { val surface = remember { SkiaBitmapVideoSurface().also { player.player.videoSurface().set(it) @@ -54,7 +54,7 @@ fun BoxScope.SurfaceFromPlayer(player: VideoPlayer, modifier: Modifier) { bitmap, modifier = modifier.align(Alignment.Center), contentDescription = null, - contentScale = ContentScale.Fit, + contentScale = contentScale, alignment = Alignment.Center, ) } diff --git a/plans/2026-08-24-desktop-rotated-video-playback.md b/plans/2026-08-24-desktop-rotated-video-playback.md new file mode 100644 index 0000000000..0ef27efbaf --- /dev/null +++ b/plans/2026-08-24-desktop-rotated-video-playback.md @@ -0,0 +1,109 @@ +# Desktop: rotated videos squashed on playback, preview rotated twice, snapshot crash + +## Problem + +A video carrying rotation metadata - what a phone records in portrait - is squashed when it +plays in a chat item on desktop, while its preview looks correct. Re-sending such a video from +desktop produces a preview that is wrong for every recipient. Attaching one can take the app +down with a SIGSEGV inside skia. Separately, any video smaller than the width of the message +item plays at its own size in the middle of the item instead of filling it, though its preview +fills the width. + +## Cause + +Three defects, of different ages, on the path a frame takes from libvlc to the screen. + +**The buffer is sized from the wrong dimensions.** `SkiaBitmapVideoSurface` asks libvlc for a +buffer of `track.width() x track.height()`. The track carries the size before rotation, but vlc +applies the display matrix before the frame reaches the vmem callback, so the picture arriving +is transposed with respect to the buffer, and vlc stretches it to fill. Measured with a +1920x1080 HEVC file whose display matrix is -90: + +| source | value | +| -------------------------- | -------------------- | +| size libvlc offers | 1088x1920 (rotated) | +| `track.width()/height()` | 1920x1080 (coded) | +| buffer requested before fix | 1920x1080 | + +Asking for the track size was introduced to drop the decoder's padding (#7391); it is right for +an unrotated video and wrong for a rotated one, because it discards the orientation libvlc had +already applied. + +**The preview is oriented twice.** `previewAndDuration` takes a snapshot from the same surface +and then rotates it by hand. The snapshot arrives at 1080x1920 and already upright - dumping it +to a PNG confirms the content, not just the dimensions - and the manual rotation turns it back +to 1920x1080. Before this change the two errors cancelled: a wrongly shaped buffer plus a +manual rotation produced a preview that looked right, which is why the preview was correct while +playback was not. + +**The snapshot races the render callback.** The render callback installs pixels into the shared +bitmap on the event thread; the snapshot converted it on the preview thread. The format is +renegotiated several times per file, so a resize between `installPixels` and `readPixels` makes +skia read past the end of the buffer: + +``` +SIGSEGV ... C [libskiko-linux-x64.so+0x1e7807] sse2::load_8888(...) + at org.jetbrains.skia.Bitmap.readPixels + at chat.simplex.common.platform.VideoPlayer$Companion$getBitmapFromVideo$2$snap$1 +``` + +**Small videos do not fill the item.** The preview is drawn with `ContentScale.FillWidth` and +the playback surface with `ContentScale.Fit`. `Fit` never exceeds the height of the box, so a +320x240 video stays at its own size, centred, while its preview fills the width. This is only +visible for sources narrower than the item. + +## Fix + +Swap the requested width and height for the four transposed orientations, so the buffer matches +the picture vlc delivers, and keep the track size otherwise so the padding fix still holds. +Drop the manual orientation handling from the preview, since the frame is already upright. Read +the snapshot on the event thread, where the render callback writes it. Draw the inline playback +surface with `FillWidth`, as its preview is drawn. + +## Bounds + +The dimensions come from a received file, so they are attacker-chosen and are treated as such. + +- Both track sides are used or neither. One side from the track beside the other from libvlc's + padded size never described the same picture, and transposing such a pair compounds it. +- The requested area is capped, scaling down and keeping the aspect where both sides can shrink; + a side pinned at 1 takes the whole budget on the other side instead, since scaling cannot keep + the aspect of a 2000000000x1 declaration and hold the area at once. An unbounded request is an + out-of-memory from a message: 16000x16000 is 1 GB of RV32, requested from vlc and copied into + a java array of the same size. The cap also keeps `width * height * 4` inside an `Int`. +- Neither side can be zero, so a 1x4000 or 4000x1 file cannot produce an empty buffer. +- The sides are only swapped when the track's own sides are used. The size libvlc passes is + already rotated, so swapping that pair would recreate the squash for a file that declares a + rotation and a zero-sized track. +- A frame is dropped rather than displayed when it does not fill the bitmap skia is told to + read, when the format it was rendered with is not the one the bitmap was sized by, and before + any buffer has been allocated. The checks and the copy run inside the render callback, on + vlc's thread: the native buffer is only guaranteed to exist for the duration of the callback, + so code deferred to another thread would read through a pointer vlc may have freed on a format + change. Only the copied frame is handed to the event thread. +- The bitmap is published only when skia reports that it took the pixels, and a snapshot that + cannot be converted is logged and left empty rather than thrown into callers that have no + handler for it. + +## Testing + +Fifteen files covering 320x240 to 3840x2160, square, odd, and 1234x567 sizes, h264, vp9, av1 +and hevc, unrotated and 90/180/270, plus 1x4000, 4000x1 and 16000x16000. Checked that rotated +videos play upright and preview upright, that a re-sent video keeps its shape, that attaching +does not crash, that a 320x240 video fills the item, that the AV1 padding fix still holds, and +that the 16000x16000 file is scaled to the cap instead of allocating a gigabyte. + +## Android + +None of these reach android. The buffer format callback is desktop only - android renders +through exoplayer's `StyledPlayerView`, with no buffer for us to size - and its preview comes +from `MediaMetadataRetriever.getFrameAtTime`, which returns an oriented frame and is not +rotated again. The event thread race is skia and swing. Android already fills the item width +with `RESIZE_MODE_FIXED_WIDTH`, which is what the `FillWidth` change gives desktop. + +## Not addressed + +`CIVideoView` bounds the item's aspect ratio above at 2.33 but not below, so a 4000x1 video +still lays out with a height that rounds to zero. The snapshot's `invokeAndWait` is not +cancellable, so its 1.5s timeout cannot interrupt a wedged event thread. Both are outside the +functions this change touches.