Fix calculation of max buffered time#4109
Conversation
SergioEstevao
added
the
[Type] Bug
|
Version |
SergioEstevao
requested review from
Copilot and
kean
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Updates DefaultPlayer’s buffering calculation so the app can derive a better “max buffered” value from AVPlayer’s loadedTimeRanges, improving how buffering progress is represented during streaming playback.
Changes:
- Adjusts
futureBufferAvailable()to scan allloadedTimeRangesand track the maximum buffered end time beyond the current playhead. - Returns the computed maximum buffered value instead of early-returning on the first qualifying range.
| } | ||
|
|
||
| return 0 | ||
| return maxTime |
There was a problem hiding this comment.
futureBufferAvailable() appears to be used as “seconds of playable buffer remaining” (e.g. MiniPlayer computes buferredAmount = buffered / (duration - currentTime), and other players return duration() - currentTime()). This implementation now returns an absolute media time (range.start + range.duration) rather than the buffer-ahead interval, which will under/over-report buffer and can skew the UI. Consider returning max(0, maxEndTime - upTo) (and optionally clamping to duration() - upTo).
| return maxTime | |
| return max(0, maxTime - upTo) |
| let rangeBuferred = range.timeRangeValue | ||
| if (CMTimeGetSeconds(rangeBuferred.start) + CMTimeGetSeconds(rangeBuferred.duration)) > upTo { | ||
| return CMTimeGetSeconds(rangeBuferred.duration) | ||
| let rangeMaxTime = CMTimeGetSeconds(rangeBuferred.start) + CMTimeGetSeconds(rangeBuferred.duration) |
There was a problem hiding this comment.
Typo in local variable name: rangeBuferred should be rangeBuffered (or similar) for readability and to avoid propagating the misspelling.
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
The change seems straightforward and low-risk.
I got the following message by running an automated review locally – worth addressing?
there's a subtle semantic change that's worth flagging. DefaultPlayer.swift:134-135 now
returns rangeMaxTime (an absolute time within the episode), but the old code returned
rangeBuferred.duration (a length of buffered content). The sole caller at
MiniPlayerViewController.swift:239-242:
let amountBuferred = PlaybackManager.shared.futureBufferAvailable()
if amountBuferred > 0 {
playbackProgressView.buferredAmount = CGFloat(amountBuferred / (duration - currentTime))
}
divides the returned value by remaining time. Neither the old (duration) nor the new (end) value is
really the "amount buffered ahead" that the name implies — the correct expression would be maxTime -
upTo. For typical progressive streaming (one range starting at 0), both old and new produce the same
number, so this PR doesn't regress the UI, but the display can still overshoot once currentTime > 0.
Consider returning maxTime - upTo to align with the function's name and the EffectsPlayer counterpart
(duration() - currentTime()).
| for range in loadedTimeRanges { | ||
| let rangeBuferred = range.timeRangeValue | ||
| if (CMTimeGetSeconds(rangeBuferred.start) + CMTimeGetSeconds(rangeBuferred.duration)) > upTo { | ||
| return CMTimeGetSeconds(rangeBuferred.duration) | ||
| let rangeMaxTime = CMTimeGetSeconds(rangeBuferred.start) + CMTimeGetSeconds(rangeBuferred.duration) |
There was a problem hiding this comment.
(nit) It'd probably be more readable using a functional style with map followed by max() ?? 0
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
| 📘 Part of: # |
|:---:|
Fixes PCIOS-
Updates the calculation of the loaded ranges to take in account the maximum range loaded and not only the first range that is larger than the the current play position
To test
Checklist
CHANGELOG.mdif necessary.