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

[HLSTree] Reworked EXT-X-STREAM-INF parsing #1342

Closed
wants to merge 3 commits into from

Conversation

CastagnaIT
Copy link
Collaborator

@CastagnaIT CastagnaIT commented Jul 27, 2023

Description

While testing i found over time old HLS parsing problems not solved yet:

  • On manifest with multi-codec video streams the representations mixes all codecs together, this was causing problems with the "Ask quality" stream selector (show mixed codecs) and adaptive streaming, because each adaptation set must have a single codec type as dash specs
  • With manifests that have EXT-X-STREAM-INF variants with video streams but also variants with only audio (without video), these variants was wrongly added as a video stream causing broken playback on all sense, this can be confirmed by using "Ask quality" stream selector where wrong video streams are shown with lack of metadata

Related to this will be fixed #1259 since now EXT-X-STREAM-INF variants with only audio are now correctly handled

Has been full removed the ExtGroup struct because media groups are now parsed when required by variants itself

Since merge adaptation sets with a separate method should not needed anymore, has been removed in full the merging code on AdaptiveTree::SortTree
This also revert/remove the code to urn:mpeg:dash:adaptation-set-switching:2016 introduced by PR #694

EDIT: After some more investigations i choosed to reintroduce "adaptation sets merging" method but for dash only, the reason behind is due to amazon manifests that appears to have uncommon adaptation sets with same data, for this reason i have add a new test case AdaptionSetMerge so to not forgot in future this peculiarity.
AdaptiveTree::SortTree method now does exactly what it describes and does not merge adpsets,
the adaptation set merging code that include also urn:mpeg:dash:adaptation-set-switching:2016 introduced by PR #694 has been moved to a separate method in dash parser, but i am somewhat puzzled as to how it was implemented because:

  • wipe out adaptation sets data
  • for example on this manifest SwitchingManifestExample.txt there are different protection levels i may be wrong but it would be good to keep this and other possible different data

Moving this code i have also limited the merging code to adaptation sets with same codec, currently mixing more codecs to the same adaptation set can cause broken playback, because we have no way to determine supported codecs in advance and also currently there is no way to fallback if playback dont works with a specific codec

How has this been tested?

https://demo.unified-streaming.com/k8s/features/stable/video/tears-of-steel/tears-of-steel-hoh-subs.ism/.m3u8

and HLS audio stream from
https://stream.rtl.lu/live/hls/radio/rtllx

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • I have read the Contributing document
  • My code follows the Code Guidelines of this project
  • My change requires a change to the Wiki documentation
  • I have updated the documentation accordingly

@CastagnaIT CastagnaIT added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality Component: HLS v21 Omega labels Jul 27, 2023
@CastagnaIT CastagnaIT marked this pull request as ready for review July 31, 2023 07:30
@CastagnaIT CastagnaIT requested a review from glennguy July 31, 2023 07:41
@CastagnaIT CastagnaIT added the WIP label Aug 2, 2023
@CastagnaIT
Copy link
Collaborator Author

i found a problem with manifest like this
https://storage.googleapis.com/shaka-demo-assets/apple-advanced-stream-ts/master.m3u8
here EXT-X-MEDIA are declared after and not before EXT-X-STREAM-INF this now cause a wrong parsing

this because we have a linear text parsing by line,
instead we should have an additional middle step where we store data grouped by tags or a middle struct, after that parse the data, in this way doesnt matter if some tag is declared before or after another one

this require a full rework of CHLSTree::ParseManifest

@CastagnaIT CastagnaIT closed this Aug 3, 2023
@CastagnaIT CastagnaIT deleted the hls_streaminf_rework branch August 3, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: HLS Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality v21 Omega WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inputstream adaptive can not play hls without video
1 participant