Feat: add an option to play without audio or video if audio or video failed #1624
Conversation
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
724b4a3 to
5a073f9
Compare
|
relates to #1602 |
Shouldn't it be "tracks" here?
Doesn't it break the API this way? Its complex but it changes the default behavior on which some applications may rely? Wouldn't it be safer to default it to |
Yes I think I should default it to "error" instead. |
I have updated it. |
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
ef25c83 to
4b6acd7
Compare
|
As this adds a similar logic in the TrackStore + Manifest + Init, and as the manifest is more or less intended to just regroup metadata, I'm wondering if it would not be easier to follow if that option was mainly handled by a part of the What do you think? |
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
2 times, most recently
from
5e29ebe to
dc7bfe7
Compare
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
40f9dfa to
d9cd1bb
Compare
| "NO_PLAYABLE_REPRESENTATION", | ||
| `No ${trackType} Representation can be played`, | ||
| { tracks: undefined }, | ||
| ); |
There was a problem hiding this comment.
So this event now is also sent when the codec is not supported? That's a big change
There was a problem hiding this comment.
This error was also sent in previous version
There was a problem hiding this comment.
Not for when the codec wasn't supported which now can happen due to the "continue" part
There was a problem hiding this comment.
In that case, I'm wondering if a MANIFEST_INCOMPATIBLE_CODECS_ERROR error wouldn't be preferable
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
d9cd1bb to
48f85a9
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
48f85a9 to
28dbe1f
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
force-pushed
the
dev
branch
3 times, most recently
from
00fc806 to
b7216b4
Compare
| expect(playerError.message).toBe( | ||
| "NO_AUDIO_VIDEO_TRACKS: No audio and no video tracks are set.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Maybe we should add a second video-disabling test which re-enables the video on the noPlayableTrack for audio?
Just as it seems to me like a potential application use case.
E.g. let's imagine an application that allows for both audio-only and onAudioTracksNotPlayable: "continue".
The user enabled audio-only mode, and the next Period is only in "ec-3" audio that their device does not support.
In that case, it could make sense for the application to prefer re-enabling video over throwing an error (both throwing or continuing video-only make as much sense to me - so their choice :D):
player.addEventListener("noPlayableTrack", (evt) => {
if (evt.type === "audio" && player.getVideoTrack(evt.periodId) === null) {
// ... reset videoTrack to something.
}
});to ensure we're not breaking that potential usage we could add a test to check that we're not stopping the content before the application had a time to re-enable video, I think. Do you agree?
There was a problem hiding this comment.
As discussed it is hard to do this test now, we may do it later
|
CI is failing but I think it's just due to older gh-actions dependencies that since recently leads to an HTTP 422. I since fixed it on |
|
Hm it's difficult to review, I think a rebase needs to be done. Also there are some comments without answers so I'm unsure of their status in your POV, maybe the best would be to set a meeting to see all of this so we can work towards merging this nice addition instead of keeping it in limbos. |
This add "onAudioTracksNotPlayable" and "onVideoTracksNotPlayable" that defines the behavior of the player upon encountering a track that is not playable. With value "continue", the player will play the content without the unsupported track type. With value "error", the player will trigger an error. (behavior before this change)
Florent-Bouisset
force-pushed
the
feat/play-without-audio-or-video
branch
from
b4a0bf2 to
ad128b5
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
force-pushed
the
feat/play-without-audio-or-video
branch
from
1bf9e8e to
e2d10f3
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
Currently, the player throws an error if either all audio or all video tracks are not playable. This PR introduces two new options, onAudioTrackNotPlayable and onVideoTrackNotPlayable, to the loadVideo method. These options allow the player to continue playback even when all audio or video tracks are not playable. In such cases, the content will play without audio or video, respectively, instead of failing.
API
loadVideo(options)options object has two more keys:onAudioTrackNotPlayable:"continue"or"error". Default if not set:"continue"onVideoTrackNotPlayable:"continue"or"error". Default if not set:"continue"Usage
Example
https://codesandbox.io/p/sandbox/amazing-pare-kdf65f