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

Source Handler Options #2616

Closed
heff opened this issue Sep 18, 2015 · 17 comments
Closed

Source Handler Options #2616

heff opened this issue Sep 18, 2015 · 17 comments

Comments

@heff
Copy link
Member

heff commented Sep 18, 2015

We need a way to pass options to the source handlers from the player options. The API will most likely look like...

videojs(id, { html5: { hls: { /* options to be passed to the Html5-HLS source handlers */ } } });
videojs(id, { html5: { flash: { /* options to be passed to the Flash-HLS source handlers */ } } });
@heff heff added this to the v5.0.0 milestone Sep 18, 2015
@heff heff modified the milestones: v5.1.0, v5.0.0 Sep 28, 2015
@richardbushell
Copy link
Contributor

Look forward to the Source Handler options. This should address our issue:
videojs/videojs-contrib-dash#24
Please schedule asap, thanks.

@richardbushell
Copy link
Contributor

Any chance we can get #2616 and #2617 into 5.3 please?
We want to have exposure to methods in the Tech (in our case DASH), as referenced by: videojs/videojs-contrib-dash#24
This milestone keeps getting pushed back, please try and prioritise if possible.

@gkatsev
Copy link
Member

gkatsev commented Nov 25, 2015

@richardbushell we haven't forgotten about this. Unfortunately, we've had other things come up. Also, instead of letting a PR sit around, I decided we should just merge and release it.
Most of the releases that have happened since 5.0 are due to bug fixes and also PRs from the community.
After the holidays, we'll be re-prioritizing work and this will be one of the top ones on the list to talk about. Thanks for your patience.

@richardbushell
Copy link
Contributor

Thanks, appreciate it.

@nickygerritsen
Copy link
Contributor

I do like @heff's idea (although I think his example is wrong; the second line should be ...flash: { hls:..., right?).

However, we then need a way to know which options-object to pass to which source handler. What about the following:

  • We either modify _Tech.registerSourceHandler to also have a name or we modify the source handler interface to require a getName() function. I think for me the first one would make more sense, as we also have registerComponent(name, ...) and registerTech(name, ...), so then we also get registerSourceHandler(name, ...)
  • We either store the handlers in an object indexed by name (thus automatically making sure the names are unique) and add a sourceHandlerOrder similar to the techOrder, but we update this dynamically when calling registerSourceHandler OR we keep storing it in an array but have them be objects with the name and actual source handler.
  • I think it will be better if we have the source handler options in a separate object in the config object. i.e. videojs(id, { html5: { sourcehandlers: { hls: { /* options to be passed to the Html5-HLS source handlers */ } } } } );. Because we can only store the sourcehandlers object in the tech's constructor as we will need it later.
  • We modify the handleSource API to accept a third argument with the options for the specific source handler. Optionally we can also modify the canPlayType and canHandleSource methods of the source handler in a similar way.

What do you guys think about this idea?

@richardbushell
Copy link
Contributor

Hi Nicky
With regard to your original question in #3129, as you are using videojs-contrib-dash you can do this in the videojs player callback function using:
this.tech_.sourceHandler_.mediaPlayer_.setLimitBitrateByPortal(...);
Hope that helps...

@nickygerritsen
Copy link
Contributor

@richardbushell I know we could do that, but we load sources dynamically in the player so doing it there is not that easy. We could do it someplace else, but as a nice OS citizen I think this issue is a nice way to solve it and provide something anybody can use :)

@nickygerritsen
Copy link
Contributor

Any opinions on this? So I could start implementing it :)

@gkatsev
Copy link
Member

gkatsev commented Mar 4, 2016

Yeah, I believe the second line should've been flash: {hls. One thing to keep in mind that any changes have to be more or less backwards compatible.

  • sounds good as long as we have an intelligent default for source handlers that don't currently do that yet, with a deprecation warning.
  • Having a sourceHandlerOrder is interesting. For HLS, we've often wondered how you could register its html and flash source handlers but make sure that only the flash handler is running without disabling the html5 tech altogether; though, I guess we could just pass options to the source handler directly instead of making a sourceHandlerOrder? At the same time, having more options >.<,
  • having it be under a sourceHandlers property sounds good to me
  • having handleSource accept an options object seems ok to me. However, that means that we can only modify source handler options whenever the source changes, right? I think we need to come up with a solution for being able to change source handler options whenever we want (this can be done separately, probably).

Thoughts, @videojs/core-committers?

@nickygerritsen
Copy link
Contributor

  • sounds good as long as we have an intelligent default for source handlers that don't currently do that yet, with a deprecation warning.

Maybe generating a dynamic name by default + deprecation warning?

  • Having a sourceHandlerOrder is interesting. For HLS, we've often wondered how you could register its html and flash source handlers but make sure that only the flash handler is running without disabling the html5 tech altogether; though, I guess we could just pass options to the source handler directly instead of making a sourceHandlerOrder? At the same time, having more options >.<,

Hmmm. The tech handler for html5 and flash HLS now have the same name, but I guess we should then change that to be html5-hls and flash-hls.
And if we make it so that sourceHandlerOrder on the player is dynamically updated if and only if none is provided in the options, we do not need a default option?

  • having handleSource accept an options object seems ok to me. However, that means that we can only modify source handler options whenever the source changes, right? I think we need to come up with a solution for being able to change source handler options whenever we want (this can be done separately, probably).

Yeah I guess we should do this separately, probably by something like player.sourceHandlerOption(sourceHandlerName, key[, value]) or similar?

@imbcmdth
Copy link
Member

imbcmdth commented Mar 4, 2016

@gkatsev I don't @nickygerritsen proposal for a sourceHandlerOrder is meant to enable any more flexibility than already exists. Right now we have implicit ordering with an array and a second parameter on registerSourceHandler that SourceHandlers can use to (somewhat) control ordering when they register themselves. Instead it sounds like sourceHanderOrder was a way to get the extant array-ordering in a plain ol' object.

This may not be as elegant but instead of extending SourceHandler registration to accept names, can't we just pass the entire options object for the tech (the entire object in the html5 property) to the SourceHandler via handleSource and let the SourceHandler's handle options themselves?

If you want to enforce segregation of tech-level options from SourceHandler options, you could have a sourceHandlers property on the tech-options and only pass that object along to the handlers.

The reason I make this suggestion is that SourceHandlers are intended of being the extension mechanism for techs. My opinion is that instead of building more book-keeping into video.js we should consider ways to let SourceHandlers do most of the decision making.

@nickygerritsen
Copy link
Contributor

@imbcmdth I guess your idea would work yes. If I understand correctly you want to add sourceHandlerOrder and pass all tech-options to source handlers? I could definitely agree with that.

@imbcmdth
Copy link
Member

imbcmdth commented Mar 4, 2016

@nickygerritsen Yes, that is exactly what I am proposing: keeping the SourceHandler <-> Tech interface as simple as possible.

@nickygerritsen
Copy link
Contributor

OK let's wait what @dmlap thinks and then I'll start on this.

@dmlap
Copy link
Member

dmlap commented Mar 4, 2016

I like the idea of cutting down on the bookkeeping-- I'm in favor of @imbcmdth's suggestion. Thanks for offering to take a crack at it, @nickygerritsen!

@nickygerritsen
Copy link
Contributor

Btw for now should I just use this.options_ to pass everything? Because this is going away in the future

@gkatsev
Copy link
Member

gkatsev commented Jul 25, 2016

I think we have some version of source handler options. Going to close this one.

@gkatsev gkatsev closed this as completed Jul 25, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants