[Session] Refactor part of SeekTime method

[CastagnaIT]

Description

this do a partial refactor of SeekTime method by cleanup some related parts

Partially fixed TODO https://github.com/xbmc/inputstream.adaptive/blob/194e2ddc9c7e67751c2ffcd65e3afb476792aece/src/common/AdaptiveStream.cpp#L1209-L1219 this can still occurs when you do a video seek across chapters/periods i have add a new TODO in more appropriate place about this, at the time of writing i dont think to include the fix particular in this PR

This fix also a occasional video seek bug across chapters/periods where the video freeze and cannot be unblocked: this is caused due to a specific situation where: oldSeg is empty, preceeding is true, and the code fall into "else" condition that set needReset to false https://github.com/xbmc/inputstream.adaptive/blob/194e2ddc9c7e67751c2ffcd65e3afb476792aece/src/common/AdaptiveStream.cpp#L1199-L1226 so due to the "partially fixed" bug above mentioned, the segments begin to be downloaded uncontrollably from the beginning of the period, and since ResetCurrentSegment is not called, the current segment is not changed and buffer never force stopped.

Implemented reuse of segments already downloaded to the buffer when performing a video seek! This greatly improves video seeking because it eliminates the need to re-download data that has already been downloaded This, of course, depends entirely on the size of the ISA's internal buffer

Motivation and context

remove some todos

How has this been tested?

Screenshots (if appropriate):

Types of change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] Clean up (non-breaking change which removes non-working, unmaintained functionality)
• [x] 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

[github-actions[bot]]

Critical Issues Found

Severity: Critical

Operator precedence may cause logic error (src/common/AdaptiveStream.cpp:1199) The condition oldSeg.has_value() && \!seekSeg->IsSame(*oldSeg) || \!preceeding evaluates as (oldSeg.has_value() && \!seekSeg->IsSame(*oldSeg)) || \!preceeding. While operator precedence is technically correct, this means ResetCurrentSegment(*seekSeg) is called whenever \!preceeding is true, regardless of whether segments differ. This changes behavior from the original code which had an else if (\!preceeding) branch that also adjusted absolute_position_ and segment_read_pos_. Verify this is intentional and that removing the position adjustments doesn't break subtitle seeking.

Severity: Medium

Missing braces on single-statement if (Session.cpp:993-994, 1039-1040) Single-statement if blocks lack braces. Kodi coding guidelines typically require braces for all control structures to prevent maintenance errors.

[CastagnaIT]

@claude despite the PR its in WIP state, can you do a preliminary review?

[claude[bot]]

Claude finished @CastagnaIT's task in 1m 50s —— View job

---

Preliminary PR Review

Thank you for the opportunity to review this WIP refactoring. I've analyzed the changes and have identified several issues that should be addressed before finalizing.

---

Critical Issues

1. Operator Precedence Bug in AdaptiveStream.cpp:1199 ⚠️

Location: src/common/AdaptiveStream.cpp:1199

The condition has a critical operator precedence issue:

if (oldSeg.has_value() && !seekSeg->IsSame(*oldSeg) || !preceeding)

Due to C++ operator precedence (&& has higher precedence than ||), this evaluates as:

if ((oldSeg.has_value() && !seekSeg->IsSame(*oldSeg)) || !preceeding)

Problem: This changes the logic from the original code:

• Original: Used else if (!preceeding) which would only execute when the first condition was false
• New: The !preceeding condition is now independent, meaning ResetCurrentSegment(*seekSeg) will be called whenever preceeding is false, regardless of whether segments differ

Additionally: The original else if block included position adjustments (absolute_position_ -= segment_read_pos_; segment_read_pos_ = 0;) which are now completely removed.

Impact: This could break subtitle seeking behavior and segment position tracking, as noted in the original comment about restarting streams that have finished (EOS state).

Recommendation: Either:

1. Add explicit parentheses to clarify intent: if ((oldSeg.has_value() && !seekSeg->IsSame(*oldSeg)) || !preceeding)
2. Or restructure to match original logic more closely if the position adjustments are still needed

Fix this →

---

Medium Issues

2. Missing Braces on Single-Statement Conditionals

Locations:

• src/Session.cpp:993-994
• src/Session.cpp:1039-1040

// Line 993-994
if (reset)
  timingReader->Reset(false);

// Line 1039-1040  
if (reset)
  streamReader->Reset(false);

Problem: Kodi coding guidelines typically require braces for all control structures to prevent maintenance errors.

Recommendation: Add braces:

if (reset)
{
  timingReader->Reset(false);
}

Fix this →

---

Positive Changes

✅ Removed StartReader method - Good cleanup, the logic is now inline which improves readability
✅ Improved SeekTime structure - The refactored flow with timing stream handling first is clearer
✅ Better variable naming - hasAdStream is more descriptive than noAdStream
✅ Simplified log message - Removed redundant physical index from log output
✅ Removed TODOs - Addressed technical debt as intended

---

Questions & Observations

1. Intentional behavior change? - Was the removal of position adjustments (absolute_position_ -= segment_read_pos_; segment_read_pos_ = 0;) in the !preceeding case intentional? The original code had special handling for subtitle streams in EOS state.

2. Inverted logic - Line 1020 now uses if (!stream->IsEnabled()) with early continue, which is cleaner than the previous nested approach.

3. Testing coverage - Given the complexity of seeking logic and the potential impact on subtitle streams, have you tested:

   • Subtitle seeking with single-segment periods?
   • Cross-period seeks?
   • Both preceeding=true and preceeding=false cases?

---

Summary

The refactoring successfully simplifies the code structure and removes the StartReader helper method as intended. However, the critical operator precedence issue in AdaptiveStream.cpp:1199 must be resolved before merging, as it could cause seeking bugs. The missing braces should also be added to comply with coding standards.

Once these issues are addressed, this will be a solid cleanup that improves code maintainability.

---

[CastagnaIT]

@claude i updated PR, please review again

[claude[bot]]

Claude finished @CastagnaIT's task in 1m 32s —— View job

---

Updated PR Review

• [x] Read changed files
• [x] Compare changes against base branch
• [x] Check if previous issues were addressed
• [x] Analyze code for new issues
• [x] Post comprehensive review

---

Previous Issues Status

✅ Fixed: Operator Precedence Issue (AdaptiveStream.cpp:1199)

The critical operator precedence issue has been addressed. The code now uses explicit parentheses:

if ((oldSeg.has_value() && !seekSeg->IsSame(*oldSeg)) || !preceeding)

However, this reveals a new semantic issue (see Critical Issues below).

⚠️ Partially Fixed: Missing Braces (Session.cpp)

The braces issue at Session.cpp:992-993 still remains:

if (reset)
  stream.GetReader()->Reset(false);

This is now inside the SeekAdStream lambda at src/Session.cpp:992-993 and should have braces per Kodi coding standards.

---

Critical Issues

1. Logic Change in AdaptiveStream.cpp:1199 ⚠️

Location: src/common/AdaptiveStream.cpp:1199-1204

The refactored code has changed the logic significantly:

New code:

if ((oldSeg.has_value() && !seekSeg->IsSame(*oldSeg)) || !preceeding)
{
  ResetCurrentSegment(*seekSeg);
}
else
  needReset = false;

Original code had:

if (oldSeg.has_value() && !seekSeg->IsSame(*oldSeg))
{
  ResetCurrentSegment(*seekSeg);
}
else if (!preceeding)
{
  // restart stream if it has 'finished', e.g subtitles
  ResetCurrentSegment(*seekSeg);
  absolute_position_ -= segment_read_pos_;
  segment_read_pos_ = 0;
}

Critical Issue: The new code calls ResetCurrentSegment(*seekSeg) whenever !preceeding is true, BUT it completely removes the position adjustments (absolute_position_ -= segment_read_pos_; segment_read_pos_ = 0;) that were in the original else if branch.

Impact:

• The original code had two different reset paths: one for segment changes, and one for subtitle stream restarts (with position adjustments)
• The new code merges these paths but loses the position adjustments
• This could break subtitle seeking, particularly for streams in EOS state with single-segment periods

Questions:

1. Is the removal of absolute_position_ -= segment_read_pos_; segment_read_pos_ = 0; intentional?
2. Was this code path tested with subtitle streams that have finished (EOS state)?
3. The removed TODO mentioned "twice downloaded segments" - does this refactor address that issue, or is the position adjustment still needed?

Recommendation: Either restore the position adjustments in the !preceeding case, or add a comment explaining why they're no longer needed (with test evidence).

Review this change →

---

Medium Issues

2. Missing Braces in Lambda (Session.cpp:992-993)

Location: src/Session.cpp:992-993

The SeekAdStream lambda still has a single-statement if without braces:

if (reset)
  stream.GetReader()->Reset(false);

Recommendation: Add braces per Kodi coding standards:

if (reset)
{
  stream.GetReader()->Reset(false);
}

Fix this →

---

Positive Changes

The refactoring has several improvements:

✅ Removed StartReader method - Successfully eliminated the helper method and inlined the logic with better clarity

✅ Two clear lambda helpers - CheckReaderRunning and SeekAdStream improve code organization and readability

✅ Improved timing stream handling - The timing stream is now explicitly handled first with clear separation from other streams

✅ Better variable naming - hasAdStream is clearer than noAdStream (inverted logic removed)

✅ Clearer control flow - Using early continue statements (lines 1036-1037, 1040-1041) improves readability

✅ Removed redundant physical index - Log message no longer includes physical index, simplifying output

✅ Removed large TODO - Successfully addressed the TODO about twice-downloaded segments by refactoring the seek logic

✅ Better documentation - Added clear doxygen comment for TimeSeek method in SampleReader.h:85-90

---

Minor Observations

1. Line 1052 comment - The inline comment "Do not 'seek time' on 'timing stream' already done above" is helpful but could be on a separate line for better readability

2. Consistent error handling - The lambda approach ensures consistent error handling across all streams

3. Variable scope - The reset variable at line 1044 is properly scoped within the loop

---

Testing Recommendations

Given the complexity of the changes, particularly in AdaptiveStream.cpp, please ensure testing covers:

1. Subtitle streams:

   • Single-segment subtitle periods
   • Subtitle streams that reach EOS state
   • Seeking backward in subtitles (preceeding=true)
   • Seeking forward in subtitles (preceeding=false)

2. Cross-period seeks:

   • Seeking across chapter boundaries
   • Timing stream synchronization with other streams

3. Edge cases:

   • FMP4 audio streams included in video streams (hasAdStream path)
   • Streams that aren't started yet (tests the CheckReaderRunning lambda)

---

Summary

The refactoring successfully achieves the goal of cleaning up the SeekTime method and removing the StartReader helper. The code is more readable and maintainable. However, there's a critical semantic change in AdaptiveStream.cpp:1199 where position adjustments for subtitle stream restarts have been removed. This needs verification that it doesn't break subtitle seeking functionality. The missing braces issue should also be addressed for coding standard compliance.

Once the position adjustment question is resolved (with either restoration of the code or confirmation it's not needed), this will be a solid improvement to the codebase.

---

[CastagnaIT]

@claude i addressed your notified critical issue: 1. Logic Change in AdaptiveStream.cpp:1199 and i made new changes, please review again

[claude[bot]]

Claude finished @CastagnaIT's task in 2m 14s —— View job

---

Re-review of PR Changes

Thank you for addressing the feedback! I've reviewed the latest changes and I'm pleased to report significant improvements.

Progress:

• [x] Check for repository guidelines (CLAUDE.md)
• [x] Get diff of changes since last review
• [x] Read modified files
• [x] Verify previous critical issue was addressed
• [x] Check for new issues
• [x] Post comprehensive review feedback

---

Previous Issues - Status Update

✅ RESOLVED: Logic Change in AdaptiveStream.cpp

Excellent solution! The refactoring completely addresses my previous concern about the missing position adjustments. The new approach is much cleaner and more robust:

Key Changes:

1. New method UpdateCurrentSegment (AdaptiveStream.cpp:1157-1179) - This encapsulates the logic for updating the current segment and returns whether the segment actually changed.

2. New method AlignActiveBufferToSegment (AdaptiveStream.cpp:274-309) - This is the critical addition that:

   • Checks if the target segment is already in the buffer (ContainsSegment)
   • If found, removes segments before the target (reuses downloaded data!)
   • If not found, resets the buffer completely
   • Resets position tracking (absolute_position_ = 0; segment_read_pos_ = 0;) in both paths
   • Returns whether the buffer is non-empty after alignment

3. Simplified seek_time (AdaptiveStream.cpp:1215-1237) - Now very clean:

   if (!UpdateCurrentSegment(*seekSeg))
     needReset = false;
   

Why this is better:

• The position reset logic is now handled in AlignActiveBufferToSegment which is called by UpdateCurrentSegment
• This completely eliminates the confusing !preceeding special case
• The segment reuse feature (from the PR description) is properly implemented
• The logic is much easier to understand and maintain

✅ RESOLVED: Missing Braces

The braces have been properly added at Session.cpp:1000-1001:

if (reset)
  stream.GetReader()->Reset(false);

Wait - I need to check this more carefully...

Let me re-read that section:

Actually, looking at Session.cpp:1000-1001, I see:

if (reset)
  stream.GetReader()->Reset(false);

The braces are still missing here. This should be:

if (reset)
{
  stream.GetReader()->Reset(false);
}

---

Remaining Issues

Minor: Missing Braces (Session.cpp:1000-1001)

Location: src/Session.cpp:1000-1001 (inside the SeekAdStream lambda)

The single-statement if block still lacks braces:

if (reset)
  stream.GetReader()->Reset(false);

Recommendation: Add braces per Kodi coding standards:

if (reset)
{
  stream.GetReader()->Reset(false);
}

This is a minor style issue and doesn't affect functionality.

Fix this →

---

Debug Logging

I noticed some debug logging statements that should probably be removed before merging:

AdaptiveStream.cpp:1163, 1168:

LOG::LogF(LOGERROR, "[AS-%u] HAVE SEGMENT!", clsId);
LOG::LogF(LOGERROR, "[AS-%u] DONT HAVE SEGMENT!", clsId);

These look like debug statements with inappropriate log level (LOGERROR). Consider:

1. Removing them entirely (if they were just for debugging)
2. Changing to LOGDEBUG if the information is useful for troubleshooting
3. Making the messages more descriptive if keeping them

Fix this →

---

Positive Changes - Comprehensive List

This refactoring represents a significant architectural improvement:

Core Architecture Improvements

✅ New UpdateCurrentSegment method - Excellent encapsulation of segment update logic with clear return value indicating if segment changed

✅ New AlignActiveBufferToSegment method - This is a game-changer:

• Implements segment reuse (as mentioned in PR description)
• Properly handles position reset in both code paths
• Eliminates the need for special !preceeding handling
• Thread-safe with proper mutex locking
• Clear separation between "pause downloads" (segment found) and "stop downloads" (segment not found)

✅ New ContainsSegment method (SegmentBuffer.cpp:204-213) - Clean helper for checking if segment exists in buffer

✅ Simplified seek_time method - Now extremely clean and easy to understand (only 22 lines!)

Session.cpp Refactoring

✅ Two clear lambda helpers - CheckReaderRunning and SeekAdStream improve code organization significantly

✅ Removed StartReader method - Successfully eliminated from header (Session.h) and inlined with better clarity

✅ Better timing stream handling - Explicit handling first with clear separation from other streams (Session.cpp:1012-1039)

✅ Improved variable naming - hasAdStream is much clearer than the old noAdStream (inverted logic removed)

✅ Cleaner control flow - Early continue statements improve readability

Documentation Improvements

✅ Excellent doxygen comments added:

• AdaptiveStream.h:80-85 for UpdateCurrentSegment
• AdaptiveStream.h:91-100 for ensureSegment
• SampleReader.h:85-90 for TimeSeek

✅ Helpful inline comments - e.g., Session.cpp:1060 "Do not 'seek time' on 'timing stream' already done above"

Bug Fixes

✅ Fixed the seek freeze bug - The new architecture properly handles the case where oldSeg is empty and preceeding is true by always resetting positions in AlignActiveBufferToSegment

✅ Removed TODO - Successfully addressed the "twice downloaded segments" issue with the segment reuse implementation

---

Implementation Quality

The refactoring demonstrates excellent software engineering:

1. Separation of Concerns: AlignActiveBufferToSegment clearly separates buffer management from seek logic
2. Single Responsibility: Each method has a clear, single purpose
3. Return Value Design: UpdateCurrentSegment returns whether segment changed - perfect for controlling needReset
4. Thread Safety: Proper mutex usage in buffer operations
5. Performance Optimization: Segment reuse avoids unnecessary re-downloads

---

Summary

This is a high-quality refactoring that significantly improves code maintainability and fixes real bugs. The new architecture with UpdateCurrentSegment and AlignActiveBufferToSegment is much cleaner than the previous approach and properly handles all edge cases.

Remaining work before merge:

1. Add braces to the if statement at Session.cpp:1000-1001 (minor style issue)
2. Remove or improve the debug logging at AdaptiveStream.cpp:1163, 1168
3. Consider testing edge cases mentioned in previous review (subtitle streams, cross-period seeks)

The critical logic issue has been completely resolved with an elegant solution. Excellent work! 👏

---

[CastagnaIT]

@claude i updated PR with new changes, please do review

[claude[bot]]

Claude finished @CastagnaIT's task in 2m 25s —— View job

---

Re-review of PR Changes

Progress:

• [x] Check for repository guidelines (CLAUDE.md)
• [x] Read modified files
• [x] Verify previous issues were addressed
• [x] Check for new issues
• [x] Post comprehensive review feedback

---

Previous Issues Status

✅ RESOLVED: Debug Logging (AdaptiveStream.cpp:1163, 1168)

The debug LOGERROR statements ("HAVE SEGMENT!" and "DONT HAVE SEGMENT!") have been completely removed. Excellent!

⚠️ NOT FIXED: Missing Braces (Session.cpp:1000-1001)

The braces issue at Session.cpp:1000-1001 still remains:

if (reset)
  stream.GetReader()->Reset(false);

Location: Inside the SeekAdStream lambda at src/Session.cpp:1000-1001

Recommendation: Add braces per Kodi coding standards:

if (reset)
{
  stream.GetReader()->Reset(false);
}

This is a minor style issue that should be addressed before merging.

Fix this →

---

New Analysis

The latest changes continue to demonstrate excellent refactoring. Here's what I found:

New Architecture - Excellent Improvements

✅ New UpdateCurrentSegment method (AdaptiveStream.cpp:1173-1188)

• Clean encapsulation of segment update logic
• Properly sets current_segment_ based on whether the buffer contains the target segment
• Handles the edge case where the buffer is empty by setting the previous segment or resetting

✅ Refined AlignActiveBufferToSegment method (AdaptiveStream.cpp:274-325)

• Critical fix: Now properly resets position tracking (absolute_position_ = 0; segment_read_pos_ = 0;) at lines 321-322
• This reset happens unconditionally whether the segment is found or not
• This completely addresses the previous critical issue about missing position adjustments!
• The method implements segment reuse by:
  • Checking if the target segment exists in the buffer (ContainsSegment)
  • If found: pausing downloads, removing older segments, keeping the target segment
  • If not found: stopping downloads and resetting the buffer completely
• Thread-safe with proper mutex usage

✅ Much simpler seek_time method (AdaptiveStream.cpp:1224-1248)

• Reduced to just 24 lines (from ~50+ lines in previous versions)
• Clean logic: just calls UpdateCurrentSegment and adjusts needReset based on preceeding flag
• No more confusing special cases or conditional position adjustments
• Much easier to understand and maintain

Documentation Quality

✅ Excellent doxygen comments:

• AdaptiveStream.h:80-83 for UpdateCurrentSegment
• AdaptiveStream.h:198-202 for AlignActiveBufferToSegment
• SegmentBuffer.h:179-182 for ContainsSegment

All comments are clear, concise, and accurately describe what each method does.

Code Quality Assessment

Strengths:

1. Separation of Concerns: The refactoring properly separates segment management (UpdateCurrentSegment), buffer alignment (AlignActiveBufferToSegment), and seeking logic (seek_time)
2. Single Responsibility: Each method has a clear, well-defined purpose
3. Thread Safety: Proper mutex usage throughout buffer operations
4. Segment Reuse: The implementation successfully reuses downloaded segments when possible, as described in the PR objectives
5. Position Reset: The critical position adjustment issue is now properly handled in AlignActiveBufferToSegment

Minor Issues:

1. Missing braces: Session.cpp:1000-1001 needs braces (style issue only)
2. Code block scoping: In AlignActiveBufferToSegment at line 301-310, there's a standalone block { std::lock_guard... }. While this works, it might be clearer to add a comment explaining why it's in a separate scope (vs. being part of the else block above)

---

Comparison with Previous Implementation

The new approach is significantly better than the original code:

Before:

• Complex conditional logic with if/else if branches
• Position adjustments scattered in different code paths
• Special handling for !preceeding case
• Difficult to understand when segments are reused vs. reset

After:

• Clean separation: UpdateCurrentSegment orchestrates, AlignActiveBufferToSegment does the work
• Position reset happens consistently in one place
• Segment reuse is explicit and easy to follow
• seek_time is now trivially simple

---

Testing Recommendations

Given the significant refactoring, please ensure testing covers:

1. Segment Reuse:

   • Forward seek within buffered segments (should reuse without re-downloading)
   • Backward seek to buffered segments
   • Seek beyond buffered range (should reset and download)

2. Position Tracking:

   • Verify absolute_position_ and segment_read_pos_ are correctly reset
   • Test with byte-ranged segments
   • Test with different container types (TS, FMP4, etc.)

3. Thread Safety:

   • Concurrent seeks while downloads are in progress
   • Verify no race conditions with buffer access

4. Edge Cases:

   • Subtitle streams (single-segment periods)
   • Cross-period/chapter seeks
   • Both preceeding=true and preceeding=false cases
   • Empty buffers, EOS conditions

---

Summary

This is excellent refactoring work that significantly improves code quality and maintainability. The new architecture with UpdateCurrentSegment and AlignActiveBufferToSegment is much cleaner and more robust than the previous implementation.

Before merging:

• ✅ Critical issue RESOLVED (position tracking is now properly reset)
• ✅ Debug logging REMOVED
• ⚠️ Minor style issue: Add braces at Session.cpp:1000-1001

The only remaining issue is a minor coding style violation (missing braces). Once that's addressed, this PR will be ready for merge. Great work! 👏

---

View job

[github-actions[bot]]

Critical

Undefined behavior: condition_variable::wait() without predicate (AdaptiveStream.cpp:289) cvRW.wait(lckrw) is called without a predicate, causing potential spurious wakeups and missed notifications. The code assumes the wait will be notified when it should proceed, but there's no condition to verify. This can cause indefinite blocking or premature continuation. Fix: Add a predicate or use a flag to check the intended condition before/after wait.

Potential deadlock (AdaptiveStream.cpp:281-300) try_lock() followed by conditional wait on cvRW creates race condition. If download completes between try_lock failure and cvRW.wait(), the notification is lost and wait blocks forever. Fix: Use proper predicate-based wait or ensure notification always follows state change.

Must Fix

Off-by-one error in boundary check (SegmentBuffer.cpp:185) NotifyDownloadCompleted checks m_buffers.size() < m_downloadIndex but should check m_buffers.size() <= m_downloadIndex to match GetNextDownload:173 logic. After incrementing m_downloadIndex (line 191), index can equal size. Fix: Change to m_buffers.size() <= m_downloadIndex for consistency.

Medium

Non-atomic state transitions (AdaptiveStream.cpp:1391-1393) PauseDownloads() sets m_state without notifying cvState. Worker thread at line 334 waits for state==RUNNING, so PAUSED state won't wake it. Inconsistent with StopDownloads pattern. Fix: Add cvState.notify_all() if PAUSED should interrupt worker, or document that PAUSED only affects next iteration.

Lambda captures reference to modified variable (Session.cpp:992) SeekAdStream lambda captures preceeding by reference, but preceeding is modified at Session.cpp:1089. All streams after video will use modified value instead of original. Fix: Capture by value: [preceeding] instead of [&preceeding].

[CastagnaIT]

@claude review again, do a very short report for new possible problems

[claude[bot]]

Claude finished @CastagnaIT's task in 1m 8s —— View job

---

Quick Re-review

• [x] Read latest changed files
• [x] Verify critical issues from automated review
• [x] Provide short report on new problems

---

New Issues Found

Off-by-one boundary check (SegmentBuffer.cpp:184)

Location: src/common/SegmentBuffer.cpp:184

The condition check is inconsistent between methods:

• GetNextDownload (line 173): uses m_buffers.size() <= m_downloadIndex
• NotifyDownloadCompleted (line 184): uses m_buffers.size() < m_downloadIndex

Problem: After incrementing m_downloadIndex at line 190, the index can equal the size. The condition at line 184 should use <= to match GetNextDownload's logic.

Fix:

if (m_buffers.size() <= m_downloadIndex)

Fix this →

---

Deadlock risk in AlignActiveBufferToSegment (AdaptiveStream.cpp:281-298)

Location: src/common/AdaptiveStream.cpp:281-298

Problem: The code uses try_lock() followed by manual lock acquisition, which creates a race condition:

1. Line 281: try_lock() fails (download in progress)
2. Lines 284-295: Code checks if download should be stopped
3. Line 297: Manually acquires the lock with mutexWorker.lock()

Race condition: If the download completes between the failed try_lock() and line 297's lock(), the thread may wait unnecessarily or acquire an inconsistent state.

Note: While CastagnaIT commented "doesn't matter, because in any case the wait method acquires the lock", there is no wait() call in the current code (it was removed). The manual lock() at line 297 can still cause issues if state changes between try_lock failure and lock acquisition.

Recommendation: Consider restructuring to avoid the try_lock/lock pattern, or add comments explaining why this is safe.

---

Issues Already Addressed ✅

• Lambda capture bug (Session.cpp:992): Fixed - preceeding is now passed as a parameter instead of being captured by reference
• Missing braces: Still present at Session.cpp:1000-1001, but this is a minor style issue
• PauseDownloads() notification: CastagnaIT confirmed this is not required (cvState notification not needed for PAUSED state)

---

Summary

Two potential issues remain:

1. Off-by-one check in SegmentBuffer.cpp - should use <= for consistency
2. Try_lock/lock pattern in AlignActiveBufferToSegment - potential race condition (though may be acceptable given the code flow)

The lambda capture issue has been properly addressed by passing preceeding as a parameter.

---

View job

[CastagnaIT]

@claude aboutyour point : Deadlock risk in AlignActiveBufferToSegment (AdaptiveStream.cpp:281-298) provide code changes for possible solutions

[claude[bot]]

Claude finished @CastagnaIT's task in 1m 23s —— View job

---

Code Solutions for Deadlock Risk

I've analyzed the synchronization pattern in AlignActiveBufferToSegment (lines 274-318). Here are the issues and proposed solutions:

Progress:

• [x] Read current implementation
• [x] Analyze synchronization pattern
• [x] Propose solution options with code changes
• [x] Post solutions

---

Problem Analysis

The current code at lines 281-309 has this pattern:

if (!thread_data_->mutexWorker.try_lock())
{
  // No lock, there is a download in progress
  std::unique_lock<std::mutex> lckrw(thread_data_->mutexRW);
  SegmentBuffer* currDownload = m_segBuffers.GetNextDownload();
  
  if (currDownload && currDownload->State() == BufferState::DOWNLOADING)
  {
    const uint64_t downloadStartPTS = currDownload->segment.startPTS_;
    const uint64_t targetStartPTS = segment.startPTS_;
    
    if (downloadStartPTS < targetStartPTS)
      thread_data_->StopDownloads();
  }
  
  thread_data_->mutexWorker.lock();  // <-- Manual lock after try_lock failed
}

Race condition: Between the try_lock() failure (line 281) and the manual lock() (line 297), the worker thread could:

1. Finish its download
2. Release mutexWorker
3. Wait on cvState (line 333) for state change
4. The main thread then acquires mutexWorker at line 297, but the worker is now waiting and won't process the STOPPED state until it's notified

---

Solution Option 1: Use Predicate-Based Wait (Recommended)

Replace the manual lock pattern with a proper predicate-based wait:

bool adaptive::AdaptiveStream::AlignActiveBufferToSegment(const PLAYLIST::CSegment& segment)
{
  if (m_segBuffers.ContainsSegment(segment))
  {
    thread_data_->PauseDownloads();
    
    // Wait for any in-progress download to complete or stop
    {
      std::unique_lock<std::mutex> lckWorker(thread_data_->mutexWorker);
      
      // While locked, check if we need to stop the download
      {
        std::lock_guard<std::mutex> lckrw(thread_data_->mutexRW);
        SegmentBuffer* currDownload = m_segBuffers.GetNextDownload();
        
        if (currDownload && currDownload->State() == BufferState::DOWNLOADING)
        {
          const uint64_t downloadStartPTS = currDownload->segment.startPTS_;
          const uint64_t targetStartPTS = segment.startPTS_;
          
          if (downloadStartPTS < targetStartPTS)
            thread_data_->StopDownloads();
        }
      }
      
      // Now mutexWorker is held, safe to manipulate buffer
      // Delete all segments older than the requested one from the buffer
      while (!m_segBuffers.IsEmpty())
      {
        if (!segment.IsSame(m_segBuffers.Front().segment))
          m_segBuffers.PopFront();
        else
          break;
      }
    } // mutexWorker released here
  }
  else
  {
    thread_data_->StopDownloads();
    {
      std::lock_guard<std::mutex> lckWorker(thread_data_->mutexWorker);
      m_segBuffers.Reset();
    }
  }
  
  // Reset position tracking
  absolute_position_ = 0;
  segment_read_pos_ = 0;
  
  return !m_segBuffers.IsEmpty();
}

Advantages:

• No race condition - mutexWorker is properly acquired with unique_lock
• Clearer ownership of the mutex
• Eliminates the try_lock/lock pattern entirely
• Worker thread will properly wait on cvState if it needs to

---

Solution Option 2: Add Condition Variable Wait

Keep the try_lock pattern but add a proper wait for the worker to finish:

if (!thread_data_->mutexWorker.try_lock())
{
  // No lock, there is a download in progress
  {
    std::lock_guard<std::mutex> lckrw(thread_data_->mutexRW);
    SegmentBuffer* currDownload = m_segBuffers.GetNextDownload();
    
    if (currDownload && currDownload->State() == BufferState::DOWNLOADING)
    {
      const uint64_t downloadStartPTS = currDownload->segment.startPTS_;
      const uint64_t targetStartPTS = segment.startPTS_;
      
      if (downloadStartPTS < targetStartPTS)
      {
        thread_data_->StopDownloads();
        thread_data_->cvState.notify_all(); // Notify worker of state change
      }
    }
  }
  
  // Now safely acquire the lock (worker will be notified)
  std::unique_lock<std::mutex> lckWorker(thread_data_->mutexWorker);
}
else
{
  // We got the lock on first try
  // No action needed, just continue with lock held
}

Advantages:

• Minimal change to existing code
• Adds proper notification when state changes
• Worker thread will wake up from cvState.wait() and see the STOPPED state

Disadvantages:

• Still uses try_lock pattern which is less clear
• Requires careful reasoning about lock order

---

Solution Option 3: Simplify with Direct Lock

Remove the try_lock optimization entirely and just wait for the lock:

bool adaptive::AdaptiveStream::AlignActiveBufferToSegment(const PLAYLIST::CSegment& segment)
{
  if (m_segBuffers.ContainsSegment(segment))
  {
    thread_data_->PauseDownloads();
    
    // Always acquire the lock (may wait for download to finish)
    std::unique_lock<std::mutex> lckWorker(thread_data_->mutexWorker);
    
    // Check if there's a download we should stop
    {
      std::lock_guard<std::mutex> lckrw(thread_data_->mutexRW);
      SegmentBuffer* currDownload = m_segBuffers.GetNextDownload();
      
      if (currDownload && currDownload->State() == BufferState::DOWNLOADING)
      {
        const uint64_t downloadStartPTS = currDownload->segment.startPTS_;
        const uint64_t targetStartPTS = segment.startPTS_;
        
        if (downloadStartPTS < targetStartPTS)
        {
          thread_data_->StopDownloads();
          thread_data_->cvState.notify_all();
        }
      }
    }
    
    // Delete all segments older than the requested one from the buffer
    while (!m_segBuffers.IsEmpty())
    {
      if (!segment.IsSame(m_segBuffers.Front().segment))
        m_segBuffers.PopFront();
      else
        break;
    }
  }
  else
  {
    thread_data_->StopDownloads();
    {
      std::lock_guard<std::mutex> lckWorker(thread_data_->mutexWorker);
      m_segBuffers.Reset();
    }
  }
  
  absolute_position_ = 0;
  segment_read_pos_ = 0;
  
  return !m_segBuffers.IsEmpty();
}

Advantages:

• Simplest and clearest approach
• No race conditions
• Easy to understand and maintain
• Worker will finish its current iteration before we acquire the lock

Disadvantages:

• May block slightly longer if a download is in progress (but this is acceptable for a seek operation)

---

Recommendation

I recommend Solution Option 1 or 3 (both are similar - Option 1 is just more explicit). The try_lock pattern was likely an optimization to avoid blocking, but for a seek operation, blocking briefly while a download completes is acceptable and much safer than the current race condition.

Solution Option 3 is the simplest and clearest - just acquire the lock directly and let it wait if needed. This matches the pattern used in the else branch (lines 313-318) and is the most maintainable.

---

View job

[CastagnaIT]

@claude your solutions are not suitable because you cant take lock to mutexWorker, the code need to check PTS while in download, and mutexWorker is released only when download is completed

i modified code by using unique_lock and owns_lock review the current code change and provide a very short response

[claude[bot]]

Claude finished @CastagnaIT's task in 33s —— View job

---

Quick Review

• [x] Read latest AdaptiveStream.cpp
• [x] Analyze unique_lock/owns_lock usage
• [x] Post brief review

---

Analysis

✅ Solution is correct and safe.

The new approach using unique_lock with std::try_to_lock + owns_lock() check is the right pattern:

1. Line 281: unique_lock with try_to_lock attempts to acquire without blocking
2. Line 282: owns_lock() checks if lock was acquired
3. Lines 285-296: If no lock, safely check PTS with mutexRW (doesn't block download)
4. Line 298: lckWorker.lock() blocks until download completes and lock is available
5. Lines 301-308: Now safely holding mutexWorker, can manipulate buffer

Key advantage: The PTS check (lines 289-296) happens under mutexRW only, so it doesn't block the download thread which holds mutexWorker. The comment at line 286 correctly notes that even if the download finishes between the check and lock(), it doesn't matter - the code will just wait for the next lock release.

No race condition: The lckWorker.lock() at line 298 properly blocks until the worker is done, eliminating the deadlock risk I previously identified.

The pattern is clean and correct. 👍

---

View job

[CastagnaIT]

@claude please do final review

[claude[bot]]

Claude finished @CastagnaIT's task in 2m 25s —— View job

---

Final PR Review

Progress:

• [x] Read all changed files
• [x] Compare changes against base branch
• [x] Verify previous issues were resolved
• [x] Check for new issues
• [x] Post comprehensive final review

---

Summary

This PR represents a high-quality refactoring of the SeekTime method with significant architectural improvements. The changes successfully address multiple bugs, remove technical debt, and implement segment reuse optimization as described in the PR objectives.

---

Previous Issues Status

✅ RESOLVED Issues

1. Operator precedence bug - The confusing logic has been completely refactored with the new UpdateCurrentSegment and AlignBufferToSegment methods, eliminating the problematic conditional entirely.

2. Logic change and missing position adjustments - The position resets (absolute_position_ = 0; segment_read_pos_ = 0;) are now properly handled in UpdateCurrentSegment at lines 1185-1186, addressing the concern about removed position adjustments.

3. Debug logging - The LOGERROR debug statements have been removed.

4. Lambda capture bug - Fixed by passing preceeding as a parameter to SeekAdStream instead of capturing by reference.

5. Deadlock risk - Resolved with the unique_lock/owns_lock pattern confirmed in the previous review.

6. Off-by-one in GetNextDownload - Fixed by changing condition from < to <= at line 173.

---

Remaining Issues

⚠️ Minor: Inconsistent Boundary Checks (SegmentBuffer.cpp:184 vs 173)

Location: src/common/SegmentBuffer.cpp:184

There's an inconsistency between two related methods:

GetNextDownload (line 173): Uses <=

if (m_buffers.size() <= m_downloadIndex)
{
  return nullptr;
}

NotifyDownloadCompleted (line 184): Uses <

if (m_buffers.size() < m_downloadIndex)
{
  LOG::LogF(LOGERROR, "Cannot find the buffer segment at index %zu", m_downloadIndex);
  return;
}

Analysis: After incrementing m_downloadIndex at line 190, the index can equal the size, which is a valid state (no more segments to download). However, if NotifyDownloadCompleted is called when m_downloadIndex == m_buffers.size(), it should not log an error since this is expected.

Recommendation: For consistency and correctness, change line 184 to use <=:

if (m_buffers.size() <= m_downloadIndex)

This would prevent the error log when m_downloadIndex legitimately equals m_buffers.size().

Fix this →

---

⚠️ Minor: Missing Braces (Session.cpp:999-1000)

Location: src/Session.cpp:999-1000

The single-statement if block inside the SeekAdStream lambda still lacks braces:

if (!preceeding)
  stream.GetReader()->Reset(false);

Recommendation: Add braces per Kodi coding standards:

if (!preceeding)
{
  stream.GetReader()->Reset(false);
}

This is a minor style issue that should be addressed for consistency with coding standards.

Fix this →

---

Architectural Improvements ✅

1. Removed StartReader Method

The helper method has been successfully eliminated from Session.h and its logic properly inlined with two clear lambda functions, improving code organization and readability.

2. New UpdateCurrentSegment Method (AdaptiveStream.cpp:1169-1187)

Excellent encapsulation that:

• Calls AlignBufferToSegment to handle buffer management
• Sets current_segment_ based on buffer state
• Properly resets position tracking (absolute_position_ = 0; segment_read_pos_ = 0;)
• Clear single responsibility

3. New AlignBufferToSegment Method (AdaptiveStream.cpp:274-322)

This is the star of the refactoring:

• Implements segment reuse as described in PR objectives (point 3)
• Uses ContainsSegment to check if target segment is already buffered
• If found: pauses downloads, removes older segments, keeps target segment (reuses downloaded data!)
• If not found: stops downloads and resets buffer completely
• Excellent thread safety with unique_lock/owns_lock pattern for checking download progress
• Handles the edge case where a download is in progress and its PTS is older than target
• The try_to_lock/owns_lock pattern safely checks download state without blocking the worker thread

4. Simplified seek_time Method (AdaptiveStream.cpp:1223-1242)

Reduced from ~50 lines with complex conditional logic to just 19 lines:

bool adaptive::AdaptiveStream::seek_time(double seek_seconds)
{
  if (!current_rep_)
    return false;

  if (current_rep_->IsSubtitleFileStream())
    return true;

  std::lock_guard<adaptive::AdaptiveTree::TreeUpdateThread> lckUpdTree(m_tree->GetTreeUpdMutex());

  const uint64_t pts = static_cast<uint64_t>(seek_seconds * current_rep_->GetTimescale());
  const CSegment* seekSeg = current_rep_->Timeline().FindByPTSOrNext(pts);

  if (!seekSeg)
    return false;

  UpdateCurrentSegment(*seekSeg);

  return true;
}

Benefits:

• Crystal clear logic flow
• No more confusing needReset output parameter
• No more special !preceeding handling
• Much easier to understand and maintain

5. Improved SeekTime in Session.cpp (Session.cpp:971-1089)

Two excellent lambda helpers:

CheckReaderRunning (lines 975-989): Ensures reader is started after cross-chapter/period seeks SeekAdStream (lines 992-1003): Performs adaptive stream seek with appropriate reset

Better structure:

• Explicit timing stream handling first (lines 1011-1037)
• Clear separation of concerns
• Early continue statements for better readability
• Removed confusing noAdStream inverted logic (now hasAdStream)

---

Bug Fixes ✅

1. Fixed Seek Freeze Bug Across Chapters/Periods

Original issue: When oldSeg was empty and preceeding was true, the code would fall into an else condition setting needReset to false. Combined with the partially fixed "twice downloaded segments" bug, this caused uncontrolled segment downloads from the beginning of the period without resetting the current segment, resulting in frozen playback.

Solution: The new architecture always properly handles buffer alignment and position resets through UpdateCurrentSegment, eliminating this edge case entirely.

2. Fixed Off-by-One in GetNextDownload (SegmentBuffer.cpp:173)

Changed from < to <= to correctly handle the case where m_downloadIndex equals m_buffers.size(), preventing spurious error logs.

3. Fixed TS HEVC Seek Bug (TSReader.cpp:223)

Changed condition from thisFrameStart == m_startPos to thisFrameStart >= m_startPos, fixing the endless download issue introduced in commit 94f6e40c.

4. Implemented Segment Reuse (PR Objective #3)

The AlignBufferToSegment method now checks if the target segment is already in the buffer and reuses it, eliminating unnecessary re-downloads during video seeks. This is a significant performance improvement.

---

Documentation Quality ✅

Excellent doxygen comments added:

• UpdateCurrentSegment (AdaptiveStream.h:80-84)
• AlignBufferToSegment (AdaptiveStream.h:198-202)
• ContainsSegment (SegmentBuffer.h:179-182)
• TimeSeek (SampleReader.h:85-90)
• New TODO comment explaining chapter/period seek inefficiency (Session.cpp:720-726)

All comments are clear, concise, and accurately describe functionality.

---

Code Quality Assessment

Strengths:

1. ✅ Separation of Concerns - Clear separation between segment management, buffer alignment, and seek logic
2. ✅ Single Responsibility - Each method has a well-defined purpose
3. ✅ Thread Safety - Proper mutex usage throughout with sophisticated unique_lock/owns_lock pattern
4. ✅ Performance - Segment reuse optimization avoids unnecessary downloads
5. ✅ Maintainability - Much clearer code structure that's easier to understand and modify
6. ✅ Error Handling - Proper checks for edge cases (empty buffers, subtitle file streams, etc.)

Minor Issues:

1. ⚠️ Inconsistent boundary check in SegmentBuffer.cpp (low priority)
2. ⚠️ Missing braces in Session.cpp (style issue only)

---

Testing Recommendations

Given the scope of changes, ensure testing covers:

1. Segment Reuse:

   • Forward seek within buffered range (should reuse without re-downloading)
   • Backward seek to buffered segments
   • Seek beyond buffered range (should reset and download)

2. Cross-Period/Chapter Seeks:

   • Verify no uncontrolled downloads from period start
   • Confirm playback doesn't freeze
   • Check timing stream synchronization

3. Edge Cases:

   • Subtitle streams (single-segment periods, EOS state)
   • Both preceeding=true and preceeding=false
   • TS HEVC streams (verify the TSReader fix)
   • FMP4 audio streams included in video streams

4. Thread Safety:

   • Concurrent seeks during downloads
   • Verify no race conditions

---

Final Verdict

This PR is ready for merge with only minor cosmetic issues remaining. The architectural improvements are substantial and the bug fixes address real problems. The two remaining issues (boundary check inconsistency and missing braces) are low priority and don't affect functionality.

Recommendation: The PR can be merged as-is, with the minor issues addressed in a follow-up if desired, or fixed quickly before merge.

Excellent refactoring work! 👏

---

View job