Project Scaffolding and mux-video initial implementation #4

cjpillsbury · 2021-08-16T18:08:07Z

Adds initial scaffolding for the monorepo/project (issue templates, gitignore, package.json, workspace defs, other env stuff)
Adds initial impl of the mux-video package.

…tandard markdowns. License. nvmrc & npmrc for version enforcement.

…aceholder README.md badges.

…folding out components, types, and build infrastructure.

…ia-chrome but will change) plus example.

…Adding stub for env-key to use mux data/mux-embed.

… we have a src.

dylanjha

Looks like a really good start, the last missing bit (which we talked about over DM) is a prop for the application to pass in Mux Data metadata fields.

dylanjha · 2021-08-16T18:12:04Z

.github/ISSUE_TEMPLATE/bug_report.md

+## The current behavior
+
+
+## The expected behavior


this is a great template and after this is merged we should link to this and the other templates from our "Open Source at Mux" notion doc as good examples

dylanjha · 2021-08-16T18:17:01Z

packages/mux-video/src/index.ts

+
+    const env_key = this.getAttribute(Attributes.ENV_KEY);
+
+    if (Hls.isSupported()) {


I want to make sure we're intentional about using Hls.js in native Mac OS Safari.

I typically lean the other way on this and say "on native Mac Safari just use Safari's Hls"

Pros of using Hls.js in Mac Safari:

Mux Data gets request-level metrics

Cons

I've seen it have issues before

@cjpillsbury thoughts based on your experience? Maybe we can dig up some Mux Data ™️ to inform this decision

yeah let's audit/followup re: "Mux Data". I always prefer to use Hls.js for consistency/control when/where possible, but can see arguments for deferring to native safari support.

packages/mux-video/src/index.ts

dylanjha · 2021-08-16T18:26:29Z

packages/mux-video/src/index.ts

+            player_name: "mux-video", // any arbitrary string you want to use to identify this player
+            // Should this be the initialization of *THIS* player (instance) or the page?
+            player_init_time: this.__muxPlayerInitTime, // ex: 1451606400000
+            // ...


A couple other metadata fields we can automatically set:

video_duration (The length of the video in milliseconds) -- if the video is a VOD

video_stream_type (The type of video stream (e.g: 'live' or 'on-demand'))

player_version (read this value from the package.json?)

video_id (Your internal ID for the video) -- we can set this as the Mux playback_id

Particularly for video_id, we wouldn't want to over-ride a value that the user passes in. This might get a little tricky based on the order of when things are initialized and how/when the user sets Mux Data metadata props.

Maybe we can also have a debug attribute that will flip on debug mode for mux-video (& enable the debug flag mux.monitor

packages/mux-video/examples/index.html

… metadata. Update typedefs. Update example.

…llow server side smoke testing.

dylanjha · 2021-08-17T07:24:00Z

First Round QA Pass

Default styles:

Right now the default styles don't mirror what I would get with a "normal" HTML5 <video> element. These are the styles that get applied

:host {
  position: relative;
  width: 300px;
  height: 150px;
}

video {
  position: absolute;
  width: 100%;
  height: 100%;
}

As a result, I end up with a tiny postage stamp sized video player on the page, which is quite different than if I had just a regular <video> element and wired up HLS.js myself.

Ideally (and I could be persuaded off of this) I think the experience we want is someone could:

remove their <video> element + Hls.js stuff
drop in <mux-video> (if their video element had a CSS class selector like .video-player on their video element they could put that class on this element and end result would look the same)

I think by removing the styles above and keeping the rest we're able to achieve that.

Usage with query params

Mux supports a number of query params on stream.mux.com URLs (the most common being token= for signed URLs
When using the playback-id shorthand, we'll need to support query params so that playback-id="abc123?token=xyz789" gets expanded to a src that looks like https://stream.mux.com/abc123.m3u8?token=xyz789

Importing as a dep on other projects

I added a yarn build command -- does this look right?

diff --git a/packages/mux-video/package.json b/packages/mux-video/package.json
index 2c85a0e..2d6536c 100644
--- a/packages/mux-video/package.json
+++ b/packages/mux-video/package.json
@@ -12,7 +12,8 @@
   "license": "MIT",
   "private": true,
   "scripts": {
-    "dev": "snowpack dev --config snowpack.dev.config.js"
+    "dev": "snowpack dev --config snowpack.dev.config.js",
+    "build": "snowpack build --config snowpack.prod.config.js"
   },
   "dependencies": {
     "custom-video-element": "^0.0.2",
diff --git a/packages/mux-video/snowpack.prod.config.js b/packages/mux-video/snowpack.prod.config.js
index c57a372..b82e532 100644
--- a/packages/mux-video/snowpack.prod.config.js
+++ b/packages/mux-video/snowpack.prod.config.js
@@ -5,7 +5,7 @@
 module.exports = {
   extends: './snowpack.common.config.js',
   mount: {
-    'src/js': { url: '/' },
+    'src': { url: '/' },
   },
   optimize: {
     bundle: false,

Next, when I imported @mux/mux-video into a Next.js application I got this error:

Looks like maybe ES modules are causing a problem with SSR? I was able to work around this by opting out of SSR, and then everything was working although I am seeing a couple errors:

Typescript: 7016: Could not find a declaration file for module '@mux/mux-video'.
Typescript: 2339: Property 'mux-video' does not exist on type 'JSX.IntrinsicElements'.

Other Notes

// Should this be the initialization of THIS player (instance) or the page?

I saw this comment related to player_init_time the answer is: THIS player (instance), so you have this correct

"Some metadata values may not be overridable at this time. Make sure you set all metadata to override before setting the src."

I see the issue here, for the most part this actually doesn't cause any issues. The only time it would is if the user sets a metadata field that we're already going to automatically populate like one of these in the comment above

That should actually be pretty rare. It seems like we're recommending a workflow like this:

<mux-video env-key="mux-env-key"></mux-video>

<script>
  const muxVideo = document.querySelector('mux-video')
  muxVideo.metadata = { video_title: '', video_series: '' };
  muxVideo.setAttribute('playback-id', 'my-playback-id');
</script>

^ that feels a little bit jank, maybe we can do something like this and allow the user to pass in specific metadata overrides as attributes:

<mux-video
  env-key="mux-env-key"
  playback-id="my-playback-id"
  metadata-video-id="arbitrary-video-id"
 ></mux-video>

^ metadata-video-id is a field that mux-video will automatically populate with the playback-id at initialization time, but if the user passes it in via an attribute then we'll let them override it

Or maybe we can expose the most common subset of the Mux Data metadata fields with metadata-* attributes?

The thing I'm getting at is that it's nice if we can lean on HTML here as much as possible and having that work for the 90% of use cases, and then allow folks to drop down into JS land for the other 10% of advanced cases.

cjpillsbury · 2021-08-17T18:42:10Z

Will be creating followup issues for various talking points/issues @dylanjha identified in his feedback

heff

Good stuff!

heff · 2021-08-20T17:58:37Z

packages/mux-video/src/index.ts

+    return this.__hls;
+  }
+
+  get mux(): Readonly<typeof mux> | undefined {


We should really rename this getter. Maybe to muxData .

heff · 2021-08-20T18:03:15Z

packages/mux-video/src/index.ts

+    this.__muxPlayerInitTime = Date.now();
+  }
+
+  get hls() {


I think we're on the same page here where we should probably expose this now, but aim to deprecated it when we can cover most things people are doing with it. Otherwise we lock ourselves into HLS.js.

heff · 2021-08-20T19:21:10Z

packages/mux-video/src/index.ts

+      console.info(
+        "Some metadata values may not be overridable at this time. Make sure you set all metadata to override before setting the src."
+      );
+      this.mux.emit("hb", this.__metadata);


Does this actually work if you call it anywhere mid playback? i.e. will the title change? Feels like this might have a weird impact on real-time data.

heff · 2021-08-20T19:31:29Z

packages/mux-video/src/index.ts

+    return this.__metadata;
+  }
+
+  set metadata(val: Readonly<Metadata> | undefined) {


This feels pretty specifically deigned for Mux Data, whereas "metadata" might be used in multiple ways. e.g. a title might be used in the UI. I don't have a better answer but feels worth calling out.

I did have one related idea of allowing meta data elements inside the media element.

<media-container> <video> <media-meta-title>Big Buck Bunny</media-meta-title> </video> </media-container

Food for thought. No immediate change requested here.

heff · 2021-08-20T21:02:07Z

packages/mux-video/src/index.ts

+  }
+
+  disconnectedCallback() {
+    this.unload();


Does <video> reset its loaded state when you remove it from/add it to the dom?

heff · 2021-08-20T21:37:37Z

packages/mux-video/src/index.ts

+/** @TODO Refactor once using `globalThis` polyfills */
+if (!window.customElements.get("mux-video")) {
+  window.customElements.define("mux-video", MuxVideoElement);
+  /** @TODO consider externalizing this (breaks standard modularity) */


It's also a nice to have, not a must have.

heff · 2021-08-20T21:40:37Z

packages/mux-video/src/index.ts

+        const hadSrc = !!oldValue;
+        const hasSrc = !!newValue;
+        if (!hadSrc && hasSrc) {
+          this.load();


<video> doesn't automatically load() when you change the src=. I'm not sure we want to divert here.

heff · 2021-08-20T21:41:29Z

packages/mux-video/src/index.ts

+    }
+  }
+
+  // NOTE: This was carried over from hls-video-element. Is it needed for an edge case?


You can delete it. It was half progress towards support the preload= API.

heff · 2021-08-20T21:43:57Z

packages/mux-video/src/index.ts

+  }
+
+  unload() {
+    // NOTE: I believe we cannot reliably "recycle" hls player instances, but should confirm at least for optimization reasons.


I believe I tried and HLS was not happy, but worth confirming.

heff · 2021-08-20T21:46:25Z

packages/mux-video/src/index.ts

+    }
+  }
+
+  unload() {


Probably fine and helpful to add this method, but with <video> you'd set an empty src and call load(), so we should make sure that also works. For example a playlist function might expect that.

[WIP] Bar progress UI plus some bells

cjpillsbury added 11 commits August 13, 2021 11:43

chore(scaffold): Issue Templates. gitignore. package setup. various s…

862cc5f

…tandard markdowns. License. nvmrc & npmrc for version enforcement.

chore(scaffold): adding feature request issue template

7d49d8f

chore(scaffold): Update issue templates for non-form version. Stub/pl…

df43185

…aceholder README.md badges.

chore(scaffold): missing newline at eof for lerna.json

ccd1f8c

chore(scaffold): additional missing newlines at eof.

5a8ca1b

feat(mux-video-init): Initial implementation of mux-video. Start scaf…

63b6e85

…folding out components, types, and build infrastructure.

feat(mux-video-init): Add basic snowpack build (currenty based on med…

974780f

…ia-chrome but will change) plus example.

feat(mux-video-init): add controls to simplify using example

8e050c6

feat(mux-video-init): Adding basic integration of mux-embed (untested).

eca1eb3

feat(mux-video-init): Adding Hls as export of mux-video root module. …

71bd1f3

…Adding stub for env-key to use mux data/mux-embed.

feat(mux-video-init): 'bake in' hls error recovery. Only auto-load if…

b3de443

… we have a src.

cjpillsbury requested a review from dylanjha August 16, 2021 18:08

dylanjha reviewed Aug 16, 2021

View reviewed changes

cjpillsbury added 5 commits August 16, 2021 15:34

feat(mux-video-init): Beef up typings for mux-embed.

07d9953

feat(mux-video-init): Add support initial support for debug, mux data…

e122113

… metadata. Update typedefs. Update example.

feat(mux-video-init): Adding support for playback id. Clean up example.

f08a63c

feat(mux-video-init): example of loading metadata from a URL

6bd92a2

feat(mux-video-init): (temporarily) adding polyfill to mux-video to a…

156ce9b

…llow server side smoke testing.

feat(mux-video-init): handle various src attr changes. Add unload.

13a68b6

cjpillsbury merged commit bdf0559 into muxinc:main Aug 17, 2021

heff reviewed Aug 20, 2021

View reviewed changes

clearlyTHUYDOAN added a commit to clearlyTHUYDOAN/elements that referenced this pull request May 10, 2022

Merge pull request muxinc#4 from dylanjha/wip/bar-progress-ui

7f44f97

[WIP] Bar progress UI plus some bells

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project Scaffolding and mux-video initial implementation #4

Project Scaffolding and mux-video initial implementation #4

cjpillsbury commented Aug 16, 2021

dylanjha left a comment

dylanjha Aug 16, 2021

dylanjha Aug 16, 2021

cjpillsbury Aug 16, 2021

dylanjha Aug 16, 2021

dylanjha commented Aug 17, 2021

cjpillsbury commented Aug 17, 2021

heff left a comment

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021

heff Aug 20, 2021


		const env_key = this.getAttribute(Attributes.ENV_KEY);

		if (Hls.isSupported()) {

Project Scaffolding and mux-video initial implementation #4

Project Scaffolding and mux-video initial implementation #4

Conversation

cjpillsbury commented Aug 16, 2021

dylanjha left a 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

dylanjha commented Aug 17, 2021

First Round QA Pass

Default styles:

Usage with query params

Importing as a dep on other projects

Other Notes

cjpillsbury commented Aug 17, 2021

heff left a 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

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