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

Add support for AC-3 audio in DVB streams #5636

Merged
merged 2 commits into from Jul 17, 2023

Conversation

softworkz
Copy link
Contributor

This PR will...

Add support for AC-3 audio in DVB streams

The newly added AC-3 decoding assumes embedding with stream_type 0x81 according to the ATSC standard.
In DVB streams, AC-3 audio is broadcast as stream_type 0x06 (which is used for multiple purposes) with an AC-3 descriptor of type 0x6A.

Why is this Pull Request needed?

Because AC-3 support is not complete yet.

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

No. The code is proven to work with the now-merged AC-3 PR for a long time.

@robwalch
Copy link
Collaborator

robwalch commented Jul 11, 2023

For HLS, AC3 in TS should be carried over stream 0x81 (unencrypted) and 0xC1 (encrypted / not supported). EC3 (Enhanced AC-3) use stream ids 0x87 (unencrypted) and 0xC2 (encrypted) and are not supported in HLS.js currently.

DVB carriage is not in HLS players.

@softworkz
Copy link
Contributor Author

For HLS, AC3 in TS should be carried over stream 0x81 (unencrypted) and 0xC1 (encrypted / not supported). EC3 (Enhanced AC-3) use stream ids 0x87 (unencrypted) and 0xC2 (encrypted) and are not supported in HLS.js currently.

DVB carriage is not in HLS players.

Where is that specified?

@softworkz
Copy link
Contributor Author

softworkz commented Jul 11, 2023

It's working

For HLS, AC3 in TS should be carried over stream 0x81 (unencrypted) and 0xC1 (encrypted / not supported). EC3 (Enhanced AC-3) use stream ids 0x87 (unencrypted) and 0xC2 (encrypted) and are not supported in HLS.js currently.
DVB carriage is not in HLS players.

Where is that specified?

Last time I read it, there was nothing like that specified, I just checked the latest version: https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis

Other players are supporting this as well (e.g. VLC, MPV, ExoPlayer). The actual streams are identical, it's just the different stream type id and a different descriptor. That's all.

@robwalch robwalch added this to the 1.5.0 milestone Jul 11, 2023
robwalch
robwalch previously approved these changes Jul 11, 2023
@softworkz
Copy link
Contributor Author

Oh, cool, thanks for approving.
Do I need to do anything (like rebase and update).?

@softworkz
Copy link
Contributor Author

Oh, cool, thanks for approving. Do I need to do anything (like rebase and update).?

Stupid me. Of course this conflicted after the merge of the other PR. Should be fixed now.

@softworkz
Copy link
Contributor Author

I almost forgot that I have a test stream for this: https://softworkz.github.io/hlstestfiles/ac3/media.m3u8

@robwalch
Copy link
Collaborator

robwalch commented Jul 13, 2023

I almost forgot that I have a test stream for this: https://softworkz.github.io/hlstestfiles/ac3/media.m3u8

In Safari getting an internal exception in parseAC3PES with this test. Can you take a look (length is not defined, and the intention seems to be data.length):

https://github.com/video-dev/hls.js/pull/5562/files#diff-4a467ddfe2310c97c29aac6fda61c534714123a1b68c7f198b0f8ddd0598c611R1012-R1013

In Chrome we log AC3 is not supported and then just do best effort and play the video. This is expected behavior, but not consistent with other support check failures. I don't think we'll change this. The workaround and expected solution is to provide a Multivariant Playlist with CODECS so that the player filters out variants with unsupported codecs.

@softworkz
Copy link
Contributor Author

In Safari getting an internal exception in parseAC3PES with this test. Can you take a look (length is not defined, and the intention seems to be data.length):

#5562 (files)

Correct, you accidentally dropped this line: https://github.com/video-dev/hls.js/pull/5562/files#diff-4a467ddfe2310c97c29aac6fda61c534714123a1b68c7f198b0f8ddd0598c611L1007

In Chrome we log AC3 is not supported and then just do best effort and play the video.

In Edge, AC3 is supported (both Chromium and legacy). After fixing the data.length bug and with my PR applied, the test stream is playing with audio in Edge.

@softworkz
Copy link
Contributor Author

In Edge, AC3 is supported (both Chromium and legacy).

Note: It might depend on the installed audio device or driver, I'm not exactly sure whether it has changed. Earlier, it was only working when you had an audio device connected which supports AC3, e.g. a TV with HDMI or an AV Receiver connected with SP/DIF, or a multi-channel (surround sound) audio device. But today it even worked with a Bluetooth stereo headphone, so this might have changed.

@softworkz
Copy link
Contributor Author

In Chrome we log AC3 is not supported and then just do best effort and play the video. This is expected behavior, but not consistent with other support check failures. I don't think we'll change this. The workaround and expected solution is to provide a Multivariant Playlist with CODECS so that the player filters out variants with unsupported codecs.

I'm planning to do another PR which prepares for dealing with multiple audio streams (even of the same codecs). The first (small) step would be to build a list of all audio streams when reading the PMT (instead of stopping once the first is found) and make the choice afterwards - but no different behavior yet.
Would that sound reasonable to you?

@robwalch
Copy link
Collaborator

I'm planning to do another PR which prepares for dealing with multiple audio streams (even of the same codecs). The first (small) step would be to build a list of all audio streams when reading the PMT (instead of stopping once the first is found) and make the choice afterwards - but no different behavior yet.
Would that sound reasonable to you?

Would all this happen in the demuxer? That wouldn't be too bad. I image if the tracks and data needed to be bubbled up further before making the choice, then this would be a more significant change.

@softworkz
Copy link
Contributor Author

Would all this happen in the demuxer? That wouldn't be too bad. I image if the tracks and data needed to be bubbled up further before making the choice, then this would be a more significant change.

I'm open for any advice and discussion. I thought this (small) step would be the starting point in any case, and when it's there it will be easier to talk about how to make use of it.
I remember that we had an earlier discussion about this. The most trivial and minimal use case would be to allow specifying a desired audio PID to be used for playback. But more would be possible, like matching audio streams by codec or language.

@softworkz
Copy link
Contributor Author

I've submitted a PR to fix the regression above: #5673

@robwalch robwalch merged commit c36d220 into video-dev:master Jul 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants