-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: Tizen video error fixed by checking the extended MIME type #4973
fix: Tizen video error fixed by checking the extended MIME type #4973
Conversation
This should fix #4634 and possibly #4503 Some specific content was throwing 3016 error in some Tizen TV's, while playback worked fine in older versions of shaka. In these cases, one of the variants from the asset was not supported on these TV's, the lack of support was correctly detected in older versions, the variant was filtered out, and playback worked. In newer versions it was incorrectly identified as supported, and the playback error happened when we tried to play it. The difference comes from checking the extended MIME type, including info on the width, height, framerate, bitrate and channels as well. In the specific case that was failing, the variant was rejected if we included the width, and accepted if we didn't include it.
…ded type With the previous commit 4f3152d I had lost the conversion of codecs and mimeType that is done in some cases at getFullOrConvertedType function. This change's purpose is to convert the values first and then use those converted values as the input to get the extended type, using it to check if it is supported or not.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Incremental code coverage: 7.69% |
@FernandoGarciaDiez Please, sign the CLA. Thanks! |
I did already, but I think that as it is a corporate CLA from a new corporation, it can take some days until it's processed (https://opensource.google/documentation/reference/cla#cla-accepted) |
I can see the logic of this, however, resolution constraints are meant to be checked through MediaCapabilities. Is MediaCapabilities implemented on this model of Tizen, or is our polyfill being used instead? If it's the polyfill, let's make this change at that level (building the extended MIME type there). If it's a broken native implementation, let's still make the change at the level of the polyfill, but let's force its use on Tizen. |
I see what you mean, yes, the polyfill is installed and I see how that's where the fix should be, as it is not taking into account the resolution. |
As I'm working on this, I have realized that this issue is pretty much the same as #4726 which was done exclusively for the Cast platform. @JulianDomingo since you worked on that issue some months ago, why was is so tailored for the video component just in the cast platform? Do you think there would be any problem in checking the extended mime type parameters in all platforms and both for video and audio? Or would it be better to add some similar code only for tizen video? |
Only for Tizen, because there are some caches with https://github.com/shaka-project/shaka-player/blob/main/lib/polyfill/media_capabilities.js#L124, and the extended mime type produces many new possibilities |
In general, these "extended MIME types" are non-standard, and the parameters are ignored by most platforms. Cast is an exception, though I hope to see the platform embrace the web-standard way through the MediaCapabilities API.
Also, this. 😁 |
…supported Tizen platform can reject some videos due to their resolution, framerate or bitrate, so we should take into account all these parameters when we check if a video is supported.
I'll run the GitHub tests and the lab. |
We have reproduced an issue in some tizen tv's, and we think it's the cause of #4634 and possibly #4503
It happens with some streams that have 3 variants, 2 of them are supported on these TVs, but the one with higher resolution isn't. This variant uses this video: video/mp4;codecs="avc1.64002a";framerate="25";bitrate="6000000";width="1980";height="1080" and in this TV the higher resolution that is supported is 1920x1080
At filterManifestByMediaCapabilities function from stream_utils.js we were checking
Capabilities.isTypeSupported(fullType)
, where this "fullType" only includes the mime type and the codecs (video/mp4; codecs="avc1.64002a"
for example). This is currently returning true, so the player believes that this variant is supported, but then it fails when it tries to play it.However, there was an older implementation that used shaka.media.MediaSourceEngine.isStreamSupported() function which internally checks
Capabilities.isTypeSupported(extendedMimeType)
, where "extendedMimeType" includes also the width, height, bitrate and framerate (for example,video/mp4;codecs="avc1.64002a";framerate="25";bitrate="6000000";width="1980";height="1080"
), which in this specific case returns false because in this case the 1980 width is not supported (it should be 1920), the variant is filtered out, and the playback is started with the other 2 variants.In this proposed change I'm basically changing isTypeSupported(fullType) by isTypeSupported(extendedMimeType), keeping in mind that the codecs and mimeType could have been modified in the call to getFullOrConvertedType and we must keep using those modified values.
In this change I haven't modified the mutiplexed streams case (the case from the
if (video.codecs.includes(','))
at filterManifestByMediaCapabilities), because the getExtendedType function gets the whole variant as parameter, I don't have any good way of testing the case, and also I think that the audio tracks are less likely to be affected by this issue, so it's probably not that important.Fixes #4634