From f8f366a513954dd5072aabc61be7e5574053628b Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:16:24 +0200 Subject: [PATCH] fix(native): harden player teardown and shelf recovery --- .../plezy/exoplayer/ExoPlayerPlugin.kt | 20 +- .../com/edde746/plezy/mpv/MpvPlayerCore.kt | 102 ++- .../watchnext/SystemShelfArtworkProvider.kt | 363 +++++++-- .../plezy/watchnext/WatchNextProvider.kt | 241 ++++-- .../ExoPlayerFallbackTerminalTest.kt | 3 +- .../plezy/exoplayer/ExoPlayerPluginTest.kt | 120 +++ .../edde746/plezy/mpv/MpvPlayerPluginTest.kt | 223 ++++++ .../plezy/watchnext/WatchNextProviderTest.kt | 748 +++++++++++++++++- ios/RunnerTests/RunnerTests.swift | 24 +- linux/runner/mpv/mpv_player.cc | 2 +- linux/runner/mpv/mpv_player_lifecycle_test.cc | 2 +- linux/runner/mpv/mpv_plugin.cc | 9 +- macos/RunnerTests/RunnerTests.swift | 24 +- .../apple/MpvPlayer/MpvPlayerCoreBase.swift | 22 +- .../MpvPlayer/MpvPlayerPluginShared.swift | 10 +- shared/mpv/mpv_player_common.h | 4 + shared/mpv/mpv_player_common_test.cpp | 19 +- tvos/Runner.xcodeproj/project.pbxproj | 4 + tvos/Runner/SystemShelfPlugin.swift | 11 +- tvos/RunnerTests/MpvPlayerContractTests.swift | 78 ++ windows/runner/mpv/display_mode_manager.cpp | 478 +++++++++-- windows/runner/mpv/display_mode_manager.h | 16 +- .../runner/mpv/display_mode_manager_test.cpp | 662 +++++++++++++++- windows/runner/mpv/mpv_player.cpp | 2 +- .../mpv/mpv_player_property_contract_test.cpp | 2 +- windows/runner/mpv/mpv_plugin.cpp | 8 +- 26 files changed, 2895 insertions(+), 302 deletions(-) create mode 100644 tvos/RunnerTests/MpvPlayerContractTests.swift diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt index 0fe6cb2c..bfbd58f4 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt @@ -18,6 +18,7 @@ import io.flutter.embedding.engine.plugins.activity.ActivityPluginBinding import io.flutter.plugin.common.EventChannel import io.flutter.plugin.common.MethodCall import io.flutter.plugin.common.MethodChannel +import java.util.concurrent.CancellationException import java.util.concurrent.atomic.AtomicBoolean class ExoPlayerPlugin : @@ -774,16 +775,17 @@ class ExoPlayerPlugin : } pendingMpvProperties[name] = value core.setProperty(name, value) { outcome -> - val currentOutcome = if ( + val fallbackIsCurrent = usingMpvFallback && - generation == sessionGeneration && - activity === currentActivity && - mpvCore === core - ) { - outcome - } else { - Result.failure(IllegalStateException("MPV fallback unavailable")) - } + generation == sessionGeneration && + activity === currentActivity && + mpvCore === core + val currentOutcome = + if (fallbackIsCurrent || outcome.isFailure) { + outcome + } else { + Result.failure(CancellationException("MPV fallback unavailable")) + } completeMpvPropertyResult(result, currentOutcome, successValue) } } diff --git a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt index c81a093a..d21589a6 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt @@ -700,8 +700,15 @@ class MpvPlayerCore private constructor( } private fun resumeAfterAudioFocusGain(reason: String) { - pausedForAudioFocusLoss = false - requestAutoResume(reason) + val shouldResume = synchronized(publicPauseIntentLock) { + if (!pausedForAudioFocusLoss) { + false + } else { + pausedForAudioFocusLoss = false + true + } + } + if (shouldResume) requestAutoResume(reason) } private fun rollbackFailedPublicPauseIntent(intent: PublicPauseIntent) { @@ -713,6 +720,22 @@ class MpvPlayerCore private constructor( } } + private fun completeFailedPublicResume( + intent: PublicPauseIntent, + failure: Throwable, + onComplete: ((Result) -> Unit)? + ) { + rollbackFailedPublicPauseIntent(intent) + runOnMain { + val completion: Result = if (disposing || !isInitialized || !scope.isActive) { + Result.failure(CancellationException("MPV core unavailable")) + } else { + Result.failure(failure) + } + onComplete?.invoke(completion) + } + } + private fun requestAutoResume(reason: String) { val p = player if (p == null && propertyWriterOverride == null) return @@ -847,44 +870,89 @@ class MpvPlayerCore private constructor( } } - if (paused == false && (pausedForAudioFocusLoss || !hasReadyVideoOutput())) { + if (paused == false && pauseIntent != null) { + val shouldReclaimAudioFocus = synchronized(publicPauseIntentLock) { + publicPauseIntentGeneration == pauseIntent.generation && pausedForAudioFocusLoss + } + if (shouldReclaimAudioFocus) { + val focusGranted = audioFocusManager?.requestAudioFocus() == true + if (!focusGranted) { + Log.w(TAG, "Audio focus request denied for public resume") + completeFailedPublicResume( + pauseIntent, + IllegalStateException("Audio focus unavailable for resume"), + onComplete + ) + return + } + + synchronized(publicPauseIntentLock) { + if (publicPauseIntentGeneration == pauseIntent.generation && pausedForAudioFocusLoss) { + pausedForAudioFocusLoss = false + } + } + } + } + + if (paused == false && pauseIntent != null && !hasReadyVideoOutput()) { runOnMain { if (!isInitialized || disposing || !scope.isActive) { onComplete?.invoke(Result.failure(CancellationException("MPV core unavailable"))) return@runOnMain } - val isCurrent = synchronized(publicPauseIntentLock) { - pauseIntent != null && publicPauseIntentGeneration == pauseIntent.generation + var deferredForSurface = false + val interruptedAgain = synchronized(publicPauseIntentLock) { + if (publicPauseIntentGeneration != pauseIntent.generation) { + false + } else if (pausedForAudioFocusLoss) { + true + } else { + deferredResumeRequested = true + deferredForSurface = true + false + } } - if (isCurrent) { - deferredResumeRequested = true - Log.d( - TAG, - if (pausedForAudioFocusLoss) { - "Deferring public resume until audio focus returns" - } else { - "Deferring public resume until video output is ready" - } + if (interruptedAgain) { + Log.d(TAG, "Public resume interrupted by a newer audio-focus loss") + completeFailedPublicResume( + pauseIntent, + IllegalStateException("Audio focus unavailable for resume"), + onComplete ) + } else { + if (deferredForSurface) { + Log.d(TAG, "Deferring public resume until video output is ready") + } + onComplete?.invoke(Result.success(Unit)) } - onComplete?.invoke(Result.success(Unit)) } return } scope.launch(mpvWriteDispatcher, start = CoroutineStart.ATOMIC) { + var interruptedBeforeWrite = false val writeResult = try { if (pauseIntent == null) { writeProperty(name, value) } else { publicPauseWriteMutex.withLock { val shouldWrite = synchronized(publicPauseIntentLock) { - publicPauseIntentGeneration == pauseIntent.generation + val isCurrent = publicPauseIntentGeneration == pauseIntent.generation + if (isCurrent && paused == false && pausedForAudioFocusLoss) { + interruptedBeforeWrite = true + false + } else { + isCurrent + } } if (shouldWrite) writeProperty(name, value) } } - Result.success(Unit) + if (interruptedBeforeWrite) { + Result.failure(IllegalStateException("Audio focus unavailable for resume")) + } else { + Result.success(Unit) + } } catch (error: CancellationException) { Result.failure(error) } catch (error: Exception) { diff --git a/android/app/src/main/kotlin/com/edde746/plezy/watchnext/SystemShelfArtworkProvider.kt b/android/app/src/main/kotlin/com/edde746/plezy/watchnext/SystemShelfArtworkProvider.kt index 8068b160..808f6298 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/watchnext/SystemShelfArtworkProvider.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/watchnext/SystemShelfArtworkProvider.kt @@ -8,13 +8,14 @@ import android.net.Uri import android.os.Binder import android.os.ParcelFileDescriptor import android.os.Process +import android.system.Os import java.io.ByteArrayOutputStream import java.io.File import java.io.FileNotFoundException +import java.io.RandomAccessFile import java.net.HttpURLConnection import java.net.URL import java.security.MessageDigest -import java.util.UUID import java.util.concurrent.Executors import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.TimeUnit @@ -88,14 +89,28 @@ internal class SystemShelfArtworkStore(private val cacheDir: File) { const val CONNECT_TIMEOUT_MS = 2_500 const val READ_TIMEOUT_MS = 2_500 private val opaquePart = Regex("^[a-f0-9]{64}$") - private val artworkKey = Regex("^[a-f0-9]{32}\\.art$") + private val artworkKey = Regex("^(?:[a-f0-9]{32}|[a-f0-9]{64})\\.art$") private val deadlineAborter: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor { runnable -> Thread(runnable, "system-shelf-deadline").apply { isDaemon = true } } } - data class Materialized(val key: String, val uri: Uri, val file: File) + data class Materialized(val uri: Uri, val file: File) + + sealed class Prepared { + abstract val materialized: Materialized + + data class Existing(override val materialized: Materialized) : Prepared() + + data class Staged( + override val materialized: Materialized, + val stagingFile: File + ) : Prepared() + } + + data class Publication(val materialized: Materialized, val newlyPublished: Boolean) + class Budget(var remaining: Int = MAX_SYNC_BYTES) { var consumed: Long = 0 private set @@ -114,20 +129,37 @@ internal class SystemShelfArtworkStore(private val cacheDir: File) { private val root: File get() = File(cacheDir, "system_shelf_artwork") - fun materialize(ownerId: String, source: String, session: SystemShelfSyncSession): Materialized? { - if (ownerId.isBlank() || session.budget.remaining <= 0 || !session.isActive()) return null + fun prepare(ownerId: String, source: String, session: SystemShelfSyncSession): Prepared? { + if (ownerId.isBlank()) return null val url = runCatching { URL(source) }.getOrNull() ?: return null if (url.protocol != "https" && url.protocol != "http") return null - val connection = (url.openConnection() as? HttpURLConnection) ?: return null + if (!session.isActive()) return null + + val ownerKey = sha256(ownerId) + val key = sha256(ownerId + "\u0000" + source) + ".art" + val uri = contentUri(ownerKey, key) + val destination = File(File(root, ownerKey), key) + val materialized = Materialized(uri, destination) + resolveOwned(ownerId, uri)?.let { existing -> + return Prepared.Existing(materialized.copy(file = existing)) + } + if (session.budget.remaining <= 0) return null + val remainingNanos = session.remainingNanos() if (remainingNanos <= 0) return null + val connection = runCatching { url.openConnection() as? HttpURLConnection }.getOrNull() + ?: return null val abort = deadlineAborter.schedule( { connection.disconnect() }, remainingNanos, TimeUnit.NANOSECONDS ) + var stagingFile: File? = null + var retainStagingFile = false return try { - val remainingMillis = TimeUnit.NANOSECONDS.toMillis(remainingNanos).coerceIn(1, Int.MAX_VALUE.toLong()).toInt() + val remainingMillis = TimeUnit.NANOSECONDS.toMillis(remainingNanos) + .coerceIn(1, Int.MAX_VALUE.toLong()) + .toInt() connection.instanceFollowRedirects = true connection.connectTimeout = minOf(CONNECT_TIMEOUT_MS, remainingMillis) connection.readTimeout = minOf(READ_TIMEOUT_MS, remainingMillis) @@ -155,41 +187,73 @@ internal class SystemShelfArtworkStore(private val cacheDir: File) { } val read = input.read(buffer, 0, minOf(buffer.size, remaining)) if (read < 0) break - session.budget.charge(read) + if (!session.budget.charge(read)) return null total += read output.write(buffer, 0, read) } output.toByteArray() } if (!session.isActive() || !isSupportedImage(bytes)) return null - val ownerKey = sha256(ownerId) - val directory = File(root, ownerKey) + + val directory = destination.parentFile ?: return null if (!directory.mkdirs() && !directory.isDirectory) return null - val key = UUID.randomUUID().toString().replace("-", "") + ".art" - val staged = File(directory, ".$key.tmp") + val staged = File.createTempFile(".$key.", ".tmp", directory) + stagingFile = staged staged.outputStream().use { output -> output.write(bytes) output.flush() output.fd.sync() } - if (!session.isActive()) { - staged.delete() - return null - } - val destination = File(directory, key) - if (!session.commitIfActive { staged.renameTo(destination) }) { - staged.delete() - return null - } - Materialized(key, contentUri(ownerKey, key), destination) + if (!isSupportedImage(staged) || !session.isActive()) return null + retainStagingFile = true + Prepared.Staged(materialized, staged) } catch (_: Exception) { null } finally { + if (!retainStagingFile) stagingFile?.delete() abort.cancel(false) connection.disconnect() } } + fun publish(prepared: Prepared): Publication? { + val expected = confinedCandidate(prepared.materialized.uri) ?: return null + val recorded = runCatching { prepared.materialized.file.canonicalFile }.getOrNull() ?: return null + if (recorded != expected) return null + + resolve(prepared.materialized.uri)?.let { existing -> + if (prepared is Prepared.Staged && !deleteStagingFile(prepared.stagingFile, expected)) { + return null + } + return Publication(prepared.materialized.copy(file = existing), newlyPublished = false) + } + if (prepared !is Prepared.Staged) return null + val staged = confinedStagingFile(prepared.stagingFile, expected) ?: return null + if (!isSupportedImage(staged)) return null + + val renamed = try { + Os.rename(staged.absolutePath, expected.absolutePath) + !staged.exists() || staged.renameTo(expected) + } catch (_: Exception) { + runCatching { staged.renameTo(expected) }.getOrDefault(false) + } + if (!renamed) return null + return if (!isSupportedImage(expected)) { + expected.delete() + null + } else { + Publication(prepared.materialized.copy(file = expected), newlyPublished = true) + } + } + + fun discard(prepared: Iterable) { + prepared.forEach { candidate -> + if (candidate is Prepared.Staged) { + deleteStagingFile(candidate.stagingFile, candidate.materialized.file) + } + } + } + fun contentUri(ownerKey: String, key: String): Uri = Uri.Builder() .scheme("content") .authority(SystemShelfArtworkProvider.AUTHORITY) @@ -199,60 +263,257 @@ internal class SystemShelfArtworkStore(private val cacheDir: File) { .build() fun resolve(uri: Uri): File? { + val candidate = confinedCandidate(uri) ?: return null + return candidate.takeIf(::isSupportedImage) + } + + fun resolveOwned(ownerId: String, uri: Uri): File? { + if (ownerId.isBlank() || uri.pathSegments.getOrNull(1) != sha256(ownerId)) return null + return resolve(uri) + } + + fun deleteExcept(keep: Set) { + val canonicalKeep = keep.mapNotNullTo(HashSet()) { + runCatching { it.canonicalFile }.getOrNull() + } + val canonicalRoot = runCatching { root.canonicalFile }.getOrNull() ?: return + for (ownerDirectory in root.listFiles().orEmpty()) { + for (file in ownerDirectory.listFiles().orEmpty()) { + val candidate = confinedCacheFile(file, canonicalRoot) ?: continue + if (candidate !in canonicalKeep) candidate.delete() + } + } + removeEmptyDirectories() + } + + fun delete(files: Set) { + val canonicalRoot = runCatching { root.canonicalFile }.getOrNull() ?: return + files.forEach { file -> + confinedCacheFile(file, canonicalRoot)?.delete() + } + removeEmptyDirectories() + } + + fun deleteAll(): Boolean = !root.exists() || root.deleteRecursively() + + private fun confinedCandidate(uri: Uri): File? { if (uri.scheme != "content" || uri.authority != SystemShelfArtworkProvider.AUTHORITY) return null val segments = uri.pathSegments if (segments.size != 3 || segments[0] != "art") return null val owner = segments[1] val key = segments[2] if (!opaquePart.matches(owner) || !artworkKey.matches(key)) return null - val canonicalRoot = root.canonicalFile - val candidate = File(File(canonicalRoot, owner), key).canonicalFile - if (candidate.parentFile?.parentFile != canonicalRoot || !candidate.isFile) return null - return candidate + + return runCatching { + val canonicalRoot = root.canonicalFile + val ownerDirectory = File(canonicalRoot, owner).canonicalFile + if (ownerDirectory.parentFile != canonicalRoot || ownerDirectory.name != owner) return null + val candidate = File(ownerDirectory, key).canonicalFile + if (candidate.parentFile != ownerDirectory || candidate.name != key) return null + candidate + }.getOrNull() } - fun deleteExcept(keep: Set) { - val canonicalKeep = keep.mapTo(HashSet()) { it.canonicalFile } - root.listFiles()?.forEach { ownerDirectory -> - ownerDirectory.listFiles()?.forEach { file -> - if (file.canonicalFile !in canonicalKeep) file.delete() + private fun confinedCacheFile(file: File, canonicalRoot: File): File? { + return runCatching { + val originalDirectory = file.parentFile ?: return null + val directory = originalDirectory.canonicalFile + if ( + directory.parentFile != canonicalRoot || + directory.name != originalDirectory.name + ) { + return null } - if (ownerDirectory.listFiles().isNullOrEmpty()) ownerDirectory.delete() - } + val candidate = file.canonicalFile + if (candidate.parentFile != directory || candidate.name != file.name) return null + candidate + }.getOrNull() } - fun delete(files: Set) { - val canonicalRoot = root.canonicalFile - files.forEach { file -> - val candidate = runCatching { file.canonicalFile }.getOrNull() ?: return@forEach - if (candidate.parentFile?.parentFile == canonicalRoot) candidate.delete() + private fun confinedStagingFile(stagingFile: File, destination: File): File? { + val staged = runCatching { stagingFile.canonicalFile }.getOrNull() ?: return null + val canonicalDestination = runCatching { destination.canonicalFile }.getOrNull() ?: return null + val directory = canonicalDestination.parentFile ?: return null + if ( + staged == canonicalDestination || + staged.parentFile != directory || + !staged.name.startsWith(".${canonicalDestination.name}.") || + !staged.name.endsWith(".tmp") || + !staged.isFile + ) { + return null } + return staged + } + + private fun deleteStagingFile(stagingFile: File, destination: File): Boolean { + if (!stagingFile.exists()) return true + return confinedStagingFile(stagingFile, destination)?.delete() == true + } + + private fun removeEmptyDirectories() { root.listFiles()?.forEach { directory -> if (directory.listFiles().isNullOrEmpty()) directory.delete() } } - fun deleteAll(): Boolean = !root.exists() || root.deleteRecursively() + private enum class ImageFormat { + PNG, + JPEG, + GIF, + WEBP + } + + private fun isSupportedImage(file: File): Boolean { + val length = file.length() + if (!file.isFile || length !in 1L..MAX_IMAGE_BYTES.toLong()) return false + val header = ByteArray(12) + val headerSize = runCatching { + file.inputStream().use { input -> + var total = 0 + while (total < header.size) { + val read = input.read(header, total, header.size - total) + if (read <= 0) break + total += read + } + total + } + }.getOrNull() ?: return false + val format = imageFormat(header, headerSize) ?: return false + if (!hasCompleteContainer(file, format, header, length)) return false + + val options = BitmapFactory.Options().apply { inJustDecodeBounds = true } + return runCatching { + BitmapFactory.decodeFile(file.absolutePath, options) + hasSupportedDimensions(options) + }.getOrDefault(false) + } private fun isSupportedImage(bytes: ByteArray): Boolean { - if (bytes.size < 4) return false - val png = bytes.size >= 8 && + val format = imageFormat(bytes, bytes.size) ?: return false + if (!hasCompleteContainer(bytes, format)) return false + val options = BitmapFactory.Options().apply { inJustDecodeBounds = true } + return runCatching { + BitmapFactory.decodeByteArray(bytes, 0, bytes.size, options) + hasSupportedDimensions(options) + }.getOrDefault(false) + } + + private fun imageFormat(bytes: ByteArray, size: Int): ImageFormat? { + if ( + size >= 8 && bytes[0] == 0x89.toByte() && bytes[1] == 0x50.toByte() && bytes[2] == 0x4e.toByte() && - bytes[3] == 0x47.toByte() - val jpeg = bytes[0] == 0xff.toByte() && bytes[1] == 0xd8.toByte() && bytes[2] == 0xff.toByte() - val gif = bytes[0] == 0x47.toByte() && bytes[1] == 0x49.toByte() && bytes[2] == 0x46.toByte() - val webp = bytes.size >= 12 && - bytes.copyOfRange(0, 4).contentEquals("RIFF".toByteArray()) && - bytes.copyOfRange(8, 12).contentEquals("WEBP".toByteArray()) - if (!png && !jpeg && !gif && !webp) return false + bytes[3] == 0x47.toByte() && + bytes[4] == 0x0d.toByte() && + bytes[5] == 0x0a.toByte() && + bytes[6] == 0x1a.toByte() && + bytes[7] == 0x0a.toByte() + ) { + return ImageFormat.PNG + } + if ( + size >= 3 && + bytes[0] == 0xff.toByte() && + bytes[1] == 0xd8.toByte() && + bytes[2] == 0xff.toByte() + ) { + return ImageFormat.JPEG + } + if ( + size >= 6 && + bytes[0] == 0x47.toByte() && + bytes[1] == 0x49.toByte() && + bytes[2] == 0x46.toByte() && + bytes[3] == 0x38.toByte() && + (bytes[4] == 0x37.toByte() || bytes[4] == 0x39.toByte()) && + bytes[5] == 0x61.toByte() + ) { + return ImageFormat.GIF + } + if ( + size >= 12 && + bytes[0] == 0x52.toByte() && + bytes[1] == 0x49.toByte() && + bytes[2] == 0x46.toByte() && + bytes[3] == 0x46.toByte() && + bytes[8] == 0x57.toByte() && + bytes[9] == 0x45.toByte() && + bytes[10] == 0x42.toByte() && + bytes[11] == 0x50.toByte() + ) { + return ImageFormat.WEBP + } + return null + } - val options = BitmapFactory.Options().apply { inJustDecodeBounds = true } - BitmapFactory.decodeByteArray(bytes, 0, bytes.size, options) + private fun hasCompleteContainer( + file: File, + format: ImageFormat, + header: ByteArray, + length: Long + ): Boolean = runCatching { + if (format == ImageFormat.WEBP) return@runCatching webpLength(header) == length + RandomAccessFile(file, "r").use { input -> + when (format) { + ImageFormat.PNG -> { + if (length < 12) return@use false + input.seek(length - 12) + input.readInt() == 0 && + input.readInt() == 0x49454e44 && + input.readInt() == 0xae426082.toInt() + } + ImageFormat.JPEG -> { + if (length < 2) return@use false + input.seek(length - 2) + input.readUnsignedByte() == 0xff && input.readUnsignedByte() == 0xd9 + } + ImageFormat.GIF -> { + input.seek(length - 1) + input.readUnsignedByte() == 0x3b + } + ImageFormat.WEBP -> false + } + } + }.getOrDefault(false) + + private fun hasCompleteContainer(bytes: ByteArray, format: ImageFormat): Boolean { + val size = bytes.size + return when (format) { + ImageFormat.PNG -> + size >= 12 && + bytes[size - 8] == 0x49.toByte() && + bytes[size - 7] == 0x45.toByte() && + bytes[size - 6] == 0x4e.toByte() && + bytes[size - 5] == 0x44.toByte() && + bytes[size - 4] == 0xae.toByte() && + bytes[size - 3] == 0x42.toByte() && + bytes[size - 2] == 0x60.toByte() && + bytes[size - 1] == 0x82.toByte() + ImageFormat.JPEG -> + size >= 2 && + bytes[size - 2] == 0xff.toByte() && + bytes[size - 1] == 0xd9.toByte() + ImageFormat.GIF -> size >= 1 && bytes[size - 1] == 0x3b.toByte() + ImageFormat.WEBP -> webpLength(bytes) == size.toLong() + } + } + + private fun webpLength(header: ByteArray): Long { + val riffSize = (header[4].toLong() and 0xff) or + ((header[5].toLong() and 0xff) shl 8) or + ((header[6].toLong() and 0xff) shl 16) or + ((header[7].toLong() and 0xff) shl 24) + return riffSize + 8 + } + + private fun hasSupportedDimensions(options: BitmapFactory.Options): Boolean { val width = options.outWidth val height = options.outHeight - return width in 1..4096 && height in 1..4096 && width.toLong() * height <= 16_777_216L + return width in 1..4096 && + height in 1..4096 && + width.toLong() * height <= 16_777_216L } private fun sha256(value: String): String = MessageDigest.getInstance("SHA-256") diff --git a/android/app/src/main/kotlin/com/edde746/plezy/watchnext/WatchNextProvider.kt b/android/app/src/main/kotlin/com/edde746/plezy/watchnext/WatchNextProvider.kt index 2ebd4120..fbdf0a1b 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/watchnext/WatchNextProvider.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/watchnext/WatchNextProvider.kt @@ -5,6 +5,7 @@ import android.content.ContentUris import android.content.Context import android.content.Intent import android.content.pm.PackageManager +import android.database.Cursor import android.net.Uri import android.os.Build import android.util.Log @@ -118,6 +119,12 @@ class WatchNextProvider internal constructor( internal data class PreparedWatchNextItem(val metadata: WatchNextItem, val localPosterUri: Uri?) + private data class CommittedRow( + val id: Long, + val contentId: String, + val posterUri: Uri? + ) + private val prefs = context.getSharedPreferences(PREFS, Context.MODE_PRIVATE) private val artwork = SystemShelfArtworkStore(context.cacheDir) @@ -133,34 +140,71 @@ class WatchNextProvider internal constructor( if (items.size > SystemShelfArtworkStore.MAX_ITEMS) return false val operationOwnership = ownership ?: claimOwnership(ownerId, generation) ?: return false if (!isOperationActive()) return false - - val oldUris = prefs.getStringSet(GRANTED_URIS, emptySet()).orEmpty().mapNotNull(Uri::parse).toSet() - val oldPackages = storedPackages() - val oldSchemaVersion = prefs.getInt(SHELF_SCHEMA_VERSION_KEY, 0) val session = SystemShelfSyncSession( operationOwnership, syncDurationMillis ) - val materializedFiles = LinkedHashSet() - var committed = false + + val previousArtwork = snapshotCommittedArtwork(ownerId) ?: return false + if (!isOperationActive() || !session.isActive()) return false + val oldUris = prefs.getStringSet(GRANTED_URIS, emptySet()).orEmpty().mapNotNull(Uri::parse).toSet() + val oldPackages = storedPackages() + val oldSchemaVersion = prefs.getInt(SHELF_SCHEMA_VERSION_KEY, 0) + val preparedArtwork = LinkedHashSet() + val preparedBySource = HashMap() try { - val prepared = items.map { item -> - if (!isOperationActive() || !session.isActive()) { - PreparedWatchNextItem(item, null) + val preparedItems = items.map { item -> + val source = item.posterSourceUri + val localArtwork = if (source == null) { + null } else { - val materialized = item.posterSourceUri?.let { artwork.materialize(ownerId, it, session) } - materialized?.file?.let(materializedFiles::add) - PreparedWatchNextItem(item, materialized?.uri) + val candidate = if (preparedBySource.containsKey(source)) { + preparedBySource[source] + } else { + artwork.prepare(ownerId, source, session).also { prepared -> + preparedBySource[source] = prepared + prepared?.let(preparedArtwork::add) + } + } + candidate?.materialized ?: previousArtwork[item.contentId] } + PreparedWatchNextItem(item, localArtwork?.uri) } - val newUris = prepared.mapNotNullTo(LinkedHashSet()) { it.localPosterUri } + val newUris = preparedItems.mapNotNullTo(LinkedHashSet()) { it.localPosterUri } val newPackages = consumerPackages() if (!isOperationActive() || !session.isActive()) return false - committed = SystemShelfLifecycle.whileCurrent(session.ownership) { + + return SystemShelfLifecycle.whileCurrent(session.ownership) { if (!isOperationActive() || session.isExpired()) return@whileCurrent false - reconcileReadAccess(oldUris, oldPackages, newUris, newPackages) + val publishedFiles = LinkedHashSet() + preparedArtwork.forEach { candidate -> + val publication = artwork.publish(candidate) + if (publication == null) { + artwork.delete(publishedFiles) + return@whileCurrent false + } + if (publication.newlyPublished) publishedFiles += publication.materialized.file + } + + val referencedFiles = LinkedHashSet() + preparedItems.forEach { item -> + val posterUri = item.localPosterUri ?: return@forEach + val file = artwork.resolveOwned(ownerId, posterUri) + if (file == null) { + artwork.delete(publishedFiles) + return@whileCurrent false + } + referencedFiles += file + } + if (session.isExpired()) { + artwork.delete(publishedFiles) + return@whileCurrent false + } + + grantReadAccess(newUris, newPackages) if (session.isExpired()) { reconcileReadAccess(newUris, newPackages, oldUris, oldPackages) + artwork.delete(publishedFiles) return@whileCurrent false } val preferencesCommitted = prefs.edit() @@ -170,9 +214,10 @@ class WatchNextProvider internal constructor( .commit() if (!preferencesCommitted) { reconcileReadAccess(newUris, newPackages, oldUris, oldPackages) + artwork.delete(publishedFiles) return@whileCurrent false } - if (session.isExpired() || !replaceRows(prepared)) { + if (session.isExpired() || !replaceRows(preparedItems)) { val rollback = prefs.edit() .putStringSet(GRANTED_URIS, oldUris.mapTo(LinkedHashSet(), Uri::toString)) .putStringSet(GRANTED_PACKAGES, oldPackages) @@ -183,17 +228,16 @@ class WatchNextProvider internal constructor( } rollback.commit() reconcileReadAccess(newUris, newPackages, oldUris, oldPackages) + artwork.delete(publishedFiles) return@whileCurrent false } - artwork.deleteExcept(materializedFiles) + reconcileReadAccess(oldUris, oldPackages, newUris, newPackages) + artwork.deleteExcept(referencedFiles) true } ?: false - return committed } finally { - if (!committed) { - materializedFiles.forEach { it.delete() } - } + artwork.discard(preparedArtwork) } } @@ -256,6 +300,95 @@ class WatchNextProvider internal constructor( .commit() } + private fun snapshotCommittedArtwork( + ownerId: String + ): Map? { + val rows = queryCommittedRows() ?: return null + val snapshot = LinkedHashMap() + rows.forEach { row -> + val uri = row.posterUri ?: return@forEach + val file = artwork.resolveOwned(ownerId, uri) ?: return@forEach + if (row.contentId !in snapshot) { + snapshot[row.contentId] = SystemShelfArtworkStore.Materialized(uri, file) + } + } + return snapshot + } + + private fun queryCommittedRows(): List? { + return try { + val cursor = context.contentResolver.query( + TvContractCompat.WatchNextPrograms.CONTENT_URI, + arrayOf( + TvContractCompat.WatchNextPrograms._ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA, + TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI, + TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI + ), + null, + null, + null + ) ?: return null + cursor.use { + val idIndex = it.getColumnIndex(TvContractCompat.WatchNextPrograms._ID) + val providerIdIndex = it.getColumnIndex( + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID + ) + val providerDataIndex = it.getColumnIndex( + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA + ) + val intentIndex = it.getColumnIndex( + TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI + ) + val posterIndex = it.getColumnIndex(TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI) + if ( + idIndex < 0 || + posterIndex < 0 || + providerIdIndex < 0 && + providerDataIndex < 0 && + intentIndex < 0 + ) { + return null + } + val rows = ArrayList(it.count) + while (it.moveToNext()) { + val providerId = cursorString(it, providerIdIndex) + ?.takeIf(String::isNotBlank) + val providerData = cursorString(it, providerDataIndex) + ?.takeIf(String::isNotBlank) + val contentId = providerId + ?: providerData + ?: contentIdFromIntent(cursorString(it, intentIndex)) + ?: continue + val posterUri = it.getString(posterIndex) + ?.takeIf(String::isNotBlank) + ?.let(Uri::parse) + rows += CommittedRow(it.getLong(idIndex), contentId, posterUri) + } + rows + } + } catch (_: Exception) { + Log.e(TAG, "Failed to query committed Watch Next programs") + null + } + } + + private fun cursorString(cursor: Cursor, index: Int): String? { + if (index < 0 || cursor.isNull(index)) return null + return if (cursor.getType(index) == Cursor.FIELD_TYPE_BLOB) { + cursor.getBlob(index)?.toString(Charsets.UTF_8) + } else { + cursor.getString(index) + } + } + + private fun contentIdFromIntent(value: String?): String? { + val uri = value?.let { runCatching { Uri.parse(it) }.getOrNull() } ?: return null + if (uri.scheme != "plezy" || uri.authority != "play") return null + return uri.getQueryParameter("content_id")?.takeIf(String::isNotBlank) + } + internal fun removeItem( ownerId: String, generation: Long, @@ -266,46 +399,27 @@ class WatchNextProvider internal constructor( val operationOwnership = ownership ?: claimOwnership(ownerId, generation) ?: return false return SystemShelfLifecycle.whileCurrent(operationOwnership) { if (!isOperationActive()) return@whileCurrent false - removeItemOwned(contentId) + removeItemOwned(ownerId, contentId) } ?: false } - private fun removeItemOwned(contentId: String): Boolean { + private fun removeItemOwned(ownerId: String, contentId: String): Boolean { return try { - val cursor = context.contentResolver.query( + val rows = queryCommittedRows() ?: return false + val target = rows.firstOrNull { it.contentId == contentId } ?: return false + val deleteUri = ContentUris.withAppendedId( TvContractCompat.WatchNextPrograms.CONTENT_URI, - arrayOf( - TvContractCompat.WatchNextPrograms._ID, - TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID, - TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI - ), - null, - null, - null + target.id ) - cursor?.use { - val idIndex = it.getColumnIndex(TvContractCompat.WatchNextPrograms._ID) - val providerIdIndex = it.getColumnIndex(TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID) - val posterIndex = it.getColumnIndex(TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI) - if (idIndex < 0 || providerIdIndex < 0) return false - while (it.moveToNext()) { - if (it.getString(providerIdIndex) == contentId) { - val deleteUri = ContentUris.withAppendedId(TvContractCompat.WatchNextPrograms.CONTENT_URI, it.getLong(idIndex)) - context.contentResolver.delete(deleteUri, null, null) - if (posterIndex >= 0) { - val poster = it.getString(posterIndex)?.let(Uri::parse) - if (poster != null) { - revokeReadAccess(setOf(poster)) - artwork.resolve(poster)?.delete() - val remaining = prefs.getStringSet(GRANTED_URIS, emptySet()).orEmpty() - poster.toString() - prefs.edit().putStringSet(GRANTED_URIS, remaining).commit() - } - } - return true - } - } - } - false + if (context.contentResolver.delete(deleteUri, null, null) <= 0) return false + + val poster = target.posterUri ?: return true + if (rows.any { it.id != target.id && it.posterUri == poster }) return true + revokeReadAccess(setOf(poster)) + val remaining = prefs.getStringSet(GRANTED_URIS, emptySet()).orEmpty() - poster.toString() + prefs.edit().putStringSet(GRANTED_URIS, remaining).commit() + artwork.resolveOwned(ownerId, poster)?.let { artwork.delete(setOf(it)) } + true } catch (_: Exception) { Log.e(TAG, "Failed to remove Watch Next item") false @@ -316,8 +430,14 @@ class WatchNextProvider internal constructor( val operations = ArrayList(items.size + 1) operations += ContentProviderOperation.newDelete(TvContractCompat.WatchNextPrograms.CONTENT_URI).build() items.forEach { item -> + val values = buildProgram(item).toContentValues().apply { + put( + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA, + item.metadata.contentId.toByteArray(Charsets.UTF_8) + ) + } operations += ContentProviderOperation.newInsert(TvContractCompat.WatchNextPrograms.CONTENT_URI) - .withValues(buildProgram(item).toContentValues()) + .withValues(values) .build() } context.contentResolver.applyBatch(TvContractCompat.AUTHORITY, operations) @@ -366,8 +486,13 @@ class WatchNextProvider internal constructor( } } } - currentPackages.forEach { packageName -> - currentUris.forEach { uri -> + grantReadAccess(currentUris, currentPackages) + } + + private fun grantReadAccess(uris: Set, packages: Set) { + val flags = Intent.FLAG_GRANT_READ_URI_PERMISSION + packages.forEach { packageName -> + uris.forEach { uri -> runCatching { context.grantUriPermission(packageName, uri, flags) } } } diff --git a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerFallbackTerminalTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerFallbackTerminalTest.kt index 0d3b4684..b285e012 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerFallbackTerminalTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerFallbackTerminalTest.kt @@ -191,8 +191,7 @@ class ExoPlayerFallbackTerminalTest { } } - private fun getField(target: Any, name: String): Any? = - target.javaClass.getDeclaredField(name).apply { isAccessible = true }.get(target) + private fun getField(target: Any, name: String): Any? = target.javaClass.getDeclaredField(name).apply { isAccessible = true }.get(target) private fun requestFallback(core: ExoPlayerCore, mediaGeneration: Int): Boolean { val method = ExoPlayerCore::class.java.getDeclaredMethod( diff --git a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPluginTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPluginTest.kt index 03e00847..0c2347eb 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPluginTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPluginTest.kt @@ -1,11 +1,14 @@ package com.edde746.plezy.exoplayer import android.app.Activity +import android.media.AudioManager +import android.os.Handler import android.os.Looper import android.view.ViewGroup import android.view.ViewTreeObserver import android.widget.FrameLayout import com.edde746.plezy.mpv.MpvPlayerCore +import com.edde746.plezy.shared.AudioFocusManager import io.flutter.embedding.engine.plugins.FlutterPlugin import io.flutter.plugin.common.EventChannel import io.flutter.plugin.common.MethodCall @@ -111,6 +114,54 @@ class ExoPlayerPluginTest { assertEquals(null, result.successValue) } + @Test + fun fallbackPropertyCancelledByTeardownReturnsNotInitializedOnce() { + val writerEntered = CountDownLatch(1) + val releaseWriter = CountDownLatch(1) + val plugin = fallbackPlugin { _, _ -> + writerEntered.countDown() + releaseWriter.await(2, TimeUnit.SECONDS) + } + val property = RecordingResult() + + plugin.onMethodCall(MethodCall("setRate", mapOf("rate" to 1.5)), property) + assertTrue("fallback property writer never started", writerEntered.await(2, TimeUnit.SECONDS)) + + val dispose = RecordingResult() + plugin.onMethodCall(MethodCall("dispose", null), dispose) + awaitCompletion(dispose) + releaseWriter.countDown() + + awaitCompletion(property) + assertEquals(1, property.completionCount) + assertEquals("NOT_INITIALIZED", property.errorCode) + assertEquals(null, property.successValue) + } + + @Test + fun staleSuccessfulFallbackPropertyReturnsNotInitializedOnce() { + val writerEntered = CountDownLatch(1) + val releaseWriter = CountDownLatch(1) + val plugin = fallbackPlugin { _, _ -> + writerEntered.countDown() + releaseWriter.await(2, TimeUnit.SECONDS) + } + val core = getField(plugin, "mpvCore") as MpvPlayerCore + val result = RecordingResult() + + plugin.onMethodCall(MethodCall("setRate", mapOf("rate" to 1.5)), result) + assertTrue("fallback property writer never started", writerEntered.await(2, TimeUnit.SECONDS)) + + setField(plugin, "usingMpvFallback", false) + releaseWriter.countDown() + + awaitCompletion(result) + assertEquals(1, result.completionCount) + assertEquals("NOT_INITIALIZED", result.errorCode) + assertEquals(null, result.successValue) + core.dispose() + } + @Test fun fallbackWithoutCoreReturnsNotInitializedOnce() { val plugin = ExoPlayerPlugin() @@ -473,6 +524,35 @@ class ExoPlayerPluginTest { assertEquals(listOf("end-file"), sink.eventNames) } + @Test + fun fallbackPlayAfterFocusLossUsesExplicitResumeContract() { + val activity = Robolectric.buildActivity(Activity::class.java).setup().get() + val writes = ConcurrentLinkedQueue>() + val core = MpvPlayerCore(activity, true) { name, value -> writes += name to value } + val focusManager = testAudioFocusManager(activity, core) + core.setPrivateField("audioFocusManager", focusManager) + core.setPrivateField("desiredPaused", false) + core.setPrivateField("cachedPaused", false) + val plugin = reusedFallbackPlugin(activity, core) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS) + assertTrue(awaitQueueEntry(writes, "pause" to "yes")) + assertEquals(true, core.getPrivateField("pausedForAudioFocusLoss")) + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + + val result = RecordingResult() + plugin.onMethodCall(MethodCall("play", null), result) + awaitCompletion(result) + + assertEquals(1, result.completionCount) + assertNull(result.errorCode) + assertEquals(listOf("pause" to "yes", "pause" to "no"), writes.toList()) + assertEquals(1, writes.count { it == "pause" to "no" }) + assertEquals(false, core.getPrivateField("pausedForAudioFocusLoss")) + assertEquals(false, core.getPrivateField("deferredResumeRequested")) + core.dispose() + } + @Test fun initialHeldFallbackSynchronouslyBlocksFocusAndSurfaceResumeWithoutPausePropertyWrite() { val activity = Robolectric.buildActivity(Activity::class.java).setup().get() @@ -790,6 +870,46 @@ class ExoPlayerPluginTest { } } + private fun testAudioFocusManager( + activity: Activity, + core: MpvPlayerCore + ): AudioFocusManager = AudioFocusManager( + context = activity, + handler = Handler(Looper.getMainLooper()), + onPause = { + MpvPlayerCore::class.java.getDeclaredMethod("pauseForAudioFocusLoss").apply { + isAccessible = true + invoke(core) + } + }, + onResume = { + MpvPlayerCore::class.java.getDeclaredMethod( + "resumeAfterAudioFocusGain", + String::class.java + ).apply { + isAccessible = true + invoke(core, "audio focus gain") + } + }, + isPaused = { core.getPrivateField("desiredPaused") as Boolean } + ) + + private fun dispatchAudioFocusChange(manager: AudioFocusManager, focusChange: Int) { + val listener = AudioFocusManager::class.java.getDeclaredField("audioFocusChangeListener").run { + isAccessible = true + get(manager) as AudioManager.OnAudioFocusChangeListener + } + listener.onAudioFocusChange(focusChange) + } + + private fun setNextAudioFocusRequestResponse(manager: AudioFocusManager, response: Int) { + val audioManager = AudioFocusManager::class.java.getDeclaredField("audioManager").run { + isAccessible = true + get(manager) as AudioManager + } + shadowOf(audioManager).setNextFocusRequestResponse(response) + } + private fun invokeAutoResume(core: MpvPlayerCore, reason: String) { MpvPlayerCore::class.java.getDeclaredMethod("requestAutoResume", String::class.java).apply { isAccessible = true diff --git a/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt index 3d167b78..64b2e9b6 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt @@ -287,6 +287,221 @@ class MpvPlayerPluginTest { assertEquals(listOf("pause" to "yes"), writes.toList()) } + @Test + fun activeFocusLossExplicitResumeReacquiresFocusAndWritesOnce() { + val writes = ConcurrentLinkedQueue>() + val focusResumeCallbacks = AtomicInteger() + val completionCount = AtomicInteger() + val core = testCore { name, value -> writes += name to value } + val focusManager = testAudioFocusManager(core, focusResumeCallbacks) + setCoreField(core, "audioFocusManager", focusManager) + setBoolean(core, "desiredPaused", false) + setBoolean(core, "cachedPaused", false) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS) + awaitCondition { writes.count { it == "pause" to "yes" } == 1 } + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + + var outcome: Result? = null + core.setProperty("pause", "no") { + completionCount.incrementAndGet() + outcome = it + } + awaitCondition { outcome != null } + + assertTrue(outcome?.isSuccess == true) + assertEquals(1, completionCount.get()) + assertEquals(listOf("pause" to "yes", "pause" to "no"), writes.toList()) + assertEquals(1, writes.count { it == "pause" to "no" }) + assertEquals(false, getBoolean(core, "pausedForAudioFocusLoss")) + assertEquals(false, getBoolean(core, "deferredResumeRequested")) + assertEquals(false, getBoolean(core, "cachedPaused")) + } + + @Test + fun deniedExplicitResumeFocusRequestFailsWithoutWriting() { + val writes = ConcurrentLinkedQueue>() + val focusResumeCallbacks = AtomicInteger() + val core = testCore { name, value -> writes += name to value } + val focusManager = testAudioFocusManager(core, focusResumeCallbacks) + val plugin = MpvAudioPlayerPlugin() + setCoreField(core, "audioFocusManager", focusManager) + installCore(plugin, core) + setBoolean(core, "desiredPaused", false) + setBoolean(core, "cachedPaused", false) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS_TRANSIENT) + awaitCondition { writes.contains("pause" to "yes") } + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + + val coreCompletionCount = AtomicInteger() + var coreOutcome: Result? = null + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_FAILED) + core.setProperty("pause", "no") { + coreCompletionCount.incrementAndGet() + coreOutcome = it + } + awaitCondition { coreOutcome != null } + + assertTrue(coreOutcome?.isFailure == true) + assertFalse(coreOutcome?.exceptionOrNull() is CancellationException) + assertEquals(1, coreCompletionCount.get()) + assertEquals(0, writes.count { it == "pause" to "no" }) + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + assertEquals(true, getBoolean(core, "cachedPaused")) + + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_FAILED) + val denied = RecordingResult() + plugin.onMethodCall( + MethodCall("setProperty", mapOf("name" to "pause", "value" to "no")), + denied + ) + awaitCompletion(denied) + + assertEquals(1, denied.completionCount) + assertEquals("SET_PROPERTY_FAILED", denied.errorCode) + assertEquals(0, writes.count { it == "pause" to "no" }) + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + val retry = RecordingResult() + plugin.onMethodCall( + MethodCall("setProperty", mapOf("name" to "pause", "value" to "no")), + retry + ) + awaitCompletion(retry) + + assertEquals(1, retry.completionCount) + assertNull(retry.errorCode) + assertEquals(1, writes.count { it == "pause" to "no" }) + assertEquals(false, getBoolean(core, "pausedForAudioFocusLoss")) + assertEquals(false, getBoolean(core, "cachedPaused")) + } + + @Test + fun delayedFocusGainAfterExplicitResumeDoesNotWriteAgain() { + val writes = ConcurrentLinkedQueue>() + val explicitResumeStarted = CountDownLatch(1) + val releaseExplicitResume = CountDownLatch(1) + val duplicateResume = CountDownLatch(1) + val resumeWrites = AtomicInteger() + val focusResumeCallbacks = AtomicInteger() + val core = testCore { name, value -> + writes += name to value + if (name == "pause" && value == "no") { + if (resumeWrites.incrementAndGet() == 1) { + explicitResumeStarted.countDown() + releaseExplicitResume.await(1, TimeUnit.SECONDS) + } else { + duplicateResume.countDown() + } + } + } + val focusManager = testAudioFocusManager(core, focusResumeCallbacks) + setCoreField(core, "audioFocusManager", focusManager) + setBoolean(core, "desiredPaused", false) + setBoolean(core, "cachedPaused", false) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS) + awaitCondition { writes.contains("pause" to "yes") } + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + var outcome: Result? = null + core.setProperty("pause", "no") { outcome = it } + assertTrue(explicitResumeStarted.await(1, TimeUnit.SECONDS)) + assertEquals(false, getBoolean(core, "pausedForAudioFocusLoss")) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_GAIN) + assertEquals(1, focusResumeCallbacks.get()) + releaseExplicitResume.countDown() + awaitCondition { outcome != null } + + shadowOf(Looper.getMainLooper()).idle() + assertFalse(duplicateResume.await(100, TimeUnit.MILLISECONDS)) + assertTrue(outcome?.isSuccess == true) + assertEquals(1, resumeWrites.get()) + assertEquals(listOf("pause" to "yes", "pause" to "no"), writes.toList()) + } + + @Test + fun freshFocusLossAfterExplicitClaimPreventsThePendingResumeWrite() { + val writes = ConcurrentLinkedQueue>() + val blockerStarted = CountDownLatch(1) + val releaseBlocker = CountDownLatch(1) + val focusResumeCallbacks = AtomicInteger() + val core = testCore { name, value -> + writes += name to value + if (name == "block") { + blockerStarted.countDown() + releaseBlocker.await(1, TimeUnit.SECONDS) + } + } + val focusManager = testAudioFocusManager(core, focusResumeCallbacks) + setCoreField(core, "audioFocusManager", focusManager) + setBoolean(core, "desiredPaused", false) + setBoolean(core, "cachedPaused", false) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS) + awaitCondition { writes.count { it == "pause" to "yes" } == 1 } + var blockerOutcome: Result? = null + core.setProperty("block", "value") { blockerOutcome = it } + assertTrue(blockerStarted.await(1, TimeUnit.SECONDS)) + + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + val resumeCompletionCount = AtomicInteger() + var resumeOutcome: Result? = null + core.setProperty("pause", "no") { + resumeCompletionCount.incrementAndGet() + resumeOutcome = it + } + assertEquals(false, getBoolean(core, "pausedForAudioFocusLoss")) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS_TRANSIENT) + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + releaseBlocker.countDown() + awaitCondition { + blockerOutcome != null && + resumeOutcome != null && + writes.count { it == "pause" to "yes" } == 2 + } + + assertTrue(blockerOutcome?.isSuccess == true) + assertTrue(resumeOutcome?.isFailure == true) + assertEquals(1, resumeCompletionCount.get()) + assertEquals(0, writes.count { it == "pause" to "no" }) + assertEquals(true, getBoolean(core, "pausedForAudioFocusLoss")) + assertEquals(true, getBoolean(core, "cachedPaused")) + } + + @Test + fun explicitVideoResumeAfterFocusLossRemainsDeferredOnlyForSurface() { + val writes = ConcurrentLinkedQueue>() + val focusResumeCallbacks = AtomicInteger() + val completionCount = AtomicInteger() + val core = testVideoCore { name, value -> writes += name to value } + val focusManager = testAudioFocusManager(core, focusResumeCallbacks) + setCoreField(core, "audioFocusManager", focusManager) + setBoolean(core, "desiredPaused", false) + setBoolean(core, "cachedPaused", false) + + dispatchAudioFocusChange(focusManager, AudioManager.AUDIOFOCUS_LOSS) + awaitCondition { writes.contains("pause" to "yes") } + setNextAudioFocusRequestResponse(focusManager, AudioManager.AUDIOFOCUS_REQUEST_GRANTED) + var outcome: Result? = null + core.setProperty("pause", "no") { + completionCount.incrementAndGet() + outcome = it + } + awaitCondition { outcome != null } + + assertTrue(outcome?.isSuccess == true) + assertEquals(1, completionCount.get()) + assertEquals(listOf("pause" to "yes"), writes.toList()) + assertEquals(false, getBoolean(core, "pausedForAudioFocusLoss")) + assertEquals(true, getBoolean(core, "deferredResumeRequested")) + assertEquals(true, getBoolean(core, "cachedPaused")) + } + @Test fun pausedFocusLossAndGainWithoutResumeCallbackAllowsOneExplicitResume() { val writes = ConcurrentLinkedQueue>() @@ -624,6 +839,14 @@ class MpvPlayerPluginTest { listener.onAudioFocusChange(focusChange) } + private fun setNextAudioFocusRequestResponse(manager: AudioFocusManager, response: Int) { + val audioManager = AudioFocusManager::class.java.getDeclaredField("audioManager").run { + isAccessible = true + get(manager) as AudioManager + } + shadowOf(audioManager).setNextFocusRequestResponse(response) + } + private fun invokeAutoResume(core: MpvPlayerCore, reason: String) { MpvPlayerCore::class.java.getDeclaredMethod("requestAutoResume", String::class.java).apply { isAccessible = true diff --git a/android/app/src/test/kotlin/com/edde746/plezy/watchnext/WatchNextProviderTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/watchnext/WatchNextProviderTest.kt index 39ac5995..1684205b 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/watchnext/WatchNextProviderTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/watchnext/WatchNextProviderTest.kt @@ -13,6 +13,7 @@ import android.content.pm.ActivityInfo import android.content.pm.ApplicationInfo import android.content.pm.ResolveInfo import android.database.Cursor +import android.database.MatrixCursor import android.net.Uri import android.os.ParcelFileDescriptor.AutoCloseInputStream import androidx.tvprovider.media.tv.TvContractCompat @@ -20,6 +21,7 @@ import io.flutter.embedding.engine.plugins.FlutterPlugin import io.flutter.plugin.common.BinaryMessenger import io.flutter.plugin.common.MethodCall import io.flutter.plugin.common.MethodChannel +import java.io.Closeable import java.lang.reflect.Proxy import java.net.InetAddress import java.net.ServerSocket @@ -31,6 +33,7 @@ import java.util.concurrent.Executor import java.util.concurrent.ExecutorService import java.util.concurrent.RejectedExecutionException import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger import java.util.concurrent.atomic.AtomicReference import kotlin.concurrent.thread import org.junit.After @@ -222,6 +225,7 @@ class WatchNextProviderTest { try { assertTrue(secondRequestReceived.await(2, TimeUnit.SECONDS)) assertEquals(1, artworkFiles().size) + assertTrue(artworkFiles().single().name.endsWith(".tmp")) SystemShelfLifecycle.acquire() releaseSecondResponse.countDown() worker.join(2_000) @@ -440,7 +444,7 @@ class WatchNextProviderTest { val ownership = SystemShelfLifecycle.claim(SystemShelfLifecycle.acquire(), "owner", 1)!! val session = SystemShelfSyncSession(ownership, 2_000, budget = budget) - assertNull(SystemShelfArtworkStore(context.cacheDir).materialize("owner", source, session)) + assertNull(SystemShelfArtworkStore(context.cacheDir).prepare("owner", source, session)) assertEquals(malformed.size.toLong(), budget.consumed) assertEquals(100 - malformed.size, budget.remaining) } @@ -452,7 +456,7 @@ class WatchNextProviderTest { val ownership = SystemShelfLifecycle.claim(SystemShelfLifecycle.acquire(), "owner", 1)!! val session = SystemShelfSyncSession(ownership, 2_000, budget = budget) - assertNull(SystemShelfArtworkStore(context.cacheDir).materialize("owner", source, session)) + assertNull(SystemShelfArtworkStore(context.cacheDir).prepare("owner", source, session)) assertEquals(imageBytes.size.toLong(), budget.consumed) assertEquals(100 - imageBytes.size, budget.remaining) } @@ -480,13 +484,30 @@ class WatchNextProviderTest { val inactive = registerHandler(homeIntent, "inactive.home.launcher") selectDefaultHome(selected, selected, inactive) val owner = "a".repeat(64) - val key = "${"b".repeat(32)}.art" - context.cacheDir.resolve("system_shelf_artwork/$owner").mkdirs() - context.cacheDir.resolve("system_shelf_artwork/$owner/$key").writeBytes(imageBytes) - val valid = SystemShelfArtworkStore(context.cacheDir).contentUri(owner, key) - val invalid = Uri.parse("content://${SystemShelfArtworkProvider.AUTHORITY}/art/not/confined.art") + val legacyKey = "${"b".repeat(32)}.art" + val contentKey = "${"c".repeat(64)}.art" + val corruptKey = "${"d".repeat(64)}.art" + val directory = context.cacheDir.resolve("system_shelf_artwork/$owner").apply { mkdirs() } + directory.resolve(legacyKey).writeBytes(imageBytes) + directory.resolve(contentKey).writeBytes(imageBytes) + directory.resolve(corruptKey).writeText("corrupt") + val store = SystemShelfArtworkStore(context.cacheDir) + val legacy = store.contentUri(owner, legacyKey) + val contentAddressed = store.contentUri(owner, contentKey) + val corrupt = store.contentUri(owner, corruptKey) + val malformed = Uri.parse( + "content://${SystemShelfArtworkProvider.AUTHORITY}/art/$owner/${"e".repeat(48)}.art" + ) context.getSharedPreferences("system_shelf_state", 0).edit() - .putStringSet("granted_uris", setOf(valid.toString(), invalid.toString())) + .putStringSet( + "granted_uris", + setOf( + legacy.toString(), + contentAddressed.toString(), + corrupt.toString(), + malformed.toString() + ) + ) .putInt("shelf_schema_version", WatchNextProvider.SHELF_SCHEMA_VERSION) .commit() val recordingContext = RecordingGrantContext(context) @@ -495,12 +516,24 @@ class WatchNextProviderTest { .onReceive(recordingContext, Intent(Intent.ACTION_BOOT_COMPLETED)) assertEquals( - listOf(Grant("selected.home.launcher", valid, Intent.FLAG_GRANT_READ_URI_PERMISSION)), - recordingContext.grants + setOf( + Grant("selected.home.launcher", legacy, Intent.FLAG_GRANT_READ_URI_PERMISSION), + Grant("selected.home.launcher", contentAddressed, Intent.FLAG_GRANT_READ_URI_PERMISSION) + ), + recordingContext.grants.toSet() ) assertEquals( - setOf(valid.toString()), - context.getSharedPreferences("system_shelf_state", 0).getStringSet("granted_uris", emptySet()) + setOf(legacy.toString(), contentAddressed.toString()), + context.getSharedPreferences("system_shelf_state", 0) + .getStringSet("granted_uris", emptySet()) + ) + assertArrayEquals(imageBytes, openArtwork(legacy)) + assertArrayEquals(imageBytes, openArtwork(contentAddressed)) + assertNull(store.resolve(corrupt)) + assertEquals( + WatchNextProvider.SHELF_SCHEMA_VERSION, + context.getSharedPreferences("system_shelf_state", 0) + .getInt("shelf_schema_version", 0) ) } @@ -538,9 +571,12 @@ class WatchNextProviderTest { @Test fun packageUpdatePreservesRowsAndArtworkAtCurrentShelfSchema() { - val file = context.cacheDir.resolve("system_shelf_artwork/${"a".repeat(64)}/${"b".repeat(32)}.art") - file.parentFile?.mkdirs() - file.writeBytes(imageBytes) + val directory = context.cacheDir.resolve("system_shelf_artwork/${"a".repeat(64)}") + .apply { mkdirs() } + val legacyFile = directory.resolve("${"b".repeat(32)}.art") + val contentAddressedFile = directory.resolve("${"c".repeat(64)}.art") + legacyFile.writeBytes(imageBytes) + contentAddressedFile.writeBytes(imageBytes) context.getSharedPreferences("system_shelf_state", 0).edit() .putInt("shelf_schema_version", WatchNextProvider.SHELF_SCHEMA_VERSION) .commit() @@ -549,7 +585,9 @@ class WatchNextProviderTest { receiver.onReceive(context, Intent(Intent.ACTION_MY_PACKAGE_REPLACED)) assertEquals(0, tvProvider.deleteCount) - assertTrue(file.isFile) + assertTrue(legacyFile.isFile) + assertTrue(contentAddressedFile.isFile) + assertEquals(1, WatchNextProvider.SHELF_SCHEMA_VERSION) } @Test @@ -564,18 +602,525 @@ class WatchNextProviderTest { @Test fun providerFailureDeletesNewArtworkAndPreservesCommittedArtwork() { - val provider = WatchNextProvider(context) - withServer("image/png", imageBytes) { source -> + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes) + ) + ).use { server -> + val homeIntent = Intent(Intent.ACTION_MAIN).addCategory(Intent.CATEGORY_HOME) + val selectedHome = registerHandler(homeIntent, "selected.home.launcher") + selectDefaultHome(selectedHome, selectedHome) + val grantContext = RecordingGrantContext(context) + val provider = WatchNextProvider(grantContext) + val sourceA = "${server.baseUrl}/a" + val sourceB = "${server.baseUrl}/b" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val committedPoster = committedPoster()!! + val committedArtwork = SystemShelfArtworkStore(context.cacheDir) + .resolveOwned("owner-a", committedPoster)!! + .canonicalFile + tvProvider.failBatch = true + + assertFalse(provider.syncWatchNextPrograms("owner-a", 2, listOf(item(sourceB)))) + + assertEquals(2, server.requestCount.get()) + assertEquals(committedPoster, committedPoster()) + assertEquals(setOf(committedArtwork), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertArrayEquals(imageBytes, openArtwork(committedPoster)) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + val uncommittedPoster = grantContext.grants + .map(Grant::uri) + .first { it != committedPoster } + val revokedUris = grantContext.packageRevocations.map(PackageRevocation::uri) + + grantContext.uriWideRevocations + assertFalse(committedPoster in revokedUris) + assertTrue(uncommittedPoster in revokedUris) + assertEquals( + setOf(committedPoster.toString()), + context.getSharedPreferences("system_shelf_state", 0) + .getStringSet("granted_uris", emptySet()) + ) + } + } + + @Test + fun repeatedSameSourceSyncReusesValidatedArtwork() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes) + ) + ).use { server -> + val homeIntent = Intent(Intent.ACTION_MAIN).addCategory(Intent.CATEGORY_HOME) + val selectedHome = registerHandler(homeIntent, "selected.home.launcher") + selectDefaultHome(selectedHome, selectedHome) + val grantContext = RecordingGrantContext(context) + val provider = WatchNextProvider(grantContext) + val store = SystemShelfArtworkStore(context.cacheDir) + val stableSource = "${server.baseUrl}/stable" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(stableSource)))) + val firstPoster = committedPoster()!! + val firstFile = store.resolveOwned("owner-a", firstPoster)!!.canonicalFile + val stableTimestamp = 1_600_000_000_000L + assertTrue(firstFile.setLastModified(stableTimestamp)) + assertTrue(Regex("[a-f0-9]{64}\\.art").matches(firstPoster.lastPathSegment.orEmpty())) + + assertTrue( + provider.syncWatchNextPrograms( + "owner-a", + 2, + listOf(item(stableSource).copy(title = "Updated title", lastPlaybackPosition = 20)) + ) + ) + + assertEquals(1, server.requestCount.get()) + assertEquals(firstPoster, committedPoster()) + assertEquals(setOf(firstFile), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertEquals(stableTimestamp, firstFile.lastModified()) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + assertTrue(grantContext.uriWideRevocations.isEmpty()) + assertTrue(grantContext.packageRevocations.isEmpty()) + val zeroBudget = SystemShelfArtworkStore.Budget(0) + val cachedOwnership = provider.claimOwnership("owner-a", 3)!! + val cached = store.prepare( + "owner-a", + stableSource, + SystemShelfSyncSession(cachedOwnership, 2_000, budget = zeroBudget) + ) + assertTrue(cached is SystemShelfArtworkStore.Prepared.Existing) + assertEquals(0L, zeroBudget.consumed) + assertEquals(0, zeroBudget.remaining) + + val sharedSource = "${server.baseUrl}/shared" + assertTrue( + provider.syncWatchNextPrograms( + "owner-a", + 4, + listOf( + item(sharedSource).copy(contentId = "first"), + item(sharedSource).copy(contentId = "second") + ) + ) + ) + assertEquals(2, server.requestCount.get()) + val sharedPosters = tvProvider.inserted.map { + it.getAsString(TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI) + }.toSet() + assertEquals(1, sharedPosters.size) + val sharedPoster = Uri.parse(sharedPosters.single()) + assertEquals(1, artworkFiles().size) + assertArrayEquals(imageBytes, openArtwork(sharedPoster)) + grantContext.uriWideRevocations.clear() + grantContext.packageRevocations.clear() + + val firstRemoved = provider.removeItem("owner-a", 5, "first") + assertEquals(listOf("first", "second"), tvProvider.lastQueryContentIds) + assertEquals( + listOf( + TvContractCompat.WatchNextPrograms._ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA, + TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI, + TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI + ), + tvProvider.lastQueryProjection + ) + assertEquals( + listOf(sharedPoster.toString(), sharedPoster.toString()), + tvProvider.lastQueryPosters + ) + assertTrue(firstRemoved) + assertTrue(grantContext.uriWideRevocations.isEmpty()) + assertTrue(grantContext.packageRevocations.isEmpty()) + assertEquals(listOf("second"), tvProvider.contentIds()) + assertArrayEquals(imageBytes, openArtwork(sharedPoster)) + assertEquals( + setOf(sharedPoster.toString()), + context.getSharedPreferences("system_shelf_state", 0) + .getStringSet("granted_uris", emptySet()) + ) + + assertTrue(provider.removeItem("owner-a", 6, "second")) + val finalReferenceRevocations = + grantContext.packageRevocations.map(PackageRevocation::uri) + + grantContext.uriWideRevocations + assertEquals(listOf(sharedPoster), finalReferenceRevocations) + assertTrue(artworkFiles().isEmpty()) + assertTrue( + runCatching { + Robolectric.buildContentProvider(SystemShelfArtworkProvider::class.java) + .create() + .get() + .openFile(sharedPoster, "r") + .close() + }.isFailure + ) + } + } + + @Test + fun changedSourceFetchFailureRetainsLastKnownGoodArtwork() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(status = 503, contentType = "text/plain", body = "retry".toByteArray()), + ScriptedResponse(body = imageBytes) + ) + ).use { server -> + val homeIntent = Intent(Intent.ACTION_MAIN).addCategory(Intent.CATEGORY_HOME) + val selectedHome = registerHandler(homeIntent, "selected.home.launcher") + selectDefaultHome(selectedHome, selectedHome) + val grantContext = RecordingGrantContext(context) + val provider = WatchNextProvider(grantContext) + val sourceA = "${server.baseUrl}/a" + val sourceB = "${server.baseUrl}/b" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val originalPoster = committedPoster()!! + val originalFile = SystemShelfArtworkStore(context.cacheDir) + .resolveOwned("owner-a", originalPoster)!! + .canonicalFile + + assertTrue( + provider.syncWatchNextPrograms( + "owner-a", + 2, + listOf(item(sourceB).copy(title = "Progress update", lastPlaybackPosition = 40)) + ) + ) + assertEquals(2, server.requestCount.get()) + assertEquals(listOf("plezy_server_item"), tvProvider.lastQueryContentIds) + assertEquals(listOf(originalPoster.toString()), tvProvider.lastQueryPosters) + assertEquals(originalPoster, committedPoster()) + assertEquals( + "Progress update", + tvProvider.inserted.single() + .getAsString(TvContractCompat.WatchNextPrograms.COLUMN_TITLE) + ) + assertTrue(originalFile.isFile) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + + tvProvider.blockNextBatch = true + val retryResult = AtomicReference() + val retry = thread(start = true, name = "system-shelf-source-retry") { + retryResult.set( + provider.syncWatchNextPrograms( + "owner-a", + 3, + listOf(item(sourceB).copy(title = "Replacement ready")) + ) + ) + } + try { + assertTrue(tvProvider.batchStarted.await(2, TimeUnit.SECONDS)) + assertTrue(originalFile.isFile) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + assertEquals(2, artworkFiles().count { it.name.endsWith(".art") }) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + val revokedBeforeCommit = + grantContext.packageRevocations.map(PackageRevocation::uri) + + grantContext.uriWideRevocations + assertFalse(originalPoster in revokedBeforeCommit) + } finally { + tvProvider.releaseBatch.countDown() + retry.join(2_000) + } + assertEquals(true, retryResult.get()) + assertEquals(3, server.requestCount.get()) + val replacementPoster = committedPoster()!! + assertFalse(replacementPoster == originalPoster) + assertFalse(originalFile.exists()) + assertArrayEquals(imageBytes, openArtwork(replacementPoster)) + val revokedAfterCommit = + grantContext.packageRevocations.map(PackageRevocation::uri) + + grantContext.uriWideRevocations + assertTrue(originalPoster in revokedAfterCommit) + + assertTrue( + provider.syncWatchNextPrograms( + "owner-a", + 4, + listOf(item(sourceB).copy(posterSourceUri = null)) + ) + ) + assertNull(committedPoster()) + assertTrue(artworkFiles().isEmpty()) + assertEquals(3, server.requestCount.get()) + } + } + + @Test + fun malformedReplacementRetainsLastKnownGoodArtwork() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = "not an image".toByteArray()) + ) + ).use { server -> + val provider = WatchNextProvider(context) + val sourceA = "${server.baseUrl}/valid" + val sourceB = "${server.baseUrl}/malformed" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val originalPoster = committedPoster()!! + val originalFile = artworkFiles().single().canonicalFile + + assertTrue( + provider.syncWatchNextPrograms( + "owner-a", + 2, + listOf(item(sourceB).copy(lastPlaybackPosition = 50)) + ) + ) + + assertEquals(2, server.requestCount.get()) + assertEquals(listOf("plezy_server_item"), tvProvider.lastQueryContentIds) + assertEquals(listOf(originalPoster.toString()), tvProvider.lastQueryPosters) + assertEquals(originalPoster, committedPoster()) + assertEquals(setOf(originalFile), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + } + } + + @Test + fun corruptContentAddressIsRefetchedWithoutPartialPublish() { + val replacementRequested = CountDownLatch(1) + val releaseReplacement = CountDownLatch(1) + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse( + body = imageBytes, + beforeResponse = { + replacementRequested.countDown() + releaseReplacement.await(2, TimeUnit.SECONDS) + } + ), + ScriptedResponse(status = 503, contentType = "text/plain"), + ScriptedResponse(body = imageBytes), + ScriptedResponse(status = 503, contentType = "text/plain") + ) + ).use { server -> + val provider = WatchNextProvider(context) + val store = SystemShelfArtworkStore(context.cacheDir) + val source = "${server.baseUrl}/stable" assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(source)))) - } - val committedArtwork = artworkFiles().single().canonicalFile - tvProvider.failBatch = true + val stablePoster = committedPoster()!! + val deterministicFile = store.resolveOwned("owner-a", stablePoster)!! + val corruptBytes = "corrupt-cache".toByteArray() + deterministicFile.writeBytes(corruptBytes) + assertNull(store.resolveOwned("owner-a", stablePoster)) + assertTrue(runCatching { openArtwork(stablePoster) }.isFailure) - withServer("image/png", imageBytes) { source -> - assertFalse(provider.syncWatchNextPrograms("owner-a", 2, listOf(item(source)))) - } + val refetchResult = AtomicReference() + val refetch = thread(start = true, name = "system-shelf-corrupt-refetch") { + refetchResult.set( + provider.syncWatchNextPrograms("owner-a", 2, listOf(item(source))) + ) + } + try { + assertTrue(replacementRequested.await(2, TimeUnit.SECONDS)) + assertArrayEquals(corruptBytes, deterministicFile.readBytes()) + assertEquals(1, artworkFiles().size) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + } finally { + releaseReplacement.countDown() + refetch.join(2_000) + } - assertEquals(setOf(committedArtwork), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertEquals(true, refetchResult.get()) + assertEquals(stablePoster, committedPoster()) + assertArrayEquals(imageBytes, deterministicFile.readBytes()) + assertArrayEquals(imageBytes, openArtwork(stablePoster)) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + + deterministicFile.writeBytes(corruptBytes) + assertTrue(provider.syncWatchNextPrograms("owner-a", 3, listOf(item(source)))) + assertEquals(3, server.requestCount.get()) + assertNull(committedPoster()) + assertTrue(artworkFiles().isEmpty()) + + assertTrue(provider.syncWatchNextPrograms("owner-a", 4, listOf(item(source)))) + assertEquals(stablePoster, committedPoster()) + assertTrue(deterministicFile.delete()) + assertTrue(provider.syncWatchNextPrograms("owner-a", 5, listOf(item(source)))) + assertEquals(5, server.requestCount.get()) + assertNull(committedPoster()) + assertTrue(artworkFiles().isEmpty()) + } + } + + @Test + fun committedRowQueryFailureAbortsBeforeArtworkOrShelfMutation() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes) + ) + ).use { server -> + val provider = WatchNextProvider(context) + val sourceA = "${server.baseUrl}/a" + val sourceB = "${server.baseUrl}/b" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val originalPoster = committedPoster()!! + val originalFile = artworkFiles().single().canonicalFile + val preferences = context.getSharedPreferences("system_shelf_state", 0) + val grantedBefore = preferences.getStringSet("granted_uris", emptySet())!!.toSet() + val packagesBefore = preferences.getStringSet("granted_packages", emptySet())!!.toSet() + + tvProvider.failQuery = true + assertFalse(provider.syncWatchNextPrograms("owner-a", 2, listOf(item(sourceB)))) + tvProvider.failQuery = false + tvProvider.returnNullQuery = true + assertFalse(provider.syncWatchNextPrograms("owner-a", 3, listOf(item(sourceB)))) + tvProvider.returnNullQuery = false + + assertEquals(1, server.requestCount.get()) + assertEquals(3, tvProvider.queryCount) + assertEquals(originalPoster, committedPoster()) + assertEquals(setOf(originalFile), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertEquals(grantedBefore, preferences.getStringSet("granted_uris", emptySet())) + assertEquals(packagesBefore, preferences.getStringSet("granted_packages", emptySet())) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + } + } + + @Test + fun interruptedPublishLeavesCommittedArtworkAndUniqueStagesUntouched() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes) + ) + ).use { server -> + val provider = WatchNextProvider(context) + val store = SystemShelfArtworkStore(context.cacheDir) + val sourceA = "${server.baseUrl}/a" + val sourceB = "${server.baseUrl}/b" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val originalPoster = committedPoster()!! + val originalFile = store.resolveOwned("owner-a", originalPoster)!!.canonicalFile + + val firstOwnership = provider.claimOwnership("owner-a", 2)!! + val firstStage = store.prepare( + "owner-a", + sourceB, + SystemShelfSyncSession(firstOwnership, 2_000) + ) as SystemShelfArtworkStore.Prepared.Staged + val secondOwnership = provider.claimOwnership("owner-a", 3)!! + val secondStage = store.prepare( + "owner-a", + sourceB, + SystemShelfSyncSession(secondOwnership, 2_000) + ) as SystemShelfArtworkStore.Prepared.Staged + assertFalse(firstStage.stagingFile.canonicalFile == secondStage.stagingFile.canonicalFile) + assertFalse(firstStage.materialized.file.exists()) + + assertTrue(secondStage.stagingFile.delete()) + val publication = SystemShelfLifecycle.whileCurrent(secondOwnership) { + store.publish(secondStage) + } + assertNull(publication) + store.discard(listOf(firstStage, secondStage)) + + assertEquals(3, server.requestCount.get()) + assertEquals(originalPoster, committedPoster()) + assertTrue(originalFile.isFile) + assertFalse(firstStage.materialized.file.exists()) + assertEquals(setOf(originalFile), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + } + } + + @Test + fun contentAddressesAndFallbackAreOwnerIsolated() { + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse(body = imageBytes), + ScriptedResponse(status = 503, contentType = "text/plain") + ) + ).use { server -> + val provider = WatchNextProvider(context) + val source = "${server.baseUrl}/shared" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(source)))) + val ownerAPoster = committedPoster()!! + + assertTrue(provider.syncWatchNextPrograms("owner-b", 2, listOf(item(source)))) + val ownerBPoster = committedPoster()!! + assertFalse(ownerAPoster == ownerBPoster) + assertFalse(ownerAPoster.pathSegments[1] == ownerBPoster.pathSegments[1]) + assertEquals(2, server.requestCount.get()) + assertArrayEquals(imageBytes, openArtwork(ownerBPoster)) + + assertTrue( + provider.syncWatchNextPrograms( + "owner-c", + 3, + listOf(item(source).copy(title = "Owner C metadata")) + ) + ) + assertEquals(3, server.requestCount.get()) + assertNull(committedPoster()) + assertTrue(artworkFiles().isEmpty()) + } + } + + @Test + fun staleOperationCannotPublishOrPruneCommittedArtwork() { + val replacementRequest = CountDownLatch(1) + val releaseReplacement = CountDownLatch(1) + ScriptedHttpServer( + listOf( + ScriptedResponse(body = imageBytes), + ScriptedResponse( + body = imageBytes, + beforeResponse = { + replacementRequest.countDown() + releaseReplacement.await(2, TimeUnit.SECONDS) + } + ) + ) + ).use { server -> + val provider = WatchNextProvider(context) + val sourceA = "${server.baseUrl}/a" + val sourceB = "${server.baseUrl}/b" + assertTrue(provider.syncWatchNextPrograms("owner-a", 1, listOf(item(sourceA)))) + val originalPoster = committedPoster()!! + val originalFile = artworkFiles().single().canonicalFile + val staleResult = AtomicReference() + val staleWorker = thread(start = true, name = "system-shelf-stale-artwork") { + staleResult.set( + provider.syncWatchNextPrograms("owner-a", 2, listOf(item(sourceB))) + ) + } + try { + assertTrue(replacementRequest.await(2, TimeUnit.SECONDS)) + assertTrue(provider.claimOwnership("owner-a", 3) != null) + } finally { + releaseReplacement.countDown() + staleWorker.join(2_000) + } + + assertFalse(staleWorker.isAlive) + assertEquals(false, staleResult.get()) + assertEquals(originalPoster, committedPoster()) + assertEquals(setOf(originalFile), artworkFiles().mapTo(HashSet()) { it.canonicalFile }) + assertFalse(artworkFiles().any { it.name.endsWith(".tmp") }) + assertArrayEquals(imageBytes, openArtwork(originalPoster)) + } + } + + private fun committedPoster(): Uri? = tvProvider.inserted.singleOrNull() + ?.getAsString(TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI) + ?.let(Uri::parse) + + private fun openArtwork(uri: Uri): ByteArray { + val provider = Robolectric.buildContentProvider(SystemShelfArtworkProvider::class.java) + .create() + .get() + return AutoCloseInputStream(provider.openFile(uri, "r")).use { it.readBytes() } } private fun registerHandler(intent: Intent, packageName: String): ComponentName { @@ -687,6 +1232,59 @@ class WatchNextProviderTest { } } +private data class ScriptedResponse( + val status: Int = 200, + val contentType: String = "image/png", + val body: ByteArray = ByteArray(0), + val beforeResponse: (() -> Unit)? = null +) + +private class ScriptedHttpServer(responses: List) : Closeable { + private val server = ServerSocket(0, 8, InetAddress.getByName("127.0.0.1")) + private val scriptedResponses = ArrayDeque().apply { addAll(responses) } + val requestCount = AtomicInteger() + val baseUrl = "http://127.0.0.1:${server.localPort}" + private val responder = thread(start = true, name = "system-shelf-scripted-http") { + while (!server.isClosed) { + val socket = runCatching { server.accept() }.getOrNull() ?: break + runCatching { + socket.use { + val reader = it.getInputStream().bufferedReader() + while (reader.readLine()?.isNotEmpty() == true) { + // Consume request headers. + } + requestCount.incrementAndGet() + val response = synchronized(scriptedResponses) { + if (scriptedResponses.isEmpty()) { + ScriptedResponse(status = 500, contentType = "text/plain") + } else { + scriptedResponses.removeFirst() + } + } + response.beforeResponse?.invoke() + val reason = if (response.status in 200..299) "OK" else "Injected" + val headers = ( + "HTTP/1.1 ${response.status} $reason\r\n" + + "Content-Type: ${response.contentType}\r\n" + + "Content-Length: ${response.body.size}\r\n" + + "Connection: close\r\n\r\n" + ).toByteArray() + it.getOutputStream().use { output -> + output.write(headers) + output.write(response.body) + output.flush() + } + } + } + } + } + + override fun close() { + server.close() + responder.join(2_000) + } +} + private data class Grant(val packageName: String, val uri: Uri, val modeFlags: Int) private data class PackageRevocation(val packageName: String, val uri: Uri, val modeFlags: Int) @@ -715,22 +1313,60 @@ private class RecordingGrantContext(base: Context) : ContextWrapper(base) { private class CapturingTvProvider : ContentProvider() { val inserted = mutableListOf() + fun contentIds(): List = inserted.map { values -> + valueAsString(values, TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID) + ?.takeIf(String::isNotBlank) + ?: valueAsString( + values, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA + )?.takeIf(String::isNotBlank) + ?: valueAsString(values, TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI) + ?.let(Uri::parse) + ?.takeIf { uri -> uri.scheme == "plezy" && uri.authority == "play" } + ?.getQueryParameter("content_id") + } + private val rowIds = mutableListOf() + private var nextRowId = 1L var deleteCount = 0 + var queryCount = 0 + var lastQueryContentIds: List = emptyList() + var lastQueryPosters: List = emptyList() + var lastQueryProjection: List = emptyList() + var failQuery = false + var returnNullQuery = false var failBatch = false var blockNextBatch = false val batchStarted = CountDownLatch(1) val releaseBatch = CountDownLatch(1) + override fun onCreate(): Boolean = true + override fun insert(uri: Uri, values: ContentValues?): Uri { + val id = nextRowId++ inserted += ContentValues(values) - return uri.buildUpon().appendPath(inserted.size.toString()).build() + rowIds += id + return uri.buildUpon().appendPath(id.toString()).build() } + override fun delete(uri: Uri, selection: String?, selectionArgs: Array?): Int { deleteCount++ - inserted.clear() + if (uri == TvContractCompat.WatchNextPrograms.CONTENT_URI) { + val deleted = inserted.size + inserted.clear() + rowIds.clear() + return deleted + } + val id = uri.lastPathSegment?.toLongOrNull() ?: return 0 + val index = rowIds.indexOf(id) + if (index < 0) return 0 + rowIds.removeAt(index) + inserted.removeAt(index) return 1 } - override fun applyBatch(operations: ArrayList): Array { + + override fun applyBatch( + operations: ArrayList + ): Array { if (failBatch) throw IllegalStateException("Injected provider failure") if (blockNextBatch) { blockNextBatch = false @@ -739,9 +1375,61 @@ private class CapturingTvProvider : ContentProvider() { } return super.applyBatch(operations) } + + override fun query( + uri: Uri, + projection: Array?, + selection: String?, + selectionArgs: Array?, + sortOrder: String? + ): Cursor? { + queryCount++ + if (failQuery) throw IllegalStateException("Injected query failure") + if (returnNullQuery) return null + val columns = projection ?: arrayOf( + TvContractCompat.WatchNextPrograms._ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA, + TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI, + TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI + ) + lastQueryProjection = columns.toList() + lastQueryContentIds = contentIds() + lastQueryPosters = inserted.map { values -> + valueAsString(values, TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI) + } + return MatrixCursor(columns).apply { + inserted.indices.forEach { index -> + addRow( + columns.map { column -> + when (column) { + TvContractCompat.WatchNextPrograms._ID -> rowIds[index] + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_DATA -> + inserted[index].get(column) + TvContractCompat.WatchNextPrograms.COLUMN_INTERNAL_PROVIDER_ID, + TvContractCompat.WatchNextPrograms.COLUMN_INTENT_URI, + TvContractCompat.PreviewPrograms.COLUMN_POSTER_ART_URI -> + valueAsString(inserted[index], column) + else -> inserted[index].get(column) + } + }.toTypedArray() + ) + } + } + } + + private fun valueAsString(values: ContentValues, column: String): String? = when (val value = values.get(column)) { + is ByteArray -> value.toString(Charsets.UTF_8) + else -> values.getAsString(column) + } + override fun getType(uri: Uri): String? = null - override fun query(uri: Uri, projection: Array?, selection: String?, selectionArgs: Array?, sortOrder: String?): Cursor? = null - override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?): Int = 0 + override fun update( + uri: Uri, + values: ContentValues?, + selection: String?, + selectionArgs: Array? + ): Int = 0 } private class RecordingResult : MethodChannel.Result { diff --git a/ios/RunnerTests/RunnerTests.swift b/ios/RunnerTests/RunnerTests.swift index cc606df2..c7d388d5 100644 --- a/ios/RunnerTests/RunnerTests.swift +++ b/ios/RunnerTests/RunnerTests.swift @@ -179,6 +179,17 @@ final class MpvPlayerContractTests: XCTestCase { XCTAssertEqual((invalidResults[0] as? FlutterError)?.code, "INVALID_ARGS") } + func testSharedSetPropertyMapsLifecycleCancellationAsNotInitialized() { + let core = ControllablePropertyCore() + let plugin = RecordingMpvPlugin(core: core) + core.nextResult = .failure(MpvLifecycleUnavailableError("controlled cancellation")) + + let cancelled = invokeSetProperty(plugin, name: "volume", value: "50") + + XCTAssertEqual(cancelled.count, 1) + XCTAssertEqual((cancelled[0] as? FlutterError)?.code, "NOT_INITIALIZED") + } + func testRealSetPropertyValidInvalidNonexistentAndPauseCache() { let core = MpvAudioPlayerCore() XCTAssertTrue(core.initialize()) @@ -314,10 +325,14 @@ final class MpvPlayerContractTests: XCTestCase { let completion = expectation(description: "cancelled property completion") completion.assertForOverFulfill = true var completionCount = 0 + var completionError: Error? core.setPropertyAsync("volume", value: "51") { result in completionCount += 1 - if case .success = result { + switch result { + case .success: XCTFail("Disposal must fail an accepted-but-pending property request") + case .failure(let error): + completionError = error } completion.fulfill() } @@ -327,7 +342,12 @@ final class MpvPlayerContractTests: XCTestCase { wait(for: [completion], timeout: 2) core.queue.sync {} XCTAssertEqual(completionCount, 1) - XCTAssertFailure(awaitProperty(core, name: "volume", value: "52")) + XCTAssertTrue(completionError is MpvLifecycleUnavailableError) + let unavailableResult = awaitProperty(core, name: "volume", value: "52") + XCTAssertFailure(unavailableResult) + if case .failure(let error) = unavailableResult { + XCTAssertTrue(error is MpvLifecycleUnavailableError) + } } func testRapidAudioCoreReplacementOwnsLifecycleOnce() { diff --git a/linux/runner/mpv/mpv_player.cc b/linux/runner/mpv/mpv_player.cc index 38879bf7..1dea5b6d 100644 --- a/linux/runner/mpv/mpv_player.cc +++ b/linux/runner/mpv/mpv_player.cc @@ -560,7 +560,7 @@ void MpvPlayer::Dispose() { auto cancelled = pending_requests_.CancelAll(); for (auto& callback : cancelled.status) { - callback(-1); + callback(MPV_ERROR_UNINITIALIZED); } for (auto& callback : cancelled.properties) { callback(-1, ""); diff --git a/linux/runner/mpv/mpv_player_lifecycle_test.cc b/linux/runner/mpv/mpv_player_lifecycle_test.cc index f7f97e43..fd67baeb 100644 --- a/linux/runner/mpv/mpv_player_lifecycle_test.cc +++ b/linux/runner/mpv/mpv_player_lifecycle_test.cc @@ -446,7 +446,7 @@ void TestPendingPropertyWriteFailsOnDispose() { player.Dispose(); Check(callback_count == 1, "dispose must complete a pending property write exactly once"); - Check(status < 0, "dispose must fail a pending property write"); + Check(status == MPV_ERROR_UNINITIALIZED, "dispose must cancel a pending property write as uninitialized"); player.Dispose(); Check(callback_count == 1, "repeated dispose must not complete a property write twice"); diff --git a/linux/runner/mpv/mpv_plugin.cc b/linux/runner/mpv/mpv_plugin.cc index 77e81174..921f31bc 100644 --- a/linux/runner/mpv/mpv_plugin.cc +++ b/linux/runner/mpv/mpv_plugin.cc @@ -379,9 +379,12 @@ static void mpv_plugin_handle_method_call(FlMethodChannel* channel, FlMethodCall if (plezy::mpv_common::SetPropertyStatusSucceeded(error)) { async_response = FL_METHOD_RESPONSE(fl_method_success_response_new(nullptr)); } else { - const std::string description = plezy::mpv_common::SetPropertyErrorDescription(error); - async_response = FL_METHOD_RESPONSE(fl_method_error_response_new( - plezy::mpv_common::kSetPropertyFailedCode, description.c_str(), nullptr)); + const char* error_code = plezy::mpv_common::SetPropertyErrorCode(error); + const std::string description = error == MPV_ERROR_UNINITIALIZED + ? std::string("Player not initialized") + : plezy::mpv_common::SetPropertyErrorDescription(error); + async_response = + FL_METHOD_RESPONSE(fl_method_error_response_new(error_code, description.c_str(), nullptr)); } fl_method_call_respond(method_call, async_response, nullptr); g_object_unref(method_call); diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index e40b03f2..9596c873 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -94,6 +94,17 @@ final class MpvPlayerContractTests: XCTestCase { XCTAssertEqual((invalidResults[0] as? FlutterError)?.code, "INVALID_ARGS") } + func testSharedSetPropertyMapsLifecycleCancellationAsNotInitialized() { + let core = ControllablePropertyCore() + let plugin = RecordingMpvPlugin(core: core) + core.nextResult = .failure(MpvLifecycleUnavailableError("controlled cancellation")) + + let cancelled = invokeSetProperty(plugin, name: "volume", value: "50") + + XCTAssertEqual(cancelled.count, 1) + XCTAssertEqual((cancelled[0] as? FlutterError)?.code, "NOT_INITIALIZED") + } + func testRealSetPropertyValidInvalidNonexistentAndPauseCache() { let core = MpvAudioPlayerCore() XCTAssertTrue(core.initialize()) @@ -161,10 +172,14 @@ final class MpvPlayerContractTests: XCTestCase { let completion = expectation(description: "cancelled property completion") completion.assertForOverFulfill = true var completionCount = 0 + var completionError: Error? core.setPropertyAsync("volume", value: "51") { result in completionCount += 1 - if case .success = result { + switch result { + case .success: XCTFail("Disposal must fail an accepted-but-pending property request") + case .failure(let error): + completionError = error } completion.fulfill() } @@ -174,7 +189,12 @@ final class MpvPlayerContractTests: XCTestCase { wait(for: [completion], timeout: 2) core.queue.sync {} XCTAssertEqual(completionCount, 1) - XCTAssertFailure(awaitProperty(core, name: "volume", value: "52")) + XCTAssertTrue(completionError is MpvLifecycleUnavailableError) + let unavailableResult = awaitProperty(core, name: "volume", value: "52") + XCTAssertFailure(unavailableResult) + if case .failure(let error) = unavailableResult { + XCTAssertTrue(error is MpvLifecycleUnavailableError) + } } func testRapidAudioCoreReplacementOwnsLifecycleOnce() { diff --git a/shared/apple/MpvPlayer/MpvPlayerCoreBase.swift b/shared/apple/MpvPlayer/MpvPlayerCoreBase.swift index 5a6939df..1fa7c896 100644 --- a/shared/apple/MpvPlayer/MpvPlayerCoreBase.swift +++ b/shared/apple/MpvPlayer/MpvPlayerCoreBase.swift @@ -10,6 +10,14 @@ import QuartzCore import Metal #endif +struct MpvLifecycleUnavailableError: LocalizedError { + let errorDescription: String? + + init(_ description: String) { + errorDescription = description + } +} + protocol MpvPlayerDelegate: AnyObject { func onPropertyChange(name: String, value: Any?) func onEvent(name: String, data: [String: Any]?) @@ -944,11 +952,7 @@ class MpvPlayerCoreBase: NSObject { pendingRequests.removeAll() pendingRequestsLock.unlock() - let error = NSError( - domain: "mpv", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Player disposed"] - ) + let error = MpvLifecycleUnavailableError("Player disposed") for (_, request) in pending { DispatchQueue.main.async { switch request { @@ -977,12 +981,8 @@ class MpvPlayerCoreBase: NSObject { return pendingRequests.removeValue(forKey: requestId) } - private func lifecycleUnavailableError() -> NSError { - NSError( - domain: "mpv", - code: -1, - userInfo: [NSLocalizedDescriptionKey: "Player is not initialized or has been disposed"] - ) + private func lifecycleUnavailableError() -> MpvLifecycleUnavailableError { + MpvLifecycleUnavailableError("Player is not initialized or has been disposed") } private func mpvError(_ status: CInt) -> NSError { diff --git a/shared/apple/MpvPlayer/MpvPlayerPluginShared.swift b/shared/apple/MpvPlayer/MpvPlayerPluginShared.swift index 3e1ff21a..a5bad347 100644 --- a/shared/apple/MpvPlayer/MpvPlayerPluginShared.swift +++ b/shared/apple/MpvPlayer/MpvPlayerPluginShared.swift @@ -49,11 +49,15 @@ extension MpvPluginShared { self?.didSetPauseProperty(value: value) } result(nil) - case .failure: + case .failure(let error): + let lifecycleUnavailable = error is MpvLifecycleUnavailableError result( FlutterError( - code: "SET_PROPERTY_FAILED", - message: "MPV rejected or cancelled the property write", + code: lifecycleUnavailable ? "NOT_INITIALIZED" : "SET_PROPERTY_FAILED", + message: + lifecycleUnavailable + ? "MPV player is not initialized" + : "MPV rejected or cancelled the property write", details: nil)) } } diff --git a/shared/mpv/mpv_player_common.h b/shared/mpv/mpv_player_common.h index 257cd345..f60e1ac6 100644 --- a/shared/mpv/mpv_player_common.h +++ b/shared/mpv/mpv_player_common.h @@ -26,6 +26,10 @@ static constexpr size_t kSetPropertyErrorDescriptionLimit = 160; inline bool SetPropertyStatusSucceeded(int status) { return status >= 0; } +inline const char* SetPropertyErrorCode(int status) { + return status == MPV_ERROR_UNINITIALIZED ? kSetPropertyNotInitializedCode : kSetPropertyFailedCode; +} + inline std::string SetPropertyErrorDescription(int status) { const char* description = mpv_error_string(status); if (!description || description[0] == '\0') { diff --git a/shared/mpv/mpv_player_common_test.cpp b/shared/mpv/mpv_player_common_test.cpp index ac0100b7..44d9293f 100644 --- a/shared/mpv/mpv_player_common_test.cpp +++ b/shared/mpv/mpv_player_common_test.cpp @@ -81,14 +81,27 @@ void TestSetPropertyResultContract() { assert(SetPropertyStatusSucceeded(MPV_ERROR_SUCCESS)); assert(SetPropertyStatusSucceeded(1)); - constexpr int kFailureStatuses[] = { + assert(!SetPropertyStatusSucceeded(MPV_ERROR_UNINITIALIZED)); + + assert(std::string(SetPropertyErrorCode(MPV_ERROR_UNINITIALIZED)) == kSetPropertyNotInitializedCode); + + constexpr int kRejectedStatuses[] = { + MPV_ERROR_INVALID_PARAMETER, + MPV_ERROR_PROPERTY_ERROR, + -1, + }; + for (const int status : kRejectedStatuses) { + assert(!SetPropertyStatusSucceeded(status)); + assert(std::string(SetPropertyErrorCode(status)) == kSetPropertyFailedCode); + } + + constexpr int kDescribedStatuses[] = { MPV_ERROR_INVALID_PARAMETER, MPV_ERROR_PROPERTY_ERROR, -1, MPV_ERROR_UNINITIALIZED, }; - for (const int status : kFailureStatuses) { - assert(!SetPropertyStatusSucceeded(status)); + for (const int status : kDescribedStatuses) { const std::string description = SetPropertyErrorDescription(status); assert(!description.empty()); assert(description.size() <= kSetPropertyErrorDescriptionLimit); diff --git a/tvos/Runner.xcodeproj/project.pbxproj b/tvos/Runner.xcodeproj/project.pbxproj index 525b4906..2172c48f 100644 --- a/tvos/Runner.xcodeproj/project.pbxproj +++ b/tvos/Runner.xcodeproj/project.pbxproj @@ -10,6 +10,7 @@ 055E465F9095D0B6E9B91D46 /* PathProviderPlugin.swift in Sources */ = {isa = PBXBuildFile; fileRef = D41AA251EF365516E2AC5287 /* PathProviderPlugin.swift */; }; 1C6B234E223CDFFE5BF3FC0B /* TVServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 25B7925EFD8A2C1C7EB667D5 /* TVServices.framework */; }; 28DB4404B17342F46BC2B0A1 /* TopShelfProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = F2F829B3F190657106F66379 /* TopShelfProvider.swift */; }; + 2A7C0B1D9E5F4A6381027C12 /* MpvPlayerContractTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2A7C0B1D9E5F4A6381027C11 /* MpvPlayerContractTests.swift */; }; 35DB0C8FEF635A3BCA0B722A /* PackageInfoPlusPlugin.swift in Sources */ = {isa = PBXBuildFile; fileRef = F9426EFA282CDA8E0E98EEE9 /* PackageInfoPlusPlugin.swift */; }; 3B3967161E833CAA004F5970 /* AppFrameworkInfo.plist in Resources */ = {isa = PBXBuildFile; fileRef = 3B3967151E833CAA004F5970 /* AppFrameworkInfo.plist */; }; 5C71F5F7B33075F2B007825B /* MpvPlayerPluginShared.swift in Sources */ = {isa = PBXBuildFile; fileRef = 73645904F226A24585A092CE /* MpvPlayerPluginShared.swift */; }; @@ -117,6 +118,7 @@ 97C147021CF9000F007C117D /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; 9D7A998830EDC8F77BF521D1 /* MpvPlayerPlugin.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; name = MpvPlayerPlugin.swift; path = ../ios/Runner/MpvPlayer/MpvPlayerPlugin.swift; sourceTree = ""; }; A12B8610AE5D580077264851 /* MpvPlayerCore.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; name = MpvPlayerCore.swift; path = ../ios/Runner/MpvPlayer/MpvPlayerCore.swift; sourceTree = ""; }; + 2A7C0B1D9E5F4A6381027C11 /* MpvPlayerContractTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = MpvPlayerContractTests.swift; sourceTree = ""; }; A2484B9C94406BF0A99EB64A /* Pods-RunnerTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-RunnerTests.debug.xcconfig"; path = "Target Support Files/Pods-RunnerTests/Pods-RunnerTests.debug.xcconfig"; sourceTree = ""; }; A2635E12EB9322B151EE5127 /* MpvPipController.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; name = MpvPipController.swift; path = ../ios/Runner/MpvPlayer/MpvPipController.swift; sourceTree = ""; }; A5E1F001234567890ABCDE01 /* SystemShelfPluginTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SystemShelfPluginTests.swift; sourceTree = ""; }; @@ -335,6 +337,7 @@ F9BDF6E09E3B9347861D2A50 /* RunnerTests */ = { isa = PBXGroup; children = ( + 2A7C0B1D9E5F4A6381027C11 /* MpvPlayerContractTests.swift */, FE6C8125B201A8BC3261AE2B /* TvosEventDeliveryCoordinatorTests.swift */, 934BC4E316D2AC788C954766 /* ConnectivityPlusPluginTests.swift */, A5E1F001234567890ABCDE01 /* SystemShelfPluginTests.swift */, @@ -587,6 +590,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( + 2A7C0B1D9E5F4A6381027C12 /* MpvPlayerContractTests.swift in Sources */, D2004D7BB4A40340AB7A01E0 /* TvosEventDeliveryCoordinatorTests.swift in Sources */, 65AC2C222043B3E6723E2076 /* ConnectivityPlusPluginTests.swift in Sources */, A5E1F001234567890ABCDE02 /* SystemShelfPluginTests.swift in Sources */, diff --git a/tvos/Runner/SystemShelfPlugin.swift b/tvos/Runner/SystemShelfPlugin.swift index 0ff60efe..3b212a48 100644 --- a/tvos/Runner/SystemShelfPlugin.swift +++ b/tvos/Runner/SystemShelfPlugin.swift @@ -561,18 +561,15 @@ import TVServices else { return nil } let data = download.data let key = UUID().uuidString.lowercased().replacingOccurrences(of: "-", with: "") + ".art" - let staged = directory.appendingPathComponent(".\(key).tmp") let destination = directory.appendingPathComponent(key) do { - try data.write(to: staged, options: [.atomic, .completeFileProtectionUntilFirstUserAuthentication]) - if FileManager.default.fileExists(atPath: destination.path) { - try FileManager.default.removeItem(at: destination) - } - try FileManager.default.moveItem(at: staged, to: destination) + try data.write( + to: destination, + options: [.atomic, .completeFileProtectionUntilFirstUserAuthentication] + ) remaining -= data.count return key } catch { - try? FileManager.default.removeItem(at: staged) return nil } } diff --git a/tvos/RunnerTests/MpvPlayerContractTests.swift b/tvos/RunnerTests/MpvPlayerContractTests.swift new file mode 100644 index 00000000..62cbcdeb --- /dev/null +++ b/tvos/RunnerTests/MpvPlayerContractTests.swift @@ -0,0 +1,78 @@ +import Foundation +import Flutter +import XCTest + +@testable import Runner + +private final class TvosControllablePropertyCore: MpvPlayerCoreBase { + var nextResult: Result? + + override func setPropertyAsync( + _ name: String, + value: String, + completion: @escaping (Result) -> Void + ) { + guard let nextResult else { + XCTFail("A controlled property result was not configured") + return + } + self.nextResult = nil + completion(nextResult) + } +} + +private final class TvosRecordingMpvPlugin: MpvPluginShared { + var coreBase: MpvPlayerCoreBase? + var eventSink: FlutterEventSink? + var nameToId: [String: Int] = [:] + + init(core: MpvPlayerCoreBase?) { + coreBase = core + } + + func setPlayerVisible(_ visible: Bool, restoreOnWindowVisible: Bool) {} + func updatePlayerFrame() {} + func didSetPauseProperty(value: String) {} +} + +final class MpvPlayerContractTests: XCTestCase { + func testSharedSetPropertyMapsLifecycleCancellationAsNotInitialized() { + let core = TvosControllablePropertyCore() + let plugin = TvosRecordingMpvPlugin(core: core) + core.nextResult = .failure(MpvLifecycleUnavailableError("controlled cancellation")) + + let results = invokeSetProperty(plugin) + + XCTAssertEqual(results.count, 1) + XCTAssertEqual((results[0] as? FlutterError)?.code, "NOT_INITIALIZED") + } + + func testSharedSetPropertyKeepsGenuineRejectionNonRecoverable() { + let core = TvosControllablePropertyCore() + let plugin = TvosRecordingMpvPlugin(core: core) + core.nextResult = .failure( + NSError( + domain: "MpvPlayerContractTests", + code: 1, + userInfo: [NSLocalizedDescriptionKey: "controlled rejection"] + )) + + let results = invokeSetProperty(plugin) + + XCTAssertEqual(results.count, 1) + XCTAssertEqual((results[0] as? FlutterError)?.code, "SET_PROPERTY_FAILED") + } + + private func invokeSetProperty(_ plugin: TvosRecordingMpvPlugin) -> [Any?] { + var results: [Any?] = [] + plugin.handleSetProperty( + call: FlutterMethodCall( + methodName: "setProperty", + arguments: ["name": "volume", "value": "50"] + ) + ) { + results.append($0) + } + return results + } +} diff --git a/windows/runner/mpv/display_mode_manager.cpp b/windows/runner/mpv/display_mode_manager.cpp index c360ed5e..31f2b44b 100644 --- a/windows/runner/mpv/display_mode_manager.cpp +++ b/windows/runner/mpv/display_mode_manager.cpp @@ -23,6 +23,20 @@ constexpr wchar_t kRegOriginalHeight[] = L"OriginalHeight"; constexpr wchar_t kRegOriginalHDR[] = L"OriginalHDREnabled"; constexpr wchar_t kRegModeChanged[] = L"ModeChanged"; constexpr wchar_t kRegHDRChanged[] = L"HDRChanged"; +constexpr wchar_t kRegModeTakeoverEligible[] = L"ModeTakeoverEligible"; +constexpr wchar_t kRegHDRTakeoverEligible[] = L"HDRTakeoverEligible"; +constexpr wchar_t kRegModeRecoverySlot[] = L"ModeRecoverySlot"; +constexpr wchar_t kRegHDRRecoverySlot[] = L"HDRRecoverySlot"; +constexpr wchar_t kRegModeDeviceNameAlternate[] = L"ModeDeviceNameAlternate"; +constexpr wchar_t kRegHDRDeviceNameAlternate[] = L"HDRDeviceNameAlternate"; +constexpr wchar_t kRegOriginalRefreshRateAlternate[] = L"OriginalRefreshRateAlternate"; +constexpr wchar_t kRegOriginalWidthAlternate[] = L"OriginalWidthAlternate"; +constexpr wchar_t kRegOriginalHeightAlternate[] = L"OriginalHeightAlternate"; +constexpr wchar_t kRegOriginalHDRAlternate[] = L"OriginalHDREnabledAlternate"; +constexpr wchar_t kRegModeHandoffPending[] = L"ModeHandoffPending"; +constexpr wchar_t kRegHDRHandoffPending[] = L"HDRHandoffPending"; +constexpr wchar_t kRegModePreviousRecoverySlot[] = L"ModePreviousRecoverySlot"; +constexpr wchar_t kRegHDRPreviousRecoverySlot[] = L"HDRPreviousRecoverySlot"; std::recursive_mutex g_display_override_mutex; bool g_live_mode_recovery_record = false; @@ -46,7 +60,11 @@ class RecoveryRunGuard { bool PrepareModeRecoveryAtRegistry(const std::wstring& device_name, DWORD width, DWORD height, DWORD refresh_rate); bool PrepareHDRRecoveryAtRegistry(const std::wstring& device_name, bool enabled); -bool CompleteRecoveryOperationAtRegistry(const wchar_t* marker); +bool CompleteRecoveryOperationAtRegistry( + const wchar_t* marker, const wchar_t* takeover_disposition, const wchar_t* handoff_pending); +bool FinalizeModeRecoveryAtRegistry(bool os_apply_succeeded); +bool FinalizeHDRRecoveryAtRegistry(bool os_apply_succeeded); +bool MarkTakeoverEligibleAtRegistry(const wchar_t* takeover_disposition); } // namespace @@ -240,12 +258,14 @@ bool DisplayModeManager::SetDisplayMode(HWND window, DWORD width, DWORD height, } if (changed) { + FinalizeModeRecoveryAtRegistry(true); mode_changed_ = true; } else if (!mode_was_changed) { - // Do not discard an independently persisted HDR operation owned by - // another manager or retained from startup recovery. - CompleteRecoveryOperationAtRegistry(kRegModeChanged); + // A failed takeover rolls back to its prior marked original. A fresh + // failed operation still clears only its own marker. + const bool recovery_completed = FinalizeModeRecoveryAtRegistry(false); g_live_mode_recovery_record = false; + if (!recovery_completed) MarkTakeoverEligibleAtRegistry(kRegModeTakeoverEligible); } return changed; @@ -256,6 +276,7 @@ bool DisplayModeManager::RestoreOriginalMode(HWND) { if (!mode_changed_) return false; if (original_device_name_.empty()) { g_live_mode_recovery_record = false; + MarkTakeoverEligibleAtRegistry(kRegModeTakeoverEligible); return false; } @@ -269,14 +290,18 @@ bool DisplayModeManager::RestoreOriginalMode(HWND) { } if (rc != DISP_CHANGE_SUCCESSFUL) { // The explicit owner has given up. Keep the durable marker, but release it - // so a later topology notification can restore a reconnected target. + // so a later topology notification can restore a reconnected target or a + // conflicting mode request can consume the failed recovery disposition. g_live_mode_recovery_record = false; + MarkTakeoverEligibleAtRegistry(kRegModeTakeoverEligible); return false; } mode_changed_ = false; - CompleteRecoveryOperationAtRegistry(kRegModeChanged); + const bool recovery_completed = + CompleteRecoveryOperationAtRegistry(kRegModeChanged, kRegModeTakeoverEligible, kRegModeHandoffPending); g_live_mode_recovery_record = false; + if (!recovery_completed) MarkTakeoverEligibleAtRegistry(kRegModeTakeoverEligible); return true; } @@ -385,8 +410,9 @@ bool DisplayModeManager::SetHDREnabled(HWND window, bool enabled) { const LONG result = SetHDRStateForTarget(*target_id, enabled); if (result != ERROR_SUCCESS) { if (!hdr_was_changed) { - CompleteRecoveryOperationAtRegistry(kRegHDRChanged); + const bool recovery_completed = FinalizeHDRRecoveryAtRegistry(false); g_live_hdr_recovery_record = false; + if (!recovery_completed) MarkTakeoverEligibleAtRegistry(kRegHDRTakeoverEligible); } return false; } @@ -398,6 +424,7 @@ bool DisplayModeManager::SetHDREnabled(HWND window, bool enabled) { ChangeDisplaySettingsExW(device_name.c_str(), &pre_toggle_dm, nullptr, CDS_FULLSCREEN, nullptr); } + FinalizeHDRRecoveryAtRegistry(true); hdr_changed_ = true; return true; } @@ -407,12 +434,14 @@ bool DisplayModeManager::RestoreOriginalHDRState(HWND window) { if (!hdr_changed_) return false; if (original_hdr_device_name_.empty()) { g_live_hdr_recovery_record = false; + MarkTakeoverEligibleAtRegistry(kRegHDRTakeoverEligible); return false; } const auto target_id = GetDisplayTargetId(original_hdr_device_name_); if (!target_id) { g_live_hdr_recovery_record = false; + MarkTakeoverEligibleAtRegistry(kRegHDRTakeoverEligible); return false; } @@ -428,6 +457,7 @@ bool DisplayModeManager::RestoreOriginalHDRState(HWND window) { if (SetHDRStateForTarget(*target_id, original_hdr_enabled_) != ERROR_SUCCESS) { g_live_hdr_recovery_record = false; + MarkTakeoverEligibleAtRegistry(kRegHDRTakeoverEligible); return false; } @@ -439,8 +469,10 @@ bool DisplayModeManager::RestoreOriginalHDRState(HWND window) { } hdr_changed_ = false; - CompleteRecoveryOperationAtRegistry(kRegHDRChanged); + const bool recovery_completed = + CompleteRecoveryOperationAtRegistry(kRegHDRChanged, kRegHDRTakeoverEligible, kRegHDRHandoffPending); g_live_hdr_recovery_record = false; + if (!recovery_completed) MarkTakeoverEligibleAtRegistry(kRegHDRTakeoverEligible); return true; } @@ -528,8 +560,30 @@ bool RecoveryRecordExists() { } bool exists = false; for (const wchar_t* value_name : - {kRegVersion, kRegModeDeviceName, kRegLegacyDeviceName, kRegHDRDeviceName, kRegOriginalRefreshRate, - kRegOriginalWidth, kRegOriginalHeight, kRegOriginalHDR, kRegModeChanged, kRegHDRChanged}) { + {kRegVersion, + kRegModeDeviceName, + kRegLegacyDeviceName, + kRegHDRDeviceName, + kRegOriginalRefreshRate, + kRegOriginalWidth, + kRegOriginalHeight, + kRegOriginalHDR, + kRegModeChanged, + kRegHDRChanged, + kRegModeTakeoverEligible, + kRegHDRTakeoverEligible, + kRegModeRecoverySlot, + kRegHDRRecoverySlot, + kRegModeDeviceNameAlternate, + kRegHDRDeviceNameAlternate, + kRegOriginalRefreshRateAlternate, + kRegOriginalWidthAlternate, + kRegOriginalHeightAlternate, + kRegOriginalHDRAlternate, + kRegModeHandoffPending, + kRegHDRHandoffPending, + kRegModePreviousRecoverySlot, + kRegHDRPreviousRecoverySlot}) { DWORD size = 0; const LONG result = RegQueryValueExW(key, value_name, nullptr, nullptr, nullptr, &size); if (result == ERROR_SUCCESS || result == ERROR_MORE_DATA) { @@ -606,8 +660,30 @@ class Win32DisplayRecoveryBackend final : public DisplayRecoveryBackend { bool deleted = true; for (const wchar_t* value_name : - {kRegVersion, kRegModeDeviceName, kRegLegacyDeviceName, kRegHDRDeviceName, kRegOriginalRefreshRate, - kRegOriginalWidth, kRegOriginalHeight, kRegOriginalHDR, kRegModeChanged, kRegHDRChanged}) { + {kRegVersion, + kRegModeDeviceName, + kRegLegacyDeviceName, + kRegHDRDeviceName, + kRegOriginalRefreshRate, + kRegOriginalWidth, + kRegOriginalHeight, + kRegOriginalHDR, + kRegModeChanged, + kRegHDRChanged, + kRegModeTakeoverEligible, + kRegHDRTakeoverEligible, + kRegModeRecoverySlot, + kRegHDRRecoverySlot, + kRegModeDeviceNameAlternate, + kRegHDRDeviceNameAlternate, + kRegOriginalRefreshRateAlternate, + kRegOriginalWidthAlternate, + kRegOriginalHeightAlternate, + kRegOriginalHDRAlternate, + kRegModeHandoffPending, + kRegHDRHandoffPending, + kRegModePreviousRecoverySlot, + kRegHDRPreviousRecoverySlot}) { const LONG result = RegDeleteValueW(key, value_name); deleted = deleted && (result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND); } @@ -620,33 +696,78 @@ namespace { bool ReadValidModeValues( DisplayRecoveryBackend& backend, const wchar_t* device_value_name, std::wstring& device_name, DWORD& width, - DWORD& height, DWORD& refresh_rate) { - return backend.ReadString(device_value_name, device_name) && backend.ReadDWORD(kRegOriginalWidth, width) && - width > 0 && backend.ReadDWORD(kRegOriginalHeight, height) && height > 0 && - backend.ReadDWORD(kRegOriginalRefreshRate, refresh_rate) && refresh_rate > 0; + DWORD& height, DWORD& refresh_rate, bool alternate = false) { + const wchar_t* width_value_name = alternate ? kRegOriginalWidthAlternate : kRegOriginalWidth; + const wchar_t* height_value_name = alternate ? kRegOriginalHeightAlternate : kRegOriginalHeight; + const wchar_t* refresh_value_name = alternate ? kRegOriginalRefreshRateAlternate : kRegOriginalRefreshRate; + return backend.ReadString(device_value_name, device_name) && backend.ReadDWORD(width_value_name, width) && + width > 0 && backend.ReadDWORD(height_value_name, height) && height > 0 && + backend.ReadDWORD(refresh_value_name, refresh_rate) && refresh_rate > 0; } bool ReadValidHDRValues( - DisplayRecoveryBackend& backend, const wchar_t* device_value_name, std::wstring& device_name, DWORD& original_hdr) { - return backend.ReadString(device_value_name, device_name) && backend.ReadDWORD(kRegOriginalHDR, original_hdr) && + DisplayRecoveryBackend& backend, const wchar_t* device_value_name, std::wstring& device_name, DWORD& original_hdr, + bool alternate = false) { + const wchar_t* original_value_name = alternate ? kRegOriginalHDRAlternate : kRegOriginalHDR; + return backend.ReadString(device_value_name, device_name) && backend.ReadDWORD(original_value_name, original_hdr) && original_hdr <= 1; } -bool ReadValidMarkedMode( - DisplayRecoveryBackend& backend, std::wstring& device_name, DWORD& width, DWORD& height, DWORD& refresh_rate) { - DWORD version = 0; - DWORD marker = 0; - return backend.ReadDWORD(kRegVersion, version) && version == kRecoveryVersion && - backend.ReadDWORD(kRegModeChanged, marker) && marker == 1 && - ReadValidModeValues(backend, kRegModeDeviceName, device_name, width, height, refresh_rate); +bool ReadRecoverySlot(DisplayRecoveryBackend& backend, const wchar_t* slot_value_name, bool& alternate) { + DWORD slot = 0; + if (!backend.ReadDWORD(slot_value_name, slot)) { + // Version-1 records created before alternate slots implicitly use the + // original value set. + alternate = false; + return true; + } + if (slot > 1) return false; + alternate = slot == 1; + return true; } -bool ReadValidMarkedHDR(DisplayRecoveryBackend& backend, std::wstring& device_name, DWORD& original_hdr) { +bool ReadValidModeSlot( + DisplayRecoveryBackend& backend, bool alternate, std::wstring& device_name, DWORD& width, DWORD& height, + DWORD& refresh_rate) { + return ReadValidModeValues( + backend, alternate ? kRegModeDeviceNameAlternate : kRegModeDeviceName, device_name, width, height, refresh_rate, + alternate); +} + +bool ReadValidHDRSlot(DisplayRecoveryBackend& backend, bool alternate, std::wstring& device_name, DWORD& original_hdr) { + return ReadValidHDRValues( + backend, alternate ? kRegHDRDeviceNameAlternate : kRegHDRDeviceName, device_name, original_hdr, alternate); +} + +bool ReadValidMarkedMode( + DisplayRecoveryBackend& backend, std::wstring& device_name, DWORD& width, DWORD& height, DWORD& refresh_rate, + bool* alternate_slot = nullptr) { DWORD version = 0; DWORD marker = 0; - return backend.ReadDWORD(kRegVersion, version) && version == kRecoveryVersion && - backend.ReadDWORD(kRegHDRChanged, marker) && marker == 1 && - ReadValidHDRValues(backend, kRegHDRDeviceName, device_name, original_hdr); + bool alternate = false; + if (!backend.ReadDWORD(kRegVersion, version) || version != kRecoveryVersion || + !backend.ReadDWORD(kRegModeChanged, marker) || marker != 1 || + !ReadRecoverySlot(backend, kRegModeRecoverySlot, alternate) || + !ReadValidModeSlot(backend, alternate, device_name, width, height, refresh_rate)) { + return false; + } + if (alternate_slot) *alternate_slot = alternate; + return true; +} + +bool ReadValidMarkedHDR( + DisplayRecoveryBackend& backend, std::wstring& device_name, DWORD& original_hdr, bool* alternate_slot = nullptr) { + DWORD version = 0; + DWORD marker = 0; + bool alternate = false; + if (!backend.ReadDWORD(kRegVersion, version) || version != kRecoveryVersion || + !backend.ReadDWORD(kRegHDRChanged, marker) || marker != 1 || + !ReadRecoverySlot(backend, kRegHDRRecoverySlot, alternate) || + !ReadValidHDRSlot(backend, alternate, device_name, original_hdr)) { + return false; + } + if (alternate_slot) *alternate_slot = alternate; + return true; } bool DeleteRecordIfNoMarkedOperations(DisplayRecoveryBackend& backend) { @@ -662,12 +783,82 @@ bool DeleteRecordIfNoMarkedOperations(DisplayRecoveryBackend& backend) { return true; } -bool CompleteRecoveryOperation(DisplayRecoveryBackend& backend, const wchar_t* marker) { - if (!backend.ClearMarker(marker)) return false; +bool IsTakeoverEligible(DisplayRecoveryBackend& backend, const wchar_t* takeover_disposition) { + DWORD value = 0; + return backend.ReadDWORD(takeover_disposition, value) && value == 1; +} + +bool MarkTakeoverEligible(DisplayRecoveryBackend& backend, const wchar_t* takeover_disposition) { + return backend.WriteDWORD(takeover_disposition, 1); +} + +bool ResetTakeoverEligibility(DisplayRecoveryBackend& backend, const wchar_t* takeover_disposition) { + return backend.WriteDWORD(takeover_disposition, 0); +} + +bool ReadHandoffPending(DisplayRecoveryBackend& backend, const wchar_t* handoff_value_name, bool& pending) { + DWORD value = 0; + if (!backend.ReadDWORD(handoff_value_name, value)) { + pending = false; + return true; + } + if (value > 1) return false; + pending = value == 1; + return true; +} + +bool ReadStoredRecoverySlot(DisplayRecoveryBackend& backend, const wchar_t* slot_value_name, bool& alternate) { + DWORD value = 0; + if (!backend.ReadDWORD(slot_value_name, value) || value > 1) return false; + alternate = value == 1; + return true; +} + +bool CompleteRecoveryOperation( + DisplayRecoveryBackend& backend, const wchar_t* marker, const wchar_t* takeover_disposition, + const wchar_t* handoff_pending) { + // Handoff metadata must be inactive before its shared marker disappears. + if (!backend.WriteDWORD(handoff_pending, 0) || !ResetTakeoverEligibility(backend, takeover_disposition) || + !backend.ClearMarker(marker)) { + return false; + } DeleteRecordIfNoMarkedOperations(backend); return true; } +bool ConfirmPreparedRecovery(DisplayRecoveryBackend& backend, const wchar_t* handoff_pending) { + bool pending = false; + if (!ReadHandoffPending(backend, handoff_pending, pending)) return false; + return !pending || backend.WriteDWORD(handoff_pending, 0); +} + +bool RollbackRecoveryHandoff( + DisplayRecoveryBackend& backend, const wchar_t* takeover_disposition, const wchar_t* recovery_slot, + const wchar_t* previous_recovery_slot, const wchar_t* handoff_pending) { + bool previous_alternate = false; + if (!ReadStoredRecoverySlot(backend, previous_recovery_slot, previous_alternate) || + !backend.WriteDWORD(recovery_slot, previous_alternate ? 1 : 0) || + !MarkTakeoverEligible(backend, takeover_disposition) || !backend.WriteDWORD(handoff_pending, 0)) { + return false; + } + return true; +} + +bool FinalizePreparedRecovery( + DisplayRecoveryBackend& backend, bool os_apply_succeeded, const wchar_t* marker, + const wchar_t* takeover_disposition, const wchar_t* recovery_slot, const wchar_t* previous_recovery_slot, + const wchar_t* handoff_pending) { + if (os_apply_succeeded) return ConfirmPreparedRecovery(backend, handoff_pending); + + bool pending = false; + if (!ReadHandoffPending(backend, handoff_pending, pending)) return false; + if (pending) { + return RollbackRecoveryHandoff( + backend, takeover_disposition, recovery_slot, previous_recovery_slot, handoff_pending); + } + return CompleteRecoveryOperation(backend, marker, takeover_disposition, handoff_pending); +} + bool PreserveValidModeSiblingOrClear(DisplayRecoveryBackend& backend) { DWORD marker = 0; if (!backend.ReadDWORD(kRegModeChanged, marker)) { @@ -711,22 +902,59 @@ bool DisplayModeManager::PrepareModeRecovery( DWORD existing_height = 0; DWORD existing_refresh_rate = 0; std::wstring existing_device_name; - if (ReadValidMarkedMode(backend, existing_device_name, existing_width, existing_height, existing_refresh_rate)) { - // A valid marked original is already protecting a live or failed - // operation. Reuse it only when this manager has the same original; - // replacing it would lose the only restoration point. - return existing_device_name == device_name && existing_width == width && existing_height == height && - existing_refresh_rate == refresh_rate; + bool existing_alternate = false; + if (ReadValidMarkedMode( + backend, existing_device_name, existing_width, existing_height, existing_refresh_rate, &existing_alternate)) { + bool handoff_pending = false; + if (!ReadHandoffPending(backend, kRegModeHandoffPending, handoff_pending) || handoff_pending) return false; + + const bool same_original = existing_device_name == device_name && existing_width == width && + existing_height == height && existing_refresh_rate == refresh_rate; + if (same_original) { + // Reusing the restoration point makes it live again, so stale takeover + // permission must be durably revoked before the Windows mutation. + return ResetTakeoverEligibility(backend, kRegModeTakeoverEligible); + } + if (!IsTakeoverEligible(backend, kRegModeTakeoverEligible)) return false; + + // Stage the replacement in the inactive slot. Handoff metadata keeps both + // originals recoverable from the selector switch until the OS apply is + // confirmed. + const bool replacement_alternate = !existing_alternate; + const wchar_t* replacement_device_name = replacement_alternate ? kRegModeDeviceNameAlternate : kRegModeDeviceName; + const wchar_t* replacement_width = replacement_alternate ? kRegOriginalWidthAlternate : kRegOriginalWidth; + const wchar_t* replacement_height = replacement_alternate ? kRegOriginalHeightAlternate : kRegOriginalHeight; + const wchar_t* replacement_refresh = + replacement_alternate ? kRegOriginalRefreshRateAlternate : kRegOriginalRefreshRate; + if (!backend.WriteDWORD(kRegVersion, kRecoveryVersion) || + !backend.WriteString(replacement_device_name, device_name) || !backend.WriteDWORD(replacement_width, width) || + !backend.WriteDWORD(replacement_height, height) || !backend.WriteDWORD(replacement_refresh, refresh_rate) || + !backend.WriteDWORD(kRegModePreviousRecoverySlot, existing_alternate ? 1 : 0) || + !ResetTakeoverEligibility(backend, kRegModeTakeoverEligible)) { + return false; + } + if (!backend.WriteDWORD(kRegModeHandoffPending, 1)) { + MarkTakeoverEligible(backend, kRegModeTakeoverEligible); + return false; + } + if (!backend.WriteDWORD(kRegModeRecoverySlot, replacement_alternate ? 1 : 0)) { + RollbackRecoveryHandoff( + backend, kRegModeTakeoverEligible, kRegModeRecoverySlot, kRegModePreviousRecoverySlot, + kRegModeHandoffPending); + return false; + } + return true; } - // Deactivate an incomplete old mode operation before replacing any - // originals. A crash anywhere before the final write is therefore a - // harmless pre-mutation prefix. + // An incomplete operation has no authoritative original to preserve. Keep + // the existing marker-last preparation sequence and select the primary slot + // before publishing the new operation. if (!backend.ClearMarker(kRegModeChanged)) return false; - return backend.WriteDWORD(kRegVersion, kRecoveryVersion) && backend.WriteString(kRegModeDeviceName, device_name) && backend.WriteDWORD(kRegOriginalWidth, width) && backend.WriteDWORD(kRegOriginalHeight, height) && - backend.WriteDWORD(kRegOriginalRefreshRate, refresh_rate) && backend.WriteDWORD(kRegModeChanged, 1); + backend.WriteDWORD(kRegOriginalRefreshRate, refresh_rate) && backend.WriteDWORD(kRegModeHandoffPending, 0) && + ResetTakeoverEligibility(backend, kRegModeTakeoverEligible) && backend.WriteDWORD(kRegModeRecoverySlot, 0) && + backend.WriteDWORD(kRegModeChanged, 1); } bool DisplayModeManager::PrepareHDRRecovery( @@ -736,14 +964,42 @@ bool DisplayModeManager::PrepareHDRRecovery( DWORD existing_original = 0; std::wstring existing_device_name; - if (ReadValidMarkedHDR(backend, existing_device_name, existing_original)) { - return existing_device_name == device_name && existing_original == (enabled ? 1u : 0u); + bool existing_alternate = false; + if (ReadValidMarkedHDR(backend, existing_device_name, existing_original, &existing_alternate)) { + bool handoff_pending = false; + if (!ReadHandoffPending(backend, kRegHDRHandoffPending, handoff_pending) || handoff_pending) return false; + + const bool same_original = existing_device_name == device_name && existing_original == (enabled ? 1u : 0u); + if (same_original) return ResetTakeoverEligibility(backend, kRegHDRTakeoverEligible); + if (!IsTakeoverEligible(backend, kRegHDRTakeoverEligible)) return false; + + const bool replacement_alternate = !existing_alternate; + const wchar_t* replacement_device_name = replacement_alternate ? kRegHDRDeviceNameAlternate : kRegHDRDeviceName; + const wchar_t* replacement_original = replacement_alternate ? kRegOriginalHDRAlternate : kRegOriginalHDR; + if (!backend.WriteDWORD(kRegVersion, kRecoveryVersion) || + !backend.WriteString(replacement_device_name, device_name) || + !backend.WriteDWORD(replacement_original, enabled ? 1 : 0) || + !backend.WriteDWORD(kRegHDRPreviousRecoverySlot, existing_alternate ? 1 : 0) || + !ResetTakeoverEligibility(backend, kRegHDRTakeoverEligible)) { + return false; + } + if (!backend.WriteDWORD(kRegHDRHandoffPending, 1)) { + MarkTakeoverEligible(backend, kRegHDRTakeoverEligible); + return false; + } + if (!backend.WriteDWORD(kRegHDRRecoverySlot, replacement_alternate ? 1 : 0)) { + RollbackRecoveryHandoff( + backend, kRegHDRTakeoverEligible, kRegHDRRecoverySlot, kRegHDRPreviousRecoverySlot, kRegHDRHandoffPending); + return false; + } + return true; } if (!backend.ClearMarker(kRegHDRChanged)) return false; - return backend.WriteDWORD(kRegVersion, kRecoveryVersion) && backend.WriteString(kRegHDRDeviceName, device_name) && - backend.WriteDWORD(kRegOriginalHDR, enabled ? 1 : 0) && backend.WriteDWORD(kRegHDRChanged, 1); + backend.WriteDWORD(kRegOriginalHDR, enabled ? 1 : 0) && backend.WriteDWORD(kRegHDRHandoffPending, 0) && + ResetTakeoverEligibility(backend, kRegHDRTakeoverEligible) && backend.WriteDWORD(kRegHDRRecoverySlot, 0) && + backend.WriteDWORD(kRegHDRChanged, 1); } namespace { @@ -758,9 +1014,29 @@ bool PrepareHDRRecoveryAtRegistry(const std::wstring& device_name, bool enabled) return DisplayModeManager::PrepareHDRRecovery(backend, device_name, enabled); } -bool CompleteRecoveryOperationAtRegistry(const wchar_t* marker) { +bool CompleteRecoveryOperationAtRegistry( + const wchar_t* marker, const wchar_t* takeover_disposition, const wchar_t* handoff_pending) { Win32DisplayRecoveryBackend backend; - return CompleteRecoveryOperation(backend, marker); + return CompleteRecoveryOperation(backend, marker, takeover_disposition, handoff_pending); +} + +bool FinalizeModeRecoveryAtRegistry(bool os_apply_succeeded) { + Win32DisplayRecoveryBackend backend; + return FinalizePreparedRecovery( + backend, os_apply_succeeded, kRegModeChanged, kRegModeTakeoverEligible, kRegModeRecoverySlot, + kRegModePreviousRecoverySlot, kRegModeHandoffPending); +} + +bool FinalizeHDRRecoveryAtRegistry(bool os_apply_succeeded) { + Win32DisplayRecoveryBackend backend; + return FinalizePreparedRecovery( + backend, os_apply_succeeded, kRegHDRChanged, kRegHDRTakeoverEligible, kRegHDRRecoverySlot, + kRegHDRPreviousRecoverySlot, kRegHDRHandoffPending); +} + +bool MarkTakeoverEligibleAtRegistry(const wchar_t* takeover_disposition) { + Win32DisplayRecoveryBackend backend; + return MarkTakeoverEligible(backend, takeover_disposition); } bool RecoverRecord(DisplayRecoveryBackend& backend, bool mode_is_live, bool hdr_is_live) { @@ -794,9 +1070,32 @@ bool RecoverRecord(DisplayRecoveryBackend& backend, bool mode_is_live, bool hdr_ DWORD width = 0; DWORD height = 0; DWORD refresh_rate = 0; - const bool mode_requested = - mode_marker_read && mode_marker == 1 && - ReadValidModeValues(backend, mode_device_value_name, mode_device_name, width, height, refresh_rate); + bool mode_handoff_pending = false; + std::wstring previous_mode_device_name; + DWORD previous_width = 0; + DWORD previous_height = 0; + DWORD previous_refresh_rate = 0; + bool mode_requested = false; + if (mode_marker_read && mode_marker == 1) { + if (has_version) { + bool handoff_state_valid = ReadHandoffPending(backend, kRegModeHandoffPending, mode_handoff_pending); + if (handoff_state_valid && mode_handoff_pending) { + bool previous_alternate = false; + handoff_state_valid = + ReadStoredRecoverySlot(backend, kRegModePreviousRecoverySlot, previous_alternate) && + ReadValidModeSlot(backend, !previous_alternate, mode_device_name, width, height, refresh_rate) && + ReadValidModeSlot( + backend, previous_alternate, previous_mode_device_name, previous_width, previous_height, + previous_refresh_rate); + } else if (handoff_state_valid) { + handoff_state_valid = ReadValidMarkedMode(backend, mode_device_name, width, height, refresh_rate); + } + mode_requested = handoff_state_valid; + } else { + mode_requested = + ReadValidModeValues(backend, mode_device_value_name, mode_device_name, width, height, refresh_rate); + } + } if (!mode_is_live && (!mode_marker_read || mode_marker > 1 || (mode_marker == 1 && !mode_requested))) { // Malformation in one operation does not erase a valid or live sibling. backend.ClearMarker(kRegModeChanged); @@ -806,8 +1105,27 @@ bool RecoverRecord(DisplayRecoveryBackend& backend, bool mode_is_live, bool hdr_ const bool hdr_marker_read = backend.ReadDWORD(kRegHDRChanged, hdr_marker); std::wstring hdr_device_name; DWORD original_hdr = 0; - const bool hdr_requested = hdr_marker_read && hdr_marker == 1 && - ReadValidHDRValues(backend, hdr_device_value_name, hdr_device_name, original_hdr); + bool hdr_handoff_pending = false; + std::wstring previous_hdr_device_name; + DWORD previous_original_hdr = 0; + bool hdr_requested = false; + if (hdr_marker_read && hdr_marker == 1) { + if (has_version) { + bool handoff_state_valid = ReadHandoffPending(backend, kRegHDRHandoffPending, hdr_handoff_pending); + if (handoff_state_valid && hdr_handoff_pending) { + bool previous_alternate = false; + handoff_state_valid = + ReadStoredRecoverySlot(backend, kRegHDRPreviousRecoverySlot, previous_alternate) && + ReadValidHDRSlot(backend, !previous_alternate, hdr_device_name, original_hdr) && + ReadValidHDRSlot(backend, previous_alternate, previous_hdr_device_name, previous_original_hdr); + } else if (handoff_state_valid) { + handoff_state_valid = ReadValidMarkedHDR(backend, hdr_device_name, original_hdr); + } + hdr_requested = handoff_state_valid; + } else { + hdr_requested = ReadValidHDRValues(backend, hdr_device_value_name, hdr_device_name, original_hdr); + } + } if (!hdr_is_live && (!hdr_marker_read || hdr_marker > 1 || (hdr_marker == 1 && !hdr_requested))) { backend.ClearMarker(kRegHDRChanged); } @@ -821,19 +1139,43 @@ bool RecoverRecord(DisplayRecoveryBackend& backend, bool mode_is_live, bool hdr_ bool completed = true; if (recover_mode) { - if (backend.IsDevicePresent(mode_device_name) && - backend.RestoreMode(mode_device_name, width, height, refresh_rate)) { - // A failed marker clear leaves an idempotent restoration for a later pass. - completed = CompleteRecoveryOperation(backend, kRegModeChanged) && completed; - } else { + const bool restored = + backend.IsDevicePresent(mode_device_name) && backend.RestoreMode(mode_device_name, width, height, refresh_rate); + if (mode_handoff_pending) { + // Restore the replacement first and the pre-handoff original last. Both + // remain marked until both idempotent restores and cleanup succeed. + const bool previous_restored = + backend.IsDevicePresent(previous_mode_device_name) && + backend.RestoreMode(previous_mode_device_name, previous_width, previous_height, previous_refresh_rate); + if (!restored || !previous_restored || + !CompleteRecoveryOperation(backend, kRegModeChanged, kRegModeTakeoverEligible, kRegModeHandoffPending)) { + completed = false; + } + } else if ( + !restored || + !CompleteRecoveryOperation(backend, kRegModeChanged, kRegModeTakeoverEligible, kRegModeHandoffPending)) { + // Retain the restoration point for topology retries, but remember that + // one real non-live recovery attempt failed so a later conflicting mode + // request may deliberately replace it. + MarkTakeoverEligible(backend, kRegModeTakeoverEligible); completed = false; } } if (recover_hdr) { - if (backend.IsDevicePresent(hdr_device_name) && backend.RestoreHDR(hdr_device_name, original_hdr != 0)) { - completed = CompleteRecoveryOperation(backend, kRegHDRChanged) && completed; - } else { + const bool restored = + backend.IsDevicePresent(hdr_device_name) && backend.RestoreHDR(hdr_device_name, original_hdr != 0); + if (hdr_handoff_pending) { + const bool previous_restored = backend.IsDevicePresent(previous_hdr_device_name) && + backend.RestoreHDR(previous_hdr_device_name, previous_original_hdr != 0); + if (!restored || !previous_restored || + !CompleteRecoveryOperation(backend, kRegHDRChanged, kRegHDRTakeoverEligible, kRegHDRHandoffPending)) { + completed = false; + } + } else if ( + !restored || + !CompleteRecoveryOperation(backend, kRegHDRChanged, kRegHDRTakeoverEligible, kRegHDRHandoffPending)) { + MarkTakeoverEligible(backend, kRegHDRTakeoverEligible); completed = false; } } @@ -860,7 +1202,19 @@ bool DisplayModeManager::RecoverIfNeeded(DisplayRecoveryBackend& backend) { #if defined(PLEZY_DISPLAY_MODE_MANAGER_TESTING) bool DisplayModeManager::CompleteRecoveryOperationForTesting(DisplayRecoveryBackend& backend, bool mode) { std::lock_guard transaction_lock(g_display_override_mutex); - return CompleteRecoveryOperation(backend, mode ? kRegModeChanged : kRegHDRChanged); + return CompleteRecoveryOperation( + backend, mode ? kRegModeChanged : kRegHDRChanged, mode ? kRegModeTakeoverEligible : kRegHDRTakeoverEligible, + mode ? kRegModeHandoffPending : kRegHDRHandoffPending); +} + +bool DisplayModeManager::FinalizePreparedRecoveryForTesting( + DisplayRecoveryBackend& backend, bool mode, bool os_apply_succeeded) { + std::lock_guard transaction_lock(g_display_override_mutex); + return FinalizePreparedRecovery( + backend, os_apply_succeeded, mode ? kRegModeChanged : kRegHDRChanged, + mode ? kRegModeTakeoverEligible : kRegHDRTakeoverEligible, mode ? kRegModeRecoverySlot : kRegHDRRecoverySlot, + mode ? kRegModePreviousRecoverySlot : kRegHDRPreviousRecoverySlot, + mode ? kRegModeHandoffPending : kRegHDRHandoffPending); } bool DisplayModeManager::RecoverIfNeededForTesting( diff --git a/windows/runner/mpv/display_mode_manager.h b/windows/runner/mpv/display_mode_manager.h index 937347b5..800c88e9 100644 --- a/windows/runner/mpv/display_mode_manager.h +++ b/windows/runner/mpv/display_mode_manager.h @@ -105,21 +105,29 @@ class DisplayModeManager { // --- Crash recovery --- - // Persist a complete original followed by its operation marker. These - // runner-internal seams make the crash ordering deterministic in tests. + // Persist a complete original followed by its operation marker. A failed + // non-live recovery remains retryable, but durably permits a later + // conflicting request of the same kind to replace it. Conflicting originals + // are staged in an inactive slot. A persisted handoff keeps both originals + // recoverable across the selector switch and OS call; success confirms the + // new slot, while OS failure rolls back the prior selector and eligibility. + // Reusing the same original revokes permission before it becomes live. + // These runner-internal seams make the crash ordering deterministic in tests. static bool PrepareModeRecovery( DisplayRecoveryBackend& backend, const std::wstring& device_name, DWORD width, DWORD height, DWORD refresh_rate); static bool PrepareHDRRecovery(DisplayRecoveryBackend& backend, const std::wstring& device_name, bool enabled); // Check for and recover from a prior crash that left display settings - // changed. Successful operation markers are cleared independently. Failed - // operations remain for the next startup or display-topology notification. + // changed. Successful operation markers and their takeover dispositions are + // cleared independently. Failed operations remain recovery-first until a + // conflicting same-kind preparation consumes their persisted disposition. static bool RecoverIfNeeded(); static bool RecoverIfNeeded(DisplayRecoveryBackend& backend); #if defined(PLEZY_DISPLAY_MODE_MANAGER_TESTING) // Exercise persisted lifecycle exits and per-operation live ownership without // touching a real display or registry. static bool CompleteRecoveryOperationForTesting(DisplayRecoveryBackend& backend, bool mode); + static bool FinalizePreparedRecoveryForTesting(DisplayRecoveryBackend& backend, bool mode, bool os_apply_succeeded); static bool RecoverIfNeededForTesting(DisplayRecoveryBackend& backend, bool mode_is_live, bool hdr_is_live); #endif diff --git a/windows/runner/mpv/display_mode_manager_test.cpp b/windows/runner/mpv/display_mode_manager_test.cpp index cecde9ed..8c565c81 100644 --- a/windows/runner/mpv/display_mode_manager_test.cpp +++ b/windows/runner/mpv/display_mode_manager_test.cpp @@ -19,8 +19,24 @@ constexpr wchar_t kOriginalHeight[] = L"OriginalHeight"; constexpr wchar_t kOriginalHDR[] = L"OriginalHDREnabled"; constexpr wchar_t kModeChanged[] = L"ModeChanged"; constexpr wchar_t kHDRChanged[] = L"HDRChanged"; +constexpr wchar_t kModeTakeoverEligible[] = L"ModeTakeoverEligible"; +constexpr wchar_t kHDRTakeoverEligible[] = L"HDRTakeoverEligible"; +constexpr wchar_t kModeRecoverySlot[] = L"ModeRecoverySlot"; +constexpr wchar_t kHDRRecoverySlot[] = L"HDRRecoverySlot"; +constexpr wchar_t kModeDeviceNameAlternate[] = L"ModeDeviceNameAlternate"; +constexpr wchar_t kHDRDeviceNameAlternate[] = L"HDRDeviceNameAlternate"; +constexpr wchar_t kOriginalRefreshRateAlternate[] = L"OriginalRefreshRateAlternate"; +constexpr wchar_t kOriginalWidthAlternate[] = L"OriginalWidthAlternate"; +constexpr wchar_t kOriginalHeightAlternate[] = L"OriginalHeightAlternate"; +constexpr wchar_t kOriginalHDRAlternate[] = L"OriginalHDREnabledAlternate"; +constexpr wchar_t kModeHandoffPending[] = L"ModeHandoffPending"; +constexpr wchar_t kHDRHandoffPending[] = L"HDRHandoffPending"; +constexpr wchar_t kModePreviousRecoverySlot[] = L"ModePreviousRecoverySlot"; +constexpr wchar_t kHDRPreviousRecoverySlot[] = L"HDRPreviousRecoverySlot"; constexpr wchar_t kModeDevice[] = L"\\\\.\\DISPLAY1"; constexpr wchar_t kHDRDevice[] = L"\\\\.\\DISPLAY2"; +constexpr wchar_t kReplacementModeDevice[] = L"\\\\.\\DISPLAY3"; +constexpr wchar_t kReplacementHDRDevice[] = L"\\\\.\\DISPLAY4"; void Check(bool condition, const char* message) { if (!condition) { @@ -29,14 +45,35 @@ void Check(bool condition, const char* message) { } } +struct ExpectedModeRestore { + std::wstring device_name; + DWORD width; + DWORD height; + DWORD refresh_rate; +}; + +struct ExpectedHDRRestore { + std::wstring device_name; + bool enabled; +}; + class FakeRecoveryBackend final : public DisplayRecoveryBackend { public: std::map dwords; std::map strings; - std::map device_present = {{kModeDevice, true}, {kHDRDevice, true}}; + std::map device_present = { + {kModeDevice, true}, {kHDRDevice, true}, {kReplacementModeDevice, true}, {kReplacementHDRDevice, true}}; std::vector events; std::wstring expected_mode_device = kModeDevice; + DWORD expected_mode_width = 3840; + DWORD expected_mode_height = 2160; + DWORD expected_mode_refresh_rate = 60; std::wstring expected_hdr_device = kHDRDevice; + bool expected_original_hdr = false; + std::vector expected_mode_restores; + std::vector expected_hdr_restores; + size_t mode_restore_attempts = 0; + size_t hdr_restore_attempts = 0; bool mode_restore_succeeds = true; bool hdr_restore_succeeds = true; bool mode_marker_clear_succeeds = true; @@ -44,6 +81,7 @@ class FakeRecoveryBackend final : public DisplayRecoveryBackend { bool delete_succeeds = true; bool final_mode_marker_write_succeeds = true; bool final_hdr_marker_write_succeeds = true; + std::wstring rejected_dword_write_event; int delete_attempts = 0; void SeedBoth() { @@ -76,8 +114,10 @@ class FakeRecoveryBackend final : public DisplayRecoveryBackend { bool WriteDWORD(const wchar_t* value_name, DWORD value) override { const std::wstring name(value_name); - events.push_back(L"write:" + name + L"=" + std::to_wstring(value)); - if ((name == kModeChanged && value == 1 && !final_mode_marker_write_succeeds) || + const std::wstring event = L"write:" + name + L"=" + std::to_wstring(value); + events.push_back(event); + if (event == rejected_dword_write_event || + (name == kModeChanged && value == 1 && !final_mode_marker_write_succeeds) || (name == kHDRChanged && value == 1 && !final_hdr_marker_write_succeeds)) { return false; } @@ -98,15 +138,35 @@ class FakeRecoveryBackend final : public DisplayRecoveryBackend { } bool RestoreMode(const std::wstring& device_name, DWORD width, DWORD height, DWORD refresh_rate) override { - Check(device_name == expected_mode_device, "mode restore must use its persisted display"); - Check(width == 3840 && height == 2160 && refresh_rate == 60, "mode restore must use persisted originals"); + if (expected_mode_restores.empty()) { + Check(device_name == expected_mode_device, "mode restore must use its persisted display"); + Check( + width == expected_mode_width && height == expected_mode_height && refresh_rate == expected_mode_refresh_rate, + "mode restore must use persisted originals"); + } else { + Check(mode_restore_attempts < expected_mode_restores.size(), "mode recovery made an unexpected extra restore"); + const auto& expected = expected_mode_restores[mode_restore_attempts]; + Check(device_name == expected.device_name, "mode handoff restore order must be deterministic"); + Check( + width == expected.width && height == expected.height && refresh_rate == expected.refresh_rate, + "mode handoff restore must use each persisted original"); + } + ++mode_restore_attempts; events.push_back(L"restore:mode"); return mode_restore_succeeds; } bool RestoreHDR(const std::wstring& device_name, bool enabled) override { - Check(device_name == expected_hdr_device, "HDR restore must use its independently persisted display"); - Check(!enabled, "HDR restore must use the persisted original state"); + if (expected_hdr_restores.empty()) { + Check(device_name == expected_hdr_device, "HDR restore must use its independently persisted display"); + Check(enabled == expected_original_hdr, "HDR restore must use the persisted original state"); + } else { + Check(hdr_restore_attempts < expected_hdr_restores.size(), "HDR recovery made an unexpected extra restore"); + const auto& expected = expected_hdr_restores[hdr_restore_attempts]; + Check(device_name == expected.device_name, "HDR handoff restore order must be deterministic"); + Check(enabled == expected.enabled, "HDR handoff restore must use each persisted original"); + } + ++hdr_restore_attempts; events.push_back(L"restore:hdr"); return hdr_restore_succeeds; } @@ -136,18 +196,24 @@ size_t EventIndex(const std::vector& events, const std::wstring& e return events.size(); } -bool ApplyModeAfterPreparing(FakeRecoveryBackend& backend) { - if (!DisplayModeManager::PrepareModeRecovery(backend, kModeDevice, 3840, 2160, 60)) { +bool ApplyModeAfterPreparing( + FakeRecoveryBackend& backend, const std::wstring& device_name = kModeDevice, DWORD width = 3840, + DWORD height = 2160, DWORD refresh_rate = 60, bool os_apply_succeeds = true) { + if (!DisplayModeManager::PrepareModeRecovery(backend, device_name, width, height, refresh_rate)) { return false; } - backend.events.push_back(L"os:mode"); - return true; + backend.events.push_back(os_apply_succeeds ? L"os:mode" : L"os:mode:failed"); + DisplayModeManager::FinalizePreparedRecoveryForTesting(backend, true, os_apply_succeeds); + return os_apply_succeeds; } -bool ApplyHDRAfterPreparing(FakeRecoveryBackend& backend) { - if (!DisplayModeManager::PrepareHDRRecovery(backend, kHDRDevice, false)) return false; - backend.events.push_back(L"os:hdr"); - return true; +bool ApplyHDRAfterPreparing( + FakeRecoveryBackend& backend, const std::wstring& device_name = kHDRDevice, bool enabled = false, + bool os_apply_succeeds = true) { + if (!DisplayModeManager::PrepareHDRRecovery(backend, device_name, enabled)) return false; + backend.events.push_back(os_apply_succeeds ? L"os:hdr" : L"os:hdr:failed"); + DisplayModeManager::FinalizePreparedRecoveryForTesting(backend, false, os_apply_succeeds); + return os_apply_succeeds; } void TestMarkersArePersistedBeforeMutation() { @@ -160,8 +226,10 @@ void TestMarkersArePersistedBeforeMutation() { EventIndex(mode.events, L"write:ModeDeviceName") < mode_marker && EventIndex(mode.events, L"write:OriginalWidth=3840") < mode_marker && EventIndex(mode.events, L"write:OriginalHeight=2160") < mode_marker && - EventIndex(mode.events, L"write:OriginalRefreshRate=60") < mode_marker, - "all mode originals must precede the operation marker"); + EventIndex(mode.events, L"write:OriginalRefreshRate=60") < mode_marker && + EventIndex(mode.events, L"write:ModeTakeoverEligible=0") < mode_marker && + EventIndex(mode.events, L"write:ModeRecoverySlot=0") < mode_marker, + "all mode originals, slot, and reset disposition must precede the operation marker"); FakeRecoveryBackend hdr; Check(ApplyHDRAfterPreparing(hdr), "a complete HDR recovery record must admit the OS mutation"); @@ -169,8 +237,10 @@ void TestMarkersArePersistedBeforeMutation() { Check( EventIndex(hdr.events, L"write:HDRDeviceName") < hdr_marker && EventIndex(hdr.events, L"write:OriginalHDREnabled=0") < hdr_marker && + EventIndex(hdr.events, L"write:HDRTakeoverEligible=0") < hdr_marker && + EventIndex(hdr.events, L"write:HDRRecoverySlot=0") < hdr_marker && hdr_marker < EventIndex(hdr.events, L"os:hdr"), - "the HDR original and marker must be durable before the OS mutation"); + "the HDR original, slot, reset disposition, and marker must be durable before the OS mutation"); FakeRecoveryBackend failed_marker; failed_marker.final_mode_marker_write_succeeds = false; @@ -256,8 +326,12 @@ void TestModeAndHDRRestoreIndependently() { EventIndex(backend.events, L"restore:mode") < backend.events.size() && EventIndex(backend.events, L"restore:hdr") < backend.events.size(), "mode failure must not prevent the independent HDR restore"); - Check(backend.dwords[kModeChanged] == 1, "the failed mode marker must remain set"); - Check(backend.dwords[kHDRChanged] == 0, "the successful HDR marker must be cleared"); + Check( + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeTakeoverEligible] == 1, + "the failed mode marker and its takeover disposition must remain set"); + Check( + backend.dwords[kHDRChanged] == 0 && backend.dwords[kHDRTakeoverEligible] == 0, + "the successful HDR marker and matching disposition must be cleared"); backend.events.clear(); backend.mode_restore_succeeds = true; @@ -275,7 +349,9 @@ void TestFailedRestoreRemainsForTopologyRetry() { backend.device_present[kModeDevice] = false; Check(!DisplayModeManager::RecoverIfNeeded(backend), "an absent display must retain its marked restore"); - Check(backend.dwords[kModeChanged] == 1, "an absent display must keep its operation marker"); + Check( + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeTakeoverEligible] == 1, + "an absent display must keep its operation marker and persist takeover eligibility"); Check( EventIndex(backend.events, L"restore:mode") == backend.events.size(), "an absent display must not call its restore API"); @@ -287,6 +363,9 @@ void TestFailedRestoreRemainsForTopologyRetry() { Check( EventIndex(backend.events, L"restore:mode") < backend.events.size(), "the topology retry must attempt the retained restore"); + Check( + !backend.RecordExists() && backend.dwords.find(kModeTakeoverEligible) == backend.dwords.end(), + "reconnect recovery must clear the marker and remove its disposition through normal cleanup"); } void TestMarkerClearFailureRemainsRetryable() { @@ -296,7 +375,9 @@ void TestMarkerClearFailureRemainsRetryable() { backend.mode_marker_clear_succeeds = false; Check(!DisplayModeManager::RecoverIfNeeded(backend), "marker persistence is part of recovery completion"); - Check(backend.dwords[kModeChanged] == 1, "a failed marker clear must retain idempotent recovery evidence"); + Check( + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeTakeoverEligible] == 1, + "a failed marker clear must retain recovery evidence and become takeover eligible"); backend.events.clear(); backend.mode_marker_clear_succeeds = true; @@ -304,17 +385,21 @@ void TestMarkerClearFailureRemainsRetryable() { Check( EventIndex(backend.events, L"restore:mode") < backend.events.size(), "the retained marker must cause the restore to be retried"); + Check(!backend.RecordExists(), "the successful retry must remove the marker and stale disposition"); } void TestLifecycleCleanupPreservesPersistedSibling() { FakeRecoveryBackend mode_completed; mode_completed.SeedBoth(); + mode_completed.dwords[kModeTakeoverEligible] = 1; + mode_completed.dwords[kHDRTakeoverEligible] = 1; Check( DisplayModeManager::CompleteRecoveryOperationForTesting(mode_completed, true), "successful mode cleanup must durably clear its own marker"); Check( - mode_completed.dwords[kModeChanged] == 0 && mode_completed.dwords[kHDRChanged] == 1, - "mode cleanup must preserve a persisted HDR sibling even without local HDR ownership"); + mode_completed.dwords[kModeChanged] == 0 && mode_completed.dwords[kModeTakeoverEligible] == 0 && + mode_completed.dwords[kHDRChanged] == 1 && mode_completed.dwords[kHDRTakeoverEligible] == 1, + "mode cleanup must reset only its disposition while preserving a persisted HDR sibling"); Check( mode_completed.dwords[kOriginalHDR] == 0 && mode_completed.strings[kHDRDeviceName] == kHDRDevice && mode_completed.delete_attempts == 0, @@ -322,12 +407,15 @@ void TestLifecycleCleanupPreservesPersistedSibling() { FakeRecoveryBackend hdr_failed_apply; hdr_failed_apply.SeedBoth(); + hdr_failed_apply.dwords[kModeTakeoverEligible] = 1; + hdr_failed_apply.dwords[kHDRTakeoverEligible] = 1; Check( DisplayModeManager::CompleteRecoveryOperationForTesting(hdr_failed_apply, false), "failed HDR apply cleanup must durably clear its own marker"); Check( - hdr_failed_apply.dwords[kHDRChanged] == 0 && hdr_failed_apply.dwords[kModeChanged] == 1, - "HDR cleanup must preserve a persisted mode sibling even without local mode ownership"); + hdr_failed_apply.dwords[kHDRChanged] == 0 && hdr_failed_apply.dwords[kHDRTakeoverEligible] == 0 && + hdr_failed_apply.dwords[kModeChanged] == 1 && hdr_failed_apply.dwords[kModeTakeoverEligible] == 1, + "HDR cleanup must reset only its disposition while preserving a persisted mode sibling"); Check( hdr_failed_apply.dwords[kOriginalWidth] == 3840 && hdr_failed_apply.strings[kModeDeviceName] == kModeDevice && hdr_failed_apply.delete_attempts == 0, @@ -343,13 +431,18 @@ void TestReleasedLiveOperationRecoversAfterReconnect() { !DisplayModeManager::RecoverIfNeededForTesting(backend, true, true), "topology recovery must not take either genuinely live operation"); Check(backend.events.empty(), "live operations must not reach restore or persistence APIs"); + Check( + backend.dwords.find(kModeTakeoverEligible) == backend.dwords.end() && + backend.dwords.find(kHDRTakeoverEligible) == backend.dwords.end(), + "live operations must not acquire takeover dispositions"); Check( !DisplayModeManager::RecoverIfNeededForTesting(backend, false, true), "a released operation must remain marked while its target is absent"); Check( - backend.dwords[kModeChanged] == 1 && backend.dwords[kHDRChanged] == 1, - "an absent released mode and its live HDR sibling must both retain their markers"); + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeTakeoverEligible] == 1 && + backend.dwords[kHDRChanged] == 1, + "an absent released mode must become eligible without changing its live HDR sibling"); backend.events.clear(); backend.device_present[kModeDevice] = true; @@ -361,8 +454,503 @@ void TestReleasedLiveOperationRecoversAfterReconnect() { EventIndex(backend.events, L"restore:hdr") == backend.events.size(), "reconnect recovery must restore the released mode without stealing live HDR"); Check( - backend.dwords[kModeChanged] == 0 && backend.dwords[kHDRChanged] == 1 && backend.delete_attempts == 0, - "reconnect recovery must preserve the genuinely live sibling record"); + backend.dwords[kModeChanged] == 0 && backend.dwords[kModeTakeoverEligible] == 0 && + backend.dwords[kHDRChanged] == 1 && backend.delete_attempts == 0, + "reconnect recovery must clear its disposition while preserving the live sibling record"); +} + +void TestUnfailedMarkersRejectSameKindTakeover() { + FakeRecoveryBackend mode; + mode.SeedBoth(); + const auto mode_dwords = mode.dwords; + const auto mode_strings = mode.strings; + + Check( + !ApplyModeAfterPreparing(mode, kReplacementModeDevice, 2560, 1440, 120), + "a version-1 mode marker without a failed-recovery disposition must remain authoritative"); + Check( + mode.dwords == mode_dwords && mode.strings == mode_strings && mode.events.empty(), + "rejected mode takeover must not rewrite either operation or reach the OS mutation"); + + FakeRecoveryBackend hdr; + hdr.SeedBoth(); + hdr.dwords[kHDRTakeoverEligible] = 2; + const auto hdr_dwords = hdr.dwords; + const auto hdr_strings = hdr.strings; + + Check( + !ApplyHDRAfterPreparing(hdr, kReplacementHDRDevice, true), + "only an HDR takeover disposition of exactly one may replace a valid marker"); + Check( + hdr.dwords == hdr_dwords && hdr.strings == hdr_strings && hdr.events.empty(), + "rejected HDR takeover must preserve its mode sibling and avoid the OS mutation"); +} + +void TestAbsentModeTakeoverSurvivesRelaunchAndPreservesHDRSibling() { + FakeRecoveryBackend failed_recovery; + failed_recovery.SeedBoth(); + failed_recovery.dwords[kModeTakeoverEligible] = 0; + failed_recovery.dwords[kHDRTakeoverEligible] = 1; + failed_recovery.device_present[kModeDevice] = false; + + Check( + !DisplayModeManager::RecoverIfNeededForTesting(failed_recovery, false, true), + "an absent mode target must produce a retained failed recovery"); + Check( + failed_recovery.dwords[kModeChanged] == 1 && failed_recovery.dwords[kModeTakeoverEligible] == 1, + "the failed non-live mode recovery must durably become takeover eligible"); + Check( + failed_recovery.dwords[kHDRChanged] == 1 && failed_recovery.dwords[kHDRTakeoverEligible] == 1 && + failed_recovery.dwords[kOriginalHDR] == 0 && failed_recovery.strings[kHDRDeviceName] == kHDRDevice, + "recording mode failure must not alter the live HDR sibling"); + + // A new backend represents the next process reading only persisted state. + FakeRecoveryBackend relaunched; + relaunched.dwords = failed_recovery.dwords; + relaunched.strings = failed_recovery.strings; + Check( + ApplyModeAfterPreparing(relaunched, kReplacementModeDevice, 2560, 1440, 120), + "a persisted failed mode recovery must admit a conflicting request after relaunch"); + Check( + relaunched.dwords[kModeChanged] == 1 && relaunched.dwords[kModeTakeoverEligible] == 0 && + relaunched.dwords[kModeRecoverySlot] == 1 && relaunched.dwords[kOriginalWidthAlternate] == 2560 && + relaunched.dwords[kOriginalHeightAlternate] == 1440 && + relaunched.dwords[kOriginalRefreshRateAlternate] == 120 && + relaunched.strings[kModeDeviceNameAlternate] == kReplacementModeDevice, + "mode takeover must atomically select its staged original and revoke the consumed disposition"); + Check( + relaunched.dwords[kOriginalWidth] == 3840 && relaunched.dwords[kOriginalHeight] == 2160 && + relaunched.dwords[kOriginalRefreshRate] == 60 && relaunched.strings[kModeDeviceName] == kModeDevice, + "mode takeover must retain the prior originals in the inactive slot"); + Check( + relaunched.dwords[kHDRChanged] == 1 && relaunched.dwords[kHDRTakeoverEligible] == 1 && + relaunched.dwords[kOriginalHDR] == 0 && relaunched.strings[kHDRDeviceName] == kHDRDevice, + "mode takeover must preserve every persisted HDR sibling value"); + Check( + EventIndex(relaunched.events, L"write:ModeTakeoverEligible=0") < + EventIndex(relaunched.events, L"write:ModeRecoverySlot=1") && + EventIndex(relaunched.events, L"write:ModeRecoverySlot=1") < EventIndex(relaunched.events, L"os:mode") && + EventIndex(relaunched.events, L"clear:ModeChanged") == relaunched.events.size() && + EventIndex(relaunched.events, L"write:ModeChanged=1") == relaunched.events.size(), + "mode takeover must keep its marker authoritative and commit the staged slot immediately before mutation"); + + FakeRecoveryBackend recovery_relaunch; + recovery_relaunch.dwords = relaunched.dwords; + recovery_relaunch.strings = relaunched.strings; + recovery_relaunch.expected_mode_device = kReplacementModeDevice; + recovery_relaunch.expected_mode_width = 2560; + recovery_relaunch.expected_mode_height = 1440; + recovery_relaunch.expected_mode_refresh_rate = 120; + Check( + DisplayModeManager::RecoverIfNeededForTesting(recovery_relaunch, false, true), + "relaunch recovery must follow the committed mode slot"); + Check( + EventIndex(recovery_relaunch.events, L"restore:mode") < recovery_relaunch.events.size(), + "the committed alternate mode original must reach recovery"); +} + +void TestBadModeHDRRestoreAllowsSameKindTakeover() { + FakeRecoveryBackend backend; + backend.SeedBoth(); + backend.dwords[kModeTakeoverEligible] = 1; + backend.dwords[kHDRTakeoverEligible] = 0; + backend.hdr_restore_succeeds = false; + + Check( + !DisplayModeManager::RecoverIfNeededForTesting(backend, true, false), + "a present HDR target whose restore fails must retain its marker"); + Check( + EventIndex(backend.events, L"restore:hdr") < backend.events.size() && backend.dwords[kHDRChanged] == 1 && + backend.dwords[kHDRTakeoverEligible] == 1, + "a failed HDR restore must durably grant only HDR takeover"); + + backend.events.clear(); + Check( + ApplyHDRAfterPreparing(backend, kReplacementHDRDevice, true), + "a later conflicting HDR request must consume the failed-restore disposition"); + Check( + backend.dwords[kHDRChanged] == 1 && backend.dwords[kHDRTakeoverEligible] == 0 && + backend.dwords[kHDRRecoverySlot] == 1 && backend.dwords[kOriginalHDRAlternate] == 1 && + backend.strings[kHDRDeviceNameAlternate] == kReplacementHDRDevice, + "HDR takeover must atomically select its staged replacement original"); + Check( + backend.dwords[kOriginalHDR] == 0 && backend.strings[kHDRDeviceName] == kHDRDevice, + "HDR takeover must retain the prior original in the inactive slot"); + Check( + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeTakeoverEligible] == 1 && + backend.dwords[kOriginalWidth] == 3840 && backend.dwords[kOriginalHeight] == 2160 && + backend.dwords[kOriginalRefreshRate] == 60 && backend.strings[kModeDeviceName] == kModeDevice, + "HDR takeover must preserve every persisted mode sibling value"); + Check( + EventIndex(backend.events, L"write:HDRTakeoverEligible=0") < + EventIndex(backend.events, L"write:HDRRecoverySlot=1") && + EventIndex(backend.events, L"write:HDRRecoverySlot=1") < EventIndex(backend.events, L"os:hdr") && + EventIndex(backend.events, L"clear:HDRChanged") == backend.events.size() && + EventIndex(backend.events, L"write:HDRChanged=1") == backend.events.size(), + "HDR takeover must keep its marker authoritative and commit the staged slot immediately before mutation"); + + FakeRecoveryBackend recovery_relaunch; + recovery_relaunch.dwords = backend.dwords; + recovery_relaunch.strings = backend.strings; + recovery_relaunch.expected_hdr_device = kReplacementHDRDevice; + recovery_relaunch.expected_original_hdr = true; + Check( + DisplayModeManager::RecoverIfNeededForTesting(recovery_relaunch, true, false), + "relaunch recovery must follow the committed HDR slot"); + Check( + EventIndex(recovery_relaunch.events, L"restore:hdr") < recovery_relaunch.events.size(), + "the committed alternate HDR original must reach recovery"); +} + +void TestFailedOSApplyRollsBackTakeover() { + FakeRecoveryBackend mode; + mode.SeedBoth(); + mode.dwords[kModeTakeoverEligible] = 1; + mode.dwords[kHDRTakeoverEligible] = 1; + Check( + !ApplyModeAfterPreparing(mode, kReplacementModeDevice, 2560, 1440, 120, false), + "a failed mode OS apply must report failure"); + Check( + mode.dwords[kModeChanged] == 1 && mode.dwords[kModeRecoverySlot] == 0 && + mode.dwords[kModeTakeoverEligible] == 1 && mode.dwords[kModeHandoffPending] == 0 && + mode.dwords[kOriginalWidth] == 3840 && mode.dwords[kOriginalHeight] == 2160 && + mode.dwords[kOriginalRefreshRate] == 60 && mode.strings[kModeDeviceName] == kModeDevice && + mode.dwords[kHDRChanged] == 1 && mode.dwords[kHDRTakeoverEligible] == 1, + "failed mode apply must restore the prior selector, eligibility, original, and sibling"); + Check( + EventIndex(mode.events, L"write:ModeHandoffPending=1") < EventIndex(mode.events, L"write:ModeRecoverySlot=1") && + EventIndex(mode.events, L"write:ModeRecoverySlot=1") < EventIndex(mode.events, L"os:mode:failed") && + EventIndex(mode.events, L"os:mode:failed") < EventIndex(mode.events, L"write:ModeRecoverySlot=0") && + EventIndex(mode.events, L"clear:ModeChanged") == mode.events.size(), + "mode rollback must retain both records until the failed OS apply is observed"); + + FakeRecoveryBackend mode_relaunch; + mode_relaunch.dwords = mode.dwords; + mode_relaunch.strings = mode.strings; + Check( + DisplayModeManager::RecoverIfNeededForTesting(mode_relaunch, false, true), + "relaunch after failed mode apply must recover the prior original"); + + FakeRecoveryBackend hdr; + hdr.SeedBoth(); + hdr.dwords[kModeTakeoverEligible] = 1; + hdr.dwords[kHDRTakeoverEligible] = 1; + Check(!ApplyHDRAfterPreparing(hdr, kReplacementHDRDevice, true, false), "a failed HDR OS apply must report failure"); + Check( + hdr.dwords[kHDRChanged] == 1 && hdr.dwords[kHDRRecoverySlot] == 0 && hdr.dwords[kHDRTakeoverEligible] == 1 && + hdr.dwords[kHDRHandoffPending] == 0 && hdr.dwords[kOriginalHDR] == 0 && + hdr.strings[kHDRDeviceName] == kHDRDevice && hdr.dwords[kModeChanged] == 1 && + hdr.dwords[kModeTakeoverEligible] == 1, + "failed HDR apply must restore the prior selector, eligibility, original, and sibling"); + Check( + EventIndex(hdr.events, L"write:HDRHandoffPending=1") < EventIndex(hdr.events, L"write:HDRRecoverySlot=1") && + EventIndex(hdr.events, L"write:HDRRecoverySlot=1") < EventIndex(hdr.events, L"os:hdr:failed") && + EventIndex(hdr.events, L"os:hdr:failed") < EventIndex(hdr.events, L"write:HDRRecoverySlot=0") && + EventIndex(hdr.events, L"clear:HDRChanged") == hdr.events.size(), + "HDR rollback must retain both records until the failed OS apply is observed"); + + FakeRecoveryBackend hdr_relaunch; + hdr_relaunch.dwords = hdr.dwords; + hdr_relaunch.strings = hdr.strings; + Check( + DisplayModeManager::RecoverIfNeededForTesting(hdr_relaunch, true, false), + "relaunch after failed HDR apply must recover the prior original"); +} + +void TestCrashDuringTakeoverHandoffRecoversBothOriginals() { + FakeRecoveryBackend mode; + mode.SeedBoth(); + mode.dwords[kModeTakeoverEligible] = 1; + mode.dwords[kHDRTakeoverEligible] = 1; + Check( + DisplayModeManager::PrepareModeRecovery(mode, kReplacementModeDevice, 2560, 1440, 120), + "mode takeover preparation must reach its pre-apply handoff"); + Check( + mode.dwords[kModeChanged] == 1 && mode.dwords[kModeHandoffPending] == 1 && + mode.dwords[kModePreviousRecoverySlot] == 0 && mode.dwords[kModeRecoverySlot] == 1, + "mode pre-apply handoff must retain both slot identities under one marker"); + mode.expected_mode_restores = {{kReplacementModeDevice, 2560, 1440, 120}, {kModeDevice, 3840, 2160, 60}}; + Check( + DisplayModeManager::RecoverIfNeededForTesting(mode, false, true), + "crash recovery must restore both mode originals from a pending handoff"); + Check( + mode.mode_restore_attempts == 2 && mode.dwords[kModeChanged] == 0 && mode.dwords[kModeHandoffPending] == 0 && + mode.dwords[kHDRChanged] == 1, + "mode handoff recovery must restore replacement then prior and preserve its sibling"); + + FakeRecoveryBackend hdr; + hdr.SeedBoth(); + hdr.dwords[kModeTakeoverEligible] = 1; + hdr.dwords[kHDRTakeoverEligible] = 1; + Check( + DisplayModeManager::PrepareHDRRecovery(hdr, kReplacementHDRDevice, true), + "HDR takeover preparation must reach its pre-apply handoff"); + Check( + hdr.dwords[kHDRChanged] == 1 && hdr.dwords[kHDRHandoffPending] == 1 && + hdr.dwords[kHDRPreviousRecoverySlot] == 0 && hdr.dwords[kHDRRecoverySlot] == 1, + "HDR pre-apply handoff must retain both slot identities under one marker"); + hdr.expected_hdr_restores = {{kReplacementHDRDevice, true}, {kHDRDevice, false}}; + Check( + DisplayModeManager::RecoverIfNeededForTesting(hdr, true, false), + "crash recovery must restore both HDR originals from a pending handoff"); + Check( + hdr.hdr_restore_attempts == 2 && hdr.dwords[kHDRChanged] == 0 && hdr.dwords[kHDRHandoffPending] == 0 && + hdr.dwords[kModeChanged] == 1, + "HDR handoff recovery must restore replacement then prior and preserve its sibling"); +} + +void TestHDRReconnectRecoversBeforeTakeover() { + FakeRecoveryBackend backend; + backend.SeedBoth(); + backend.dwords[kModeChanged] = 0; + backend.device_present[kHDRDevice] = false; + + Check(!DisplayModeManager::RecoverIfNeeded(backend), "an absent HDR target must retain its recovery marker"); + Check( + backend.dwords[kHDRChanged] == 1 && backend.dwords[kHDRTakeoverEligible] == 1 && + EventIndex(backend.events, L"restore:hdr") == backend.events.size(), + "absent HDR recovery must become eligible without calling the restore API"); + + backend.events.clear(); + backend.device_present[kHDRDevice] = true; + Check( + DisplayModeManager::RecoverIfNeeded(backend), + "reconnecting HDR before a conflicting request must recover the old target"); + Check( + EventIndex(backend.events, L"restore:hdr") < backend.events.size() && !backend.RecordExists(), + "successful reconnect recovery must win and remove its disposition"); +} + +void TestEligibleSameOriginalReuseResetsDisposition() { + FakeRecoveryBackend mode; + mode.SeedBoth(); + mode.dwords[kModeTakeoverEligible] = 1; + mode.dwords[kHDRTakeoverEligible] = 1; + + Check(ApplyModeAfterPreparing(mode), "an eligible mode marker must remain reusable by the same original"); + Check( + mode.dwords[kModeTakeoverEligible] == 0 && mode.dwords[kHDRTakeoverEligible] == 1, + "same-original mode reuse must revoke only mode takeover"); + mode.events.clear(); + Check( + !ApplyModeAfterPreparing(mode, kReplacementModeDevice, 2560, 1440, 120) && mode.events.empty(), + "a mismatched mode request must not steal a marker after same-original reuse"); + + FakeRecoveryBackend hdr; + hdr.SeedBoth(); + hdr.dwords[kModeTakeoverEligible] = 1; + hdr.dwords[kHDRTakeoverEligible] = 1; + + Check(ApplyHDRAfterPreparing(hdr), "an eligible HDR marker must remain reusable by the same original"); + Check( + hdr.dwords[kHDRTakeoverEligible] == 0 && hdr.dwords[kModeTakeoverEligible] == 1, + "same-original HDR reuse must revoke only HDR takeover"); + hdr.events.clear(); + Check( + !ApplyHDRAfterPreparing(hdr, kReplacementHDRDevice, true) && hdr.events.empty(), + "a mismatched HDR request must not steal a marker after same-original reuse"); +} + +void TestRepeatedModeTakeoverStagesOnlyTheInactiveSlot() { + FakeRecoveryBackend backend; + backend.SeedBoth(); + backend.dwords[kModeTakeoverEligible] = 1; + backend.dwords[kHDRTakeoverEligible] = 1; + Check( + ApplyModeAfterPreparing(backend, kReplacementModeDevice, 2560, 1440, 120), + "the first mode takeover must commit the alternate slot"); + + backend.events.clear(); + backend.device_present[kReplacementModeDevice] = false; + Check( + !DisplayModeManager::RecoverIfNeededForTesting(backend, false, true), + "a later failed recovery must make the alternate mode original eligible"); + Check( + backend.dwords[kModeRecoverySlot] == 1 && backend.dwords[kModeTakeoverEligible] == 1, + "the active alternate slot must remain selected after recovery failure"); + + backend.events.clear(); + backend.device_present[kReplacementModeDevice] = true; + backend.rejected_dword_write_event = L"write:OriginalRefreshRate=75"; + Check( + !ApplyModeAfterPreparing(backend, kModeDevice, 1920, 1080, 75), + "a repeated takeover must fail when staging into the primary slot fails"); + Check( + backend.dwords[kModeChanged] == 1 && backend.dwords[kModeRecoverySlot] == 1 && + backend.dwords[kOriginalWidthAlternate] == 2560 && backend.dwords[kOriginalHeightAlternate] == 1440 && + backend.dwords[kOriginalRefreshRateAlternate] == 120 && + backend.strings[kModeDeviceNameAlternate] == kReplacementModeDevice && + EventIndex(backend.events, L"os:mode") == backend.events.size(), + "failed repeated staging must not overwrite or deselect the active alternate original"); + + FakeRecoveryBackend relaunched; + relaunched.dwords = backend.dwords; + relaunched.strings = backend.strings; + relaunched.expected_mode_device = kReplacementModeDevice; + relaunched.expected_mode_width = 2560; + relaunched.expected_mode_height = 1440; + relaunched.expected_mode_refresh_rate = 120; + Check( + DisplayModeManager::RecoverIfNeededForTesting(relaunched, false, true), + "relaunch must still recover the alternate original after repeated takeover staging fails"); +} + +void TestLiveMarkersCannotBecomeEligibleOrBeTakenOver() { + FakeRecoveryBackend mode; + mode.SeedBoth(); + mode.dwords[kHDRChanged] = 0; + Check( + !DisplayModeManager::RecoverIfNeededForTesting(mode, true, false), + "a live mode marker must be skipped by synchronous recovery"); + Check( + mode.events.empty() && mode.dwords.find(kModeTakeoverEligible) == mode.dwords.end(), + "skipping live mode recovery must not grant takeover"); + Check( + !ApplyModeAfterPreparing(mode, kReplacementModeDevice, 2560, 1440, 120), + "a mismatched request must not steal a live mode marker"); + + FakeRecoveryBackend hdr; + hdr.SeedBoth(); + hdr.dwords[kModeChanged] = 0; + Check( + !DisplayModeManager::RecoverIfNeededForTesting(hdr, false, true), + "a live HDR marker must be skipped by synchronous recovery"); + Check( + hdr.events.empty() && hdr.dwords.find(kHDRTakeoverEligible) == hdr.dwords.end(), + "skipping live HDR recovery must not grant takeover"); + Check( + !ApplyHDRAfterPreparing(hdr, kReplacementHDRDevice, true), + "a mismatched request must not steal a live HDR marker"); +} + +void TestTakeoverPersistenceFailuresRemainConservative() { + FakeRecoveryBackend failed_grant; + failed_grant.SeedBoth(); + failed_grant.dwords[kHDRChanged] = 0; + failed_grant.device_present[kModeDevice] = false; + failed_grant.rejected_dword_write_event = L"write:ModeTakeoverEligible=1"; + Check( + !DisplayModeManager::RecoverIfNeeded(failed_grant), + "failure to persist mode takeover eligibility must leave recovery incomplete"); + Check( + failed_grant.dwords[kModeChanged] == 1 && + failed_grant.dwords.find(kModeTakeoverEligible) == failed_grant.dwords.end(), + "a failed eligibility write must leave the old marker authoritative"); + failed_grant.rejected_dword_write_event.clear(); + failed_grant.events.clear(); + Check( + !ApplyModeAfterPreparing(failed_grant, kReplacementModeDevice, 2560, 1440, 120) && failed_grant.events.empty(), + "an unpersisted failure must not admit takeover"); + + FakeRecoveryBackend failed_reuse_reset; + failed_reuse_reset.SeedBoth(); + failed_reuse_reset.dwords[kHDRTakeoverEligible] = 1; + failed_reuse_reset.rejected_dword_write_event = L"write:HDRTakeoverEligible=0"; + Check(!ApplyHDRAfterPreparing(failed_reuse_reset), "same-original reuse must fail when takeover cannot be reset"); + Check( + failed_reuse_reset.dwords[kHDRChanged] == 1 && failed_reuse_reset.dwords[kHDRTakeoverEligible] == 1 && + EventIndex(failed_reuse_reset.events, L"os:hdr") == failed_reuse_reset.events.size(), + "failed HDR disposition reset must retain the marker and prevent mutation"); + + FakeRecoveryBackend failed_mode_stage; + failed_mode_stage.SeedBoth(); + failed_mode_stage.dwords[kModeTakeoverEligible] = 1; + failed_mode_stage.dwords[kHDRTakeoverEligible] = 1; + failed_mode_stage.rejected_dword_write_event = L"write:OriginalRefreshRateAlternate=120"; + Check( + !ApplyModeAfterPreparing(failed_mode_stage, kReplacementModeDevice, 2560, 1440, 120), + "mode takeover must fail if a staged replacement original cannot be persisted"); + Check( + failed_mode_stage.dwords[kModeChanged] == 1 && failed_mode_stage.dwords[kModeTakeoverEligible] == 1 && + failed_mode_stage.dwords.find(kModeRecoverySlot) == failed_mode_stage.dwords.end() && + failed_mode_stage.dwords[kOriginalWidth] == 3840 && failed_mode_stage.dwords[kOriginalHeight] == 2160 && + failed_mode_stage.dwords[kOriginalRefreshRate] == 60 && + failed_mode_stage.strings[kModeDeviceName] == kModeDevice && failed_mode_stage.dwords[kHDRChanged] == 1 && + failed_mode_stage.dwords[kHDRTakeoverEligible] == 1 && failed_mode_stage.dwords[kOriginalHDR] == 0 && + failed_mode_stage.strings[kHDRDeviceName] == kHDRDevice && + EventIndex(failed_mode_stage.events, L"os:mode") == failed_mode_stage.events.size(), + "failed mode staging must leave the old marker and originals authoritative"); + + FakeRecoveryBackend failed_mode_commit; + failed_mode_commit.SeedBoth(); + failed_mode_commit.dwords[kModeTakeoverEligible] = 1; + failed_mode_commit.dwords[kHDRTakeoverEligible] = 1; + failed_mode_commit.rejected_dword_write_event = L"write:ModeRecoverySlot=1"; + Check( + !ApplyModeAfterPreparing(failed_mode_commit, kReplacementModeDevice, 2560, 1440, 120), + "mode takeover must fail when its atomic authority switch cannot be persisted"); + Check( + failed_mode_commit.dwords[kModeChanged] == 1 && failed_mode_commit.dwords[kModeTakeoverEligible] == 1 && + failed_mode_commit.dwords[kModeRecoverySlot] == 0 && failed_mode_commit.dwords[kModeHandoffPending] == 0 && + failed_mode_commit.dwords[kOriginalWidth] == 3840 && failed_mode_commit.dwords[kOriginalHeight] == 2160 && + failed_mode_commit.dwords[kOriginalRefreshRate] == 60 && + failed_mode_commit.strings[kModeDeviceName] == kModeDevice && + failed_mode_commit.dwords[kOriginalWidthAlternate] == 2560 && + failed_mode_commit.strings[kModeDeviceNameAlternate] == kReplacementModeDevice && + failed_mode_commit.dwords[kHDRChanged] == 1 && failed_mode_commit.dwords[kHDRTakeoverEligible] == 1 && + failed_mode_commit.dwords[kOriginalHDR] == 0 && failed_mode_commit.strings[kHDRDeviceName] == kHDRDevice && + EventIndex(failed_mode_commit.events, L"os:mode") == failed_mode_commit.events.size(), + "failed mode commit must keep the primary recovery point authoritative despite complete staging"); + + FakeRecoveryBackend mode_relaunch; + mode_relaunch.dwords = failed_mode_commit.dwords; + mode_relaunch.strings = failed_mode_commit.strings; + Check( + DisplayModeManager::RecoverIfNeededForTesting(mode_relaunch, false, true), + "relaunch after a failed mode commit must recover the old primary original"); + Check( + EventIndex(mode_relaunch.events, L"restore:mode") < mode_relaunch.events.size(), + "failed mode authority switch must not redirect relaunch recovery to staging"); + + FakeRecoveryBackend failed_hdr_stage; + failed_hdr_stage.SeedBoth(); + failed_hdr_stage.dwords[kModeTakeoverEligible] = 1; + failed_hdr_stage.dwords[kHDRTakeoverEligible] = 1; + failed_hdr_stage.rejected_dword_write_event = L"write:OriginalHDREnabledAlternate=1"; + Check( + !ApplyHDRAfterPreparing(failed_hdr_stage, kReplacementHDRDevice, true), + "HDR takeover must fail if its staged replacement original cannot be persisted"); + Check( + failed_hdr_stage.dwords[kHDRChanged] == 1 && failed_hdr_stage.dwords[kHDRTakeoverEligible] == 1 && + failed_hdr_stage.dwords.find(kHDRRecoverySlot) == failed_hdr_stage.dwords.end() && + failed_hdr_stage.dwords[kOriginalHDR] == 0 && failed_hdr_stage.strings[kHDRDeviceName] == kHDRDevice && + failed_hdr_stage.dwords[kModeChanged] == 1 && failed_hdr_stage.dwords[kModeTakeoverEligible] == 1 && + failed_hdr_stage.dwords[kOriginalWidth] == 3840 && failed_hdr_stage.dwords[kOriginalHeight] == 2160 && + failed_hdr_stage.dwords[kOriginalRefreshRate] == 60 && + failed_hdr_stage.strings[kModeDeviceName] == kModeDevice && + EventIndex(failed_hdr_stage.events, L"os:hdr") == failed_hdr_stage.events.size(), + "failed HDR staging must leave the old marker and original authoritative"); + + FakeRecoveryBackend failed_hdr_commit; + failed_hdr_commit.SeedBoth(); + failed_hdr_commit.dwords[kModeTakeoverEligible] = 1; + failed_hdr_commit.dwords[kHDRTakeoverEligible] = 1; + failed_hdr_commit.rejected_dword_write_event = L"write:HDRRecoverySlot=1"; + Check( + !ApplyHDRAfterPreparing(failed_hdr_commit, kReplacementHDRDevice, true), + "HDR takeover must fail when its atomic authority switch cannot be persisted"); + Check( + failed_hdr_commit.dwords[kHDRChanged] == 1 && failed_hdr_commit.dwords[kHDRTakeoverEligible] == 1 && + failed_hdr_commit.dwords[kHDRRecoverySlot] == 0 && failed_hdr_commit.dwords[kHDRHandoffPending] == 0 && + failed_hdr_commit.dwords[kOriginalHDR] == 0 && failed_hdr_commit.strings[kHDRDeviceName] == kHDRDevice && + failed_hdr_commit.dwords[kOriginalHDRAlternate] == 1 && + failed_hdr_commit.strings[kHDRDeviceNameAlternate] == kReplacementHDRDevice && + failed_hdr_commit.dwords[kModeChanged] == 1 && failed_hdr_commit.dwords[kModeTakeoverEligible] == 1 && + failed_hdr_commit.dwords[kOriginalWidth] == 3840 && failed_hdr_commit.dwords[kOriginalHeight] == 2160 && + failed_hdr_commit.dwords[kOriginalRefreshRate] == 60 && + failed_hdr_commit.strings[kModeDeviceName] == kModeDevice && + EventIndex(failed_hdr_commit.events, L"os:hdr") == failed_hdr_commit.events.size(), + "failed HDR commit must keep the primary recovery point authoritative despite complete staging"); + + FakeRecoveryBackend hdr_relaunch; + hdr_relaunch.dwords = failed_hdr_commit.dwords; + hdr_relaunch.strings = failed_hdr_commit.strings; + Check( + DisplayModeManager::RecoverIfNeededForTesting(hdr_relaunch, true, false), + "relaunch after a failed HDR commit must recover the old primary original"); + Check( + EventIndex(hdr_relaunch.events, L"restore:hdr") < hdr_relaunch.events.size(), + "failed HDR authority switch must not redirect relaunch recovery to staging"); } void TestVersionlessModeRecoveryAndCleanup() { @@ -431,7 +1019,9 @@ void TestCleanupFailureDoesNotBlockNewOverride() { Check( DisplayModeManager::RecoverIfNeeded(backend), "successful restoration must complete even when stale-value deletion fails"); - Check(backend.dwords[kModeChanged] == 0, "the successful restore marker must be clear"); + Check( + backend.dwords[kModeChanged] == 0 && backend.dwords[kModeTakeoverEligible] == 0, + "the successful restore marker and matching disposition must be clear"); Check(backend.delete_attempts == 1, "completed recovery must make one best-effort cleanup attempt"); backend.events.clear(); @@ -456,6 +1046,16 @@ int main() { mpv::TestMarkerClearFailureRemainsRetryable(); mpv::TestLifecycleCleanupPreservesPersistedSibling(); mpv::TestReleasedLiveOperationRecoversAfterReconnect(); + mpv::TestUnfailedMarkersRejectSameKindTakeover(); + mpv::TestAbsentModeTakeoverSurvivesRelaunchAndPreservesHDRSibling(); + mpv::TestBadModeHDRRestoreAllowsSameKindTakeover(); + mpv::TestFailedOSApplyRollsBackTakeover(); + mpv::TestCrashDuringTakeoverHandoffRecoversBothOriginals(); + mpv::TestHDRReconnectRecoversBeforeTakeover(); + mpv::TestEligibleSameOriginalReuseResetsDisposition(); + mpv::TestRepeatedModeTakeoverStagesOnlyTheInactiveSlot(); + mpv::TestLiveMarkersCannotBecomeEligibleOrBeTakenOver(); + mpv::TestTakeoverPersistenceFailuresRemainConservative(); mpv::TestVersionlessModeRecoveryAndCleanup(); mpv::TestVersionlessHDRRecoveryAndCleanup(); mpv::TestVersionlessModeAndHDRUseSharedDevice(); diff --git a/windows/runner/mpv/mpv_player.cpp b/windows/runner/mpv/mpv_player.cpp index 07cf262c..09c9879f 100644 --- a/windows/runner/mpv/mpv_player.cpp +++ b/windows/runner/mpv/mpv_player.cpp @@ -547,7 +547,7 @@ void MpvPlayer::Dispose() { auto cancelled = pending_requests_.CancelAll(); for (auto& callback : cancelled.status) { - callback(-1); + callback(MPV_ERROR_UNINITIALIZED); } for (auto& callback : cancelled.properties) { callback(-1, ""); diff --git a/windows/runner/mpv/mpv_player_property_contract_test.cpp b/windows/runner/mpv/mpv_player_property_contract_test.cpp index c2a28beb..c4475117 100644 --- a/windows/runner/mpv/mpv_player_property_contract_test.cpp +++ b/windows/runner/mpv/mpv_player_property_contract_test.cpp @@ -94,7 +94,7 @@ void TestPendingPropertyWriteFailsOnDispose() { player.Dispose(); Check(callback_count == 1, "dispose must complete a pending property write exactly once"); - Check(status < 0, "dispose must fail a pending property write"); + Check(status == MPV_ERROR_UNINITIALIZED, "dispose must cancel a pending property write as uninitialized"); player.Dispose(); Check(callback_count == 1, "repeated dispose must not complete a property write twice"); diff --git a/windows/runner/mpv/mpv_plugin.cpp b/windows/runner/mpv/mpv_plugin.cpp index 8d3a4f03..fe3b2e7e 100644 --- a/windows/runner/mpv/mpv_plugin.cpp +++ b/windows/runner/mpv/mpv_plugin.cpp @@ -282,9 +282,11 @@ void MpvPlayerPlugin::HandleMethodCall( if (plezy::mpv_common::SetPropertyStatusSucceeded(error)) { (*result_ptr)->Success(); } else { - (*result_ptr) - ->Error( - plezy::mpv_common::kSetPropertyFailedCode, plezy::mpv_common::SetPropertyErrorDescription(error)); + const auto* error_code = plezy::mpv_common::SetPropertyErrorCode(error); + const auto description = error == MPV_ERROR_UNINITIALIZED + ? std::string("Player not initialized") + : plezy::mpv_common::SetPropertyErrorDescription(error); + (*result_ptr)->Error(error_code, description); } }); });