-
Notifications
You must be signed in to change notification settings - Fork 91
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
Feat/media UI extensions live et al #476
Changes from 7 commits
bb88e4b
2c4f58f
3095a79
42add6b
de10896
cfb357d
5ac376a
d5c757f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This more circumscribed attribute should be able to be reliably updated based on events and would simplify There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the updated implementation of |
||
}, | ||
'loadedmetadata,emptied,progress': () => { | ||
this.propagateMediaState( | ||
|
@@ -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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this move into its own getter function for consistency? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Name change. Might require a docs change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}, | ||
}; | ||
|
||
|
@@ -1154,6 +1135,33 @@ const getShowingCaptionTracks = (controller) => { | |
}); | ||
}; | ||
|
||
const getStreamType = (controller) => { | ||
const { media } = controller; | ||
|
||
if (!media) return undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, this returned There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")