-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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(mp4-tools): Fix two AV1 parsing issues #5774
Conversation
Thank you for the contribution @nyanmisaka! I'll review soon with the hopes of including these fixes in the next release. |
src/utils/mp4-tools.ts
Outdated
const monochrome = (av1CBox[2] & 0x10) >> 4; | ||
const chromaSubsamplingX = (av1CBox[2] & 0x08) >> 3; | ||
const chromaSubsamplingY = (av1CBox[2] & 0x04) >> 2; | ||
const chromaSamplePosition = av1CBox[2] & 0x03; | ||
codec += | ||
'.' + | ||
profile + | ||
'.' + | ||
addLeadingZero(level) + | ||
tierFlag + | ||
'.' + | ||
addLeadingZero(bitDepth) + | ||
'.' + | ||
monochrome + | ||
'.' + | ||
chromaSubsamplingX + | ||
chromaSubsamplingY + | ||
chromaSamplePosition; | ||
addLeadingZero(bitDepth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned that some user-agents may still depend these fields being present.
https://aomediacodec.github.io/av1-isobmff/#av1codecconfigurationbox-syntax
The parameters sample entry 4CC, profile, level, tier, and bitDepth are all mandatory fields. If any of these fields are empty, or not within their allowed range, the processing device SHOULD treat it as an error. All the other fields (including their leading '.') are optional, mutually inclusive (all or none) fields. If not specified then the values listed in the table below are assumed.
mono_chrome 0 chromaSubsampling 110 (4:2:0) colorPrimaries 01 (ITU-R BT.709) transferCharacteristics 01 (ITU-R BT.709) matrixCoefficients 01 (ITU-R BT.709) videoFullRangeFlag 0 (studio swing representation)
Would adding the default values defined above work for Firefox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nyanmisaka,
Eventually (not in this PR), we may need a workaround for incomplete AV1 codec strings in Multivariant Playlist CODECS sort of like the workaround for opus/flac strings:
Lines 132 to 172 in 6227816
function getCodecCompatibleNameLower( | |
lowerCaseCodec: LowerCaseCodecType, | |
preferManagedMediaSource = true, | |
): string { | |
if (CODEC_COMPATIBLE_NAMES[lowerCaseCodec]) { | |
return CODEC_COMPATIBLE_NAMES[lowerCaseCodec]!; | |
} | |
// Idealy fLaC and Opus would be first (spec-compliant) but | |
// some browsers will report that fLaC is supported then fail. | |
// see: https://bugs.chromium.org/p/chromium/issues/detail?id=1422728 | |
const codecsToCheck = { | |
flac: ['flac', 'fLaC', 'FLAC'], | |
opus: ['opus', 'Opus'], | |
}[lowerCaseCodec]; | |
for (let i = 0; i < codecsToCheck.length; i++) { | |
if ( | |
isCodecMediaSourceSupported( | |
codecsToCheck[i], | |
'audio', | |
preferManagedMediaSource, | |
) | |
) { | |
CODEC_COMPATIBLE_NAMES[lowerCaseCodec] = codecsToCheck[i]; | |
return codecsToCheck[i]; | |
} | |
} | |
return lowerCaseCodec; | |
} | |
const AUDIO_CODEC_REGEXP = /flac|opus/i; | |
export function getCodecCompatibleName( | |
codec: string, | |
preferManagedMediaSource = true, | |
): string { | |
return codec.replace(AUDIO_CODEC_REGEXP, (m) => | |
getCodecCompatibleNameLower( | |
m.toLowerCase() as LowerCaseCodecType, | |
preferManagedMediaSource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Currently AV1 is not popular yet in HLS.
But as Apple provides native support for AV1, this problem will become apparent.
def894d
to
05737e6
Compare
src/utils/mp4-tools.ts
Outdated
const monochrome = (av1CBox[2] & 0x10) >> 4; | ||
const chromaSubsamplingX = (av1CBox[2] & 0x08) >> 3; | ||
const chromaSubsamplingY = (av1CBox[2] & 0x04) >> 2; | ||
const chromaSamplePosition = av1CBox[2] & 0x03; | ||
codec += | ||
'.' + | ||
profile + | ||
'.' + | ||
addLeadingZero(level) + | ||
tierFlag + | ||
'.' + | ||
addLeadingZero(bitDepth) + | ||
'.' + | ||
monochrome + | ||
'.' + | ||
chromaSubsamplingX + | ||
chromaSubsamplingY + | ||
chromaSamplePosition; | ||
addLeadingZero(bitDepth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nyanmisaka,
Eventually (not in this PR), we may need a workaround for incomplete AV1 codec strings in Multivariant Playlist CODECS sort of like the workaround for opus/flac strings:
Lines 132 to 172 in 6227816
function getCodecCompatibleNameLower( | |
lowerCaseCodec: LowerCaseCodecType, | |
preferManagedMediaSource = true, | |
): string { | |
if (CODEC_COMPATIBLE_NAMES[lowerCaseCodec]) { | |
return CODEC_COMPATIBLE_NAMES[lowerCaseCodec]!; | |
} | |
// Idealy fLaC and Opus would be first (spec-compliant) but | |
// some browsers will report that fLaC is supported then fail. | |
// see: https://bugs.chromium.org/p/chromium/issues/detail?id=1422728 | |
const codecsToCheck = { | |
flac: ['flac', 'fLaC', 'FLAC'], | |
opus: ['opus', 'Opus'], | |
}[lowerCaseCodec]; | |
for (let i = 0; i < codecsToCheck.length; i++) { | |
if ( | |
isCodecMediaSourceSupported( | |
codecsToCheck[i], | |
'audio', | |
preferManagedMediaSource, | |
) | |
) { | |
CODEC_COMPATIBLE_NAMES[lowerCaseCodec] = codecsToCheck[i]; | |
return codecsToCheck[i]; | |
} | |
} | |
return lowerCaseCodec; | |
} | |
const AUDIO_CODEC_REGEXP = /flac|opus/i; | |
export function getCodecCompatibleName( | |
codec: string, | |
preferManagedMediaSource = true, | |
): string { | |
return codec.replace(AUDIO_CODEC_REGEXP, (m) => | |
getCodecCompatibleNameLower( | |
m.toLowerCase() as LowerCaseCodecType, | |
preferManagedMediaSource, |
Profile: unsigned right shift 5-bits for the 2nd av1CBox Level: bitwise AND the 2nd av1CBox with 0b11111(0x1f) Without this, Main 'av01.0.15M.08' is parsed as High 'av01.1.15M.08'. In this case the High profile is not supprted in Firefox. Signed-off-by: nyanmisaka <nst799610810@gmail.com>
In Firefox either you omit the chroma values, or you have to complete the rest of the color descriptions. Otherwise even the most basic AV1 Main 4:2:0 8-bit video will be reported as unsupported in HTMLMediaElement.canPlayType and MediaSource.isTypeSupported. Chromium is not affected by this. Signed-off-by: nyanmisaka <nst799610810@gmail.com>
05737e6
to
0d4cfde
Compare
This PR will...
Fix two AV1 parsing issues that make Firefox fails to play HLS stream
Why is this Pull Request needed?
fix(mp4-tools): Fix AV1 profile parsing
Profile: unsigned right shift 5-bits for the 2nd av1CBox
Level: bitwise AND the 2nd av1CBox with 0b11111(0x1f)
Without this, Main
av01.0.15M.08
is parsed as Highav01.1.15M.08
.In this case the High profile is not supprted in Firefox.
fix(mp4-tools): set default color descriptions to satisfy Firefox
In Firefox either you omit the chroma values, or you have to complete the rest of the color descriptions.
Otherwise even the most basic AV1 Main 4:2:0 8-bit video will be reported as unsupported in HTMLMediaElement.canPlayType and MediaSource.isTypeSupported. Chromium is not affected by this.
Are there any points in the code the reviewer needs to double check?
See also
Resolves issues:
av1 Main is mistakenly parsed as High profile and eventually fails in Firefox
playlist:
log:
Firefox is unhapply with the chroma values
Checklist