-
Notifications
You must be signed in to change notification settings - Fork 132
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
feature: add the possibility to rely on managedMediaSource on iOS devices #1562
base: dev
Are you sure you want to change the base?
Conversation
Aren't there also supplementary events we're supposed to handle as a player with a I know there's the potentiality of GC at any time, which I think we prefer and already support, and that brings the |
27776f2
to
a6b5a49
Compare
So if I get it, we will not be reacting to I wonder if it's what we want:
|
We could also have a supplementary parameter |
babacfe
to
2fd5e0c
Compare
*/ | ||
if ( | ||
"disableRemotePlayback" in videoElement && | ||
disableRemotePlaybackPreviousValue !== undefined |
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.
Why having that condition?
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.
- first condition
"disableRemotePlayback" in videoElement
because on browsers that does not havedisableRemotePlayback
I don't see the point of setting it. - 2nd condition
disableRemotePlaybackPreviousValue !== undefined
is because typescript don't allow to possibly assignvideoElement.disableRemotePlayback
toundefined
. Because it's typed asdisableRemote?: boolean
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.
It was for the second condition.
So it creates a weird situation where we would reset the value UNLESS it was previously set to undefined
...
Maybe calling something like removeAttribute
in that case is more logical / simpler to understand?
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.
UNLESS it was previously set to undefined
The property is a boolean so it should never be set to undefined
I can use removeAttribute but this will not reset it to true if that was already true before using the RxPlayer
Don't know if the comment is outdated, but it actually send an event |
…ices add unit test for managed media source handle start and end stream event from managedMediaSource fix unit test react to event endStreaming and startStreaming to immediately trigger a new observation and to download the segments sooner review feedback restart Loading queue if it canLoad is updated add clearSignal to prevent memory leak remove undefined from canLoad only set disableRemotePlayback if it's defined by the browser fmt
6571e83
to
90e74cc
Compare
* in order to leave the <video> element in the same state has it was set | ||
* by the application before calling the RxPlayer. | ||
*/ | ||
if (disableRemotePlaybackPreviousValue !== undefined) { |
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 thought that if videoElement.disableRemotePlayback
was defined, it was always a boolean. We already checked that "disableRemotePlayback" in videoElement
, so how come disableRemotePlaybackPreviousValue
can now be set to undefined
for TypeScript?
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.
Yes, done.
* an Airplay source alternative, such as HLS. | ||
* https://github.com/w3c/media-source/issues/320 | ||
*/ | ||
mediaElement.disableRemotePlayback = true; |
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.
We could also put that one inside a if
like in the other place no?
* an Airplay source alternative, such as HLS. | ||
* https://github.com/w3c/media-source/issues/320 | ||
*/ | ||
mediaElement.disableRemotePlayback = true; |
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.
Is it possible to just factorize all places where we doing this as it looks fairly similar?
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.
Done it
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 see the factorized code but why isn't it used here?
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.
Yes I have replaced it once it that file but it was actually used twice, thanks will update it
mediaElement: IMediaElement, | ||
cancellationSignal: CancellationSignal, | ||
) { | ||
if (isManagedMediaSource && "disableRemotePlayback" in mediaElement) { |
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.
It's a weird combination when I read this to:
- have a method just called
disableRemotePlayback
that takes a media element and a cancel signal (which I would assume just disable remote playback until the cancel signal emits I guess) - have it inside
create_media_source.ts
and have it checkingisManagedMediaSource
(and doing nothing if that isn't the case)
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.
Yes but the other way around I would need to repeat the condition
if (isManagedMediaSource && "disableRemotePlayback" in mediaElement)
every time disableRemotePlayback()
is used. I found it to be more factorized like that, do you prefer the other way and repeat the condition?
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.
Or maybe I could rename it so it is more clear that it could disable it or not depending of the type of mediaSource
Add the possibility to rely on
ManagedMediaSource
for devices that has no access toMediaSource
such as iOS devices with version > 17.1In practice that allow devices like iPhone to play DASH content with the RxPlayer.
Note that it is not possible to cast a DASH content with AirPlay, so the RxPlayer disable the possibility to stream to AirPlay by adding the
disableRemotePlayback
attribute to the video Element when a DASH content is loaded.#1294