Skip to content
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

Merged
merged 7 commits into from
Mar 6, 2023

Conversation

FernandoGarciaDiez
Copy link
Contributor

@FernandoGarciaDiez FernandoGarciaDiez commented Feb 6, 2023

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

This should fix shaka-project#4634 and possibly shaka-project#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.
@google-cla
Copy link

google-cla bot commented Feb 6, 2023

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.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Incremental code coverage: 7.69%

@avelad
Copy link
Collaborator

avelad commented Feb 6, 2023

@FernandoGarciaDiez Please, sign the CLA. Thanks!

@FernandoGarciaDiez
Copy link
Contributor Author

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)

@joeyparrish
Copy link
Member

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.

@avelad avelad added type: bug Something isn't working correctly status: waiting on response Waiting on a response from the reporter(s) of the issue platform: Tizen Issues affecting Tizen priority: P2 Smaller impact or easy workaround labels Feb 11, 2023
@FernandoGarciaDiez
Copy link
Contributor Author

FernandoGarciaDiez commented Feb 15, 2023

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.

@FernandoGarciaDiez
Copy link
Contributor Author

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?

@avelad
Copy link
Collaborator

avelad commented Feb 23, 2023

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

@joeyparrish
Copy link
Member

why was is so tailored for the video component just in the cast platform?

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.

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

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.
@avelad avelad removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Mar 2, 2023
lib/polyfill/media_capabilities.js Outdated Show resolved Hide resolved
@avelad
Copy link
Collaborator

avelad commented Mar 3, 2023

I'll run the GitHub tests and the lab.

@avelad avelad added this to the v4.4 milestone Mar 3, 2023
@joeyparrish joeyparrish merged commit eb01c60 into shaka-project:main Mar 6, 2023
@FernandoGarciaDiez FernandoGarciaDiez deleted the tizenVideoError branch March 7, 2023 08:32
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform: Tizen Issues affecting Tizen priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
4 participants