Repository navigation
fix(transcode): preserve portrait geometry with short-side rungs - #5232
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Retain the portrait quest while strict browser validation exposes the subscription cancellation failure. Co-Authored-By: GPT-6 Astra <noreply@openai.com>
A retired instance aborted its requests with a plain Error, which players log as an unexpected failure. Rust ends them with Unroutable; mirror it so watch treats a superseded instance as going offline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
race() returned as soon as an input had already settled, without observing the rest. A loser that rejected later, such as a subscription's response reader after a local close, surfaced as an unhandled rejection in the browser. Observe every input like Promise.race, keeping one shared reaction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A CLI peer that falls back to WebSocket and then upgrades to QUIC replaces its session. On lite-06 that is a new epochless instance, so the source and derivative catalog requests fail as unroutable by design. Lite-07 epochs carry them across the upgrade. The strict browser run now passes, so the portrait quest is complete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed the full current PR at b146695 against base 62d727c; no earlier review with this attribution was present.
No actionable correctness findings in the current changes. The direction is sound: scaling both coded dimensions from the short side keeps portrait and rotated-landscape geometry consistent while retaining the no-upscale/bitrate filters. Observing race losers in the shared primitive and giving retired instances a typed Unroutable error address the underlying lifecycle behavior rather than special-casing the player. The existing shared promise reaction remains useful for repeated races; a bare Promise.race replacement would reintroduce accumulating reactions on long-lived inputs.
The geometry/rotation unit coverage and strict browser harness fit the change. Verification was GitHub-only static inspection of the patch and relevant surrounding code; I did not execute tests. Exact-head CI, including Check and Interop, was still running when checked. No independent review findings were present at publication.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b14669573d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Portrait transcode | ||
| run: nix develop --command just test portrait |
There was a problem hiding this comment.
Unlike the surrounding suites, this step has no explicit if, so GitHub Actions applies the implicit success() condition. If any earlier independently-run suite fails during a nightly or manual run, the portrait test is skipped instead of reporting its own result, contrary to the workflow's stated independent-suite behavior. Give it the same !cancelled() && steps.setup.outcome == 'success' condition as the adjacent test steps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in d78aa1b: the portrait step now uses the same !cancelled() && steps.setup.outcome == 'success' condition as its neighbours.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed d78aa1b against our previous b146695 review. The base is unchanged; the only delta is .github/workflows/interop.yml:130.
The added condition fixes the independent Codex finding at #5232 (comment): portrait now runs after an earlier suite fails, while still requiring successful setup and honoring cancellation. This is the minimal fix and matches adjacent suites. No new actionable findings; our previous review had no open findings.
Verification: GitHub-only static comparison and surrounding workflow inspection. I did not execute the workflow or tests.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Problem
A 360-pixel rung from 1080x1920 portrait input became 202x360, spending a landscape bitrate on a much smaller picture. Rungs should size the coded short side, producing 360x640 while preserving rotation metadata.
The strict portrait end-to-end harness (publish, transcode,
<moq-watch>in Chromium, failing on any page or console error) also exposed two JS bugs and one harness race:subscribe stream ended without SUBSCRIBE_ENDsurfaced as an uncaught page error when a superseded catalog subscription closed. Reproduced 3/3 on the previous head.Error("broadcast instance retired"), which@moq/watchlogs withconsole.error. Reproduced 4/4.unroutableand the process exits.Approach
Transcode. Resolve both dimensions from the source's coded short side, round for I420 chroma, and apply the no-upscale and below-source bitrate checks to that side. Preserve catalog rotation. CLI help, README and
doc/bin/cli.mddescribe--rung size:bitrateas short-side pixels.@moq/signalsrace. Root cause of the missing-END error.racereturned as soon as an input had already settled, without observing the rest. The lite subscriber races[closed, producer.closed, updates.then(() => closed)]. If the producer had already closed, the third input was never observed, so the response reader's rejection after the local close went unhandled.racenow observes every input likePromise.race, still through one shared reaction per promise. The same applies to an already-abortedeffect.race. Confirmed in the browser: patching onlyraceon the old head turned 3/3 failures into 3/3 passes. A unit regression test fails without the fix.@moq/netretirement. A retired instance now aborts withStreamError(StreamCode.Unroutable), mirroring Rust'sError::Unroutable, so players treat it as the instance going offline rather than as a failure. A regression assertion inconsume.test.tsfails without the fix.Harness. The portrait relay accepts
moq-lite-07-wipandmoq-lite-06. The CLI publisher and transcoder negotiate lite-07, whose epochs carry an initial request across a session replacement, per the decision in #5333. Chromium stays on lite-06. Page errors are still rejected. Nothing waits, retries or disables a fallback. The portrait quest is complete and removed.Validation:
just checkandjust test interop --allpass, andjust test portraitpassed 5/5 on a loaded machine.Public API
Breaking, Rust
moq-transcode:Rung.heightis nowRung.size.Rung::new(size, bitrate)takes coded short-side pixels.ladder::Error::Empty { size, .. }andladder::Error::Unordered { size, below_size, .. }replace theheight/below_heightfields.Behavior only:
moq transcode --rung size:bitrate: the syntax is unchanged, but the value is now the short side. A portrait source gets the intended larger picture, and landscape output is unchanged.@moq/net: requests on a retired broadcast instance reject withStreamErrorcodeUnroutableinstead of a plainError. Signatures are unchanged.@moq/signalsrace: no longer leaves a losing input unobserved. Signatures are unchanged.Wire: none. Catalog format and media framing are unchanged. Catalog dimension values for portrait sources change. No JS or binding exposes
Rung.Decisions
Rung field name (maintainer):
size, documented as short-side pixelsheight(misdescribes portrait input)short_sideMissing SUBSCRIBE_END page error (maintainer: fix at the source, don't relax the harness):
raceso every input is observed, asPromise.racedoesRetired-instance error type:
StreamError(Unroutable)in@moq/net, mirroring Rust@moq/watchHarness across a WebSocket-to-QUIC upgrade:
Follow-ups
race. The common path, where no input has already settled, is unchanged apart from one branch. The decided path now does one WeakMap lookup per remaining input instead of returning early.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)