Support group-based fallback to backup/redundant tracks for subtitles (like we did for audio) #2838
Conversation
… level and set initial track accordingly
… handling and complete docs
…at we set the level to the matching group ID
…all go to Level class long-term)
(making this an actual unit test)
in certain switching scenarious like for fallback we apparently get loaded segments after the details have been switched. this passes the above checks because it is the same level index (redundant URL)
|
@mtoczko Update: I do acknowledge and experience now clearly there is something wrong, I can reproduce your report. Sorry for the confusion on bug reproduction and having you have to record a screen video. I guess more formal bug report and repro steps as well as providing the content link for running here local instead of via netlify would have helped me. |
…fallback to urlId = 0 in case of no redundant levels existing at all (various media groups may be used nevertheless for each unique level) + centralize trigger LEVEL_LOADING event, flatten _setLevelInternal to allow less ambiguious control flow and deduplication of logic/LOCs
|
Hi @mtoczko :) The above latest commit fixes the error you have reported and it will now switch the audio-tracks as expected. Please see the commit message or ask if you want further infos on what the fix is about. I have managed to flatten some redundant code in level-controller and make things more clear, but essentially it was about falling back to 0 when evaluating the url-id on audio/sub track switch listeners. Your test data is great, and demos another case of multiple media-groups per media-type than the one we have. As your case is not about having actually redundant levels, but however different audio/subs media representations tied specifically to each quality level. You can now probably manage this at least properly in a manual way. Our handling we added here was based on assuming redundant levels, and ensure the fallback logic to allow consistent media-groups matched when switching to a backup level. For your case to be handled automatically (so that the level loaded will select the tracks matching the group-id) we need to add a little bit more handling on top of what we already have here. It means we need to generalize further over the case of redundant levels only. However, it will be very similar and use a lot of common ground to what we have built here, in fact it should be just a few lines to add in audio/subtitle-track-controller now to really get this right :) I would actualy go as far as saying, not handling your case is a bug atm in HLS.js still :) For example, if I set with your content I would also recommend for your test-data to end up in our VoD test-streams hosted by Mux. |
|
TODOs: Functionality:
Testing:
|
|
Hi @tchakabam ;) Resolution:
|
docs/API.md
Outdated
| ## Group-ID Aware Media Selection API | ||
|
|
||
| Using these methods is required in order to manage track selection in the context of backup/redundant media. Setting the track via `audioTracks` or `subtitlesTrack` in this case, can lead to switching to for example a failover group unintentionaly, which would cause an immediate level switch and all other media tracks to fallback to the then new active media groups of this level. In this context, track selection should only be done via the media properties (which *can* exist redundantly across groups of same media type, and *should* in case of redundant failover levels), to avoid the said effects causing unnecessary rebuffering instead of seamless switching. |
There was a problem hiding this comment.
I've been looking at this feature in more depth and have come to the conclusion that exposing audio and subtitle groups not belonging to the active level is a bug.
When hls.js changes to a redundant level (level.urlId > 0), if the redundant level uses a different audio or subs group, we must change to that group. That works as expected with audio. I haven't tested subtitles yet, but that's really the most important change, and ideally would have been where we start.
Next up is the question: Should we expose the other groups and/or allow changing to them if they are not associated with the current level? To both I say no. By limiting hls.js to the active audio and subs groups we solve both problems. (#2972)
It would be great if this PR could address these issues this way. That is how I would like it to work in v1, and starting here would be great. The API changes add an necessary workaround for behavior that we do not need to maintain as-is. They also complicate things by diverging the direction of v0 with the the vision for v1 and more importantly cross-compatibility with Safari. Safari only lists audioTracks and textTracks associated with the current level set.
In an effort to provide direction for completing this body work, after addressing the bugs and test streams in the TODOs above, please remove the new API methods, and make the audio and subtitle track controllers emit track events that reflect their active groups.
tchakabam
changed the title
|
Here is new test data. This one does work on Safari/Quicktime and allows to validate against the fallback logic active there as well - @robwalch, it also passed mediastreamvalidator except for target-duration, but that wont hinder you from testing with it. Image and CCs are tagged with their group identifiers, audio is mixed in with respective tones:
Master playlist with primary-main media 404ing, Safari native HLS will select all media-groups belonging to backup "redundant" variant upon fallback:
We will also add respective cases for individual or combined failures of audio/subtitles alternate media playlists. It might be a bit slow on startup as bitrate is quite high in parts and this isn't a CDN hosting also. But once cached should be fine. |
|
@mtoczko the problem you report only appears when switching text-tracks through the native video element controls (native text-track events), but not when using the Hls.js API That it is not working via native text-tracks is expected in the current state, and relates to what @robwalch was mentioning regarding native text tracks support harmed by the fix we did on the event handling happening when we are toggling the modes from within the subtitle-track-controller. So, this last issue you report is/was expected from our side so far. This is the only remaining fix needed to fix here now. And we will provide a fix for this by now. As for how the API exposes the media i.e tracks that are in the master playlist, I think should be thought through, with either things that have been said by me or Rob here on this PR. What @mtoczko suggesting on the ticket #2972, that the tracks should not appear is imo by no means a requirement or a necessity, or even a "bug". This API has worked like this since HLS.js supports alternate media playlists. And it makes total sense. Changing that may harm/break existing integrations. What needed to be fixed clearly from our perspective and product needs (and how @mtoczko also points it out in the ticket report) is that the groups should play aligned to the main media selected and the audio/subs groups that it has. This is what this now fixes for subtitles. (we already had provided the patch done for audio almost 2 years ago) As we will have fixed the native text-tracks events issue remaining, from our side this is complete. Nothing else needs to be done. We will remove the API addition, but not change the behavior of existing API. Everyone is free to propose or fork changes in any other direction if they have a need for it. This is the bare feature from a pure logical point of view providing what the HLS format implies. We hope that the project can make good use of it. But we can not help it if in the finished state it wont, after having provided all necessary elements for that. |
native text-track event handler into toggleTrackModes, w/o breaking native UI controls support.
by audio/subtitle track group switch. necessary to comply with failover behavior spec on alternate media playlist failure to have all main/media stay coherent group-wise.
|
@mtoczko 7a7a0d1 here above now fixes the issue mentioned before. With the last commit this also now passes (again,since we rebased this whole feature from 2 years ago on current master) our tests when an alternate media playlist is failing (404ing, with URL changed to wrong path for example to test), immitating a failure in a redundant encoder setup of the audio or subtitles components. This now works with either audio or subtitles media failing (tested based on test-data setup above). If the "main" level playlist itself fails, obviously it will also switch to redundant, and will (as it should) switch all media to the groups of this level. This can be tested with this setup: https://emliri.com/hls-backup-groups-test/mg/master-primary-main-failure.m3u8 In all of these cases HLS.js should (and with this PR will) play the redundant media level, and with that correctly the media-groups referenced in it. This fixes the part of #2972 that overlaps with our own requirements. |
|
To whomever wants to merge this @ video-dev, feel free to squash these commits into a single one, or rebase it in any other way you would find helpful (as long as meaningful author attribution is conserved i guess). Also, of course they might be tastes and ideas about how to implement the functionality (like for example latest changes done on level-controller around url-id mutation), and anyone should feel free to adapt that if they feel like it, on top of this patch. The paramount being the test-cases needing to pass, and other side-effects to be avoided. To sum up: This now fulfills certain product requirements (specificaly for primary/backup redundant live encoding setups in practice, even if our test here are VOD for convenience) in handling failover to redundant media, for main variants and alternate media tracks, general expectations on HLS client media-group handling as to coherent selection across media types (see ticket mentioned above). This passes all of our tests, as well as the cases of other people, which can be reproduced here with the setup we provide. Unit tests have been added likewise around substantial logic required for this feature. I am providing this as-is in a take it or leave it state at this point. I think it is probably a good idea for the project to merge this for it does fix a general behavior requirements on HLS media-groups handling. Wether the API should expose only actively played groups or not is a totally different question. Some food for thought on that: If the API was to only expose the active group tracks, how would one intentionaly switch to a different group-set i.e level? If I understand well, the case of @mtoczko specifically is not one of redundant encodings, but one where one would want to switch across different groups potentially, for their media-variant parameters. The HLS media-group semantic is not one that only allows redundant media per-se,that is only a way to express that, but there are other case, so the API has to stay general in that regard, and allow either way users to manually select any group to be active. How can that be done,if it only exposes the "active" group? In that case, some sort of selection has to be allowed anyhow. So far so good, I have probably tried to explain this often enough, and have delivered what was needed in terms of production requirements, and I dont want to do anything further on this, while the communication has been a bit tiring around these topics exactly. |
There was a problem hiding this comment.
Hi @tchakabam,
See the review comments below. In summary:
- Please avoid passing
changedUrlIdthroughstartLoadand_setLevelInternal - Remove
_setPreferredMediaOptions - Revert
export class PlaylistLoaderchange from a default export - Remove
this._subtitleGroupId === nullcondition from_selectInitialSubtitleTrack - Describe why it is important for level-controller to failover a level in response to audio and subtitle track switches, rather than prevent track change based on the active level. (More on clarifying this point below)
Thank you for the new test stream. Please consider using this one which includes multiple bitrate variants and languages:
https://playertest.longtailvideo.com/adaptive/elephants_dream_v4/redundant.m3u8
I request that you review pr #3293. Please consider the API changes and concerns with regards to failover as it relates to this PR. It attempts to fix #2972 and then implement redundant failover with level based group-id/track selection. It is on top of "feature/v1.0.0" which is close to being merged into master and beta release. If that happens first, we can move this PR to a patch branch.
Upon testing this PR with elephants_dream_v4/redundant.m3u8, I found that the selected subtitle language was not maintained after level failover. I select captions in Chinese, French, then block emliri.com/hls-backup-groups-test/mg/1/*. The playlists all failover, but the active subtitleTrack switches back to Chinese. Please investigate and address the requests above.
In regards to your last comment to switch to a different group-set, the level (or level.urlId*) must be changed. *Changing the level urlId also involves clearing level.details, and triggering a level change logic. The API does not provide an easy way to do this (other than removeLevel which is destructive). I think I've seen (or least heard of) a feature request for just picking alternate redundancy (issue #??). I do not think changing tracks should be the expected mechanism. I would like group changing to be the responsibility of the level-controller and a result of a level (urlId) change. The level-controller should have no knowledge of active tracks which is the responsibility of the track controllers. The level-controller could perform error handling for track content in the active group so that it can perform failover in scenarios where audio or subtitles error before the main stream content does.
| return; | ||
| } | ||
|
|
||
| if (changedUrlId || this.currentLevelIndex !== newLevelIdx) { |
There was a problem hiding this comment.
Rather than pass changedUrlId through so many methods, why not reset this.currentLevelIndex when the urlId changes? Then, when we get here, if this.currentLevelIndex has been reset to -1, the levels can be switched.
| currentLevel.urlId = urlId; | ||
| this.startLoad(); | ||
| this.startLoad(true); |
There was a problem hiding this comment.
This would be the point where you could set this.currentLevelIndex = -1 so that you don't need to pass a new parameter in to this.startLoad() > _setLevelInternal ()
There was a problem hiding this comment.
Why do we allow a track switch to change the level urlId?
Shouldn't level switching determine track group id, not the other way around?
| import { MediaType, PlaylistLevelType } from './types/loader'; | ||
| import { MediaPlaylist } from './types/media-playlist'; | ||
|
|
||
| function _setPreferredMediaOptions (mediaType: MediaType, name: string, language: string | null = null) { |
There was a problem hiding this comment.
This method doesn't appear to be used anywhere. Please remove if it is unused.
| @@ -23,7 +23,7 @@ const { performance } = window; | |||
| /** | |||
| * @constructor | |||
| */ | |||
| class PlaylistLoader extends EventHandler { | |||
| export class PlaylistLoader extends EventHandler { | |||
There was a problem hiding this comment.
Please revert module export change.
| let selectedTrack = null; | ||
|
|
||
| const tracks = this.tracks.filter( | ||
| (track) => this._subtitleGroupId === null || track.groupId === this._subtitleGroupId |
There was a problem hiding this comment.
If there is no selected subtitle group-id, we should not select any subtitle track. Either returning early above or filtering out all tracks should be acceptable:
| (track) => this._subtitleGroupId === null || track.groupId === this._subtitleGroupId | |
| (track) => track.groupId === this._subtitleGroupId |
This method is being called in onLevelLoaded after _subtitleGroupId is set so this shouldn't be a problem (GROUP-IDs are required on variant levels and media playlist tracks https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-4.4.6.1)
| this.timer = null; | ||
| } | ||
| } | ||
|
|
||
| startLoad () { | ||
| startLoad (changedUrlId = false) { |
There was a problem hiding this comment.
passing in changedUrlId can be avoided by setting currentLevelIndex to -1 (see related comments for details).
| } | ||
| if (urlId !== currentLevel.urlId) { | ||
| currentLevel.urlId = urlId; |
There was a problem hiding this comment.
Same question:
Why do we allow a track switch to change the level urlId?
Tracks must have a group-id. Only tracks with a group-id matching the selected variant/level should be selectable. We should not allow selection of tracks that don't match the variant group as the level or STEAM-INF defines CODECS used in the group.
https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-4.4.6.1.1
@itsjamie Support is already implemented in v1. This was kept open in case someone wanted to pick up LTS work and cut v0.15. Bugs and feedback were not addressed so this could not be merged and blocked release of v0.15. |
robwalch
moved this from Top priorities
to Fixed in v1.0, Unreleased in v0.15 (on hold)
in Release Planning and Backlog
|
Understanding!
…On Wed, Jun 16, 2021 at 3:25 PM Rob Walch ***@***.***> wrote:
I would expect that this should be retargeted and merged against the
current 1.0 source.
@itsjamie <https://github.com/itsjamie> Support is already implemented in
v1. This was kept open in case someone wanted to pick up LTS work and cut
v0.15. Bugs and feedback were not addressed so this could not be merged and
blocked release of v0.15.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2838 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO5VKMC3YU5SMAJYD67PQTTTDUBHANCNFSM4OIIPJPQ>
.
|
This PR will...
This means it will allow for having encoder groups for video/audio/subs and when one live encoder in a group fails (chunk playlist update HTTP error code or timeout), we will switch all media playlists to the other group.
This has already been done by use and merged to this master a while ago for AUDIO. Now we add support for SUBTITLES.
Add an API to allow set preferences for media options (language of audio/subs) independent of the backup encoder redundancy groups that we have switched. This allows for a UI to display options independent of redundant track groups.It is a meaningful consequence of the functionality we add support for, in order to be able to switch tracks based on the actual language options but independent of selecting the specific media. It will make sure that the preferred option is fulfilled within the currently used redundant tracks group.Why is this Pull Request needed?
ZDF uses redundants live encoding infrastructure, which allows for switching to the backup streams if either fails in production.
EDIT:
Quoting @robwalch
Resolves issues:
#2972 (only regarding correct group selection across media types)
Checklist