search

videojs / http-streaming

HLS, DASH, and future HTTP streaming protocols library for video.js
https://videojs-http-streaming.netlify.app/
Other
2.45k stars 418 forks source link

fix: check if change to the provided type is supported #1463

Closed rtkac closed 5 months ago

rtkac commented 6 months ago

Description

This change will fix an error on change to the provided codec which is not supported. Just warn about it and do not freeze the video.

welcome[bot] commented 6 months ago

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

adrums86 commented 6 months ago

Thank you for submitting this PR! The Edge changeType issue has been on our radar and we're also thinking about solutions. We have some ideas, but I think we may need to handle this by re-creating the source buffer instead of calling change type. There's some more information in the W3C spec regarding changeType throwing an error in certain cases where isTypeSupported is returning true for that same codec.

rtkac commented 6 months ago

@adrums86 thanks for your comment.

Do you mean that there will be more complex solution than the fix in the PR? Do you know when we would expect an update and release of the http-streaming library? FYI: we are using video.js v7.x which is using http-streaming v2.x. So I believe v2 version will be updated as well.

For more context: Edge already uses the MediaFoundation renderer, which doesn't support changeType https://developer.mozilla.org/en-US/docs/Web/API/SourceBuffer/changeType. According to chrome://flags/#enable-media-foundation-clear flag, there will be a transition to MediaFoundation for non DRM content. So this is where the problem begins.

Thanks a lot again.

adrums86 commented 6 months ago

@rtkac Yeah unfortunately I think the solution might be a bit more complex. We will likely have to do some re-working of the logic above changeType to create s new source buffer instead of calling changeType here for Edge. I can't provide a solid date unfortunately, but I can say that this is very high on our priority list. I am personally looking at this issue, so I can provide updates as soon as their available. Your effort and feedback are super helpful so please feel free to reach out on videojs slack or here on github if you'd like to collaborate further on the fix.

gkatsev commented 6 months ago

Of note, are we still maintaining the 2.x branch? Last commit was in February.

However, maybe worth having a change like this in main if it improves things and doesn't cause issues while the proper solution is being worked on?

rtkac commented 6 months ago

@adrums86 understand, thanks for your reply. @gkatsev I hope so :D we would really appreciate it if the changes also affects 2.x branch. It is super important for us right now because video.js 7.x still uses 2.x http-streaming. Thanks

adrums86 commented 6 months ago

I'm not opposed to using this as a bandaid until we can get the proper fix implemented around changeType for Edge. Yeah 2.x is quite stale and not actively maintained 😅 .

dzianis-dashkevich commented 6 months ago

@rtkac thank you for your contribution!

Edge already uses the MediaFoundation renderer, which doesn't support changeType https://developer.mozilla.org/en-US/docs/Web/API/SourceBuffer/changeType.

Is there any open documentation regarding this?

I am trying to understand how this fix helps avoid playback failure.

I would assume it should fail if we didn't actually changeType on the source buffer and then call appendBuffer with data encoded using different codecs (since the browser's media pipeline has to re-create different decoder)

We exclude renditions with unsupported codecs using MediaSource.isTypeSupported here https://github.com/videojs/http-streaming/blob/main/src/playlist-controller.js#L1784

In this case, if changeType fails for some reason, we should probably attempt to re-initialize sourceBuffer with the provided codecs. I expect this to work fine since we should have only supported codecs at this moment.

PS: I am definitely not opposing merging this PR.

rtkac commented 6 months ago

@adrums86 I understand that the 2.x is not maintained, but this fix would be really helpful for us because we depend on v2 right now. What do you think, is it possible to merge it into v2 and release a new version? Thanks!

adrums86 commented 5 months ago

@rtkac It might be possible to get this into 2.x and get a quick release out. I do think we should get the change @dzianis-dashkevich suggested into the main branch while we work on the proper fix however.

rtkac commented 5 months ago

@adrums86 @dzianis-dashkevich PR updated

codecov[bot] commented 5 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (aa9dfbd) 86.37% compared to head (3c4b89f) 86.37%. Report is 2 commits behind head on 2.x.

Files Patch % Lines
src/source-updater.js 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 2.x #1463 +/- ## ======================================= Coverage 86.37% 86.37% ======================================= Files 39 39 Lines 9887 9889 +2 Branches 2307 2307 ======================================= + Hits 8540 8542 +2 Misses 1347 1347 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dzianis-dashkevich commented 5 months ago

@rtkac thank you for your contribution!

I will merge it into 2.x and cherry-pick into the main.

welcome[bot] commented 5 months ago

Congrats on merging your first pull request! 🎉🎉🎉

rtkac commented 5 months ago

@dzianis-dashkevich thank you 👍

adrums86 commented 5 months ago

@rtkac I forgot to mention I released 2.x with this fix https://github.com/videojs/http-streaming/releases/tag/v2.16.3

MCab93 commented 4 months ago

@adrums86 @dzianis-dashkevich I am checking release of branch v2.16.3 https://github.com/videojs/http-streaming/actions/runs/7592761058/job/20682472936 - this build failed. When I use http-streaming v2.16.3 in my project the changes of this PR are not there. Can you please double check and make new release of 2x branch ?