refactor: checkBuffer_/fillBuffer_/generateSegmentInfo #1097
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1097 +/- ##
==========================================
+ Coverage 86.40% 86.52% +0.11%
==========================================
Files 39 39
Lines 9631 9637 +6
Branches 2177 2182 +5
==========================================
+ Hits 8322 8338 +16
+ Misses 1309 1299 -10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks like there's a merge conflict.
Most of my comments are around code coverage being missing. We should try and add tests if we can, if it's complicated to test a line, we can discuss it. It's definitely ok not to appease the code coverage just because if it's going to be a lot of work.
src/segment-loader.js
Outdated
|
|
||
| if (buffered.length) { | ||
| lastBufferedEnd = buffered.end(buffered.length - 1); | ||
| getSyncSegmentCandidate_(playlist) { |
There was a problem hiding this comment.
I think it's worth looking and seeing whether we can/should add tests for the two items that codecov says aren't covered by tests. If they can never be hit, maybe just remove those lines.
The case of currentTimeline_ === -1 and the default case.
| // 2. end of stream has been called on the media source already | ||
| // 3. the player is not seeking | ||
| if (next.mediaIndex >= (segments.length - 1) && ended && !this.seeking_()) { | ||
| return null; |
There was a problem hiding this comment.
Another place it's worth adding a test for
| const audioBufferedEnd = lastBufferedEnd(this.sourceUpdater_.audioBuffered()); | ||
|
|
||
| if (typeof audioBufferedEnd === 'number') { |
There was a problem hiding this comment.
why did the logic for this part change?
There was a problem hiding this comment.
ah, I see. lastBufferedEnd already does a null check and so if we don't get a number from it, we don't have audio buffered but if we do we can just use that value directly in the next step.
| segmentInfo.isSyncRequest | ||
| ); | ||
| // stop at the last possible segmentInfo | ||
| if (segmentInfo.mediaIndex + 1 >= segmentInfo.playlist.segments.length) { |
There was a problem hiding this comment.
Looks like codecov is saying both these changes aren't covered by tests? Do we have any tests for this piece?
brandonocasey
force-pushed
the
refactor/choose-next-request
branch
from
6f3ef44 to
5ae7b43
Compare
| const buffered = this.buffered_(); | ||
|
|
||
| if (typeof segmentInfo.timestampOffset === 'number') { | ||
| // The timestamp offset needs to be regenerated, as the buffer most likely |
There was a problem hiding this comment.
generateSegmentInfo, already did all of this.
src/segment-loader.js
Outdated
|
|
||
| // regenerate the audioAppendStart, timestampOffset, etc as they | ||
| // may have changed since this function was added to the queue. | ||
| this.isPendingTimestampOffset_ = true; |
There was a problem hiding this comment.
Was there a specific reason for adding this here?
There was a problem hiding this comment.
to force timestampoffset to regenerate in generateSegmentInfo.
There was a problem hiding this comment.
It feels a bit strange to set the property here and then clear it as a side effect of generateSegmentInfo (we definitely had side effects before, but in the interest of keeping the new function as side effect free as possible). I wonder if it would be better to set isPendingTimestampOffset_ to false in updateTransmuxerAndRequestSegment_ and then to have another option that can be passed into generateSegmentInfo_ to regenerate relevant timing info.
There was a problem hiding this comment.
This was a bit more complicated than I thought, but I generally agreed with your points here. There were two considerations: vtt-segment-loader never wants to set timestamp offsets on generated segment info. I got around that by making a member function that vtt-segment-loader will inherit and override to return null.
Second was that whenever timestampOffset is set using the timestampOffsetForSegment function we expect to set isPendingTimestampOffset_ to false. So we will still have a small side effect no matter what we do, as our code currently expects us to do that. That being said I did add an option to force a timestamp update so that we don't have to set isPendingTimestampOffset_ where the original comment was.
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
src/segment-loader.js
Outdated
|
|
||
| // if timestampoffset was set then we no longer have a timestampoffset | ||
| if (typeof segmentInfo.timestampOffset === 'number') { | ||
| this.isPendingTimestampOffset_ = false; |
There was a problem hiding this comment.
It may be worth moving this out of the function too to make the function have no side effects. I think generateSegmentInfo will only be used on chooseNextRequest (as part of fillBuffer), which has a check for timestampOffset right after, and in loadSegment_ as part of the queue callback, where it was forced to update.
Refactor segment selection logic into (mostly) one place.