Add unsupported codecs detection #2164
Conversation
…ction returns codec from the specific representation. If not, it returns codec of the first representation
…hen a codec is unsupported.
|
I am going to pull this PR local and test against the known issue listed in last release for Axinom content. |
src/streaming/Stream.js
Outdated
|
|
||
| // Filter codecs that are not supported | ||
| // But keep at least codec from lowest representation | ||
| i = 1; |
There was a problem hiding this comment.
What do you think if we do the same but using filter method of the array? I think that could help to better understand it.
Ex:
realAdaptation.Representation_asArray.filter((_, i) => {
if (i === 0) return true;
codec = dashManifestModel.getCodec(realAdaptation, i);
if (codec && !capabilities.supportsCodec(VideoModel(context).getInstance().getElement(), codec)) {
log('[Stream] codec not supported: ' + codec);
return false;
}
return true;
})
There was a problem hiding this comment.
Hi @epiclabsDASH ,
indeed, you can add a commit with this modification. The filter method is already used in other parts of the code : not a problem for me! ;-)
Nicolas
There was a problem hiding this comment.
Sure, no problem. Just my thought but happy to add the commit myself.
By the way, thanks for the PR, it is really useful.
There was a problem hiding this comment.
To add this was tested and works well. @epiclabsDASH I will leave it to you to commit filter and merge. I can test again if you need it, just ping
src/streaming/Stream.js
Outdated
| @@ -430,6 +434,26 @@ function Stream(config) { | |||
| } | |||
| } | |||
|
|
|||
| function filterCodecs(type) { | |||
| const realAdaptation = dashManifestModel.getAdaptationForType(manifestModel.getValue(), streamInfo.index, type, streamInfo); | |||
| let codec; | |||
There was a problem hiding this comment.
codec could be moved inside the .filter method and declared const?
There was a problem hiding this comment.
Absolutely. Honestly, I thought about that but limited myself to avoid touching too much...
@bbcrddave, thanks for the comment.
epiclabsDASH
force-pushed
the
addCodecsUnsupportedDetection
branch
from
b4dd714
to
c7a8d2b
Compare
nicosang
force-pushed
the
addCodecsUnsupportedDetection
branch
from
c7a8d2b
to
b4dd714
Compare
|
Hi all, as suggested by @bbcrddave in issue #2167, the call to canPlay function is replaced by isTypeSupported function. Nicolas |
|
We need to take care with this change. The only disadvantage I see on using isTypeSupported is it is supported by less browsers/devices/versions. So, it is ok moving to isTypeSupported, as pointed out by @bbcrddave seems more appropriated, but makes sense keeping canPlay as a "fallback" for cases where isTypeSupported is not available. We also need to do some testing to be sure isTypeSupported is not raising false negatives. I remember in the past there were issues in Chrome/Firefox with the implementation of this method and I am afraid this is going to affect us. |
|
In my tests everything was fine with isTypeSupported. Any other thought? Otherwise seems like a good change to be merged. |
Hi,
as explained in issue #1643, only codec of the first representation is verified by dash.js. Unfortunately, the others codec are not tested. In this PR, when an unsupported codec is detected, the representation is deleted from the adaptation.
Nicolas