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

bugfix: Check if codec is supported by browser in `setCodecs`

Open oscnord opened this issue 4 years ago • 10 comments

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

  • [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

oscnord avatar Nov 03 '21 16:11 oscnord

@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

mtoczko avatar Nov 08 '21 23:11 mtoczko

@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.

oscnord avatar Nov 10 '21 14:11 oscnord

@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.

mtoczko avatar Nov 10 '21 19:11 mtoczko

@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

bwallberg avatar Nov 12 '21 06:11 bwallberg

@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="..."

mtoczko avatar Nov 12 '21 08:11 mtoczko

@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 ).

bwallberg avatar Nov 12 '21 10:11 bwallberg

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

mtoczko avatar Nov 12 '21 10:11 mtoczko

@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 ).

bwallberg avatar Nov 12 '21 11:11 bwallberg

@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 avatar Nov 16 '21 13:11 oscnord

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

stale[bot] avatar Apr 16 '22 11:04 stale[bot]