add back manual smooth quality changes via smoothQualityChange flag #235

squarebracket · 2018-09-19T06:57:56Z

Description

#113 unfortunately completely removed smooth quality switching for manual quality switches. This PR adds it back via a configuration option.

Specific Changes proposed

Add smoothQualityChange config option
Merge it with a possible source option of the same name
Select fastQualityChange or smoothQualityChange using a ternary that checks the aforementioned config option

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

mjneil · 2018-09-19T17:43:05Z

README.md

+switches, but will cause quality switches to only be visible after a few
+seconds.
+
+Note that this _only_ affects quality changes triggered via the `qualityLevels`


qualityLevels is technically an api added by a separate plugin. It would probably be good to also mention the representations api (you can even add an anchor link to the relevant section of the readme #hlsrepresentations)

Yeah, I was wondering about this myself since it's the representations API from the point of view of VHS, but the qualityLevels API from the point of view of the programmer. I will do as you suggest.

correct, but ONLY if the quality levels plugin is included, which is optional. Manual rendition selection is still possible from the programmer point of view using the representations API. I suggested linking to the readme section since it mentions both the quality levels api (recommended approach) and the representations api

Yep, covers all bases nicely.

mjneil · 2018-09-19T18:00:01Z

src/rendition-mixin.js

- .fastQualityChange_
- .bind(hlsHandler.masterPlaylistController_);
+ // Get a reference to a bound version of the quality change function
+ let qualityChangeFunction = (hlsHandler.options_.smoothQualityChange ?


Personally not a fan of large or multi line ternaries, but feel free to ignore this suggestion as its just my preference.

const { masterPlaylistController_: mpc, options_: { smoothQualityChange } } = hlsHandler; const changeType = smoothQualityChange ? 'smooth' : 'fast'; const qualityChangeFunction = mpc[`${changeType}QualityChange_`].bind(mpc);

Dang, that's much saner. Thanks for the suggestion.

mjneil · 2018-09-19T18:01:00Z

test/configuration.test.js

@@ -28,6 +28,11 @@ const options = [{
 default: 4194304,
 test: 5,
 alt: 555
+}, {
+ name: 'smoothQualityChange',
+ default: undefined,


should the default be false instead of undefined?

Good question. Technically speaking, I don't set the default value to false, I just let it be undefined if it's not defined in the options. So I chose undefined here since that's what the default would be in absence of an explicit value. Do you think I should explicitly set the option to be false if a value isn't given, or just change the default here in the test to be false?

I think it would be good to set it to false if not set like we do with withCredentials here The explicit setting of the false default should be done here as well.

Yup, fair enough, explicit is always good.

mjneil · 2018-09-19T20:11:44Z

src/videojs-http-streaming.js

@@ -384,6 +384,9 @@ class HlsHandler extends Component {
 this.tech_.setCurrentTime(time);
 this.setCurrentTime(time);
 };
+ if (this.source_.smoothQualityChange !== undefined) {
+ this.options_.smoothQualityChange = this.source_.smoothQualityChange;


is this supposed to also be a source level option, or just a player level option?
If source level option is also valid, you should add smoothQualityChange to here instead

Ah yes thanks, in an older version I was setting the quality change function hereish which required the MPC, and thus couldn't go in setOptions. But it can now, great catch.

mjneil · 2018-09-19T20:12:43Z

test/configuration.test.js

@@ -28,6 +28,11 @@ const options = [{
 default: 4194304,
 test: 5,
 alt: 555
+}, {
+ name: 'smoothQualityChange',
+ default: undefined,


I think it would be good to set it to false if not set like we do with withCredentials here The explicit setting of the false default should be done here as well.

squarebracket · 2018-09-19T21:00:04Z

Made the review changes, thanks for the great comments!! let me know if you think README blob is sufficient.

mjneil

lgtm, I haven't been able to manually test this though

squarebracket · 2018-10-17T04:51:19Z

Rebased to master and squashed into a single commit. Unfortunately, it looks like the rebase+squash undid @mjneil's approval. I tested the code post-rebase/squash and it's working as expected.

welcome · 2018-10-22T19:24:52Z

Congrats on merging your first pull request! 🎉🎉🎉

squarebracket force-pushed the smooth-quality-switch-flag branch 3 times, most recently from 63da8cd to f808169 Compare September 19, 2018 15:06

mjneil reviewed Sep 19, 2018

View reviewed changes

mjneil suggested changes Sep 19, 2018

View reviewed changes

squarebracket force-pushed the smooth-quality-switch-flag branch from 391a66c to 2e401a8 Compare September 19, 2018 20:52

mjneil previously approved these changes Sep 19, 2018

View reviewed changes

smoothQualityChange flag

9219955

squarebracket dismissed mjneil’s stale review via 9219955 October 17, 2018 04:21

squarebracket force-pushed the smooth-quality-switch-flag branch from 2e401a8 to 9219955 Compare October 17, 2018 04:21

gesinger approved these changes Oct 22, 2018

View reviewed changes

gesinger added the tested label Oct 22, 2018

gesinger merged commit 0e4fdf9 into videojs:master Oct 22, 2018

forbesjo mentioned this pull request Nov 30, 2018

Add option to clear buffer on manual rendition selection #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add back manual smooth quality changes via smoothQualityChange flag #235

add back manual smooth quality changes via smoothQualityChange flag #235

squarebracket commented Sep 19, 2018

mjneil Sep 19, 2018

squarebracket Sep 19, 2018

mjneil Sep 19, 2018

squarebracket Sep 19, 2018

mjneil Sep 19, 2018

squarebracket Sep 19, 2018

mjneil Sep 19, 2018

squarebracket Sep 19, 2018 •

edited

Loading

mjneil Sep 19, 2018

squarebracket Sep 19, 2018

mjneil Sep 19, 2018 •

edited

Loading

squarebracket Sep 19, 2018

mjneil Sep 19, 2018

squarebracket commented Sep 19, 2018

mjneil left a comment

squarebracket commented Oct 17, 2018 •

edited

Loading

welcome bot commented Oct 22, 2018

add back manual smooth quality changes via smoothQualityChange flag #235

add back manual smooth quality changes via smoothQualityChange flag #235

Conversation

squarebracket commented Sep 19, 2018

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket commented Sep 19, 2018

mjneil left a comment

Choose a reason for hiding this comment

squarebracket commented Oct 17, 2018 • edited Loading

welcome bot commented Oct 22, 2018

squarebracket Sep 19, 2018 •

edited

Loading

mjneil Sep 19, 2018 •

edited

Loading

squarebracket commented Oct 17, 2018 •

edited

Loading