Fixed codecs to mimetype conversion to take into account all possible scenarios #1099
Conversation
imbcmdth
force-pushed
the
codec-string-fix
branch
from
0a9766f to
522cf45
Compare
src/master-playlist-controller.js
Outdated
| @@ -14,6 +14,13 @@ import Decrypter from './decrypter-worker'; | |||
|
|
|||
| let Hls; | |||
|
|
|||
| // Default codec parameters if non were provided for video and/or audio | |||
src/master-playlist-controller.js
Outdated
| // Default codec parameters if non were provided for video and/or audio | ||
| const defaultCodecs = { | ||
| videoCodec: 'avc1', | ||
| videoObjectTypeIndicator: '.4d400d', |
There was a problem hiding this comment.
Might be worth adding a comment why this is default
There was a problem hiding this comment.
I'm not sure there is a reason other than "This was a common configuration when we started writing this project..." which is a crappy comment 🥇
src/master-playlist-controller.js
Outdated
| }; | ||
|
|
||
| const getContainerType = function(media) { | ||
| // An initialization segment means the media playlists is an iframe |
src/master-playlist-controller.js
Outdated
|
|
||
| if (!media) { | ||
| // not enough information, return an error | ||
| // Not enough information, return an error |
There was a problem hiding this comment.
Since this is being modified, it's not really returning an error. Might be worth leaving it at // Not enough information
src/master-playlist-controller.js
Outdated
| // HLS with multiple-audio tracks must always get an audio codec. | ||
| // Put another way, there is no way to have a video-only multiple-audio HLS! | ||
| if (isMaat && !codecInfo.audioProfile) { | ||
| codecInfo.audioProfile = defaultCodecs.audioProfile; |
There was a problem hiding this comment.
Should we log a warning or info here? Although I agree that we have to default to some kind of codec, the codec should be specified in the manifest in a case like this, and not including it is potentially dangerous.
There was a problem hiding this comment.
I don't know.. It seems like a fairly common configuration with MAAT unfortunately.. I add a warning log.
src/master-playlist-controller.js
Outdated
| ]; | ||
| } | ||
| previousGroup = audioGroup[groupId]; | ||
| return [ | ||
| bothVideoAudio, |
There was a problem hiding this comment.
If someone provides the CODECS attribute, but only sets the audio codec, then bothVideoAudio could end up with something like: 'video/mp2t; codecs=mp4a.40.2'. Would that be valid for when we need to play both video and audio?
EDIT: Noticed there is a test for this case, so I'm assuming it's valid, but just wanted to make sure.
| @@ -19,6 +19,40 @@ import { Hls } from '../src/videojs-contrib-hls'; | |||
| /* eslint-enable no-unused-vars */ | |||
| import Playlist from '../src/playlist'; | |||
|
|
|||
| const generateMedia = function(isMaat, isMuxed, hasVideoCodec, hasAudioCodec) { | |||
| const codec = (hasVideoCodec ? 'avc1.4d400d' : '') + | |||
There was a problem hiding this comment.
Would be good to use different codecs from the defaults when testing provided codecs
|
We should also have tests for |
imbcmdth
force-pushed
the
codec-string-fix
branch
from
d4f1b3c to
15531cb
Compare
|
|
||
| assert.deepEqual(mimeTypesForPlaylist_.apply(null, | ||
| generateMedia(true, false, false, true, isFMP4)), | ||
| [`${videoMime}; codecs="mp4a.40.E"`, |
There was a problem hiding this comment.
This still seems so odd to me.
| // types | ||
| if (previousGroup && previousGroup.uri) { | ||
|
|
||
| // If there is ano video codec at all, always just return a single |
Description
There are 4 possible predicates that influence how to generate mimetypes from codec strings:
This PR adds complete unit testing of all 24 possible combinations and fixes issues around the function that caused audio-only streams with multiple audio tracks present.
Requirements Checklist