Feat/media UI extensions live et al #476

cjpillsbury · 2023-02-16T19:22:40Z

This adds support for APIs currently being drafted in media-ui-extensions, including:

Additionally, it updates the behavior of the media-live-button per prior conversations around presentation and behavior related to the media-paused state.

vercel · 2023-02-16T19:22:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
media-chrome	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 24, 2023 at 9:09PM (UTC)
media-chrome-docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 24, 2023 at 9:09PM (UTC)

codecov · 2023-02-16T19:26:00Z

Codecov Report

Merging #476 (5ac376a) into main (c77cd43) will decrease coverage by 0.05%.
The diff coverage is 73.13%.

❗ Current head 5ac376a differs from pull request most recent head d5c757f. Consider uploading reports for the commit d5c757f to get more accurate results

@@            Coverage Diff             @@
##             main     #476      +/-   ##
==========================================
- Coverage   68.51%   68.47%   -0.05%     
==========================================
  Files          43       43              
  Lines        6797     6807      +10     
==========================================
+ Hits         4657     4661       +4     
- Misses       2140     2146       +6

Impacted Files	Coverage Δ
src/js/media-live-button.js	`77.38% <42.85%> (-0.94%)`	⬇️
src/js/media-controller.js	`66.34% <76.27%> (-0.16%)`	⬇️
src/js/constants.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cjpillsbury · 2023-02-16T19:29:26Z

src/js/media-controller.js

+      'emptied,durationchange,loadedmetadata,targetlivewindowchange': () => {
+        this.propagateMediaState(
+          MediaUIAttributes.MEDIA_TARGET_LIVE_WINDOW
+        );


NOTE: On unchanged lines https://github.com/muxinc/media-chrome/pull/476/files#diff-f093c2ba0a65c5a566a4ad86659ffc5bee29bfbbc241f863bed8f634b7a0f9f7R508-R510 where we monitor for events to update MEDIA_TIME_IS_LIVE, this does not account for all cases where that value may change, nor is there any set of events on an HTMLMediaElement that would catch all cases. Given our current use case, we may want to instead model something like MEDIA_IS_PLAYING_LIVE that means:

media is currently playing

the media playing is within the live edge window

This more circumscribed attribute should be able to be reliably updated based on events and would simplify media-live-button.

I'm still not clear where these edge cases are, but I think the important thing is not that every change is captured, but that when it's needed by the UI, it's up to date. The existing version seems ok to me from that standpoint, at least as a starting point.

I don't love media-is-playing-live because we already have media-paused to tell half that story.

for the updated implementation of media-live-button in this PR, the edge case will not be a problem. However, the media-time-is-live will "lie" on a subset of initialization cases for live/dvr, particularly when autoplay is off. As long as we're ok with that attribute not always being accurate, we can definitely treat that as out of scope and a "we'll deal with it if/when it becomes an issue" scenario, since it doesn't impact any current media chrome use cases/components.

package.json

cjpillsbury · 2023-02-16T20:40:30Z

src/js/media-controller.js

-    // If there's no way to seek, assume the media element is keeping it "live"
-    if (streamIsLive && !seekable) {
-      return true;
+    if (typeof media.liveEdgeStart === 'number') {


should this move into its own getter function for consistency?

Yeah, maybe. We can wait until we need it in two places.

gkatsev · 2023-02-16T22:11:56Z

src/js/media-controller.js

+const getStreamType = (controller) => {
+  const { media } = controller;
+
+  if (!media) return undefined;


Previously, this returned null, why the change?

Just for consistency with the spec. If there's no slotted media, I'm treating this as equivalent to "unimplemented". From "the out side" (aka the attribute value), this should be identical regardless.

Interesting to look at this from either the media element or media chrome perspective. I could see this being "media chrome knows what stream type is, but it's unknown". But I'm good with matching the spec here.

@heff yup was mulling on the same thing and this is where I (soft) landed. Open to rethinking that moving forward for this kinda stuff if we wanted to discuss in one of our weekly meetings.

gkatsev · 2023-02-16T22:15:01Z

src/js/media-controller.js

-    }
-
-    return false;
+    const liveEdgeStartOffset = controller.hasAttribute('livethreshold') 


Should this attribute be with respect to the new proposal? Maybe to match the variable name?

Yeah I agree this attribute should get a rename. Since the proposal moved to liveEdgeStart, there isn't an explicit equivalent of livethreshold (it's effectively the variable name liveEdgeStartOffset). Do you think the attribute should be liveedgestartoffset? or liveedgeoffset (maybe a bit confusing, though that was the name of the property in the previous iteration of the proposal)? or something else? Def open to suggestions here (Names Are Hard™️)

liveedgeease :)

liveedgeoffset sounds pretty good to me

Cool I'll run with that unless folks have any concerns.

luwes · 2023-02-22T00:57:56Z

src/js/media-controller.js

-    }
+    const live = getStreamType(controller) === 'live';
+    // Can't be playing live if it's not a live stream
+    if (!live) return false;


nit (non-b): 1 empty line after a return statement might help reading the code

luwes · 2023-02-22T00:59:15Z

src/js/media-controller.js

+    // always assume playing live
+    if (!seekable) return true;
+    // If there is an empty `seekable`, assume we are not playing live
+    if (!seekable.length) return false;


…ow proposal.

…oposal.

…for component.

…nup per PR feedback.

heff · 2023-02-22T21:58:52Z

src/js/constants.js

@@ -84,6 +84,7 @@ export const MediaUIAttributes = {
  MEDIA_LOADING: 'media-loading',
  MEDIA_BUFFERED: 'media-buffered',
  MEDIA_STREAM_TYPE: 'media-stream-type',
+  MEDIA_TARGET_LIVE_WINDOW: 'media-target-live-window',


Do you think we'll change the name of this based on the conversation of Edge vs. Seekable window? Just a question, not a suggestion.

I don't believe so. I think "target live window" makes sense still in our "DVR" proposal. We can always amend it if necessary once we move video-dev/media-ui-extensions#4 to an official proposal PR, since there isn't a custom/extended media element that supports this yet (and mux-video will be the first, so we can treat it as "internal")

heff · 2023-02-22T22:03:19Z

src/js/media-controller.js

+      'emptied,durationchange,loadedmetadata,targetlivewindowchange': () => {
+        this.propagateMediaState(
+          MediaUIAttributes.MEDIA_TARGET_LIVE_WINDOW
+        );


I'm still not clear where these edge cases are, but I think the important thing is not that every change is captured, but that when it's needed by the UI, it's up to date. The existing version seems ok to me from that standpoint, at least as a starting point.

I don't love media-is-playing-live because we already have media-paused to tell half that story.

heff · 2023-02-22T22:52:45Z

src/js/media-controller.js

-    // If there's no way to seek, assume the media element is keeping it "live"
-    if (streamIsLive && !seekable) {
-      return true;
+    if (typeof media.liveEdgeStart === 'number') {


Yeah, maybe. We can wait until we need it in two places.

heff · 2023-02-22T22:58:57Z

src/js/media-controller.js

-    }
-
-    return false;
+    const liveEdgeStartOffset = controller.hasAttribute('liveedgeoffset') 


👍 Name change. Might require a docs change?

I did a quick search and didn't see any docs, but I'll be sure to do a more thorough audit before merging.

heff · 2023-02-22T23:01:45Z

src/js/media-controller.js

+const getStreamType = (controller) => {
+  const { media } = controller;
+
+  if (!media) return undefined;


Interesting to look at this from either the media element or media chrome perspective. I could see this being "media chrome knows what stream type is, but it's unknown". But I'm good with matching the spec here.

cjpillsbury requested review from a team and heff as code owners February 16, 2023 19:22

vercel bot deployed to Preview – media-chrome February 16, 2023 19:23 View deployment

vercel bot deployed to Preview – media-chrome-docs February 16, 2023 19:23 View deployment

cjpillsbury commented Feb 16, 2023

View reviewed changes

cjpillsbury mentioned this pull request Feb 16, 2023

proposal: live edge. video-dev/media-ui-extensions#7

Merged

cjpillsbury commented Feb 16, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

cjpillsbury commented Feb 16, 2023

View reviewed changes

gkatsev reviewed Feb 16, 2023

View reviewed changes

cjpillsbury force-pushed the feat/media-ui-extensions-live-et-al branch from 52e7e3c to f69ba9a Compare February 17, 2023 21:01

vercel bot deployed to Preview – media-chrome February 17, 2023 21:01 View deployment

vercel bot deployed to Preview – media-chrome-docs February 17, 2023 21:01 View deployment

luwes reviewed Feb 22, 2023

View reviewed changes

cjpillsbury added 6 commits February 22, 2023 12:47

feat(stream-type): Add support for m-ui-e streamType proposal.

bb88e4b

feat(target-live-window): Add basic support for m-ui-e targetLiveWind…

2c4f58f

…ow proposal.

feat(live-edge-window): Add basic support for m-ui-e liveEdgeStart pr…

3095a79

…oposal.

chore: refactor default live edge crud.

42add6b

feat(media-live-button): Implement paused behaviors and presentation …

de10896

…for component.

fix: rename live edge override attribute to liveedgeoffset. Code clea…

cfb357d

…nup per PR feedback.

cjpillsbury force-pushed the feat/media-ui-extensions-live-et-al branch from f69ba9a to cfb357d Compare February 22, 2023 21:50

vercel bot deployed to Preview – media-chrome February 22, 2023 21:50 View deployment

vercel bot deployed to Preview – media-chrome-docs February 22, 2023 21:51 View deployment

heff approved these changes Feb 22, 2023

View reviewed changes

docs: Add docs for liveedgeoffset.

5ac376a

vercel bot deployed to Preview – media-chrome February 23, 2023 19:10 View deployment

vercel bot deployed to Preview – media-chrome-docs February 23, 2023 19:10 View deployment

luwes approved these changes Feb 24, 2023

View reviewed changes

Merge branch 'main' into feat/media-ui-extensions-live-et-al

d5c757f

vercel bot deployed to Preview – media-chrome February 24, 2023 21:09 View deployment

vercel bot deployed to Preview – media-chrome-docs February 24, 2023 21:10 View deployment

cjpillsbury merged commit 2ed0630 into muxinc:main Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/media UI extensions live et al #476

Feat/media UI extensions live et al #476

cjpillsbury commented Feb 16, 2023

vercel bot commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

cjpillsbury Feb 16, 2023

heff Feb 22, 2023

cjpillsbury Feb 22, 2023 •

edited

Loading

cjpillsbury Feb 16, 2023

heff Feb 22, 2023

gkatsev Feb 16, 2023

cjpillsbury Feb 16, 2023

heff Feb 22, 2023

cjpillsbury Feb 22, 2023

gkatsev Feb 16, 2023

cjpillsbury Feb 16, 2023

luwes Feb 22, 2023

cjpillsbury Feb 22, 2023

luwes Feb 22, 2023

luwes Feb 22, 2023

heff Feb 22, 2023

cjpillsbury Feb 22, 2023

heff Feb 22, 2023

heff Feb 22, 2023

heff Feb 22, 2023

cjpillsbury Feb 22, 2023

heff Feb 22, 2023

Feat/media UI extensions live et al #476

Feat/media UI extensions live et al #476

Conversation

cjpillsbury commented Feb 16, 2023

vercel bot commented Feb 16, 2023 • edited Loading

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpillsbury Feb 22, 2023 • edited Loading

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

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

vercel bot commented Feb 16, 2023 •

edited

Loading

codecov bot commented Feb 16, 2023 •

edited

Loading

cjpillsbury Feb 22, 2023 •

edited

Loading