[FragmentedSampleReader] Rework method to find the sample index

[CastagnaIT]

Description

The old method was using two methods sequentially to find the sample index
GetSampleIndexForTimeStamp
GetNearestSyncSampleIndex

this was a method that could sometimes cause problems
the situation example:

target timestamp (TS): 155
list of samples:

• sample 150
• sample 151
• sample 152
• sample 153 (sync point)
• sample 154
• sample 155
• sample 156

GetSampleIndexForTimeStamp will return index 5
(if fails execute Advance to get next segment)
since the above have set index to 5
GetNearestSyncSampleIndex will try to find a sample sync from index 5 and subsequent, and nothing will return
causing to fails and execute Advance to get next segment

It is understandable that there are several points where Advance could be executed and force a download of more segments,
and so causing a slowdown in the seek, as the data from these downloads needs to be read immediately

this PR will rework this, by try finding a sample sync closer to the target ts (any available) in the current segment
and in case fallback to a sample with closer ts (without sync)

Motivation and context

Found while investigating
i always suspected that Advance method used was one of cause to do some delay on seek but i never looked deeply
so this will improve the video seek speed on mp4 streams

fix #1992

How has this been tested?

test a stream using MP4 container, then while seeking, check downloaded segments
but it is still better to debug the underlying code and check when Advance method is called

Screenshots (if appropriate):

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

[github-actions]

Review

Critical - Debug logging left in production code (FragmentedSampleReader.cpp)

Multiple LOG::LogF(LOGWARNING, ...) calls are clearly debug instrumentation left in the code:

• "NO SAMPLETABLE Advance"
• "sample CTS %llu -- ts %llu -- is sync: %i" (called for every sample in a potentially large loop)
• "Sync sample NOT FOUND, fallback to nearest ts"
• "sample_index %u"
• "sample_index END Advance"

The per-sample log inside the loop is a performance regression — it fires for every sample in the table on every seek.

---

Must Fix - preceedingSync parameter is now ignored (SeekSample, line ~821)

The old code passed preceedingSync to GetNearestSyncSampleIndex to control whether to prefer a sync before or after the target ts. The new loop logic always picks the nearest sync regardless of direction, silently breaking callers that pass preceeding=false.

---

Must Fix - No Advance() loop when the current segment has no matching samples (SeekSample)

The old code looped calling Advance() when GetSampleIndexForTimeStamp returned AP4_ERROR_NOT_ENOUGH_DATA, handling the case where the target ts is in a future segment. The new code does nothing if the entire current sample table has no sync sample and all CTSs are > ts — sampleIndex stays at 0, which is silently wrong rather than triggering a segment download.

[github-actions]

Review

Must Fix — Operator precedence / logic error in sync sample selection (FragmentedSampleReader.cpp:894)

if (syncIndex == indexNoValue || syncCts < ts && syncCts < cts)

Two problems:

1. If we already have a sync with syncCts >= ts, the second operand is false and we never update — even if a better sync exists.
2. The comment says "find a sync with higher ts than target" but no lower-bound (cts >= ts) is enforced. A sync far before the target can be selected and never replaced.

Likely intended (last sync at-or-before ts):

if (cts <= ts && (syncIndex == indexNoValue || cts > syncCts))

---

Must Fix — Potential null dereference after Advance() (FragmentedSampleReader.cpp:840-848)

After calling Advance() to populate tracker->m_SampleTable, the code unconditionally dereferences it on line 848 with no null-check. Add a null check after the Advance() call.

---

Minor — Trailing whitespace (FragmentedSampleReader.cpp:872)

return AP4_ERROR_INVALID_STATE; has a trailing space after the semicolon.

---

Minor — LOGWARNING with raw debug string (FragmentedSampleReader.cpp:842)

"NO SAMPLETABLE Advance" looks like a temporary debug message left in at WARNING level. Should be LOGDEBUG or have a more descriptive message.

[github-actions]

Must Fix — Logic error in sync sample update condition.

syncCts < ts && syncCts < cts means once syncCts >= ts (past target), this branch is never entered and a better sync can't replace the current one. Also, there's no guarantee the selected sync is actually near ts — a sync far before the target can be selected and never updated.

If the goal is the latest sync sample at or before ts:

Suggested change

	if (syncIndex == indexNoValue || syncCts < ts && syncCts < cts)
	if (cts <= ts && (syncIndex == indexNoValue || cts > syncCts))

[CastagnaIT]

nope, the logic must be:
find first sample sync index (doesnt matter about ts), but if the found sample sync has a lower ts than target ts, then try to find a sample sync index equal or higher ts than target (if any), that means the first found, and not go to the last one

[CastagnaIT]

@claude i updated the PR code, ignore old bot comments and review

[CastagnaIT]

@claude review

[CastagnaIT]

@kodiai please review

[kodiai]

kodiai response

Decision: NOT APPROVED

Issues:

• (1) [critical] FragmentedSampleReader.cpp (905): Operator precedence and logic error in sync selection. The condition syncCts < ts && cts > syncCts has precedence issue (&& binds tighter than intended comparison) and will fail to update sync index when syncCts >= ts. Should use parentheses and correct logic like cts >= ts && (syncIndex == indexNoValue || cts < syncCts) to find earliest sync at-or-after target.
• (2) [major] FragmentedSampleReader.cpp (905, 909): Current logic can select sync sample before target ts, contradicting the comment stating goal is "equal or higher ts than target". This happens when first sync has syncCts < ts, which passes the condition and never gets replaced.
• (3) [minor] FragmentedSampleReader.cpp (912, 913): Fallback logic updates sampleIndex on every sample where cts <= ts without tracking which is nearest to target. Should compare distance to find actual nearest sample.

[CastagnaIT]

@kodiai for your point 1:
your proposal to change with: cts >= ts && (syncIndex == indexNoValue || cts < syncCts)
will not works when there is only a single sample sync that have a lower cts than the target ts,
i expect that when there is only one single sample sync, even if it has a lower cts, it will be considered. Then if there is another one in the loop with a cts higher or equal than the target ts, then the latter will be considered

[kodiai]

kodiai response

You're correct. The current condition at line 905 does handle your requirement:

if (syncIndex == indexNoValue || syncCts < ts && cts > syncCts)

How it works:

• syncIndex == indexNoValue → Always accepts the first sync sample found (even if cts < ts)
• syncCts < ts && cts > syncCts → Only updates if the current sync is closer to target and previous sync was below target

Your scenario (single sync with cts < ts):

• First iteration: syncIndex == indexNoValue is true → sync gets selected ✓
• Subsequent syncs with cts >= ts: condition syncCts < ts && cts > syncCts evaluates to true && cts > syncCts, so any sync at-or-after target replaces the earlier one ✓

My proposed alternative would break your requirement because cts >= ts as the first condition would reject the single sync when cts < ts.

The current logic is correct for the stated goal. I withdraw point (1) from the review.

[CastagnaIT]

@kodiai review again takin in account last comments

[kodiai]

kodiai response

Decision: APPROVE

Issues: none