Skip to content

Live stream jump piers#2006

Draft
oliv3r wants to merge 4 commits intoxbmc:Piersfrom
oliv3r:live_stream_jump_piers
Draft

Live stream jump piers#2006
oliv3r wants to merge 4 commits intoxbmc:Piersfrom
oliv3r:live_stream_jump_piers

Conversation

@oliv3r
Copy link

@oliv3r oliv3r commented Mar 4, 2026

Description

To be able to jump forward (to the edge, e.g. the live moment) and jump backward (e.g. the start of the stream) on a live stream (when properly exposed/handled by the content provider/API, we need to cherry-pick commit b2da233 (Cleanups to chapter methods) from Piers which fixes the fact that we do not properly report the chapter count. Then add a commit to add a 'virtual' second chapter matching 'the end' which then gives us 2 chapters; ch1 'the start' and ch2 'now'. A third commit is to help with the start offset, like live_delay, we can add a bit of time to the start, where content providers love to add a conservative padding, to ensure they didn't miss the start of the program, or to make sure we get anything that was shown before the program (like an ad).

Motivation and context

Being able to 'jump to start' (using |< for example) or 'jump to live/now' using >| which relies on proper chapters to trigger.

How has this been tested?

Using retrospect and NLZIET we have livestreams from dutch television programs, moving this functionality to the |< and >| buttons allowed us to jump back/forward to the start/edge of a live stream reliably, with offset (where progress bar was showing a 'dot' of the offsetted jump point. Further more I used a test file as mentioned in #1517 and backported the changes to omega #2005 but with fixes in place as per review comments.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • I have read the Contributing document
  • My code follows the Code Guidelines of this project
  • My change requires a change to the Wiki documentation
  • I have updated the documentation accordingly

@oliv3r oliv3r marked this pull request as draft March 4, 2026 18:09
@oliv3r
Copy link
Author

oliv3r commented Mar 4, 2026

marking as draft, just to see the pipeline work, as i've also added tests.

@kodiai
Copy link

kodiai bot commented Mar 4, 2026

Kodiai Review Summary

What Changed

This PR adds support for jumping to live edge and start position in live streams by introducing a virtual second chapter, along with a new live_offset configuration parameter.

Reviewed: core logic, tests

Strengths

  • ✅ Well-structured test coverage for the new live_offset and live_delay features with clear test cases
  • ✅ Consistent parameter validation for live_offset (checks for unsigned number type before parsing)

Observations

Impact

[MAJOR] src/Session.cpp (1273): Unit mismatch in live offset conversion
m_liveOffset is stored in seconds (as documented and parsed in CompKodiProps.cpp), but GetChapterPos returns it directly without converting to milliseconds. The return type is int64_t representing milliseconds (consistent with line 1261 which divides m_totalTime by 1000). This causes the chapter position to be off by a factor of 1000 when live_offset is set.

[MAJOR] src/Session.cpp (1330): Potential seek to position zero when no streams enabled
If no streams are enabled in the SeekChapter method when jumping to live edge, maxTime remains 0, causing SeekTime(0, ...) to be called. This would incorrectly seek to the beginning instead of the live edge. Consider adding a check to verify at least one enabled stream exists before attempting the seek, or falling back to using m_totalTime as an alternative.

[MEDIUM] src/Session.cpp (1261): Missing validation for m_totalTime before division
When returning the virtual live chapter position, m_totalTime / 1000 is computed without verifying that m_totalTime is non-zero. While uncommon, if a live stream hasn't populated m_totalTime yet, this could return 0 for the live chapter position, making it identical to the start chapter position.

Preference

[MINOR] src/Session.cpp (1271-1273): Optional: Simplify conditional by extracting common checks
The condition number == 0 && m_adaptiveTree->IsLive() && m_adaptiveTree->m_periods.size() == 1 && m_adaptiveTree->m_liveOffset > 0 spans multiple concerns. Consider extracting the check into a helper method like ShouldApplyLiveOffset(int chapterIndex) to improve readability.

Suggestions

  • Optional: Consider adding a test case that verifies the behavior when SeekChapter is called for the virtual live chapter with no enabled streams
  • Future consideration: The TODO comment at line 1263-1267 mentions thread safety concerns with accessing m_periods asynchronously. The new virtual chapter logic also accesses m_periods.size() in multiple places without synchronization, which could be addressed in a follow-up PR

Verdict

🔴 Address before merging -- 2 blocking issues found

Review Details
  • Files reviewed: 6
  • Lines changed: +77 -9
  • Profile: strict (auto, lines changed: 86)
  • Author: first-time (adapted tone)
  • Findings: 0 critical, 2 major, 1 medium, 1 minor (includes 4 from summary observations)
  • Review completed: 2026-03-04T18:11:14.045Z
  • Keyword parsing:
    • focus hints: [MANIFESTCONFIG], [SESSION], [TEST]
    • breaking change in body

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Review

Must Fix (1) Medium (2)


Must Fix - GetChapterName regression (src/Session.cpp:1250)

The refactor changed the non-debug fallback from nullptr to CHAPTER_NAME_UNKNOWN ("[Unknown]"). Previously callers (Kodi core) received nullptr for non-debug mode on all streams; now they receive "[Unknown]", which will surface as a visible chapter label for every non-live stream. The CHAPTER_NAME_UNKNOWN return was previously guarded inside the IsDebugVerbose() block intentionally.

Fix: restore return nullptr at the bottom and only return CHAPTER_NAME_UNKNOWN inside the IsDebugVerbose() guard.


Medium - Virtual live chapter position is 0 when m_totalTime == 0 (src/Session.cpp:1261)

MPEG-DASH live streams frequently omit mediaPresentationDuration (optional for live profiles), so m_totalTime stays 0. GetChapterPos for the virtual live chapter then returns 0, making it indistinguishable from the stream start. Meanwhile SeekChapter for the same chapter correctly uses getMaxTimeMs() to find the real live edge - the two are inconsistent. If Kodi core queries GetChapterPos before calling SeekChapter (e.g. for progress bar position), it will report 0 instead of the live edge.


Medium - Silent fall-through to SeekTime(0) in live-edge seek (src/Session.cpp:1330)

If no streams are enabled when seeking to the virtual live chapter, maxTime stays 0 and SeekTime(0.0, 0, false) seeks to the very beginning rather than failing gracefully. At minimum an early-return with a log error should guard this path.

@oliv3r oliv3r force-pushed the live_stream_jump_piers branch 6 times, most recently from f783e84 to 9e3b7a6 Compare March 6, 2026 06:50
@oliv3r oliv3r marked this pull request as ready for review March 6, 2026 07:05
@oliv3r oliv3r force-pushed the live_stream_jump_piers branch from 9e3b7a6 to b440c66 Compare March 6, 2026 07:05
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Review

Medium Code duplication (Session.cpp)
The loop to find the live-edge max time is copy-pasted verbatim in both GetChapterPos (lines 1264–1274) and SeekChapter (lines 1334–1344), including the same maxTime > 0 ? maxTime : m_totalTime fallback. Any future fix to one will need to be applied to the other. Extract to a GetLiveEdgeMs() private helper.

Low Missing unit in m_liveOffset comment (AdaptiveTree.h:73, CompKodiProps.h)
m_liveDelay is documented as "in seconds" but m_liveOffset (same unit, same usage pattern) is not. The field is used directly as seconds in GetChapterPos so the missing annotation could mislead future readers.

Low SeekChapter same-period else if silently changes VOD behavior (Session.cpp:1367)
Before this PR, requesting a chapter that maps to the current period returned false. The new else if branch now seeks instead, affecting all content types, not just live. For VOD multi-period streams this changes what Kodi considers a failed seek into a successful one. Confirm this side-effect is intentional, or guard it with m_adaptiveTree->IsLive().

@oliv3r oliv3r marked this pull request as draft March 6, 2026 07:08
@oliv3r oliv3r force-pushed the live_stream_jump_piers branch from b440c66 to 90d0ba4 Compare March 6, 2026 07:13
@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Mar 6, 2026

now that i have had some time to read and understand it better
i will stop you right there before you continue wasting time
what you're doing is a hack, chapters must always match periods, and not generate fake/virtual chapters to use beyond their meaning

your proposal introduces complexities that lead to further complications in the maintenance of the add-on regards periods already difficult to maintain

managing periods is already difficult due to the multitude of different manifests that can not works well (e.g. ADS), and on HLS manifests the periods are not yet well managed

to note that already exists a property to allow start playback from start of TSB or his end
which unfortunately is not fully functional for all types of manifests due to the problems mentioned

we have more serious problems to solve in ISA before considering introducing strange things
so i cant accept this

IMO it would be better to have a shortcut key on Kodi to seek to the beginning or end of the stream, regardless of the chapters it would be a cleaner solution

@CastagnaIT CastagnaIT added the PR Cleanup: Not accepted PR closed as it is not accepted, either in approach or content label Mar 6, 2026
oliv3r and others added 4 commits March 6, 2026 09:31
Add 'live_offset' (unsigned integer, seconds) to the manifest_config
JSON blob. When set, GetChapterPos for chapter 1 of a single-period
live stream returns this offset, so the |< button lands past any
pre-program content in the timeshift buffer.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Add a virtual chapter at the end of live streams, enabling Kodi's |<
and >| OSD buttons to navigate: |< seeks to live_offset (or period
start), >| seeks to the actual live edge.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
- LiveDelayFromManifestConfig: manifest_config live_delay key sets m_liveDelay
- LiveStreamTotalTimeIsSet: m_totalTime is populated from mediaPresentationDuration

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
@oliv3r oliv3r force-pushed the live_stream_jump_piers branch from 90d0ba4 to 4e492a4 Compare March 6, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR Cleanup: Not accepted PR closed as it is not accepted, either in approach or content

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants