Merge branch 'master' into badges

2026-09-02 16:33:54 +00:00 · 2026-08-28 19:37:10 +01:00
parent 6075ea5797 d7e0f85976
commit 9c77703017
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.