Repository navigation
Conversation
Freeze the shared surface the phone and TV players will drive: the playback.buffering_quality_mode enum, the controller's inputs and outputs, and the buffering_quality_v1 request and plan fields from the spec's Wire contract. Vendor the spec's conformance vectors unchanged. The controller body follows in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er 857655f4f Take settings manifest revision 21 with playback.buffering_quality_mode and regenerate SettingKeys.kt with cmd/settingsgen, the 23-case buffering-quality vectors, and the decision fixture that now advertises buffering_quality_v1 and playback_plan_summary_v1. The server's settings conformance fixture still targets revision 20, so SettingsConformanceTest fails until the server bumps it; raised with the server thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
One state machine for the phone and TV players: the web player's stall tracker (startup grace, counted and long stalls, outages re-timed), Auto lowering with an undo in always mode, the prompt for ask mode and picked qualities, the no-lower cooldown, rearm after an unadopted replan, and per-plan plan_summary facts. BufferingQualityVectorsTest replays all 23 vendored conformance cases on a virtual clock and fails with every broken case named. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… gate its requests Add the Media3 adapter both players will use: it turns player and load callbacks into the controller's playing, waiting (network, seek, reload), pause, seek and transport-trouble events. It also tracks whether media data is still arriving, so a startup-stall transport_stall on a slow plan that is still receiving data belongs to the controller rather than the same-plan reopen. Requests carry buffering_quality_v1, bandwidth_estimate_source and learned_limit only when the server advertises the feature, either in the /api/v2 capabilities or in a decision's server_features; never mode sends learned_limit: ignore. Controller replans go out as quality_change with the step-down estimate. Each start and replan logs a one-line summary of its body, and plan_summary route events go out when the server advertises playback_plan_summary_v1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t row playback.buffering_quality_mode (Never, Ask first, Automatically; default Automatically) joins phone and TV Playback settings next to Maximum Bitrate, layered per profile and device like Skip Intros. It is newer than some supported servers, so reads and writes are gated on contract revision 21 the way the cellular quality pair is. When the server advertises buffering_quality_v1 and reports a learned limit for this device's connection, Settings shows "Limited to 44 Mbps after buffering on Oct 17" with Reset, backed by GET /api/v2/playback/connection and DELETE .../learned-limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The phone player now runs the buffering-quality controller outside Watch Party rooms and downloaded files. A new plan is handed to the controller when it mounts; the controller's replans go out as quality_change with the step-down estimate, and a replan that cannot be sent at once is reported as not adopted rather than queued without its fields. The notice overlay gains the prompt (Playback keeps buffering: Lower quality, Keep quality) and the undo (Quality lowered to stop buffering: Restore quality) with the intro pill's countdown fill; Back answers them. The Quality menu labels Auto "Auto · limited after buffering" while a learned limit holds it down. A startup-stall transport_stall on a plan that is still receiving data now belongs to the controller: no same-plan reopen, no failure replan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The spec now treats each adoption as a new plan, since a server plan_id can repeat; the controller keys its state on a local adoption identity and keeps the server plan_id for plan_summary. The step-down estimate is the exact integer floor of delivered x 7 / 10 (22120 gives 15484, not 15483). A higher-priority pill now hides a visible buffering pill with its timer frozen, and the pill returns with the remainder when that pill goes. Re-vendor the 32-case vectors and the settings conformance fixture, which now targets manifest revision 21, from silo-server bdd8892a0. On the phone, every applied mount is a new adoption, and the recap and credits skip pill joins intro skip and Next Up ahead of buffering pills. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Route events arrive on the session manager's IO scope while the test polls them; a synchronized list does not guard iteration, so the poll could throw ConcurrentModificationException. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The TV player runs the buffering-quality controller outside Watch Party rooms. Each mounted plan is a new adoption except a client-side same-plan remount, which is a reload. The controller's replans ride the quality transaction so subtitle state survives them, as a menu pick does, and a replan that fails or is dropped before commit is reported back. The notice overlay gains the prompt and undo pills with the intro pill's countdown fill and focus rules: the pill takes focus unless the viewer is mid-scrub, is not drawn while the HUD or Next Up is open, Left/Right stay inside the prompt, and Select and Back are handled at the player root. Intro, recap/credits and Next Up pills outrank it. The HUD labels Auto "Auto · limited after buffering" under a learned limit, and a slow plan that is still receiving data no longer takes the startup stall's same-plan reopen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After the buffering controller lowered Auto, the next automatic replan in the same playback (a subtitle track change, a seek reanchor or a failure recovery) went out with no estimate, so a server without buffering_quality_v1 planned the source again and playback stalled, lowered and climbed back in a loop. Every later replan in the attempt that reads as Auto now carries the step-down estimate, without bandwidth_estimate_source since it is not a new observation. Restore quality, any quality change the controller did not send, and a new attempt drop it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On a throttled link the startup stall detector reported decoder_no_output with 1.5 s buffered: the decoder had a little input and no first frame because the network had not delivered more, not because it was wedged. The failure replan that followed raced the buffering controller's lowering. While the player is buffering with less than the threshold buffered, the transport now owns the stall, as it already does for the Dolby Vision transform deadline: the decoder deadline stays anchored, and a stall that expires is a transport_stall, which the screens hand to the buffering controller when data is still arriving. A playing player with silent video is still the decoder's fault. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port the web player's probe to Android: a 40 MB download timed over a 2-second window from the first byte, with the same warm-up, first-byte deadline and minimum-window rules. It runs in the background a few seconds after sign-in or a return to the foreground, never while media loads or plays, and never on a metered or cellular link, against servers that advertise connection_speed_v1 and a bandwidth_probe_url. A redirect to the media proxy is followed without credentials. Results are cached for 30 minutes, discarded on any network change, and reported to the server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A video start now sends the probe's fresh estimate as bandwidth_estimate_kbps with source probe, so Auto can rule out a source the link clearly cannot carry. Only starts carry it: a probe estimate is a hint, and replans never raise quality from it. Servers without buffering_quality_v1 get the estimate without the source. Media3's bandwidth meter is still never sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mance The client now reads and sends bandwidth_estimate_kbps on start and replan, so the golden fixtures round-trip without the UNSENT_NETWORK_EVIDENCE exemption. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Phone and TV Playback settings gain a Connection group on servers that advertise connection_speed_v1: the measured speed with when and on what network it was measured, a Test speed action (a 429 says when to try again), and the server's own "Auto plays videos up to N Mbps without converting them" budget. On cellular or metered links the row says speed tests are skipped there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r architecture Update the quality policy and network inputs sections for the buffering controller and its server-side learned limit, the probe and its gates, the probe estimate on starts, the step-down estimate on lowering replans, and the rule that Media3's bandwidth meter is never sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rule for repeating a lowering's step-down estimate on later Auto replans lived inside PlaybackSessionManager, out of reach of the shared vector runner. The revised buffering-quality vectors now test it through client_replan, quality_changed and new_attempt events, so the rule moves to a pure function in shared/domain/player that the manager calls under its existing atomic reference. Behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ring vectors Re-vendor vectors.json unchanged at silo-server 408328dc7, which adds the kept-estimate rule and three events: client_replan, quality_changed and new_attempt. The runner now passes the controller's replans and the client's own replans through withKeptStepDownEstimate, as PlaybackSessionManager does, and takes each replan body from the wire request. All 37 cases pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The controller keeps no log, so a device log showed when a lowering was sent but not the waits and resumes that led to it. The Media3 adapter now logs every event it sends (playing, waiting with its cause, pause, seek, transport errors) under the BufferingQuality tag, which is enough to replay a field report's decisions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The player draws no pills in picture in picture, but the buffering pill's 15-second timer kept running there: the undo expired unseen, and an Ask first prompt timed out and cleared the stall history. The buffering spec now says a player that cannot show its pills hides them as it does behind a higher-priority pill (silo-server 457592aac). Picture in picture now counts as a higher-priority pill on phone and TV, so the buffering pill waits with its remaining timer and shows when the full player returns. The phone also counts a stopped activity, since playback can continue in the background there; the TV player already pauses or closes when it leaves the foreground. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the decoder falls behind, Media3 drops to the next keyframe and reports STATE_BUFFERING with seconds of media still buffered. The adapter read every such wait as the network running out, so a device that could not decode fast enough lowered quality and taught the server a network limit it never hit. On an overloaded emulator, a 2000 kbps plan through a 6 Mbit/s relay was lowered to 1120 kbps after waits that began with 3.6 s and 6.4 s buffered. That matches the 2000 and 1120 kbps step-downs seen earlier on the Mac mini. A wait that starts with more than a second buffered past the playhead is now a renderer wait, which the controller does not count, as with a seek. If its buffer drains before playback resumes, it becomes a network wait. Both players give the adapter the player's totalBufferedDuration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ring vectors The spec now defines the renderer wait Android started reporting in 2e02cf9 (silo-server 416f0d8ed): a wait that begins with more than RENDERER_WAIT_BUFFER_MS = 1000 of media buffered is not a stall, and one whose buffer falls to that threshold becomes a stall from that moment. Re-vendor vectors.json unchanged at 416f0d8ed (39 cases, with renderer_waits_do_not_count and renderer_wait_drains_into_stall), map the renderer cause in the runner, and move the adapter's threshold into the controller's constants under the spec's name, so the constants check covers it. All 39 cases pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Settings rounded every rate to whole megabits, so a 1400 kbps learned limit
read "Limited to 1 Mbps after buffering" and a 1013 kbps budget read "up to
1 Mbps". Below 10 Mbps that hides up to 40% of the figure.
The learned-limit row, the connection speed line and the Auto budget now
show one decimal below 10 Mbps ("1.4 Mbps") and whole megabits from there
("44 Mbps", as in the spec), formatted for the device's locale, with kbps
only below 100 kbps. The web already shows the decimal.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reset re-read the connection but updated only the learned-limit row. The Connection group's "Auto plays videos up to" line, which the server caps at 80% of the limit, kept the old figure until Settings was reopened: after resetting a 1400 kbps limit it still read 1.1 Mbps. A successful reset now reads the connection once more and updates both the limit row and the Connection group, on phone and TV. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Media3 adapter reports STATE_IDLE as a stop, and the controller treated every stop as the viewer pausing. After a player error and its recovery, the controller still counted the viewer as paused, so an undo pill shown while the recovered plan buffered kept its 15-second timer frozen until the first frame. On the emulator that left the pill up about 25 seconds too long. The spec freezes a pill only while the viewer has paused. A wait for any cause but a pause now means the player is trying to play: the controller stops counting the viewer as paused and resumes a frozen pill timer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w hidden Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…opens Settings read the learned limit and connection speed once, when its ViewModel was created. The TV ViewModel lives as long as the activity, so a limit learned during playback never appeared in TV Settings until the app restarted. The phone page went stale the same way when it sat in the back stack under the player. Both now read the connection whenever the Playback settings screen enters composition, which covers returning from the player. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the renderer wait from the spec (a wait that begins with media buffered is not a stall) and the pill holds behind higher-priority pills, in picture in picture and, on the phone, in the background. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clients re-tested every 30 minutes while open, and on every network change. For 300 users on 2 devices that is about 3,600 tests and 90 GB a day, each saturating the server's upload for 2 seconds. The approved redesign tests only when the server holds no current reading for this device and network. The 30-minute expiry, its re-test timer and the 5-minute retry are gone. A reading now lasts 7 days, for the server and network it was taken on, and playback starts send it for that whole time. A check, a few seconds after sign-in, a return to the foreground or a network change, uses the reading in hand when it has one. Otherwise it reads this device's stored reading from GET /api/v2/playback/connection and measures only when the server has none young enough. A check that fails or is refused waits for the next one, and after a 429 none measures before its Retry-After. Test speed always measures, replacing a running automatic check. The metered, cellular and playback gates are unchanged. The target, lower bounds, playback samples and the new measurement wait for the spec change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
| reloadMarked = false | ||
| lastBufferedPositionMs = null | ||
| lastDataAtMs = null | ||
| setTrouble(load = false, offline = false) |
There was a problem hiding this comment.
Premature outage clearing in reset() clears transport trouble during TV same-plan retries because TvPlayerViewModel.adoptBufferingQualityPlan calls reset() followed by markReload() without adopting a new plan, emitting onTransportErrorEnd before media arrives and resuming startup stall timing during the existing outage. Distinguish new-plan adoption from same-plan remounts and preserve transport trouble, including the device's offline state, for same-plan remounts.
Prompt for LLM
File android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/BufferingQualityPlayerAdapter.kt:
Line 246:
Premature outage clearing in reset() clears transport trouble during TV same-plan retries because TvPlayerViewModel.adoptBufferingQualityPlan calls reset() followed by markReload() without adopting a new plan, emitting onTransportErrorEnd before media arrives and resuming startup stall timing during the existing outage. Distinguish new-plan adoption from same-plan remounts and preserve transport trouble, including the device's offline state, for same-plan remounts.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
… phone and TV The phone and TV carried identical label code and 36 identical strings for the buffering-quality and Connection rows. They now live once in android-shared: PlaybackSettingsLabels.kt and android-shared's first values/strings.xml, which also holds the phone-only cellular note. Each app still lays out its own rows; the screens that keep app strings reach the shared ones as CommonR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A VPN keeps its Android network id when the network it runs over changes, so a direct move between two Wi-Fi networks under one VPN is not a new key. Following the network underneath needs API 31, and behind a full tunnel the server sees one connection anyway, so the gap is documented rather than closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Media3 keeps playWhenReady and READY when a transient audio-focus loss suppresses playback, so the adapter kept the player running or stalled. Pill timers and played_ms ran on while nothing played, and a suppression during a stall kept the stall timed after the buffer filled, which could lower quality and teach the server a limit. A suppression is now a stop, as a pause is, seeded when the adapter attaches. (Audit C2.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The server files a plan summary's throughput_kbps under the connection of the request that delivers it, so a plan that played across Wi-Fi and cellular could credit one network with the other's rate. The meter now notes the default network at each transfer and at each count, and a count that saw it change reports nothing (spec D1, Playback samples). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The spec's lower rung (silo-server be5b70497) is the highest entry below the lower of the rung the plan names in active_quality and the delivered bitrate, or below the delivered bitrate when the plan names no entry. The picked label no longer sets it, so a capped plan steps from what it plays. The plan carries active_quality only from a server advertising plan_active_quality_v1. The vectors are re-vendored from be5b70497: 61 controller cases, two of them new, all passing, with the harness reading active_quality. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sked for The pill timer subtracted the 1 s step it scheduled, not the time that passed, so a late resume kept a pill on screen a little past 15 s, and a freeze before an overdue tick lost that tick's elapsed time. Both now subtract the time that passed, capped at what was left. (Audit C9.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A quality pick, a version switch and the phone's move to a network's quality pair cleared the title's buffering answer and re-offer wait as soon as they were requested, so a pick the server refused still counted (spec D16: a change the player refuses is not one). Each screen now keeps what the last adopted plan played at (quality, cap, file), and an adoption the controller did not ask for that differs from it is the viewer's quality change. The pick still cancels a waiting Restore, whose owed clear rides on the pick's request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e controls On TVs below API 36, Back reaches the player's key bridge. The buffering pill and the intro pill each take the DOWN, but once the hidden pill has recomposed, the UP falls through to the transport's Back and hides the controls, so one press does two things and a second Back exits playback. Latch the press a pill answered by its downTime, which its DOWN, repeats and UP share, and swallow the rest of that press. The next press has a new downTime, so the latch cannot stick if an UP is lost. The API 36 BackHandler path is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… failure notice The phone overlay shows a session notice first, then an AI subtitle failure notice, then the buffering notice. The buffering notice's slot waited only for session notices and a hidden player, so it spent its 5 seconds behind an AI failure notice, which lasts 8, and was never seen. The overlay now reports whether it holds an AI failure notice, and the slot waits while it does. Disposing the overlay clears the report, so a notice left up then cannot block the slot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the earlier check A check that starts on a connected network Android has not validated yet can still be waiting on its connection read when validation arrives. The recheck then joined that same run, so if the read failed, nothing read again until the next network change or foreground return. On validation, cancel an automatic run that has not started its download, so the recheck reads afresh. Test speed, a running download and a result's POST carry on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three reads write the Connection group's server state with nothing ordering them: the page's read, Reset's re-read and Test speed's re-read. A slow page read that answered after Reset could bring back the learned limit the viewer had just reset, until the next read. Number each read and drop an answer once a read that started later has been shown. A newer read that was cancelled never counts as shown, so it cannot hide an older good answer. Test speed's re-read still keeps the rows when it fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test speed's "Try again in N seconds" started its N-second timer only after the connection read that follows the test, so a slow read kept the message up past the server's Retry-After. The spec says it shows until N seconds have passed. Mark the wait's end when the 429 arrives and hide the message then. A wait that ran out during the read shows no message at all. The message still says the N the server sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he controller adopted it The session manager remembers a plan's identity when a replan commits, for the plan summary it later sends. A TV replan commits before it publishes, so a replacement rolled back before it mounts left its identity queued under its plan id. Plan ids repeat within a playback attempt, so a later adoption of the same plan sent its summary under the rolled-back plan's session, attempt ids and estimate. Add PlaybackSessionManager.forgetPlanIdentity. The TV calls it when a reset or a lost context abandons a commit before adoption, and on a rollback whose mount the buffering-quality controller never adopted. A compensating rollback after the controller adopted the replacement keeps the identity, since the predecessor's remount sends that summary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… unknown The spec says a link whose type the platform cannot report counts as unmetered for the connection read, the start estimate and Test speed, but a phone or tablet runs no automatic test on it. Android names the link in almost every case. The exception is a VPN that runs over no Wi-Fi, Ethernet or cellular network it names: its own metered flag does not count, so nothing says whether it rides mobile data. The phone ran automatic tests there. Treat that link as unknown. The phone app starts the scheduler as a phone or tablet, and its automatic checks still read the server's readings there but end without a test. Test speed, starts, the main quality pair and the Settings row behave as on any unmetered link. The TV is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wnload A playback start cancelled a speed test only once it was downloading. A Test speed still reading the capability response went on to download if the player had not begun loading by then, and ran beside the start until the player's loading cancelled it. The spec stops Test speed when playback starts before its test begins. A start now also cancels a Test speed that has not reached its report. An automatic check still finishes its connection read and then holds off, as before, and a result already being reported still goes out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
…uiet while it is pending Two points of the controller's new event-table row for a menu pick (silo-server 243fb595c, D12 refinements): - Any pill hides as the pick is sent. Since bfce490 the controller heard of a pick only when its plan was adopted, so a prompt or undo pill stayed up while the pick was in flight. - A controller lowering adopted while a pick is pending shows no undo pill and no notice; the pick replaces that plan. The pick is pending from when it is sent until its request is over: its plan is on the way or it was refused. The answer, stall history and kept estimate still change only when the pick's plan is adopted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
Comment on lines
+513
to
+514
| pickPending = true | ||
| hidePill() |
There was a problem hiding this comment.
Duplicate quality prompts occur because onQualityPickSent hides the existing pill, but evaluate can recreate it while pickPending is true: when an ASK-mode long stall recovers before the quality request finishes, onPlaying records longStallEndedAt and immediately calls evaluate. Gate trigger evaluation while the viewer's pick is pending, then reevaluate the current stall when the request settles without a replacement plan.
Prompt for LLM
File shared/src/commonMain/kotlin/org/siloserver/silo/domain/player/BufferingQualityController.kt:
Line 513 to 514:
Duplicate quality prompts occur because onQualityPickSent hides the existing pill, but evaluate can recreate it while pickPending is true: when an ASK-mode long stall recovers before the quality request finishes, onPlaying records longStallEndedAt and immediately calls evaluate. Gate trigger evaluation while the viewer's pick is pending, then reevaluate the current stall when the request settles without a replacement plan.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
… once when it is refused The D12 vectors (silo-server aadc7fdf8) pin the controller's menu-pick row. Two points 668e88b missed: - Nothing prompts or lowers while a pick is pending, so a pill the pick hid during a long stall no longer comes straight back. - A refused pick changes nothing but the hidden pill, and a long stall in progress is evaluated at once. The pick stays pending until it is refused (the phone's failed quality change, a failed TV transaction), until a plan the controller did not ask for is adopted, or until the title resets. The vectors are re-vendored from aadc7fdf8: 66 controller cases, five new, all passing. The harness maps pick_sent and pick_refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
| ) | ||
| } | ||
| noteQualityCommitted(snapshot.transition.committed.qualityPreference) | ||
| if (snapshot.qualityChangeFailed) bufferingQualityController.onQualityPickRefused() |
There was a problem hiding this comment.
Unsettled pickPending after a terminal quality request failure causes the new evaluate guard to disable buffering prompts and automatic lowering until another plan adoption or reset: activePlaybackQualityId can highlight a different delivered rung, allowing selection of a preference that already equals the committed preference, but MobileSubtitleTransactionAdapter.changesQuality then makes qualityChangeFailed false. Settle pending picks on terminal request failure regardless of whether the requested preference differs from the committed value.
Prompt for LLM
File androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt:
Line 4393:
Unsettled pickPending after a terminal quality request failure causes the new evaluate guard to disable buffering prompts and automatic lowering until another plan adoption or reset: activePlaybackQualityId can highlight a different delivered rung, allowing selection of a preference that already equals the committed preference, but MobileSubtitleTransactionAdapter.changesQuality then makes qualityChangeFailed false. Settle pending picks on terminal request failure regardless of whether the requested preference differs from the committed value.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
…icks The pick rule covers a version switch and the phone's move to the quality pair of the network it is now on (D12/D16 scope of a "pick"), not only menu picks. They counted as quality changes only at adoption (bfce490), but did not hide the pill or hold the controller while in flight. Now each one calls onQualityPickSent() when issued. A failure calls onQualityPickRefused(): the phone's failed version switch before it restarts the previous version, a TV version switch whose replacement load fails while the current version keeps playing, and a network move after which the session's quality pair is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
A pick can be queued behind a replan and replaced by a newer one. The controller's pending pick was one flag, so the older pick's refusal, or the adoption of its plan while the newer one waited, ended it, and the controller could prompt or lower while the newer pick was still on its way. On the phone, a network move checked its outcome only after the menu picks it held back had been sent, so it could do this too. Each screen now keeps its latest pick in BufferingQualityPicks: how to recognize that pick's plan, and whether its request is still on the way. Only the latest pick's refusal settles the pending pick. An adopted plan settles it unless it is another plan while the latest pick is still outstanding, which onPlanAdopted's new pickStillPending carries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
…doption When a subtitle change couldn't be adopted, the phone stopped the session and then reloaded the title, and the undo pill stayed up while the session stopped. A Restore tapped then replanned the plan being ended, so its learned_limit clear could be refused or lost. Every other same-title restart ends the plan as it begins (onNewAttempt), which hides the pill; this one now does too, before the stop. A Restore that was already waiting gives its owed clear to the restart's start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Related issue: Silo-Server/silo-server#2699 (server side), Silo-Server/silo-server#637, #631
Validation tasks: changes #7 and #8 (Play and Playback robustness), #347 C3 and #348 C3 (manual quality), and #344 C1 and #345 C1 (own-device overrides); none of them is recorded as passed
When a remote connection can't keep up, Android playback stalls, recovers and stalls again until the viewer picks a lower quality by hand. Since #613, Auto requests carry no bandwidth measurement, so the server plans the source file whenever the device can play it, even on a link slower than the file. #631 is one report: a 50 Mbps file played directly over Wi-Fi that delivered about 30–40 Mbps, and it buffered four times in 90 seconds.
This brings the server's buffering-quality design to the phone and TV apps. They measure the connection and send that reading when playback starts. When playback keeps buffering anyway, they lower quality, or offer to.
Approach
The apps follow the buffering-quality spec as settled at silo-server 4ab2b8e95, after a review that compared web, Apple and Android against it.
BufferingQualityControllerinshared/drives both players, fed by one Media3 adapter. Its tests replay the spec's conformance vectors.playback.buffering_quality_mode): Automatically (the default), Ask first, or Never.active_qualityand the delivered bitrate, or below the delivered bitrate when the plan names none.learned_limit: "clear"goes out once, on whichever request comes next.quality_changereplan whose estimate is 70% of the delivered bitrate.learned_limit: "ignore"and no estimate, because this device's limits aren't the receiver's.bandwidth_estimate_kbpswith sourceprobe. A reading goes only to the server and route it was measured on.plan_summaryroute event:throughput_kbps, counted from this plan's media requests that received data for at least a second, and left out when the network changed while the plan played.throughput_kbps.buffering_quality_v1andplayback_plan_summary_v1. The speed test and the Connection group needconnection_speed_v1.playback.buffering_quality_mode. Without it the row hides and the apps run Automatically.docs/playback/01-media3-only-player-architecture.md§3.5–3.6 anddocs/playback/sequenced-api-v2.mddescribe the behavior.It is one pull request for the same reason as the server's: the speed test is how Auto gets its budget, and the learned limit and the reading feed that same budget. The server side is Silo-Server/silo-server#2699, and the Apple apps get the same behavior in Silo-Server/silo-apple#842.
Validation
./scripts/test-check-build-supply-chain.sh,./scripts/check-build-supply-chain.sh): passed../gradlew testDebugUnitTest) at c9c3d65, after mergingmain: 6,290 tests inshared,android-shared,androidApp,androidTvAppandlibass-bridge, 0 failures. They replay the spec's 66 controller cases and 13 speed-test cases, vendored from silo-server aadc7fdf8.ServerSetupPersistenceTestoccasionally fails two cases on aDispatchers.Mainrace in a full run; it passes alone and on rerun.:android-shared:lintDebug :androidApp:lintDebug :androidTvApp:lintDebug :androidApp:lintVitalRelease :androidTvApp:lintVitalRelease) at 99d9217 (unit tests and debug builds re-run at c9c3d65; CI covers the rest): no findings beyond the baselines.:androidApp:bundleRelease :androidTvApp:bundleRelease,:androidApp:assembleDebug :androidTvApp:assembleDebug) at 99d9217 (unit tests and debug builds re-run at c9c3d65; CI covers the rest): passed.maindoes the same wasn't checked.bandwidth_estimate_kbpswith sourceprobe.observed_rebufferestimate, and showed Restore quality on all four devices.main, the same title on the same link buffered with nothing offered.Evidence
Evidence: https://evidence.siloserver.org/r/silo-android/buffering-quality/
Surface and build: the first set compares
mainat b1e5774 with f3146fb on Android 16 emulators (phone, tablet, foldable folded and open, and Android TV 1080p) against an isolated dev server running the server branch. Each pair shows the same screen, profile and title: Playback settings, and Aladdin buffering on Auto at 52:37–52:48 over a 4 Mbps link. The Ask first prompt, Test speed and the learned-limit row are new, so they have after captures only.The review fixes compare f3146fb with 0b3a808 on Android 16 phone and TV emulators against the same server:
Tablet and foldable run the phone app's Settings code, so the 429 and rounding changes reach them unchanged; they weren't captured again. The menu rule is TV-only.
Risks
sync_key, Auto best-effort). Re-vendoring them is a separate change.Checklist
AI Disclosure
🤖 Generated with Claude Code
Note
Add buffering-quality lowering and connection speed testing to playback
BufferingQualityControllerthat detects network stalls during playback and either lowers Auto quality automatically or prompts the user (Lower/Keep), with an undo/Restore pill and a persistednever/ask/alwaysmode setting.TransferListenerand plan summaries are journaled as route events with a 24-hour replay window.gatedForBufferingQualityagainst server features — checkSequencedPlaybackadmission and replan paths; unsent route events now survive settled-journal compaction, so checkPlaybackJournalEntry.keepingRouteEvents; settings sync now treats unconfirmed keys as local-only, which changes which keys are read from and flushed to older servers (AndroidPlayerSettingsStore.remoteReadKeys,DefaultServerSettingsFlusher.flush).Macroscope summarized f509c13.