Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: null check return value of selectPlaylist #779

Merged
merged 10 commits into from
Mar 24, 2020
Merged

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 23, 2020

No description provided.

src/master-playlist-controller.js Outdated Show resolved Hide resolved
// ensure we have some buffer before we switch up to prevent us running out of
// buffer while loading a higher rendition.
forwardBuffer >= bufferLowWaterLine) {
if (nextPlaylist &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth having a message being logged somewhere (maybe even as a warning) in this case. Will make debugging easier if we ever run into it.

This implemention mirrors the original if statement. This is becasuse if
it were to invert it, it would actually end the chain too early. Thus,
we return true
This is because if were to invert it, we actually would end too early in
the checks chain.
@gkatsev
Copy link
Member Author

gkatsev commented Mar 23, 2020

note 70ad314

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but might be good to have a test for the null nextPlaylist

this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
assert.strictEqual(calls, 3, 'selects after additional segments');
// verify stats
assert.equal(this.player.tech_.hls.stats.bandwidth, 4194304, 'default bandwidth');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept it because it's literally a copy and paste from the above test but I guess we can remove it.

test/master-playlist-controller.test.js Show resolved Hide resolved
@gkatsev gkatsev merged commit 90a0215 into 1.x Mar 24, 2020
@gkatsev gkatsev deleted the null-check-select-playlist branch March 24, 2020 14:09
gkatsev added a commit that referenced this pull request Mar 24, 2020
gkatsev added a commit that referenced this pull request Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants