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

bugfix: Check if codec is supported by browser in setCodecs #4408

Conversation

oscnord
Copy link

@oscnord oscnord commented Nov 3, 2021

This PR will...

Fix an issue where audio-tracks that includes an unsupported codec (EC-3) would be added if the GROUP-ID is shared between multiple #EXT-X-MEDIA:TYPE=AUDIO tags.

Example:

#EXTM3U
#EXT-X-VERSION:3
#EXT-X-INDEPENDENT-SEGMENTS
#EXT-X-STREAM-INF:BANDWIDTH=10477987,AVERAGE-BANDWIDTH=5719832,RESOLUTION=1600x900,FRAME-RATE=24.000,CODECS="avc1.4D4028,mp4a.40.2",AUDIO="audio_0"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_1.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=10689065,AVERAGE-BANDWIDTH=5930910,RESOLUTION=1600x900,FRAME-RATE=24.000,CODECS="avc1.4D4028,ec-3,mp4a.40.2",AUDIO="audio_1"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_1.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=6929827,AVERAGE-BANDWIDTH=4108712,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,mp4a.40.2",AUDIO="audio_0"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_2.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=7140905,AVERAGE-BANDWIDTH=4319790,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,ec-3,mp4a.40.2",AUDIO="audio_1"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_2.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=4609531,AVERAGE-BANDWIDTH=3082828,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,mp4a.40.2",AUDIO="audio_0"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_0",CHANNELS="2",NAME="Stereo",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_7_0.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="6",NAME="Surround",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_8_0.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="2",NAME="Stereo",LANGUAGE="eng",DEFAULT=NO,AUTOSELECT=NO,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_7_0.m3u8"

Why is this Pull Request needed?

This PR updates the filtering in setCodecs() so that it utilises isCodecSupportedInMp4 to check if the codec in question is supported by the browser. If it is not supported the current quality level is ignored.

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

Resolves issues:

Resolves an issue where it would be possible to play audio-tracks that includes multiple codecs where one or more could be unsupported by the browser.

Checklist

  • 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

@oscnord oscnord changed the title check if codec is supported by browser in setCodecs() bugfix: Check if codec is supported by browser in setCodecs Nov 8, 2021
@mtoczko
Copy link
Collaborator

mtoczko commented Nov 8, 2021

@oscnord Your manifest passes without errors in mediastreamvalidator?
Seem to me, it may contain errors. Below is an example from appla (AAC, AC-3, EC-3)
https://devstreaming-cdn.apple.com/videos/streaming/examples/img_bipbop_adv_example_ts/master.m3u8

@oscnord
Copy link
Author

oscnord commented Nov 10, 2021

@oscnord Your manifest passes without errors in mediastreamvalidator?
Seem to me, it may contain errors. Below is an example from appla (AAC, AC-3, EC-3)
https://devstreaming-cdn.apple.com/videos/streaming/examples/img_bipbop_adv_example_ts/master.m3u8

The manifest is generated by AWS MediaPackage and the source is transcoded with MediaConvert so the manifest should be valid. Mediastreamvalidator doesn't throw any errors related to that the GROUP-ID is shared between multiple audio-tracks/codecs.

@mtoczko
Copy link
Collaborator

mtoczko commented Nov 10, 2021

@oscnord
I have not found in the documentation that a group can have 2 codecs. Always in examples is one group = one codec.

The manifest is generated by AWS MediaPackage and the source is transcoded with MediaConvert so the manifest should be valid.

It all depends on the configuration. As you add an additional group for EC-3 it should work properly.

BTW: The group must have the same number of members.

@bwallberg
Copy link
Contributor

@mtoczko
https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis#section-4.4.6.1.1

   Each member in a Group of Renditions MAY have a different sample
   format.  For example, an English Rendition can be encoded with AC-3
   5.1 while a Spanish Rendition is encoded with AAC stereo.  However,
   any EXT-X-STREAM-INF tag (Section 4.4.6.2) or EXT-X-I-FRAME-STREAM-
   INF tag (Section 4.4.6.3) that references such a Group MUST have a
   CODECS attribute that lists every sample format present in any
   Rendition in the Group, or client playback failures can occur.  In
   the example above, the CODECS attribute would include

@mtoczko
Copy link
Collaborator

mtoczko commented Nov 12, 2021

@bwallberg Thank you

Going back to the example above:

   A Playlist MAY contain multiple Groups of the same TYPE in order to
   provide multiple encodings of that media type.  If it does so, each
   Group of the same TYPE MUST have the same set of members, and each
   corresponding member MUST have identical attributes with the
   exception of the URI and CHANNELS attributes.

This configuration is incorrect:

#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_0",CHANNELS="2",NAME="Stereo",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_7_0.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="6",NAME="Surround",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_8_0.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="2",NAME="Stereo",LANGUAGE="eng",DEFAULT=NO,AUTOSELECT=NO,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_7_0.m3u8"

Correctly

#EXT-X-STREAM-INF:BANDWIDTH=6929827,AVERAGE-BANDWIDTH=4108712,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,mp4a.40.2",AUDIO="audio_0"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_2.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=7140905,AVERAGE-BANDWIDTH=4319790,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,ec-3",AUDIO="audio_1"
bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_2.m3u8


#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_0",CHANNELS="2",NAME="English",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_7_0.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="6",NAME="English",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="bc56f20744d3442dbc1d9d5951516ab6/42ddaab73a98422fae55af4655be136a/manifest_8_0.m3u8"

or (ec-3,mp4a.40.2)

#EXT-X-STREAM-INF:BANDWIDTH=7140905,AVERAGE-BANDWIDTH=4319790,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,mp4a.40.2,ec-3",AUDIO="audio_0"
#EXT-X-STREAM-INF:BANDWIDTH=7140905,AVERAGE-BANDWIDTH=4319790,RESOLUTION=1280x720,FRAME-RATE=24.000,CODECS="avc1.4D401F,mp4a.40.2 ",AUDIO="audio_1"

#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_0",CHANNELS="2",NAME="English",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="..."
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_0",CHANNELS="6",NAME="Polski",LANGUAGE="pl",DEFAULT=NO,AUTOSELECT=YES,URI="..."


#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="2",NAME="English",LANGUAGE="eng",DEFAULT=YES,AUTOSELECT=YES,URI="..."
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio_1",CHANNELS="2",NAME="Polski",LANGUAGE="pl",DEFAULT=NO,AUTOSELECT=YES,URI="..."

@bwallberg
Copy link
Contributor

@mtoczko

I see your point, I don't necessarily read the specc the same way though but that's neither here nor there.

Regardless this solution is required for the last example you showed regardless ( and would work with the "incorrect" configuration as well ).

@mtoczko
Copy link
Collaborator

mtoczko commented Nov 12, 2021

@bwallberg
I agree with you that a solution is needed.
Just missing a valid sample stream for verification of this patch.

@bwallberg
Copy link
Contributor

@mtoczko

That's definitely reasonable :) ( I also agree that this variant of the stream is unusual, so a multi-language variant would make a better verification stream regardless ).

@oscnord
Copy link
Author

oscnord commented Nov 16, 2021

@mtoczko Here are two test streams. The first contains two renditions where one have two audio-tracks (stereo and surround) and the other one only have one audio track (stereo). The second test stream have two renditions with stereo and surround.

  1. https://www.oscarnord.com/test-stream/elephants-dream-mp-tracks/Elephants_Dream.m3u8
  2. https://www.oscarnord.com/test-stream/elephants-dream/Elephants_Dream.m3u8

@oscnord oscnord closed this Feb 15, 2022
@oscnord oscnord reopened this Feb 23, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setCodecs is intended to reflect what is in the manifest only. Filtering is not done there. It is done in the level-controller inonManifestLoaded based on set codecs for each variant (or "level").

If this is still an issue in v1.2.4 or higher please file an issue with a sample stream that reproduces the issue.

@robwalch robwalch closed this Nov 4, 2022
@robwalch robwalch removed this from the 1.5.0 milestone Nov 4, 2022
littlespex pushed a commit to cbsinteractive/hls.js that referenced this pull request Dec 9, 2022
Introduced in video-dev#4189, as a side-effect of restricting tracks when a
network failure occurs.  We should not trigger such restrictions when
the browser is known to be offline.

Closes video-dev#4408
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants