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']); + } +}