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.

Sign up for GitHub

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

Feat/media UI extensions live et al #476

Merged
merged 8 commits into from
Feb 24, 2023
Merged

Feat/media UI extensions live et al #476

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb88e4b
feat(stream-type): Add support for m-ui-e streamType proposal.
cjpillsbury Feb 15, 2023
2c4f58f
feat(target-live-window): Add basic support for m-ui-e targetLiveWind…
cjpillsbury Feb 15, 2023
3095a79
feat(live-edge-window): Add basic support for m-ui-e liveEdgeStart pr…
cjpillsbury Feb 16, 2023
42add6b
chore: refactor default live edge crud.
cjpillsbury Feb 16, 2023
de10896
feat(media-live-button): Implement paused behaviors and presentation …
cjpillsbury Feb 16, 2023
cfb357d
fix: rename live edge override attribute to liveedgeoffset. Code clea…
cjpillsbury Feb 22, 2023
5ac376a
docs: Add docs for liveedgeoffset.
cjpillsbury Feb 23, 2023
d5c757f
Merge branch 'main' into feat/media-ui-extensions-live-et-al
cjpillsbury Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/js/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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")

MEDIA_TIME_IS_LIVE: 'media-time-is-live',
};

Expand Down
110 changes: 59 additions & 51 deletions src/js/media-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,16 @@ class MediaController extends MediaContainer {
MediaUIAttributes.MEDIA_DURATION,
getDuration(this)
);
this.propagateMediaState(MediaUIAttributes.MEDIA_STREAM_TYPE);
},
'emptied,durationchange,loadedmetadata,streamtypechange': () => {
this.propagateMediaState(
MediaUIAttributes.MEDIA_STREAM_TYPE
);
},
'emptied,durationchange,loadedmetadata,targetlivewindowchange': () => {
this.propagateMediaState(
MediaUIAttributes.MEDIA_TARGET_LIVE_WINDOW
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

  1. media is currently playing
  2. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@cjpillsbury cjpillsbury Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

},
'loadedmetadata,emptied,progress': () => {
this.propagateMediaState(
Expand Down Expand Up @@ -1007,66 +1016,38 @@ const Delegates = {
},

[MediaUIAttributes.MEDIA_STREAM_TYPE](el) {
const media = el.media;

if (!media) return null;

const duration = media.duration;

if (duration === Infinity) {
return StreamTypes.LIVE;
} else if (Number.isFinite(duration)) {
return StreamTypes.ON_DEMAND;
} else {
const defaultType = el.getAttribute('default-stream-type');

if (StreamTypeValues.includes(defaultType)) {
return defaultType;
}
}

return null;
return getStreamType(el);
},
[MediaUIAttributes.MEDIA_TARGET_LIVE_WINDOW](el) {
return getTargetLiveWindow(el);
},
[MediaUIAttributes.MEDIA_TIME_IS_LIVE](controller) {
const media = controller.media;

if (!media) return false;

const streamIsLive = controller.getAttribute(MediaUIAttributes.MEDIA_STREAM_TYPE) === 'live';
const seekable = media.seekable;

// 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') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should this move into its own getter function for consistency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if (Number.isNaN(media.liveEdgeStart)) return false;
return media.currentTime >= media.liveEdgeStart;
}

// If the slotted media does not provide a `seekable` property or its length is falsey,
// assume the media is not live.
if (!seekable?.length) {
return false;
}
const live = getStreamType(controller) === 'live';
// Can't be playing live if it's not a live stream
if (!live) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


const seekable = media.seekable;
// If the slotted media element is live but does not expose a 'seekable' `TimeRanges` object,
// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


// Default to 10 seconds
// Assuming seekable range already accounts for appropriate buffer room
let liveThreshold = 10;
let liveThresholdAttr = controller.getAttribute('livethreshold');

if (liveThresholdAttr !== null) {
liveThresholdAttr = Number(liveThresholdAttr);

if (!Number.isNaN(liveThresholdAttr)) {
liveThreshold = liveThresholdAttr;
}
}

const currentTime = media.currentTime;
const seekableEnd = seekable.end(seekable.length - 1);

if (currentTime > seekableEnd - liveThreshold) {
return true;
}

return false;
const liveEdgeStartOffset = controller.hasAttribute('liveedgeoffset')
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Name change. Might require a docs change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

? Number(controller.getAttribute('liveedgeoffset'))
: 10;
const liveEdgeStart = seekable.end(seekable.length - 1) - liveEdgeStartOffset
return media.currentTime >= liveEdgeStart;
},
};

Expand Down Expand Up @@ -1154,6 +1135,33 @@ const getShowingCaptionTracks = (controller) => {
});
};

const getStreamType = (controller) => {
const { media } = controller;

if (!media) return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, this returned null, why the change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.


if (media.streamType) return media.streamType;
const duration = media.duration;

if (duration === Infinity) {
return StreamTypes.LIVE;
} else if (Number.isFinite(duration)) {
return StreamTypes.ON_DEMAND;
} else {
const defaultType = controller.getAttribute('default-stream-type');

if (StreamTypeValues.includes(defaultType)) {
return defaultType;
}
}

return undefined;
};

const getTargetLiveWindow = (controller) => {
return controller?.media?.targetLiveWindow;
};

const MEDIA_UI_ATTRIBUTE_NAMES = Object.values(MediaUIAttributes);

const getMediaUIAttributesFrom = (child) => {
Expand Down
13 changes: 7 additions & 6 deletions src/js/media-live-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ slotTemplate.innerHTML = `
color: var(--media-live-button-icon-color, rgb(140, 140, 140));
}

:host([${MEDIA_TIME_IS_LIVE}]) slot[name=indicator] > *,
:host([${MEDIA_TIME_IS_LIVE}]) ::slotted([slot=indicator]) {
:host([${MEDIA_TIME_IS_LIVE}]:not([${MEDIA_PAUSED}])) slot[name=indicator] > *,
:host([${MEDIA_TIME_IS_LIVE}]:not([${MEDIA_PAUSED}])) ::slotted([slot=indicator]) {
fill: var(--media-live-indicator-color, rgb(255, 0, 0));
color: var(--media-live-indicator-color, rgb(255, 0, 0));
}

:host([${MEDIA_TIME_IS_LIVE}]) {
:host([${MEDIA_TIME_IS_LIVE}]:not([${MEDIA_PAUSED}])) {
cursor: not-allowed;
}

Expand Down Expand Up @@ -61,14 +61,15 @@ class MediaLiveButton extends MediaChromeButton {
}

handleClick() {
// Don't seek if already live
if (this.getAttribute(MEDIA_TIME_IS_LIVE) !== null) return;
// If we're live and not paused, don't allow seek to live
if (!this.hasAttribute(MEDIA_PAUSED) && this.hasAttribute(MEDIA_TIME_IS_LIVE)) return;

this.dispatchEvent(
new window.CustomEvent(MEDIA_SEEK_TO_LIVE_REQUEST, { composed: true, bubbles: true })
);

if (this.getAttribute(MEDIA_PAUSED) !== null) {
// If we're paused, also automatically play
if (this.hasAttribute(MEDIA_PAUSED)) {
this.dispatchEvent(
new window.CustomEvent(MEDIA_PLAY_REQUEST, { composed: true, bubbles: true })
);
Expand Down