hls.js icon indicating copy to clipboard operation
hls.js copied to clipboard

Add FLAC support

Open jprjr opened this issue 3 years ago • 9 comments

This PR will...

Add FLAC to the passthrough remuxer codecs - this allows Chrome to decode the audio, previously the SourceBuffer would be registered with the codec mp4a.40.5 (HE-AAC) and throw an error when it encounters FLAC.

This also adds FLAC support to the m3u8 parser and the stream controller.

Why is this Pull Request needed?

This should enable FLAC in fragmented mp4.

See https://developer.apple.com/documentation/http_live_streaming/http_live_streaming_hls_authoring_specification_for_apple_devices - which lists FLAC as a supported codec.

Are there any points in the code the reviewer needs to double check?

I'm unsure if there's a case where parsedCodec would equal flac (all lowercase). In the initialization segment's Sample Description Box, the codec is listed as fLaC.

The changes in passthrough-remuxer.ts seem to be enough for FLAC to work on Chrome on Desktop. The remaining changes are for Chrome on Android.

In stream-controller.ts, I'm unsure when audioCodecSwitch will be true. From reading the code, it sounds like some browsers don't support switching the codec type? I'm unsure which browsers those are, so I'm unsure what would happen to those browsers when trying to play FLAC audio.

Resolves issues:

See #4387 (at least in Chrome - I'm unsure what scenarios use the MP4 generator/remuxer)

Checklist

  • [X] changes have been done against master branch, and PR does not conflict
  • [ ] new unit / functional tests have been added (whenever applicable)
  • [ ] API or design changes are documented in API.md

Misc

Just thought I should mention what I've tested on:

  • Linux (Arch Linux, latest everything):

    • Firefox 102.0 (64-bit)
    • Chrome Version 103.0.5060.53 (Official Build) (64-bit)
  • MacOS 11.6.6 (Big Sur)

    • Safari Version 15.5 (16613.2.7.1.9, 16613)
    • Chrome Version 103.0.5060.53 (Official Build) (x86_64)
    • Firefox 102.0 (64-bit)
  • Android

    • Chrome (103.0.5060.71)
    • Firefox 102.1.1

jprjr avatar Jul 05 '22 18:07 jprjr

One thing I'm unsure of how to handle - I can't find a good source on what CODECS should equal in a multivariant playlist with FLAC.

Everything seems to work correctly with a lowercase flac (CODECS="flac"). Apple's mediastreamvalidator tool seems to want the uppercase FLAC. Adding FLAC to codecs.ts doesn't really work, since then checking for the codec support fails, it seems like most browers return false for MediaSource.isTypeSupported("audio/mp4;codecs=FLAC"); - Safari is the only one I've found that returns true for that check (it also supports using lowercase). So I think some bigger changes would need to be done to map FLAC into flac?

UPDATE

After doing some research - figured I should provide a few notes on the CODECS parameter:

  • Apple's tools want FLAC in a multivariant playlist. I haven't tested playing a FLAC stream natively in Safari, just with this patched HLS.js, which seems to work fine with the lowercase flac.
  • In developing the FLAC-in-MP4 mapping, the FLAC devs used fLaC as the fourcc in the Sample Description Box (fLaC is the same marker they use in the FLAC bitstream).
  • RFC 6381 defers to the MPEG4 Registration Authority for what codecs are allowed in the mp4 mimetypes, the MPEG4 Registration Authority does not list FLAC.
  • Every browser that I've tried (admittedly not that many, see above) that supports FLAC supports using the codec name in all lowercase in the "codecs" parameter of the mimetype (audio/mp4; codecs="flac").

The codec name listed by the MPEG4 Registration Authority has to match the fourcc in the Sample Description box, so it seems like there's an unresolved issue.

If the MPEG4-RA adopts "flac" as the fourcc, then the FLAC-in-MP4 spec will need to be updated, and tools that produce FLAC-in-MP4 files updated as well, and many existing files rendered obsolete.

If instead the MPEG4-RA adopts "fLaC" as the fourcc, the existing specs and tools don't need an update, but browsers should add support for it in the codecs parameter.

This is spread out amongst a few issues:

https://github.com/ietf-wg-cellar/flac-specification/issues/112

https://github.com/w3c/media-source/issues/188

https://github.com/xiph/flac/issues/38

jprjr avatar Jul 05 '22 21:07 jprjr

Update - the MP4 registration authority adopted fLaC as the FOURCC yesterday - https://github.com/mp4ra/mp4ra.github.io/pull/147

I've opened bugs with Firefox and Chromium about the MediaType.isSupported() not working with the now-proper codec name:

https://bugs.chromium.org/p/chromium/issues/detail?id=1350534

https://bugzilla.mozilla.org/show_bug.cgi?id=1783453

Safari works with fLaC, it seems like it's case-insensitive.

jprjr avatar Aug 05 '22 18:08 jprjr

Safari works with fLaC, it seems like it's case-insensitive.

FourCCs are always case sensitive fLaC != flac.

fLaC = 0x664C6143 while lowercase would be flac = 0x666C6163. Applications should be updated and use registered fourCCs. Please file a bug so we can verify, track and fix it.

podborski avatar Aug 05 '22 22:08 podborski

Correct. Sorry, I should clarify - Safari seems to handle the fourcc in a case-insensitive manner. This isn't correct behavior but it means it accepts "fLaC"

I opened bugs with Firefox and chromium for rejecting "fLaC".

On August 5, 2022 6:40:14 PM EDT, Dimitri Podborski @.***> wrote:

Safari works with fLaC, it seems like it's case-insensitive.

FourCCs are always case sensitive fLaC != flac.

fLaC = 0x664C6143 while lowercase would be flac = 0x666C6163. Applications should be updated and use registered fourCCs

-- Reply to this email directly or view it on GitHub: https://github.com/video-dev/hls.js/pull/4772#issuecomment-1207070339 You are receiving this because you authored the thread.

Message ID: @.***>

jprjr avatar Aug 06 '22 00:08 jprjr

@podborski sorry, didn't see the edit about filing a bug (and honestly didn't realize you probably meant with Safari until I checked your GitHub profile). Where do I file that?

Even though it works it'd be good to get it registered and on Apple's radar. Also for non-safari products, I know the hls verification tools are looking for the string "FLAC" in multivariant playlists, which I think is incorrect.

jprjr avatar Aug 06 '22 13:08 jprjr

@podborski sorry, didn't see the edit about filing a bug (and honestly didn't realize you probably meant with Safari until I checked your GitHub profile). Where do I file that?

Even though it works it'd be good to get it registered and on Apple's radar. Also for non-safari products, I know the hls verification tools are looking for the string "FLAC" in multivariant playlists, which I think is incorrect.

I already filed a radar yesterday. Thanks for checking :)

podborski avatar Aug 06 '22 15:08 podborski

Hi there! Just to be clear, what's the recommendation for the CODECS in the main m3u8 playlist moving forward? fLaC? I'm more concerned about future compatibility here than existing.

toots avatar Aug 18 '22 19:08 toots

@toots I believe fLaC is the most correct value for the CODECS parameter.

RFC 8216 and the latest draft for HTTP Live Streaming 2nd Edition have this to say for EXT-X-STREAM-INF:

CODECS

The value is a quoted-string containing a comma-separated list of formats, where each format specifies a media sample type that is present in one or more Renditions specified by the Variant Stream. Valid format identifiers are those in the ISO Base Media File Format Name Space defined by "The 'Codecs' and 'Profiles' Parameters for "Bucket" Media Types" [RFC6381].

RFC 6381, under "ISO Base Media File Format Name Space" states:

For the ISO Base Media File Format, and the QuickTime movie file format, the first element of a 'codecs' parameter value is a sample description entry four-character code as registered by the MP4 Registration Authority [MP4RA]. Values are case sensitive.

And the MP4 Registration Authority now lists fLaC.

jprjr avatar Aug 23 '22 12:08 jprjr

@toots I believe fLaC is the most correct value for the CODECS parameter.

RFC 8216 and the latest draft for HTTP Live Streaming 2nd Edition have this to say for EXT-X-STREAM-INF:

CODECS The value is a quoted-string containing a comma-separated list of formats, where each format specifies a media sample type that is present in one or more Renditions specified by the Variant Stream. Valid format identifiers are those in the ISO Base Media File Format Name Space defined by "The 'Codecs' and 'Profiles' Parameters for "Bucket" Media Types" [RFC6381].

RFC 6381, under "ISO Base Media File Format Name Space" states:

For the ISO Base Media File Format, and the QuickTime movie file format, the first element of a 'codecs' parameter value is a sample description entry four-character code as registered by the MP4 Registration Authority [MP4RA]. Values are case sensitive.

And the MP4 Registration Authority now lists fLaC.

Thanks!

toots avatar Aug 23 '22 13:08 toots

Hi @robwalch - I redid this change and pushed it up.

Basically, this adds some logic for checking if the browser will use the MP4 Registration Authority's listed codecs (fLaC and Opus), and if not - remap codec strings into whatever the browser supports.

This allows for manifest playlists to signal the FLAC codec using either flac, fLaC, or FLAC (in previous testing, Apple would recommend FLAC as the codec string in their HLS validation tools, so this may be out there in the wild). The level controller will remap these strings as needed.

I also did the same for Opus since it has basically the same issue - most browsers added Opus support and used the lowercase string opus, even though the MP4 Registration Authority lists Opus.

The main difference in this attempt vs the previous is, I'm keeping the changes in the level controller more minimal. I'm not sure what should happen if say, Safari on an iPhone encounters a manifest with multiple codecs, since Safari isn't able to change the codec type on the fly.

jprjr avatar Nov 14 '22 20:11 jprjr

Hi @robwalch - new attempt pushed, rather than performing codec name changes in the worker, it's performed just before creating buffers / changing types / etc, using String.replace. It touches a lot less areas than the previous attempt.

jprjr avatar Nov 15 '22 14:11 jprjr

Hi @robwalch - I refactored a bit based on your feedback -

So, since in buffer-controller it's possible for the trackName to be audiovideo, video, or audio - I check if the trackName contains audio.

And since it's possible for the codec string to be something like fLaC,avc1.42e01e (this is what I see when using a fragmented mp4 with audio and video in the segments), I figure using a regex-based replace makes the most sense (instead of checking for equality). I do that in level-controller as well, for consistency.

I believe using string.replace will maintain the goal of only calling isCodecSupportedInMp4 when necessary - it should only occur when the regular expression has a match.

I wrote a small wrapper function around the original getCodecCompatibleName just so I'm not having to declare anonymous functions and perform type-casting in the controllers, I figure it's easier to read:

codec = codec.replace(AUDIO_CODEC_REGEXP, getCodecCompatibleName);

as opposed to

codec = codec.replace(AUDIO_CODEC_REGEXP, (m) => getCodecCompatibleName(m.toLowerCase() as "flac" | "opus"));

One question, should the regular expression be more strict? It's doing a case-insensitive match, would it make sense to have it only match expected strings like /flac|fLaC|FLAC|Opus|opus/ ?

Or alternatively, maybe getCodecCompatible name should be a wrapper around replace and have the regex defined in that module, rather than duplicated in the controllers? So the controller code would be something like:

codec = getCodecCompatibleName(codec);

and in the codecs utility:

const AUDIO_CODEC_REGEXP  = /flac|opus/gi;
export function getCodecCompatibleName(codec: string) : string {
  return codec.replace(AUDIO_CODEC_REGEXP, (m) => getCodecCompatibleNameLower(m.toLowerCase() as LowerCaseCodecType));
}

jprjr avatar Nov 16 '22 14:11 jprjr

One question, should the regular expression be more strict? It's doing a case-insensitive match, would it make sense to have it only match expected strings like /flac|fLaC|FLAC|Opus|opus/ ?

No. /i is what we're going for.

robwalch avatar Nov 16 '22 18:11 robwalch

Hi @robwalch - I made getCodecCompatibleName into a wrapper that calls .replace with the regex, so the regex is only defined in a single place. Also, removed the /g modifier since its not needed.

jprjr avatar Nov 16 '22 18:11 jprjr

Hey, I was testing out this branch and it was working great until Chrome (on Windows) pushed out an update this week. Now it just keeps failing with Hls.ErrorTypes.MEDIA_ERROR and reloading forever while trying to play a flac stream until it finally gives up. Might want to force Chrome to update and make sure it still works. It's possible the issue is Chrome getting more strict on CORS or something and it's on my end, but so far I've been unable to isolate the issue.

Whatever happened is definitely associated with the Chrome update. Worked fine on a different system, then I updated Chrome (by going to Settings/About Chrome), and it failed. If you need a test case our stream is https://d3d4yli4hf5bmh.cloudfront.net/hls/live.m3u8

fkane avatar Mar 10 '23 11:03 fkane

I got an email notification on this issue (but I don't see the comment here on GitHub for it?) that Chrome may have updated the logic for MediaSource.isTypeSupported to support the string fLaC - but may not have updated the code around creating a SourceBuffer (it still expects the string flac).

There's a bug report open on Chromium but they're probably going to have a hard time triaging it - it mentions HLS.js and doesn't provide a minimal viable example of the issue.

I'll try to draft up an example that only uses the needed APIs to 1) verify that this is the case, and 2) demonstrate the discrepancy.

jprjr avatar Mar 10 '23 12:03 jprjr

I got an email notification on this issue (but I don't see the comment here on GitHub for it?) that Chrome may have updated the logic for MediaSource.isTypeSupported to support the string fLaC - but may not have updated the code around creating a SourceBuffer (it still expects the string flac).

There's a bug report open on Chromium but they're probably going to have a hard time triaging it - it mentions HLS.js and doesn't provide a minimal viable example of the issue.

I'll try to draft up an example that only uses the needed APIs to 1) verify that this is the case, and 2) demonstrate the discrepancy.

Hello, yeah I have no idea where my comment went. 😕

This is definitely the case as I made a patch that forced the SourceBuffer to be created with the lowercase variants and it worked fine.

Thank you for your help with the MVE, I'm not very well-versed with the MSE API.

BlakeB415 avatar Mar 11 '23 11:03 BlakeB415

I created a small demo page that confirms the issue is around creating SourceBuffers with "fLaC" as the codecs string and updated the Chrome issue.

My demo page is available at https://jprjr.github.io/chrome-issue-1422728/ - it demos trying to load FLAC and Opus using the different codec string names. I think the issue is Chrome is normalizing "fLaC" to "flac" internally, then is unable to find a matching SourceBuffer when trying to append?

A quick work-around may be changing the order that codecs are probed, to use "flac" and "opus" first. This can be done by modifying src/utils/codecs.ts, and changing the codecsToCheck variable in the getCodecCompatibleNameLower function:

https://github.com/video-dev/hls.js/pull/4772/files#diff-26dddffbfd2d74eaa0cd0bba75aaa37a3030af52b2fec25477179fb050a74ddbR106-R109

Change to:

  const codecsToCheck = {
    flac: ['flac', 'fLaC', 'FLAC'],
    opus: ['opus', 'Opus'],
  }[lowerCaseCodec];

I say "may" because I haven't actually tested it, this is just a suggestion as a temporary fix for anybody that needs this to work ASAP. I'm fairly confident that would work, though.

The most correct thing is for Chrome to fix the issue, not to change the probe order.

jprjr avatar Mar 12 '23 14:03 jprjr

Thank you for digging into this! I can confirm that changing the probe order does work around the issue for now.

fkane avatar Mar 12 '23 16:03 fkane

Hi @jprjr ,

The FLAC support feature has not yet been merged into the hls.js master, and I am wondering how I can test this using some sample .flac URLs. Specifically, I need to play this format file in the Lightning video player plugin. I know how to play m3u8 URLs by attaching hls.js to the Lightning player.

However, I am facing an issue where I am unable to import the source branch [jprjr:fmp4-flac] into the local repo package.json, which generally maintains all dependencies. I have tried running npm i, but it did not work. Moreover, I cannot find any demo player to insert the .flac URL and check this out.

Can anyone help me with this issue?

Thank you.

Akshay151248 avatar Mar 14 '23 07:03 Akshay151248

@BlakeB415 Haven't seen any movement on the Chromium issue, it wouldn't surprise me if it's stuck in some kind of "waiting for more info from the bug reporter" state, it may be worth leaving a response confirming my comment is a correct summary of your issue.

jprjr avatar Mar 26 '23 19:03 jprjr

Does anybody have a contact with the Chromium project? This bug hasn't had an update from the Chromium team since March 9 and it's still an issue in stable Chrome/Chromium. Try visiting this demo page - the media type is reported as supported but it doesn't actually work.

One work-around is to change the probe order (see https://github.com/video-dev/hls.js/pull/4772#issuecomment-1465212606) to prefer the non-compliant flac and opus strings but that doesn't seem ideal.

jprjr avatar May 30 '23 12:05 jprjr