Repository navigation
Open the next source when every rung below it has failed, where playback had reached - #195
Merged
Merged
Conversation
…ack had reached `MediaRequest.sources` has been ordered and unused since Phase 1: the first entry played and the rest were validated, retained and never opened. This is rung 4 of ADR-0011's ladder, which finishes them. The rung is split across the boundary rule 5 draws. Deciding is the ladder's: `NextSource` reads the class `ErrorClassifier` assigned and nothing else, and refuses a `Device.DecoderTransient` so it still reaches rung 5 without paying for a second manifest first. Performing is core's, because opening another source is a re-adoption and only the facade can make one. The two meet at a third slot in the engine seam, `EngineConfiguration.playerStateRungs`, which is the one that faces the other way: core asks it when Media3 surfaces a failure to the player, and a failure surfacing is itself the proof that rungs 1 to 3 declined or were spent. What core does then is deliberately not `adopt`. The identity, the measurement session and the CMCD `sid` are the ones the viewing started with, so a fallback stays one session and one cache key; the held position is read before anything replaces the item and carried into `setMediaItem` rather than re-derived from the request's `StartPosition`, which is rule 9 and which is what stops a viewer four minutes into a programme from starting it again. The remembered-position map is updated with it so `ResumeFromLastKnown` agrees at the instant of the switch rather than at the next adoption. A failure the rung repairs is also withheld from the consumer's listeners: rule 10 makes the delivered error rung 6's, and a rescued session that also reported an error would be two accounts of one viewing. That is the one callback the reflective listener wrapper may swallow, and it is asked about the error before the pass that delivers it begins, because Media3 hands one event to every listener at once. `NextSourcePlaybackTest` forces the rung through the harness with a DASH first source and an HLS second — the protocol change `PRD.md` §2.2 names — over one transport serving both (`TestContent.alsoServing`), and counts the order in the traffic: the origin spends its budget, the mirror the MPD names is tried, and only then is anything of the second source fetched. A player built with core alone opens the first source and nothing else, which the core test that pinned the old retention now says instead of deleting it; there was one such test, not two. Closes #181 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeTWwmK1KcFMrfSAre23GS
…makes from ADR-0003 The code was right and the ADR had stopped describing it. CLAUDE.md requires the argument be made rather than left implicit, so it moves out of the source comments and into the documents #182 and #183 will read. Three addenda, recorded at the rules in this repository's own convention rather than as a superseding ADR: ADR-0011 is two weeks old, this batch is its implementation, and nothing it decided is reversed. ADR-0011 rule 5 named `adopt` as rung 4's path. Rung 4 does not take it. What the rule actually requires is the list that follows the path — the position, the identity, the telemetry session, the CMCD `sid` — and `adopt` is the one call that would break three of them, because it is where a measurement session opens and its whole job is taking on *different* content. The rule is restated as what the rung must preserve rather than as which function it must call, which is the form rung 5 will need too. ADR-0011 rule 13 said two slots and there are three. The addendum states the count and the asymmetry that earns it: the two above are components core *calls*, handed over at engine construction; the third is a question core *puts*, at failure time, on the application thread. Neither existing slot could carry it — a `LoadErrorHandlingPolicy` is asked about a load and may answer only in Media3's vocabulary — and folding it in would mean the module reaching the facade. It also says why rung 5 is a second question on the same interface rather than a fourth slot, and why rule 14's accounting differs here: an empty third slot is the absence of a listener rather than of configuration. ADR-0011 rule 10 gains the consequence it implied but did not draw: a failure a rung repaired is withheld from the consumer's listeners, in both the forms Media3 delivers one. That is a deviation from ADR-0003 rule 3, which now carries the matching note at the rule — bounded to those two callbacks, to a failure the player is taking on itself, and to a wrapper that withholds nothing unless a player gives it something to ask. `SuperPlayerForwardingTest` is untouched and passes: it wraps with the default, so all 37 callbacks still go through the proxy. The wrapper's own KDoc now names ADR-0003, so the next reader of that test finds the argument rather than a puzzle. Both ADRs carry an `Amended:` line, and CLAUDE.md's summaries of each match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeTWwmK1KcFMrfSAre23GS
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
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.
Closes #181
Rung 4 of ADR-0011's ladder: when retrying, changing host and excluding the variant have all failed, the next entry of
MediaRequest.sourcesis opened at the position playback had reached.MediaRequest's "only the first is used" sentences are gone, and the one test that pinned that retention is updated rather than deleted.The argued decisions
How the ladder reaches the player from a loading thread — it doesn't; it waits. Rungs 1–3 are answers
RetryingLoadErrorsgives Media3 about a load, on a loading thread. Rung 4 cannot be one of those: a re-adoption is an operation on the player's own state, and posting one from a loading thread would race the error Media3 is about to surface anyway. So the direction is inverted.FallbackLadderstill stops where it stopped; the failure that escalates out of the top of it stops being a load and becomes a failure of the session, which Media3 delivers to the player on the application thread — where the position is readable and a media item may be replaced. Core asks the resilience there, through a third seam slot,EngineConfiguration.playerStateRungs(PlayerStateRungs.opensNextSource). A failure surfacing is itself the proof that rungs 1–3 declined or were spent, so rule 7's order is imposed by the escalation rather than by a second climb. Rung 5 joins at the same interface as a second question (#182).The split follows rule 5 exactly:
NextSourcedecides, reading only the classErrorClassifierassigned — no secondwhenovererrorCode— and core performs, contributing the half the module cannot see (is there a next source, and is this player still playing the content that named it).NextSourcerefuses two classes. A ceiling ofTYPED_ERROR(Fatal.Unsupported) is not a permission to climb, and aDevice.DecoderTransienthas its ceiling above rung 4 and its remedy at rung 5 — a ceiling caps a climb and does not route it. Writing the first of those revealed thatFallbackLadder.climband this rung needed the same reading ofrungCeiling, so it is now oneFailureClass.mayClimb(rung); the routing test caught the naive>=before the review did.Position, and the tolerance. The held position is read before anything replaces the item and passed into
setMediaItem— never the request'sStartPositionread again (rule 9), which for aBeginningwould be the restart-from-zero §3.3 calls worse than the error. The remembered-position map is updated with it soResumeFromLastKnownagrees at the instant of the switch rather than only at the next adoption. The asserted tolerance is one segment of the second source: the position is carried exactly, but the new source is seekable only at its own segment boundaries, so Media3 begins at the boundary at or before it. The test also asserts a whole segment of the first source played before the fallback, so the claim is about a viewer who was watching rather than about rounding near zero.Identity. Rung 4 is deliberately not routed through
adopt: no measurement session is opened, so the telemetry session and the CMCDsidare the ones the viewing started with and the cache key still matches. Counted with a recordingTelemetryCollectorrather than inferred.One behaviour not in the acceptance criteria, flagged for review. A failure the rung repairs is withheld from the consumer's listeners. ADR-0011 rule 10 makes the delivered error rung 6's — "what a consumer is handed once nothing below it worked" — and a rescued session that also fired
onPlayerErrorwould put an error dialogue over a playing video. That is the one callback the reflective wrapper may swallow; the decision is memoized against the failure instance because Media3 hands one event to every listener in one pass, so it has to be taken before that pass begins. If #183 wants the error delivered differently, this is the seam it moves.Was there a second retention test? No. Exploration and a second sweep found exactly one —
SuperPlayerMediaRequestTest.theFirstSourceIsPlayedAndTheRestAreRetainedUnused, nowtheFirstSourceIsPlayedAndACoreOnlyPlayerOpensNoOther, which has become rule 14's accounting for this rung.aRequestWithoutASourceIsRejectedWhereItIsBuiltwas left alone.Public API. Core's is unchanged — rung 4 is not a public call (rule 5). The only
api/diff issuperplayer-testkit'sTestContent.alsoServing, which serves two streams from one transport so a DASH→HLS fallback is playable by one player.Tests
NextSourcePlaybackTest(7, through the harness, nothing past the facade): the second source opens and plays; DASH→HLS, the protocol-change case; the position preserved within the stated tolerance; the order counted in the traffic (origin spends its budget, the MPD's mirror is tried, only then is anything of the second source fetched); a one-source request fails where it always did; identity and session unchanged; a consumer's listener told nothing. PlusFallbackLadderTest.theRungAbovePlaysAnotherSourceForEveryClassThatMayReachItAndNoOtherfor the routing.Faults are a 502 — not on Media3's own fallback table — at both of the DASH stream's hosts, so a fallback that happens is the ladder's. The mirror's half is never narrowed by index, because indices count distinct resources per host and a mirror faulted at the same number would serve the very segment the fallback moved for.
advanceTimeInStepsMsthroughout.ADR amendments (second commit)
Three departures are now argued in the ADRs rather than only in source comments, as CLAUDE.md's
binding rules require. Amended in place rather than superseded: ADR-0011 is two weeks old, this
batch is its implementation rather than a reconsideration of it, and nothing it decided is reversed —
so an addendum at each rule, in the convention ADR-0005, -0009 and -0010 already use, beats an
ADR-0012 that would restate an unchanged document to change three paragraphs. Both ADRs carry an
Amended:line;CLAUDE.md's summaries of both now match.ADR-0011 rule 5 —
adoptis named, and rung 4 does not take it. The rule says the re-adoption is"through core's own
adoptpath". Rung 4 bypasses it, and the addendum restates the rule as what therung must preserve rather than which function it must call.
adoptis the point at which ameasurement session opens, because its whole job is taking on different content: routing rung 4
through it would end the viewing's session and start a second one under the same content id — the
"one viewing reported as several" defect
MediaRequestexists to prevent — and would break the CMCDsidjoindocs/telemetry-schema.mdstates as a promise. So the rule's stated mechanism is whatwould break the three things the rule actually cares about. Rung 5 will need the same restatement.
ADR-0011 rule 13 — "two slots", and there are three. The addendum states the count and the
asymmetry that earns the third rather than folding it into either existing one. The two are things
core calls: components handed over at engine construction, filled once and then owned by the
engine.
playerStateRungsis a question core puts, at failure time, on the application thread. ALoadErrorHandlingPolicyis asked about a load, from a loading thread, and may answer only inMedia3's vocabulary — a retry delay or a fallback selection, neither of which can express "re-adopt
the player at the position it holds"; a chain layer is further from the player still. Folding it in
would mean the module reaching the facade, the direction the ADR exists to forbid. The addendum also
records why rung 5 is a second question on the same interface rather than a fourth slot (one rung
order; a second slot would let an implementation fill one and not the other), and why rule 14's
accounting differs here: an empty third slot is the absence of a listener, which a test counts,
not the absence of configuration.
ADR-0011 rule 10 / ADR-0003 rule 3 — the withheld
onPlayerError. Confirmed correct by thecoordinator and now written down twice. Rule 10's addendum draws the consequence the rule implied but
did not state: rung 4 acts from the player-error path, so Media3 has already raised the exception by
the time the rung can repair it, and delivering it would report a failure that did not happen. Rule
10 reserves the delivered error for rung 6, so it is withheld for exactly the failures a rung takes
on — in both forms,
onPlayerErrorandonPlayerErrorChangedincluding the null that clears it,since a consumer told only that an error went away is told about the failure in the one form they
cannot act on. ADR-0003 rule 3 carries the matching note, bounded three ways: those two callbacks and
no others, a failure the player takes on itself and never one that reaches the consumer's rung, and a
wrapper that withholds nothing unless a player gives it something to ask. The wrapper's KDoc now
opens by naming ADR-0003 rule 3 as the rule it excepts.
SuperPlayerForwardingTestpasses unchanged and needed no carve-out — the branch diff touches itnot at all. It wraps with
reportingSourceAs's default predicate, which withholds nothing, soMedia3's forwarding-contract assertion still drives all 37 callbacks through the proxy and still
catches the next
defaultmember Media3 adds. That is the third bound above, stated as a property ofthe seam rather than as a test exemption.
Two-axis self-review
Standards. No new
whenovererrorCode; the seam is internal and core's public API is unchanged (updateApiSurfacediff committed);docs/testing.mdrespected — every player is built as a consumer builds one and nothing asserts past the facade; every new constant carries its reason;// ref:on the 502 and the segment-index reasoning. Fowler: found and fixed Duplicated Code twice — the ceiling reading (extractedmayClimb) and the two identical proxy branches (merged, withforwardextracted from the existing body). Rejected as Speculative Generality: a second rung entry in a map, and an enum return with an unusedRECREATE_DECODER;PlayerStateRungsis a plain interface so #182 adds a method rather than reshaping one.Spec. Every acceptance box is a named test. Rule 5 ("not a public call", "only when the ladder asks", "keeping the identity, the telemetry session and the CMCD
sid") ✓. Rule 7 (order) ✓, asserted on traffic. Rule 9 ("carries the held position across the operation explicitly — the same instant, not the request'sStartPositionread again", "updates the remembered-position map") ✓, with the tolerance stated. Rule 14 ✓, counted as behaviour.What was skipped
Emulator and demo runs, for host memory — the change is in the failure path and the harness is where it is observable anyway.
./gradlew assemble checkis green locally (superplayer-telemetry's golden traces included: no trace changed, since a fallback that rescues a session opens no second session).CI: https://github.com/ramesh130/superplayer/actions/runs/35057881672 — success (first commit, the code).
CI: https://github.com/ramesh130/superplayer/actions/runs/35060645037 — success (second commit, the ADR amendments).
🤖 Generated with Claude Code
https://claude.ai/code/session_01HeTWwmK1KcFMrfSAre23GS