desktop: fix rotated video squashed on playback and preview rotated twice (#7413)

* desktop: fix rotated video squashed on playback and preview rotated twice vlc applies the display matrix before a frame reaches the vmem callback, so the buffer has to be requested with the sides swapped for the transposed orientations, and the snapshot must not be rotated again by hand. Read the snapshot on the event thread, where the render callback writes it, and draw the inline playback surface with FillWidth so a video narrower than the item fills it like its preview does. Bound the requested buffer: the size comes from a received file, so it is capped by area, cannot be zero, and a frame that does not fill the bitmap is dropped. * desktop: harden the video frame path against crafted files Only transpose the buffer for the track's own sides - the size libvlc passes is already rotated, so swapping it would recreate the squash for a file declaring a rotation with a zero-sized track. Copy the frame inside the render callback, on vlc's thread, where the native buffer is guaranteed to exist, and hand only the copy to the event thread. Drop a frame rendered with a format the bitmap was not sized by, or arriving before any buffer was allocated. Divide the pixel budget by a side pinned at 1 instead of scaling both sides, so a 2000000000x1 declaration cannot take 45 times the budget. Publish the bitmap only when skia took the pixels, size the copy after a rewind, and log a failed snapshot conversion instead of throwing it into callers that have no handler for it.
2026-08-29 05:29:08 +00:00 · 2026-08-27 11:08:05 +01:00
parent f2856c1be1
commit d7e0f85976
5 changed files with 214 additions and 24 deletions
@@ -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<Int, Int> {
+      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<ImageBitmap?>(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<ByteBuffer>) {
-      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<ByteBuffer>,
      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()
+        }
      }
    }
  }
@@ -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<BufferedImage?>(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)
@@ -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
    )
  }
 }
@@ -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,
    )
  }
@@ -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.