laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-06 11:48:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(video): five defects found auditing this PR's own diff

Browse Source 
Four were confirmed with throwaway probe tests against the branch.

An HLS master labelled `audio/x-mpegurl` or `audio/mpegurl` was read as a
separate audio track and dropped. Those are two of the four playlist MIMEs
this repo already recognises in isHlsMimeType and MediaItemCache — legacy
aliases naming the manifest format, not a claim about the content. A master
labelled that way lost to a 360p rung; when every entry used it the candidate
list emptied and selection fell through to the poster JPEG, handed to the
video player. The HLS test now precedes the audio test.

withLadderMetadataFrom filled `dimension` from every imeta, poster included,
so a 16:9 thumbnail beside a vertical short produced a 16:9 master and
JustVideoDisplay laid the box out at 16:9. Only entries that could be the
video may describe its shape; the poster still supplies the still image.

isHlsPlaylist treated any declared MIME as authoritative, so `master.m3u8`
served as application/octet-stream — a server default, not a claim — was not
HLS and lost to a correctly labelled low rung.

The metered 480px cap had no fullscreen exemption, so tapping into fullscreen
on mobile data pinned 480p and put a ceiling the quality menu's "Auto" could
not exceed. The cap exists to hold back feeds that autoplay unasked; someone
who tapped fullscreen asked.

The PiP gate skipped the viewport push entirely until isInPictureInPictureMode
turned true, with no retry. Since demoteToCold clears track overrides but not
the viewport, a pooled player kept whatever its previous view pushed if PiP was
never entered (per-app PiP off, no FEATURE_PICTURE_IN_PICTURE). It now caps the
pre-shrink measurement instead of skipping it, so a viewport is always pushed
and can never be a stale full-screen one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQs7TP2WeXNR8SwUNLgmUC
This commit is contained in:
Claude
2026-09-01 15:50:04 +00:00
parent 7ed368118c
commit 54c6521f3f
5 changed files with 99 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/RenderVideoPlayer.kt
+4 -1
View File
@@ -115,7 +115,10 @@ fun RenderVideoPlayer(
// it exists only so a connectivity flip can re-push the viewport without a layout pass.
val lastMeasured = remember { intArrayOf(0, 0) }
val isMetered by accountViewModel.settings.isMobileOrMeteredConnection.collectAsStateWithLifecycle()
val viewportCeiling = if (isMetered) METERED_MAX_SHORT_SIDE_PX else 0
// Fullscreen is exempt: the cap exists to hold back feeds that autoplay without being asked,
// and someone who tapped into fullscreen on mobile data asked. Capping there would also put a
// ceiling the quality menu's "Auto" could not exceed.
val viewportCeiling = if (isMetered && !isFullscreen) METERED_MAX_SHORT_SIDE_PX else 0
val isLive = remember(mediaItem.src.videoUri, mediaItem.src.mimeType) { isHlsMedia(mediaItem.src.videoUri, mediaItem.src.mimeType) }
val swipeState = remember { FullscreenSwipeControlsState() }
amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/VideoQualityControls.kt
+16 -3
View File
@@ -57,12 +57,15 @@ internal fun getVideoTrackGroup(tracks: Tracks): Tracks.Group? = tracks.groups.f
*/
internal fun Modifier.constrainVideoQualityToViewport(
player: Player,
shouldApply: () -> Boolean = { true },
maxShortSidePx: () -> Int = { 0 },
): Modifier =
onSizeChanged {
// Evaluated at measure time, not at composition: a caller whose window is still resizing
// (PiP) needs the answer for *this* layout pass.
if (shouldApply()) applyViewportConstraint(player, it.width, it.height)
// (PiP) needs the answer for *this* layout pass. Always pushes something — a caller that
// wants to distrust its own measurement caps it rather than skipping, so a pooled player
// can never keep a viewport left over from the view that had it last.
val (width, height) = clampViewportShortSide(it.width, it.height, maxShortSidePx())
applyViewportConstraint(player, width, height)
}
/**
@@ -76,6 +79,16 @@ internal fun Modifier.constrainVideoQualityToViewport(
*/
const val METERED_MAX_SHORT_SIDE_PX = 480
/**
* Ceiling the PiP player uses until its window has actually shrunk.
*
* `processIntentForPiP` calls `enterPictureInPictureMode` from composition, so the first layout
* pass can measure the activity at full screen. Capping rather than skipping the push means the
* selector never sees that full-screen size, and never keeps a stale one either when PiP is never
* entered at all — the window is a few inches wide, so nothing above this is ever wanted here.
*/
const val PIP_PRESHRINK_MAX_SHORT_SIDE_PX = 480
/**
* Scales a measured player size down until its short side fits [maxShortSidePx], preserving aspect
* so the viewport still describes the shape of the player and not just its area. Sizes already
amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/pip/PipVideoView.kt
+8 -4
View File
@@ -43,6 +43,7 @@ import androidx.media3.ui.compose.state.rememberPlayPauseButtonState
import com.vitorpamplona.amethyst.model.MediaAspectRatioCache
import com.vitorpamplona.amethyst.service.playback.composable.MediaControllerState
import com.vitorpamplona.amethyst.service.playback.composable.WaveformData
import com.vitorpamplona.amethyst.service.playback.composable.controls.PIP_PRESHRINK_MAX_SHORT_SIDE_PX
import com.vitorpamplona.amethyst.service.playback.composable.controls.constrainVideoQualityToViewport
import com.vitorpamplona.amethyst.service.playback.composable.mediaitem.MediaItemData
import com.vitorpamplona.amethyst.service.playback.composable.wavefront.Waveform
@@ -72,15 +73,18 @@ fun RenderPipVideo(
}
// processIntentForPiP calls enterPictureInPictureMode from composition (PiPFromIntents), so the
// first layout pass can measure the activity at full screen before the window shrinks. Pushing
// that size would hand the selector a full-screen viewport for the first seconds of a PiP that
// is a few inches wide; the shrink relayouts and pushes the real size.
// first layout pass can measure the activity at full screen before the window shrinks. Cap that
// measurement instead of skipping it: skipping would leave a pooled player on whatever viewport
// its previous view pushed if PiP is never actually entered (per-app PiP off, no
// FEATURE_PICTURE_IN_PICTURE). The shrink relayouts and pushes the real size.
val activity = LocalActivity.current
Box(
modifier.constrainVideoQualityToViewport(
player = controller.controller,
shouldApply = { activity?.isInPictureInPictureMode == true },
maxShortSidePx = {
if (activity?.isInPictureInPictureMode == true) 0 else PIP_PRESHRINK_MAX_SHORT_SIDE_PX
},
),
contentAlignment = Alignment.Center,
) {
commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip71Video/VideoTrackSelection.kt
+19 -3
View File
@@ -85,7 +85,12 @@ fun VideoEvent.selectVideoTrack(): VideoMeta? {
// `audio/*` is a separate track under PR #2255, and `image/*` is a poster. Everything else —
// including the HLS playlist MIMEs, which are neither `video/*` nor an image, and a bare URL with
// no MIME at all — belongs in the player.
private fun VideoMeta.canBeTheVideo(): Boolean = !isAudio && RichTextParser.classifyMedia(url, mimeType) != MediaContentKind.IMAGE
//
// The HLS test comes first because two of the four playlist MIMEs this repo recognises are spelled
// `audio/x-mpegurl` and `audio/mpegurl` — legacy aliases naming the *manifest* format, not audio
// content. Reading those as an audio track drops a master out of the ladder, and drops the whole
// event to its poster when every entry is labelled that way.
private fun VideoMeta.canBeTheVideo(): Boolean = (isHlsPlaylist() || !isAudio) && !isPoster()
// Mirrors MediaItemCache.toExoPlayerMimeType: a declared HLS MIME is authoritative, and the
// `.m3u8` fallback is anchored to the path so `video.mp4?ref=a.m3u8` is not mistaken for a
@@ -93,10 +98,19 @@ private fun VideoMeta.canBeTheVideo(): Boolean = !isAudio && RichTextParser.clas
// extension, so only its MIME identifies it.
private fun VideoMeta.isHlsPlaylist(): Boolean {
if (RichTextParser.isHlsMimeType(mimeType)) return true
if (mimeType != null) return false
// A declared media type is authoritative, but `application/octet-stream` and friends declare
// nothing — a server default, not a claim about the file — so the extension still gets a say.
// Without this a `master.m3u8` served as octet-stream loses to a correctly labelled 360p rung.
val declared = mimeType
if (declared != null && !declared.isUninformativeMimeType()) return false
return url.substringBefore('?').substringBefore('#').endsWith(".m3u8", ignoreCase = true)
}
private fun String.isUninformativeMimeType(): Boolean =
isBlank() ||
equals("application/octet-stream", ignoreCase = true) ||
equals("binary/octet-stream", ignoreCase = true)
private fun VideoMeta.isPoster(): Boolean = RichTextParser.classifyMedia(url, mimeType) == MediaContentKind.IMAGE
private fun VideoMeta.pixelCount(): Long {
@@ -110,7 +124,9 @@ private fun VideoMeta.withLadderMetadataFrom(ladder: List<VideoMeta>): VideoMeta
if (dimension != null && blurhash != null && thumbhash != null && image.isNotEmpty() && alt != null) return this
return copy(
dimension = dimension ?: ladder.firstNotNullOfOrNull { it.dimension },
// Deliberately not from the poster: a 16:9 thumbnail on a vertical short would size the
// player's box at 16:9. Only entries that could be the video describe its shape.
dimension = dimension ?: ladder.firstOrNull { it.canBeTheVideo() && it.dimension != null }?.dimension,
blurhash = blurhash ?: ladder.firstNotNullOfOrNull { it.blurhash },
thumbhash = thumbhash ?: ladder.firstNotNullOfOrNull { it.thumbhash },
// An `image/*` sibling carries the poster as its own url, not in its `image` list, so fall
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/nip71Video/VideoTrackSelectionTest.kt
+52
View File
@@ -194,6 +194,58 @@ class VideoTrackSelectionTest {
assertEquals("https://host/video.mp4?ref=a.m3u8", selected?.url)
}
@Test
fun treatsTheLegacyAudioMpegurlAliasAsAPlaylistNotAnAudioTrack() {
// Audit finding: `audio/x-mpegurl` and `audio/mpegurl` are two of the four HLS playlist
// MIMEs this repo recognises — legacy aliases for the manifest format, not a claim that the
// content is audio. Reading them as an audio track dropped the master out of the ladder.
val master = VideoMeta(url = "https://host/master.m3u8", mimeType = "audio/mpegurl", dimension = DimensionTag(1080, 1920))
val rung = rendition("360", 360, 640)
assertEquals("https://host/master.m3u8", event(master, rung).selectVideoTrack()?.url)
}
@Test
fun stillRendersWhenEveryPlaylistUsesTheAudioAlias() {
// Same finding, worse case: with every entry labelled that way the candidate list emptied
// and selection fell back to imetas.first() — the poster JPEG, handed to the video player.
val poster = VideoMeta(url = "https://host/poster.jpg", mimeType = "image/jpeg")
val master = VideoMeta(url = "https://host/master.m3u8", mimeType = "audio/x-mpegurl", dimension = DimensionTag(1080, 1920))
val rung = VideoMeta(url = "https://host/360.m3u8", mimeType = "audio/x-mpegurl", dimension = DimensionTag(360, 640))
assertEquals("https://host/master.m3u8", event(poster, master, rung).selectVideoTrack()?.url)
}
@Test
fun identifiesAPlaylistServedAsOctetStream() {
// A server default is not a claim about the file, so the .m3u8 path still decides. Without
// this the master lost to a correctly labelled low rung.
val master =
VideoMeta(
url = "https://host/master.m3u8",
mimeType = "application/octet-stream",
dimension = DimensionTag(1080, 1920),
)
val rung = rendition("360", 360, 640)
assertEquals("https://host/master.m3u8", event(master, rung).selectVideoTrack()?.url)
}
@Test
fun neverTakesTheAspectRatioFromThePoster() {
// A 16:9 thumbnail alongside a vertical short would otherwise size the player's box at 16:9,
// since JustVideoDisplay lays out from the selected entry's dim.
val poster = VideoMeta(url = "https://host/poster.jpg", mimeType = "image/jpeg", dimension = DimensionTag(1600, 900))
val master = VideoMeta(url = "https://host/master.m3u8", mimeType = hls)
val rung = rendition("1080", 1080, 1920)
val selected = event(poster, master, rung).selectVideoTrack()
assertEquals(1080, selected?.dimension?.width)
assertEquals(1920, selected?.dimension?.height)
// The poster itself is still picked up as the still image.
assertEquals(listOf("https://host/poster.jpg"), selected?.image)
}
@Test
fun skipsTheSeparateAudioTrack() {
// NIP-71 PR #2255 splits audio into its own imeta so resolution can change without