Skip to content

fix(moq-mux): TS export stops trimming MPEG-2 open-GOP leading pictures - #8830

Open
kixelated wants to merge 3 commits into
mainfrom
quest/m1/ts-open-gop-trim
Open

kixelated wants to merge 3 commits into
mainfrom
quest/m1/ts-open-gop-trim

Conversation

@kixelated

@kixelated kixelated commented Nov 15, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m1/ts-open-gop-trim.md.

Problem

moq-dev/moq#7365 added Mpeg2Picture, a picture-header parser that scanned every MPEG-1/2 video PES the TS export forwarded, to trim open-GOP leading B-pictures after a hole. No consumer asked for it, and decoders already skip those B-pictures. The maintainer picked deleting it on moq-dev/moq#8764.

Approach

  • rs/moq-mux/src/container/ts/export.rs: MPEG-1/2 video takes the same path as every other whole PES in Track::queue (straight to push, no leading-picture judgment). Removed Mpeg2Picture, the open_gop and orphan track fields, and its unit test. H.264 open-GOP and HEVC RASL trimming, and RADL keeping, are untouched.
  • rs/moq-mux/src/container/ts/recovery_test.rs: removed mpeg2_open_gop_trims_only_the_orphans and the harness support fix(ts): trim open-GOP MPEG-2 leading B-pictures on export #7365 added only for it (the Mpeg2 profile and encoder, the whole-PES video track pickup, the PES-PTS branch of decoded). The Catalog<Ext> republishing and the grader's join-group accounting stay, since they are general.
  • doc/bin/ts.md: the semantic recovery bullet says which codecs drop orphaned leading pictures and that MPEG-2 video goes out whole.

Choices made unattended (reviewable): deleting the MPEG-2 recovery profile rather than rewriting its test to assert the B-pictures go out, since its grader models open-GOP orphans as must-trim and the profile existed only to cover the parser.

Impact

  • Public API: none. Wire: none.
  • Behavior: after a skip, cut, or join, the TS export now forwards an MPEG-2 open GOP's leading B-pictures instead of trimming them; the receiving decoder skips them.

Validation

  • just check passes (rs lint and tests for every changed crate, markdown, quest).
  • cargo test -p moq-mux --lib container::ts: 761 passed, including every remaining recovery test (open_gop_trims_only_the_orphans, hevc_trims_only_orphaned_rasl, hevc_keeps_radl_after_a_hole) and the MPEG-2 round trips (mpeg2_video_rides_whole_and_survives_a_round_trip, export_resumes_whole_mpeg2_video_at_a_sequence_header).
  • Second check of the behavior change: ran the deleted MPEG-2 recovery test from main against the new export. Its grader now fails with video unit 23 went out without the group before it: Unit { group: 2, ..., leading: true, ..., output: true }, i.e. the join group's leading B-picture is forwarded rather than trimmed, as intended.

Alternatives

Caused by

  • moq-dev/moq#7365 - added a per-frame MPEG-2 picture parser no consumer needed (found by person; the feature review)

Follow-ups

None.

Models

Size: XS
Implementer: Claude, claude-opus-5-5, unknown effort (arm B)

🤖 Generated with Claude Code

Review

Rewritten by quest review after each round.

  • Head: b2a93ab

  • Base: origin/main at 250a13a

  • Risk: medium, the base has no RISK.md to name what is always high or customer-facing, so every change is medium

  • Classified: medium by default

  • Reviewer: Codex, gpt-6.1-sol, medium effort

  • Session: 01a1df49-1709-7852-9e00-a4bbdacbcc55, round 1

  • Score: 9/10, Source inspection found no actionable regressions in forwarding MPEG-2 PES whole while preserving H.264/H.265 recovery handling, though builds and tests were not run as instructed.

  • Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0

  • Findings: none

kixelated and others added 3 commits November 15, 2026 04:48
…ming

Drop the MPEG-2 picture-header parser that trimmed open-GOP leading
B-pictures on every frame, its tests, and the recovery harness's MPEG-2
profile. HEVC RASL and H.264 open-GOP trimming stay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The code half of the previous commit, whose staging missed it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title TS export stops trimming MPEG-2 open-GOP leading pictures fix(moq-mux): TS export stops trimming MPEG-2 open-GOP leading pictures Nov 15, 2026
@kixelated
kixelated marked this pull request as ready for review November 15, 2026 12:58
@coderabbitai

coderabbitai Bot commented Nov 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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 27 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 772c5ee8-526f-4de5-8bd5-bd3ed81b14a2
Commits

Reviewing files that changed from the base of the PR and between 250a13a and b2a93ab.

Files selected for processing: 4

  • doc/bin/ts.md
  • quest/m1/ts-open-gop-trim.md
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/recovery_test.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No problems in the code. One follow-up quest still builds on the parser this PR deletes.

Reviewed changes

This review covers how the TS export removes MPEG-1/2 open-GOP trimming, plus the test and doc cleanup that goes with it.

  • Track::queue simplified: every kind that isn't Kind::Video now goes straight to push, including whole-PES MPEG-1/2. I checked that Track.held, recovery_gap and leading_floor are only read inside queue's video path. Skipping their updates for MPEG-2 therefore has no effect on anything else.
  • Mpeg2Picture and the open_gop/orphan state removed: push's leading check now falls back to false for every kind that isn't video.
  • Recovery harness trimmed: Profile::Mpeg2, Video::Mpeg2, the whole-PES video pickup, and the PES-PTS branch of decoded are gone. CLIPS shrinks to 8, which matches the remaining Profile variants indexed by self as usize.
  • Docs: the semantic recovery bullet in doc/bin/ts.md now limits trimming to H.264 and H.265.

ℹ️ quest/m5/pes-hold-open-gop.md still plans work on the deleted parser

That quest's goal is to make the MPEG-2 export trim leading B-pictures in zero-stuffed CBR streams. Its plan extends mpeg2_orphans_are_whole_pes_of_leading_b_pictures and builds on the per-track orphan state. This PR deletes all of those. The PR says "Follow-ups: None", but if that quest is left as it is, a later run will pick it up and rebuild what this PR removes. Its second half is still valid, though: H.264 (Open) and HEVC (Rasl) recovery tests have no case with a nonzero join. Since the MPEG-2 arm went away, nothing covers a join at a group already in flight.

Technical details
# Stale follow-up quest after deleting `Mpeg2Picture`

## Affected sites
- quest/m5/pes-hold-open-gop.md — the Goal and most of the Plan target MPEG-2 orphan trimming, which this PR removes. It also names `mpeg2_orphans_are_whole_pes_of_leading_b_pictures` and the per-track `orphan` state, both now gone.

## Required outcome
- No open quest asks for MPEG-2 open-GOP trimming to come back.
- The H.264/HEVC "join at an in-flight group" coverage is kept, either rescoped into this quest or moved to a new one.

## Suggested approach
- Rewrite the quest so it covers only adding a nonzero `join` arm to `open_gop_trims_only_the_orphans` (`Profile::Open`) and `hevc_trims_only_orphaned_rasl` (`Profile::Rasl`) in `rs/moq-mux/src/container/ts/recovery_test.rs`. Alternatively, delete the quest in this PR and file that coverage on its own.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant