From e115ca69eb204ff5a7eec9fa004eb91cf9fc5fd5 Mon Sep 17 00:00:00 2001 From: tripsittr <160368852+tripsittr@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:56:01 -0700 Subject: [PATCH] Measure a file before deciding how to play it From a5's review of #304: an unprobed HEVC .mp4 was still reported direct-playable and would fail on-device until the hourly backfill reached it. Judging an unmeasured file on its container alone is optimistic in the one direction that hurts. StreamPolicy::decide() now probes a video nobody has measured and decides on its codecs. ffprobe reads a header, not a file, and it happens once. Failure is quiet: no ffprobe or an unreadable file leaves the item unmeasured and the decision falls back to the container, exactly as before. a5 also asked whether every decision path eager-loads probe. Two did not: WatchController gates the web player on isPlayableVideo(), so an HEVC mp4 passed and loaded a black rectangle; and needsConversion() decides whether to queue a conversion, so an optimistic answer meant none was queued for the files that needed one. Both load it now. The first two tests encode real HEVC and H.264 and drive the endpoint, because the point is what gets measured -- a fixture would assert the plumbing while the behaviour stayed wrong. --- app/Http/Controllers/WatchController.php | 7 + app/Services/MediaTranscoder.php | 8 +- app/Services/Streaming/StreamPolicy.php | 59 +++++++++ changelog/302-probe-on-demand.md | 48 +++++++ tests/Feature/ProbeOnDemandTest.php | 159 +++++++++++++++++++++++ 5 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 changelog/302-probe-on-demand.md create mode 100644 tests/Feature/ProbeOnDemandTest.php diff --git a/app/Http/Controllers/WatchController.php b/app/Http/Controllers/WatchController.php index d08081f7..968a0f9c 100644 --- a/app/Http/Controllers/WatchController.php +++ b/app/Http/Controllers/WatchController.php @@ -29,6 +29,13 @@ public function show(MediaItem $item): View abort_unless($item->hasReadableFile(), 404); + // The probe, because the question below is about codecs and not just + // the extension. `isPlayableVideo()` falls back to judging the + // container alone when this relation is absent, so without it an + // `.mp4` of HEVC passes the gate and the page loads a player that + // shows a black rectangle. + $item->loadMissing('probe'); + // MKV and AVI don't decode in any browser, so sending someone to a // player that shows a black rectangle helps nobody — the detail page // offers a download for those instead. diff --git a/app/Services/MediaTranscoder.php b/app/Services/MediaTranscoder.php index 51980c03..b22d7595 100644 --- a/app/Services/MediaTranscoder.php +++ b/app/Services/MediaTranscoder.php @@ -41,6 +41,12 @@ public function needsConversion(MediaItem $item): bool return false; } + // The probe decides this, so it has to be present: without it + // `isPlayableVideo()` judges the container alone and an `.mp4` of + // HEVC is called playable -- meaning no conversion is ever queued for + // precisely the files that need one. + $item->loadMissing('probe'); + return match ($item->type) { MediaItemType::Movie, MediaItemType::Show => ! $item->isPlayableVideo() || $this->hasUnplayableCodec($item), @@ -263,7 +269,7 @@ private function targetPath(MediaItem $item): string // Keyed by id so two films sharing a title can't overwrite each other. $name = preg_replace('/[^\w\- ]+/u', '', $item->title) ?: 'item'; - return $root . '/' . $item->id . '-' . trim($name) . '.mp4'; + return $root.'/'.$item->id.'-'.trim($name).'.mp4'; } private function convertedFileExists(MediaItem $item): bool diff --git a/app/Services/Streaming/StreamPolicy.php b/app/Services/Streaming/StreamPolicy.php index 72a47cd4..a043656f 100644 --- a/app/Services/Streaming/StreamPolicy.php +++ b/app/Services/Streaming/StreamPolicy.php @@ -5,10 +5,14 @@ namespace App\Services\Streaming; +use App\Enums\MediaItemType; use App\Models\MediaItem; use App\Services\MediaTranscoder; +use App\Services\Quality\MediaProber; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Log; use Symfony\Component\HttpFoundation\IpUtils; +use Throwable; /** * Whether to hand a client the file or an HLS stream (S-29). @@ -33,6 +37,48 @@ */ class StreamPolicy { + /** + * Probes a video that has never been measured, so the decision below is + * made on its codecs rather than its file extension. + * + * Deliberately quiet about failure. A probe that cannot run -- no + * ffprobe, a file on another machine, a format it refuses -- leaves the + * item unmeasured and the decision falls back to the container, which is + * exactly the behaviour that existed before. A playback request must not + * fail because a measurement did. + */ + private function measureIfUnprobed(MediaItem $item): void + { + if (! in_array($item->type, [MediaItemType::Movie, MediaItemType::Show], true)) { + return; + } + + $item->loadMissing('probe'); + + if ($item->probe !== null) { + return; + } + + try { + $prober = app(MediaProber::class); + + if (! $prober->isAvailable()) { + return; + } + + $prober->probe($item); + + // Re-read, so `isPlayableVideo()` sees what was just written + // rather than the absence it was called with. + $item->load('probe'); + } catch (Throwable $exception) { + Log::warning('Could not probe an item before deciding how to play it', [ + 'item' => $item->id, + 'error' => $exception->getMessage(), + ]); + } + } + /** Loopback and the private ranges — a client on the same network. */ private const LOCAL_RANGES = [ '127.0.0.0/8', @@ -79,6 +125,19 @@ public function decide(MediaItem $item, Request $request): PlaybackDecision return PlaybackDecision::direct('a converted copy is already web-playable'); } + // Measure it now if nobody has. + // + // Judging an unprobed file on its container alone is optimistic in + // the one direction that hurts: an `.mp4` of HEVC is called playable, + // direct-plays, and shows a black rectangle. The hourly backfill + // closes that eventually, but "eventually" is the whole viewing + // somebody is trying to start right now. + // + // ffprobe reads a header, not a file -- well under a second even for + // a large rip -- so this is cheap enough to do on the path, and it + // happens once: the row is written and every later decision reads it. + $this->measureIfUnprobed($item); + if (! $item->isPlayableVideo()) { return PlaybackDecision::transcode( 'the container or codec is not web-playable', diff --git a/changelog/302-probe-on-demand.md b/changelog/302-probe-on-demand.md new file mode 100644 index 00000000..6507b914 --- /dev/null +++ b/changelog/302-probe-on-demand.md @@ -0,0 +1,48 @@ +# Measure a file before deciding how to play it + +From a5's review of #304, which closed the extension-only playability hole and +left one window open: + +> an **unprobed HEVC `.mp4`** is still reported direct-playable and will fail +> on-device until the hourly probe backfill reaches it — the exact failure this +> fixes, just deferred. + +Correct. Judging an unmeasured file on its container alone is optimistic in the +one direction that hurts, and "eventually" is the whole viewing somebody is +trying to start now. + +## Probe on demand + +`StreamPolicy::decide()` now measures a video nobody has measured, then decides +on its codecs. ffprobe reads a header rather than a file — well under a second +even for a large rip — and it happens once: the row is written and every later +decision reads it. + +Failure is quiet by design. No ffprobe, a file on another machine, a format it +refuses: the item stays unmeasured and the decision falls back to the +container, which is exactly the behaviour that existed before. A playback +request must not fail because a measurement did. + +## Two paths that were deciding blind + +a5's second point was that the answer depends on whether the caller +eager-loaded `probe`. Auditing every caller of `isPlayableVideo()` found two +that did not: + +- **`WatchController`** gates the web player on it, so an `.mp4` of HEVC passed + the gate and loaded a player showing a black rectangle. +- **`MediaTranscoder::needsConversion()`** decides whether to *queue* a + conversion — so an optimistic answer meant no conversion was ever queued for + precisely the files that needed one. + +Both now load the relation. + +## Testing + +3 new tests, **369 pass**. The first two encode real HEVC and H.264 files and +drive the actual endpoint, because the point is what gets *measured* — a +fixture would have asserted the plumbing while the behaviour stayed wrong. +Confirmed to fail without the change. + +The third covers a file that cannot be measured at all, which must still return +a decision rather than erroring. diff --git a/tests/Feature/ProbeOnDemandTest.php b/tests/Feature/ProbeOnDemandTest.php new file mode 100644 index 00000000..06f8ce50 --- /dev/null +++ b/tests/Feature/ProbeOnDemandTest.php @@ -0,0 +1,159 @@ +successful() ? $binary : null; + } + + /** A real file, because the point is that it gets measured. */ + private function film(string $name, array $encode): ?MediaItem + { + $ffmpeg = $this->ffmpeg(); + + if ($ffmpeg === null) { + return null; + } + + $directory = sys_get_temp_dir().'/sc-ondemand-'.bin2hex(random_bytes(4)); + @mkdir($directory, 0775, true); + + $path = $directory.'/'.$name; + + $made = Process::timeout(120)->run(array_merge( + [$ffmpeg, '-hide_banner', '-loglevel', 'error', + '-f', 'lavfi', '-i', 'testsrc=duration=1:size=320x240:rate=10'], + $encode, + ['-y', $path], + )); + + if (! $made->successful() || ! is_file($path)) { + return null; + } + + return MediaItem::unresolved()->create([ + 'user_id' => User::factory()->create()->id, + 'title' => $name, + 'type' => MediaItemType::Movie, + 'file_path' => $path, + 'processing_status' => ProcessingStatus::Complete, + ])->fresh(); + } + + /** + * The window a5 identified. Before this, an unprobed HEVC `.mp4` was + * called playable and sent to direct play. + */ + public function test_an_unprobed_file_is_measured_before_the_decision(): void + { + $film = $this->film('hevc.mp4', ['-c:v', 'libx265', '-tag:v', 'hvc1', '-an']); + + if ($film === null) { + $this->markTestSkipped('No ffmpeg, or this build cannot encode HEVC.'); + } + + $this->assertNull($film->probe, 'The fixture must start unmeasured.'); + + $user = User::find($film->user_id); + + $response = $this->actingAs($user) + ->getJson("/api/v1/items/{$film->id}/playback") + ->assertOk(); + + // Measured on the way through. + $this->assertNotNull( + $film->fresh()->probe, + 'The decision path should have probed a file nobody had measured.', + ); + + // And the decision reflects the codec, not the .mp4 extension. + $this->assertTrue( + $response->json('transcode'), + 'HEVC must not be direct-played just because the container is mp4.', + ); + + @unlink((string) $film->file_path); + @rmdir(dirname((string) $film->file_path)); + } + + /** + * The other half: an H.264 file is measured too, and still direct-plays. + * A probe-on-demand that routed everything to HLS would be worse than the + * bug it replaced. + */ + public function test_measuring_does_not_make_a_playable_file_transcode(): void + { + $film = $this->film('h264.mp4', ['-c:v', 'libx264', '-pix_fmt', 'yuv420p', '-an']); + + if ($film === null) { + $this->markTestSkipped('No ffmpeg on this machine.'); + } + + $user = User::find($film->user_id); + + $response = $this->actingAs($user) + ->getJson("/api/v1/items/{$film->id}/playback") + ->assertOk(); + + $this->assertNotNull($film->fresh()->probe); + $this->assertFalse( + $response->json('transcode'), + 'An H.264 mp4 should still direct-play once measured.', + ); + + @unlink((string) $film->file_path); + @rmdir(dirname((string) $film->file_path)); + } + + /** + * A file that cannot be measured — gone, or on another machine — must + * still answer rather than failing the request. + */ + public function test_an_unmeasurable_file_still_returns_a_decision(): void + { + $user = User::factory()->create(); + + $film = MediaItem::unresolved()->create([ + 'user_id' => $user->id, + 'title' => 'Absent', + 'type' => MediaItemType::Movie, + 'file_path' => '/nonexistent/absent.mp4', + 'processing_status' => ProcessingStatus::Complete, + ]); + + $this->actingAs($user) + ->getJson("/api/v1/items/{$film->id}/playback") + ->assertOk() + ->assertJsonStructure(['transcode', 'url']); + } +}