[HLSTree] Keep a referenced rendition that shares its uri with an orphan by gizmocuz · Pull Request #2115 · xbmc/inputstream.adaptive

gizmocuz · 2026-09-27T07:31:49Z

Since #2112 / 913cf7c7 ("[HLSTree] Skip orphan renditions by default") a
multivariant playlist can end up with no audio adaptation set, when two
EXT-X-MEDIA groups share one URI and the orphaned group is listed first.

Two filters run over the rendition lists in ParseMultivariantPlaylist, and the
order they run in is the problem:

While parsing, a rendition whose URI has already been seen is marked
m_isUriDuplicate. First occurrence wins, and it cannot take variant
references into account, because EXT-X-MEDIA tags may precede
EXT-X-STREAM-INF.
The orphan check then drops a rendition when FindVariantByAudioGroupId finds
no variant.

A rendition failing either check is skipped, and nothing reconciles the two, so
both can fire within one URI.

Reproduction

The manifest added in this PR as src/test/manifests/hls/1a1v_master_orphan_shared_uri.m3u8:

#EXTM3U
#EXT-X-VERSION:7
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="group_aud_orphan",NAME="audio_0",DEFAULT=YES,URI="stream_0/out.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="group_aud_low",NAME="audio_0",DEFAULT=YES,URI="stream_0/out.m3u8"
#EXT-X-STREAM-INF:BANDWIDTH=1170400,RESOLUTION=1280x720,CODECS="avc1.64001f,mp4a.40.2",AUDIO="group_aud_low"
stream_1/out.m3u8

group_aud_orphan is parsed first, so it keeps m_isUriDuplicate == false.
group_aud_low has the same URI and is marked a duplicate, so it is skipped.
group_aud_orphan then reaches the orphan check: no variant carries
AUDIO="group_aud_orphan", so with the default
hlsAddOrphansRenditions == false it is skipped too.

The period ends up with no audio adaptation set. Before 913cf7c7,
group_aud_orphan was kept with the FOURCC_MP4A fallback codec and the audio
played. AddIncludedAudioStream does not compensate, because
var.m_groupIdAudio is not empty.

Reversing the two EXT-X-MEDIA lines makes it work, which is what makes this easy
to miss.

The subtitle loop has the same shape and is covered by the same change.

Why the shape is not contrived

It is what the comment above the audio loop already refers to - "There may be
multiple renditions with the same uri but different GROUP-ID": one audio group per
video codec family, every group pointing at the same audio playlist. A packager
that publishes only one family's variants, or a manifest filtered per device
profile, leaves the other group orphaned while still listing it.

The failure is also silent - the skip is a LOGWARNING and the stream plays,
just with no audio.

The change

Compute m_isUriDuplicate once, after parsing, letting a referenced rendition
claim a URI ahead of an orphan, and drop the parse-time marking so there is a
single writer. That removes the ordering dependency rather than compensating for
it. The parse-time comparison also used the raw URI attribute, so
audio/en.m3u8 and ./audio/en.m3u8 counted as different uris - resolution
happens later, in PrepareRepresentation.

Behaviour is unchanged whenever no URI is shared, and one surviving rendition
per URI across the audio and subtitle lists is preserved.

One deliberate difference: where an audio and a subtitle rendition share a URI,
the survivor can change - audio now wins, rather than whichever came first in the
playlist. That only arises on a manifest that is malformed either way, and audio
seemed the better thing to keep, but say the word if you would rather preserve
playlist order and I will make the passes follow it.

Tests

Adds ReferencedRenditionSurvivesSharedUriWithOrphan to
src/test/TestHLSTree.cpp, asserting one audio adaptation set on the manifest
above. Without the change that count is zero, which is the bug.

An alternative approach, if the two-filter arrangement is wanted for other
reasons: keep the parse-time marking but skip it when the GROUP-IDs differ, and
let FindMergeable collapse the adaptation sets later. Happy to redo it that way.

Summary by CodeRabbit

Bug Fixes
- Fixed HLS playlist handling so audio or subtitle renditions referenced by a stream are retained when they share a URI with an unreferenced rendition.

Two filters run over the rendition lists in ParseMultivariantPlaylist, and between them they can discard every rendition for one uri. m_isUriDuplicate is decided while parsing, first occurrence wins, and cannot consider variant references because EXT-X-MEDIA may precede EXT-X-STREAM-INF. The orphan check added in 913cf7c then drops any rendition no variant references. When two groups share a uri and the orphan is listed first, the referenced rendition is dropped as a duplicate and the orphan is dropped as an orphan, so the period gets no audio adaptation set. AddIncludedAudioStream does not compensate because m_groupIdAudio is not empty. The shape needs only one audio group per video codec family pointing at a single audio playlist - what the existing comment above the audio loop refers to - with one family's variants left unpublished. There is no error: the skip is logged at LOGWARNING and the stream plays without audio. Compute the flag once, after parsing, letting a referenced rendition claim its uri ahead of an orphan, and drop the parse-time marking so there is a single writer. That removes the ordering dependency rather than compensating for it; the parse-time comparison also used the raw URI attribute, so "audio/en.m3u8" and "./audio/en.m3u8" counted as different uris. Behaviour is unchanged whenever no uri is shared, and one surviving rendition per uri across both lists is preserved. Where an audio and a subtitle rendition share a uri the survivor can differ from before - audio now wins rather than whichever came first in the playlist - which only arises on a manifest that is malformed either way. The subtitle loop had the same problem and is covered by the same change.

coderabbitai · 2026-09-27T07:32:06Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4d51a09e-2d6e-46c8-9224-b94f7acf0ce0

📥 Commits

Reviewing files that changed from the base of the PR and between e14d199 and 12ee7ed.

📒 Files selected for processing (3)

src/parser/HLSTree.cpp
src/test/TestHLSTree.cpp
src/test/manifests/hls/1a1v_master_orphan_shared_uri.m3u8

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

The HLS parser now de-duplicates rendition URIs after parsing. It processes renditions referenced by variants before orphan renditions. A test covers audio groups that share a URI when the orphan group appears first.

Changes

HLS rendition URI de-duplication

Layer / File(s)	Summary
Prioritize referenced renditions during URI de-duplication `src/parser/HLSTree.cpp`, `src/test/manifests/hls/1a1v_master_orphan_shared_uri.m3u8`, `src/test/TestHLSTree.cpp`	The parser now marks repeated nonempty rendition URIs as duplicates after parsing, processing variant-referenced renditions before orphan renditions. The fixture and test cover an orphan audio group listed before a referenced group with the same URI.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: castagnait

Merge Risk: ⚪ Minimal · up to 12ee7

The shared-URI case is covered, and no actionable merge-blocking issue remains after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving a referenced HLS rendition when it shares a URI with an orphaned rendition.
Description check	✅ Passed	The description is detailed and relevant. It explains the bug, reproduction case, implementation, behavior changes, and test coverage. The Types of change and Checklist sections remain unmarked, but t…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

Fix all pre-merge checks with AI

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

_{Comment @coderabbitai help to get the list of available commands.}

matthuisman · 2026-09-27T23:24:22Z

@gizmocuz
would you mind adding a test for the previous PR behaviour too?

#EXTM3U
#EXT-X-INDEPENDENT-SEGMENTS
#EXT-X-MEDIA:URI="https://playertest.longtailvideo.com/adaptive/elephants_dream_v4/media/b160000-english.m3u8",LANGUAGE="en",TYPE="AUDIO",GROUP-ID="233",NAME="Default",DEFAULT="NO",AUTOSELECT="YES"
#EXT-X-MEDIA:URI="https://playertest.longtailvideo.com/adaptive/elephants_dream_v4/media/b160000-spanish.m3u8",LANGUAGE="sp",TYPE="AUDIO",GROUP-ID="234",NAME="Default2",DEFAULT="NO",AUTOSELECT="NO"
#EXT-X-STREAM-INF:BANDWIDTH=7171631,CODECS="avc1.64002A",RESOLUTION=1920x1080,FRAME-RATE=60,VIDEO-RANGE=SDR,AUDIO="234",CLOSED-CAPTIONS=NONE
https://playertest.longtailvideo.com/adaptive/elephants_dream_v4/media/b2962000-video.m3u8

the audio stream for group-id 233 should be removed

CastagnaIT · 2026-09-28T05:52:19Z

thanks for the fix! my mistake and unfortunately the AI review that wasnt working
but im thinking to rework code in a different way
your proposal use too many loops thats not so good
i will push alternative PR on next days

gizmocuz · 2026-10-03T07:53:42Z

Closing in favour of #2122, which fixes the same bug with less code - caching the
variant pointer on the rendition and sorting, rather than the loops here.

I checked it against both cases before saying so: the shared-uri manifest from
this PR, and @matthuisman's example above, where the unreferenced group is
correctly dropped. So the test asked for here is covered there, and there is
nothing further to add on this branch.

I have left one note on #2122 - the audio uri goes into the de-duplication set
lowercased while the lookups use the raw string, so de-duplication stops working
for any uri containing an uppercase character, and the same playlist can be added
twice. Worth fixing there before it lands.

Thanks both.

CastagnaIT mentioned this pull request Oct 3, 2026

[HLSTree] Fix no audio due to orphan rendition with shared uri #2122

Merged

1 of 11 tasks

gizmocuz closed this Oct 3, 2026

Navigation Menu

Repository navigation

[HLSTree] Keep a referenced rendition that shares its uri with an orphan - #2115

[HLSTree] Keep a referenced rendition that shares its uri with an orphan#2115
gizmocuz wants to merge 1 commit into
xbmc:Piersfrom
gizmocuz:fix/hls-orphan-shared-uri

gizmocuz commented Sep 27, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

matthuisman commented Sep 27, 2026

Uh oh!

CastagnaIT commented Sep 28, 2026

Uh oh!

gizmocuz commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gizmocuz commented Sep 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reproduction

Why the shape is not contrived

The change

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

matthuisman commented Sep 27, 2026

Uh oh!

CastagnaIT commented Sep 28, 2026

Uh oh!

gizmocuz commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gizmocuz commented Sep 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading