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

Multiple audio EXT-X-MEDIA with same language different format cause SampleQueueMappingException #7877

Closed
stevemayhew opened this issue Sep 9, 2020 · 16 comments
Assignees
Labels

Comments

@stevemayhew
Copy link
Contributor

Issue description

When an HLS playlist has multiple audio renditions with same language and different formats (example Stereo and Dolby Surround) ExoPlayer silently ignore one of the two (or, in the chunk-less prepare case, throws a SampleQueueMappingException.

Expected behavior is to show all the audio tracks, however The HLS metadata is not clear on how to make this work.

Note this issue is similar/related to issue #6210 and issue ##7329

Reproduction steps

I've supplied two playlists (same underlying content) in an email to ExoPlayer dev. More details on the playlists and the code area specific to the issue are comment on this commit e14a8457

[REQUIRED] Version of ExoPlayer being used

2.11.6

[REQUIRED] Device(s) and version(s) of Android being used

Proprietary Android TV STB running Android P

@AquilesCanta
Copy link
Contributor

Hi Steve, apologies for the delay in looking into this. I took some extra time because the problematic playlist (index-bug.m3u8) has an issue where the first stream in tag is followed by another STREAM-INF tag instead of a URL.

[...]
#EXT-X-MEDIA:URI="<redacted url>",TYPE=AUDIO,GROUP-ID="cinlab-e01.cdn.cbtops.net-multi-audio",LANGUAGE="eng",NAME="eng-Stereo-ac3",DEFAULT=NO,AUTOSELECT=NO
#EXT-X-STREAM-INF:BANDWIDTH=1624320,AVERAGE-BANDWIDTH=1306224,RESOLUTION=480x272,CODECS="mp4a.40.2,avc1.4D4015",AUDIO="cinlab-e01.cdn.cbtops.net-multi-audio"
#EXT-X-STREAM-INF:BANDWIDTH=1524320,AVERAGE-BANDWIDTH=1206224,RESOLUTION=480x272,CODECS="ac-3,mp4a.40.2,avc1.4D4015",AUDIO="cinlab-e01.cdn.cbtops.net-multi-audio"
<redacted url>
[...]

I removed the line locally and managed to reproduce the issue.

Regarding chunkless preparation I don't think we can do much about this type of playlists. The idea behind chunkless preparation is using solely master playlist info to prepare. But in the provided playlist the track types are ambiguous: Given an EXT-X-MEDIA-TAG, it's impossible for the player to know what track type we are dealing with (we know it's audio, but we don't know the codec) which renders track selection ineffective. So I think it's not really worth discussing further steps for chunkless preparation.

Regarding traditional preparation, there are a bunch of places in our implementation where we assume a single codec string for each track type. These assumptions are causing the misbehaviour you are observing. This is something we should fix on our side.

In general, I'd advise you structure your content so that each variant contains exactly one codec string per track type (like the first variant: CODECS="mp4a.40.2,avc1.4D4015"). The codec string is the only information that HLS gives us in the master playlist about the track type. When there are two, the player is not able to know the codec of the EXT-X-MEDIA tags. The player only knows it is audio, generally breaking track selection: ExoPlayer may pick a track with an unsupported codec, causing a playback failure.

In any case, if you still would like to use multi-audio-codec variants (legal, but I'd advise against this), please let me know so I can assess how urgent it is for us to fix this. Keep in mind that chunkless preparation will not work well in this schema.

@stevemayhew
Copy link
Contributor Author

One thing we could do is somehow mark the playlist as having "multi-audio-codec variants" during playlist parsing, maybe issue a warning about chunk less prepare not supporting this then disable any request for chunk less prepare (or suggest people use multiple "copies" of the variants with one audio codec per-copy.

As for fixing TrackOutput to correctly create multiple audio tracks for traditional prepare that is a larger issue to solve no doubt, if you want to keep this issue and the playlists for that be my guest.

We are ok with closing it too, as we want to use chunk less prepare (much faster channel change) and will advise our origin partners to code with only one codec of each type per variant.

@stevemayhew
Copy link
Contributor Author

@AquilesCanta the origin vendor is pushing back with this example from the RFC:

from 4.3.4.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.3.4.2) or EXT-X-I-FRAME-STREAM-
INF tag (Section 4.3.4.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
"ac-3,mp4a.40.2".

The bit about listing every sample format is the killer. Of course they would still want the track selection to name and select the track based on metadata from the EXT-X-MEDIA. This of course would not be possible with chunkless-prepare

The only way I see this working is if the code:

  1. detect the condition (ambiguous CODEC match) then, warn and disable chunk-less prepare (The code in buildAndPrepareMainSampleStreamWrapper could do this
  2. sniff the actual format of the EXT-X-MEDIA URL (this is already done), they would need unique names of course and create tracks for them

@AquilesCanta
Copy link
Contributor

I've already internally pushed:

  • A fix for the traditional preparation issue where audio is disabled.
  • A fix to disable chunkless preparation when the CODECS tag is ambiguous.

Unfortunately I've missed the window to get this in the next release (this week). However, it'd be nice for you to test the fixes as soon as they are in the dev branch. The commit message of the last patch is Avoid chunkless preparation if the codec mapping is ambiguous, so once that one is in the dev branch you can try it and reach back to confirm it's ok for you. Thanks for reporting.

ojw28 pushed a commit that referenced this issue Oct 23, 2020
When disabling a TsExtractor track type because of a missing codec
in the master playlist, look through the entire codecs string
instead of checking the first codec with matching type.

Issue: #7877
PiperOrigin-RevId: 338530046
ojw28 pushed a commit that referenced this issue Oct 23, 2020
This change fixes format creation for traditional preparation of streams
where the master playlist contains more than one codec string per track
type.

Issue: #7877
PiperOrigin-RevId: 338538693
@stevemayhew
Copy link
Contributor Author

stevemayhew commented Oct 27, 2020

Thanks @AquilesCanta this is timely, meeting with the origin vendor today and grateful to have a solution to provide them.

I was hoping to a add code in our solution to default chunk-less prepare to enabled then disable it internally in the HlsSampleStreamWrapper (with a warning log) when we detect one of these conditions:

  1. The CODECS tag is ambiguous (multiple CODECS for the same renderer type)
  2. There is no rendition for close-captions (EXT-X-MEDIA type=CLOSED-CAPTIONS)

We can require number 2 for our use-case for chunk-less because all of our streams originate from broadcast content and as such it is pretty much impossible for there not to be CCx embedded in-stream.

I'll look for and gather and cherry-pick all the patches into our release branch to test. Again thanks.

@AquilesCanta
Copy link
Contributor

when we detect one of these conditions:

  1. The CODECS tag is ambiguous (multiple CODECS for the same renderer type)
  2. There is no rendition for close-captions (EXT-X-MEDIA type=CLOSED-CAPTIONS)

Re 1. it will be available in dev branch 2 this week, maybe tomorrow. It's been merged internally since last week (as mentioned above, with the commit message Avoid chunkless preparation if the codec mapping is ambiguous).

Re 2. I think there are better solutions than using traditional preparation to get captions. One way I can think of is injecting the caption tag in the parsed playlist (assuming you cannot fix the master playlist). Just provide a playlist parser which wraps the default parser and puts there caption tags for whatever you like. This way you can keep using chunkless preparation. NOTE: this doesn't work if you want to rely solely on caption descriptors in the media.

Does this work for you?

@stevemayhew
Copy link
Contributor Author

Thanks, we will be looking for the commit and are ready to test it as soon as you have all the code changes public in dev-v2.

For 2., maybe, So the use case is really allowChunklessPreparation semantics change a bit... Currently, if allowChunklessPreparation is true, thenTrackGroups are created for, and only for:

  1. All video declared in CODECS on variants (this is a must anyway)
  2. Any audio declared in CODECS with or without MEDIA (un-muxed or muxed)
  3. Captions declared as instream via MEDIA
  4. Subtitles declared as a rendition

IMHO, once 1 and 2 are satisfied (basically buildAndPrepareMainSampleStreamWrapper() completed), it seems reasonable to trigger onPrepared() and begin playback (80/20 rule).

The thing is we would need subsequent tracks, not declared in the master playlist and, discovered in-stream to at least appear in the available tracks lists and possibly trigger track selection. Basically, considering this comment:

The following points apply to chunkless preparation:

  1. A muxed audio track will be exposed if the codecs list contain an audio entry and the master playlist either contains an EXT-X-MEDIA tag without the URI attribute or does not contain any EXT-X-MEDIA tag.
  2. Closed captions will only be exposed if they are declared by the master playlist.
  3. An ID3 track is exposed preemptively, in case the segments contain an ID3 track

We would like to un-do the assumption in 2. Exposing all CCx and SERVICEx captions with whatever metadata is in-stream

This is probably trickier than it looks, because of the timing of as the ExtractorOutput interface and TrackOuput interface relative to the preparation. I ran into a similar issue to this with the changes from this pull request (#7034). The extraction starts before the HlsMediaChunks are bound to SampleQueue's

Happy to help you with ideas and testing on this, but we would certainly like to use chuckless prepare (it shaves 30% off of channel changes for us) and still have in stream tracks that are detected later appear after prepare has already been signaled. I realize this is complex code with a long history, of which I've only been peripherally part of over the last couple of years.

@AquilesCanta
Copy link
Contributor

Adding tracks after preparation is something we've internally discussed in the past. It's tracked by #6124. The issue is that it does not only affect HLS chunkless preparation, but the entire ExoPlayer pipeline. I do have some ideas on how this could be implemented without disrupting the existing infrastructure too much but it requires changes in a number of components. Please track #6124 for that.

@stevemayhew
Copy link
Contributor Author

Seems like #6124 is trying to solve a much larger and more general problem at the player level. Certainly that requires messing with creating agregating MediaSource on the fly from a ProgressiveMediaSource. What I'm looking for is much simpler, is this::

  1. Closed captions will only be exposed if they are declared by the master playlist.

A requirement, that is something that is explicitly prevented, or just an artifact of how chunk-less prepare works? The ID3 track already appears on the fly, even for chunkless-prepare, this use case would allow the CC track to do the same. Note it is likely CC may not appear in the first few segments of a VOD asset anyway.

It's understood that the CC metadata (Language, Characteristics, etc) would need to come from the PMT metadata in the MPEG-TS in this case (as there is no EXT-X-MEDIA). Note, this is not much different than chuckless with the MEDIA tag, as the origin must extract the metadata from the stream in order to populate the EXT-X-MEDIA metadata.

@AquilesCanta
Copy link
Contributor

The ID3 track already appears on the fly, even for chunkless-prepare

I don't think this is accurate. We just preemptively declare the ID3 track.

this use case would allow the CC track to do the same

I don't think other users of chunkless preparation would be happy with the preemptive addition of 608 tracks (like we do in ID3). One option would be enabling preemptive declaration of 608 through the MediaSource builder, but that can be done without a huge extra complication for the app by decorating the parsed master playlist with a closed caption declaration, which is why I suggested it in this comment. ExoPlayer doesn't do "on the fly detection" for any type of tracks.

Aside, this discussion seems unrelated to the original post of this issue. I'd suggest filing a fresh issue with all the required context, explaining exactly what the proposed feature is.

@stevemayhew
Copy link
Contributor Author

Aside, this discussion seems unrelated to the original post of this issue. I'd suggest filing a fresh issue with all the required context, explaining exactly what the proposed feature is.

Agreed, this is more an RFE and orthogonal to the issue in this bug. I'll summarize the relevant comments and/or reference this issue for context. I do think people want to see the 608 tracks surfaced, with priority to what is declared in EXT-X-MEDIA (if anything). Because we are dealing with broadcast TV here in the US it is a federal requirement for these tracks to be present and presented to the viewer.

@stevemayhew
Copy link
Contributor Author

@AquilesCanta I see Oliver's commit to detect and turn off chunk less if the mapping is ambiguous, is the commit ready for surfacing the multiple audio tracks though? Even if you just have a patch we can test it, no need for it to be in dev-v2 as we will be back porting it to our release version (which is based from 2.11.6). Thanks!

@AquilesCanta
Copy link
Contributor

I'm not sure I understand the question @stevemayhew. If I remember correctly, the 3 changes referenced in this issue should be enough for playback of the discussed streams. Please try them out, and if your use case is not addressed, let me know.

@stevemayhew
Copy link
Contributor Author

stevemayhew commented Nov 9, 2020 via email

@stevemayhew
Copy link
Contributor Author

@AquilesCanta sorry again for the confusion. I merged and back-ported the two commits ccf78f9 and 4783c32 also I fixed the index-bug.m3u8 so it does not have the extra EXTINF, the changes work perfectly with this playlist.

So, I'm closing this bug out and I'll enter and RFE for the changes we want for captions and chunkless-prepare. I will link too this bug for our discussions above. We will test and start from @ojw28 's commit a184924 for this work.

Again thanks so much!

@stevemayhew
Copy link
Contributor Author

Works perfectly.

icbaker pushed a commit that referenced this issue Nov 30, 2020
When disabling a TsExtractor track type because of a missing codec
in the master playlist, look through the entire codecs string
instead of checking the first codec with matching type.

Issue: #7877
PiperOrigin-RevId: 338530046
icbaker pushed a commit that referenced this issue Nov 30, 2020
This change fixes format creation for traditional preparation of streams
where the master playlist contains more than one codec string per track
type.

Issue: #7877
PiperOrigin-RevId: 338538693
icbaker pushed a commit that referenced this issue Nov 30, 2020
@google google locked and limited conversation to collaborators Jan 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants