Conversation
|
marking as draft, just to see the pipeline work, as i've also added tests. |
Kodiai Review SummaryWhat ChangedThis PR adds support for jumping to live edge and start position in live streams by introducing a virtual second chapter, along with a new Reviewed: core logic, tests Strengths
ObservationsImpact[MAJOR] src/Session.cpp (1273): Unit mismatch in live offset conversion [MAJOR] src/Session.cpp (1330): Potential seek to position zero when no streams enabled [MEDIUM] src/Session.cpp (1261): Missing validation for m_totalTime before division Preference[MINOR] src/Session.cpp (1271-1273): Optional: Simplify conditional by extracting common checks Suggestions
Verdict🔴 Address before merging -- 2 blocking issues found Review Details
|
ReviewMust 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. |
f783e84 to
9e3b7a6
Compare
9e3b7a6 to
b440c66
Compare
ReviewMedium Code duplication ( Low Missing unit in Low |
b440c66 to
90d0ba4
Compare
|
now that i have had some time to read and understand it better 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 we have more serious problems to solve in ISA before considering introducing strange things 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 |
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>
90d0ba4 to
4e492a4
Compare
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
Checklist: