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

docs: add A Walk Through VHS #1253

Merged
merged 3 commits into from
Mar 14, 2022

Conversation

gesinger
Copy link
Contributor

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #1253 (db5c4fb) into main (211cbe8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1253   +/-   ##
=======================================
  Coverage   86.31%   86.31%           
=======================================
  Files          39       39           
  Lines        9807     9807           
  Branches     2279     2279           
=======================================
  Hits         8465     8465           
  Misses       1342     1342           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 211cbe8...db5c4fb. Read the comment docs.

<link href="//vjs.zencdn.net/7.10.2/video-js.min.css" rel="stylesheet">
<script src="//vjs.zencdn.net/7.10.2/video.min.js"></script>

<video id="myPlayer" class="video-js">
Copy link
Member

Choose a reason for hiding this comment

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

should this have data-setup="{}" to show that it'll be created automatically?
Also, should it use <video-js>?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to <video-js>. With <video> you'll likely end up with native playback on Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good calls, thank you. Updated to use <video-js> and data-setup.


Safari (and a few other browsers) will play that video natively, because Safari supports HLS content. However, other browsers don't support native playback of HLS and will fail to play the video.

VHS provides the ability to play HLS (and DASH) content in browsers that don't support native HLS (and DASH) playback.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention MSE requirement here? And that native can be overridden?

<link href="//vjs.zencdn.net/7.10.2/video-js.min.css" rel="stylesheet">
<script src="//vjs.zencdn.net/7.10.2/video.min.js"></script>

<video-js id="myPlayer" class="video-js" data-setup='{}'>
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should remove data-setup here. I'm not sure we want to encourage its use as a feature.

Also, don't need class="video-js" if using the <video-js> element.


The function which Video.js calls to see if the `VhsSourceHandler` can handle the source is aptly named `canHandleSource`.

Inside `canHandleSource`, VHS checks the source's `type`. In our case, it sees `application/x-mpegURL`, and, if we're running in a browser with MSE, then it says "I can handle it!" (It actually says "maybe," because in life there are few guarantees, and because the spec says to use "maybe.")
Copy link
Member

Choose a reason for hiding this comment

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

the spec is a bit unclear - which spec? Can we link to it?

}
```

Many properties are removed for simplicity. This is a top level manifest (often referred to as a master or main manifest), and within it there are playlists, each playlist being a Manifest itself. Since the JSON "schema" for main and media playlists is the same, you will see irrelevant properties within any given manifest object. For instance, you might see a `targetDuration` property on the main manifest object, though a main manifest doesn't have a target duration. You can ignore irrelevant properties. Eventually they should be cleaned up, and a proper schema defined for manifest objects.
Copy link
Member

Choose a reason for hiding this comment

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

There's some room for confusion here. We have names for the master manifest (master, main) and then names for the playlists, which are subsequently referred to as "media playlists". Might be room to tighten up the phrasing here.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Garrett's on leave for a bit, so I think we'll just merge this one and any improvements/fixes can be done forward.

@misteroneill misteroneill merged commit 42fe383 into videojs:main Mar 14, 2022
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.

4 participants