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

VHS-NEXT RFC #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

VHS-NEXT RFC #3

wants to merge 5 commits into from

Conversation

dzianis-dashkevich
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich commented Aug 16, 2024

Use the following URL to preview the md file with the RFC proposal:
https://github.com/videojs/rfcs/blob/vhs-next-rfc/proposals/0003-vhs-next.md

@dzianis-dashkevich dzianis-dashkevich marked this pull request as ready for review August 28, 2024 02:40
@heff
Copy link
Member

heff commented Sep 24, 2024

Hi, great writeup. 👍 Very exhaustive on the feature set. I have a few questions/comments that hopefully are helpful.

Where does something like a "youtube tech" fit into this equation? I know of VHS as a tech/source handler, but this rewrite has it owning the "playback" namespace, which in current video.js land is more than just hls/dash.
For example in the past we've talked about an idea of "video.js core" which would be video.js with the same top-level API but minus the UI. I can't tell if VHS-NEXT is meant to be Video.js Core and replace all handling of "techs", or just to replace the adaptive engine or "source handler" and be an alternative to HLS.js. My knowledge of the current architecture may be out of date.
I think this would either benefit from examples of how you'd see future users installing and configuring the basic controls-included player like they do today. Or otherwise move the topic of the repo structure and videojs/ui somewhere else and focus this on being a stand-alone library, wherever it lives.

Related, the term "player" is pretty overloaded and could lead to some confusion in this. I'd suggest finding a more specific word where you can. Like "player engine" if we're talking about an HLS.js shaped tool.

Otherwise, for the API design, if this is a major change, I think it'd be great to switch to javascript setters and getters for the API. e.g. getCurrentTime -> currentTime, and match the video element API wherever possible. The only reason Video.js doesn't is because those weren't supported in 2010.

Happy to jump on call if you want to talk through any of this IRL.

@dzianis-dashkevich
Copy link
Contributor Author

dzianis-dashkevich commented Sep 24, 2024

Hi @heff
Thank you for your feedback!

Where does something like a "youtube tech" fit into this equation? I know of VHS as a tech/source handler, but this rewrite has it owning the "playback" namespace, which in current video.js land is more than just hls/dash.

After briefly reviewing the YouTube tech source code, I noticed it does not own playback and is just an adapter for the YT Player iframe embed.
So, there is no fit for something like a "youtube tech" in @videojs/playback. This namespace is reserved for playback-only related entities, including standalone player without UI (similar to shaka player, without UI module).

We can think about such integrations (such as "youtube tech") in the next major videojs RFC.

For example in the past we've talked about an idea of "video.js core" which would be video.js with the same top-level API but minus the UI. I can't tell if VHS-NEXT is meant to be Video.js Core and replace all handling of "techs", or just to replace the adaptive engine or "source handler" and be an alternative to HLS.js. My knowledge of the current architecture may be out of date.

Answering this question, I think it is more inclined to this space:

or just to replace the adaptive engine or "source handler" and be an alternative to HLS.js

Ultimately (with @videojs/playback project), we want to provide a fast, modern, and highly customizable standalone playback-only player as an alternative to hls.js/dash.js/shaka, while the next major videojs version will use @videojs/playback and @videojs/ui under the hood.

I think this would either benefit from examples of how you'd see future users installing and configuring the basic controls-included player like they do today. Or otherwise move the topic of the repo structure and videojs/ui somewhere else and focus this on being a stand-alone library, wherever it lives.

Yeah, good call, I will remove @videojs/ui from the monorepo example since this RFC targets only playback-related repos (e.g.: hls-parser, dash-parser, playback itself, and any other standalone playback-related tools)

Related, the term "player" is pretty overloaded and could lead to some confusion in this. I'd suggest finding a more specific word where you can. Like "player engine" if we're talking about an HLS.js shaped tool.

That is a good point.
It is just a pretty common and recognizable pattern to provide API Facade in the Player namespace, but I agree that this might be confusing as well. We can discuss possible names in the collaborators chat (or any wider audience)
https://github.com/shaka-project/shaka-player/blob/main/lib/player.js
https://github.com/Dash-Industry-Forum/dash.js/blob/development/index.js#L48
https://cdn.bitmovin.com/player/web/8/docs/index.html

Otherwise, for the API design, if this is a major change, I think it'd be great to switch to javascript setters and getters for the API. e.g. getCurrentTime -> currentTime, and match the video element API wherever possible. The only reason Video.js doesn't is because those weren't supported in 2010.

Yeah, the API outlined in this doc is a very high-level example and does not reflect the final decision. Moreover, I've already altered several API aspects in the source code. However, I want to note that we do not strive to mimic video element API.

Regarding getters and setters. I don't have strong preferences here, but I would generally agree. However, here is a list of DX-related points where I would prefer methods over getters and setters:

  • Much easier to check API getters/setters in the console:
    image
  • You don't have to wrap getters and setters in functional wrappers when using functional libraries such as ramda (https://ramdajs.com/)
  • you can't enforce getters (probably can be marked as readonly) and setters in a typescript interface. It is a typical pattern to use DI with Inversion of control via typescript interfaces, so it is much easier to emphasize that the property should be computed with get* set* methods.
  • not all typescript mocking libraries work fine with getters and setters (because of TS interface limitations)

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.

2 participants