[AdaptiveStream] Rework to improve HLS manifest updates #1459

CastagnaIT · 2024-01-21T17:52:12Z

Description

While was developing this i found many weirdness that i think are not new bugs,
most of these bugs can be reproduced by using "Stream selection" setting to "Test" by switching per 1 segment.
That could happens for example on live stream, that when switch quality kodi call OpenStream, but when AdaptiveStream::start_stream is called, dont find the next segment because not yet available and stop the playback wrongly....

One single time, i catched two kodi freeze/crash, when played "akmai" local stream:
1 case when doing video seek, two times, in fast way on the "same" point (with mouse)
1 case when stop playback
more likely i think are already existings bugs

PR Changes for live streams:

HLS: Fix problem of playback stops after some time
HLS: Fix playback stop with adaptive stream switching (not fully but much better than before)
HLS: RAI live buzz sound mentioned in issue Broken HLS AES-128 decryption and live audio buzzy #1132 seem not happens anymore, and instead of buzz sound the stream pause for in right way buffering, but the workaround its still needed, to avoid better the buffer loading
HLS: Fix child manifest subtitles still in download also when are disabled
HLS: Fix subtitles segments not downloaded from new manifest updates (only show subtitles for first secs/segments) related to Subtitles from German live broadcasters are not displayed in Kodi 20 (Nexus) #1109

Unsolved bug was already existing:
if you play a HLS live stream with subtitles that are disabled,
when switch to a new period (DEMUX_SPECIALID_STREAMCHANGE) the subtitle stream will be re-enabled, this cause error
GenerateSidxSegments: [AS-20] Cannot generate segments
due to not downloaded child manifest

Unsolved bug i think was already existing:
Chapter change (DEMUX_SPECIALID_STREAMCHANGE) may be executed 2 times! by audio and video stream, i think due to slight differrent pts, could be?!, not easy to catch but its happened to me 1 or 2 times
idk if kodi expect one single chapter change?

~~tomorrow i will comment some code parts... to give some details on code changes~~ done

Motivation and context

I have lost 2 weeks of nights on this also because its a big mess from old messy implementations and things that from my part i find difficult to learn how works

i think however that what i have done here should be better of what was there before and after estenuants tests seem almost stable, at least without hard use of adaptive streaming quality switching (on my tests always: "Test" mode per 1 segment)

fix #1438

this will fix also subtitles segment problem of #1109
where subtitles was displayed only for the first segments only and then stopped

How has this been tested?

For multiperiod/chapters/ads:
Live Pluto TV i cant post the master manifest m3u8 link because expire after hours, but can be found on network flow of:
https://pluto.tv/en/live-tv/

http://mcdn.daserste.de/daserste/de/master.m3u8 (also for subs, not always there are news/tvshow)

"Akmai local stream"

Other live, no chapters:
https://cdndai.pl/tvp1hd/index.m3u8 (sometime the server may temporary ban your IP for ~30minuts)

RAI:
#KODIPROP:inputstream.adaptive.live_delay=30
https://mediapolis.rai.it/relinker/relinkerServlet.htm?cont=2606803&output=7

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 · 2024-01-22T08:22:25Z

src/common/AdaptiveStream.cpp

@@ -720,15 +722,32 @@ bool AdaptiveStream::start_stream(uint64_t startPts)
            current_rep_->get_segment(static_cast<size_t>(segmentId - current_rep_->GetStartNumber()));
      }
    }
+    else if (stream_changed_) // switching streams, align new stream segment no.


I have lost most of the time due to this method AdaptiveStream::start_stream
because i was not aware that the AdaptiveStream::start_stream method is used to adjust/set the "current segment" and so all my changes that i was doing on ensure_segment always was failing.
After discovered this, i realized that was missing set "current segment" for a stream quality switching case

CastagnaIT · 2024-01-22T08:27:21Z

src/common/AdaptiveStream.cpp

+    //! @todo: THIS MUST BE CHANGED - !! BUG !!
+    //! this will broken/stop playback on live streams when adaptive stream change stream quality (so representation)
+    //! and there are no new segments because not immediately available (despite child manifest updated),
+    //! because we cant be ensure next segment without wait the appropriate timing...imo its not the right place here.
+    //! Can be reproduced with HLS live and by using "Stream selection" setting to "Test" by switching per 1 segment


I also found this problem (read todo) i tried force remove this block of code that stop playback, and a bit below avoid set currentPTSOffset_ when there is no next_segment, playback works but show weird pts on Kodi GUI the value of time becomes sometimes negative and it can lead to weirdness in the long run.
Need to be changed on other way but idk atm, can be done separately, maybe can you help here

yes the scenario makes sense why it fails. We could try a hacky workaround until something better comes along - assume when the next segment comes it will be the same size (usually is) and add that PTS to current segment's start PTS. Sound good or not?

i understood but as you said its hacky im not 100% sure that could be so good

i found this problem even when trying to fix bugs to make this live sample playable
https://livesim.dashif.org/livesim/periods_20/testpic_2s/Manifest.mpd

This live sample have periods but no new segments provided,
then its required the use of "InsertLiveSegment" in conjuction with the manifest update worker.
This sample somewhat works on kodi <=v20 because of old code hacks and periods seem "ignored",
on kodi 21 is ofc broken, from what i see seem there are multiple problems on CDashTree::RefreshLiveSegments that seem dont take in account of "inserted" segments on timeline, delete them all and so seem to loose the last inserted segment with the updated pts (because you have to take in account that the downloaded updated manifest dont provide new segments), this will make problems with AdaptiveStream::ensureSegment that its not able to find next segment, AdaptiveStream::start_stream have problem when switch period, and also with CSession::OnSegmentChangedRead that its not able to insert new segments anymore
(these problems may be also similar with #1461)

CastagnaIT · 2024-01-22T08:33:16Z

src/common/AdaptiveStream.cpp

-      if (newRep->SegmentTimeline().IsEmpty())
-        GenerateSidxSegments(newRep);
-
-      if (!newRep->IsPrepared() && m_tree->SecondsSinceRepUpdate(newRep) > 1)


On representation class IsPrepared and m_isDownloaded plus the SecondsSinceRepUpdate imo was really a mess of behaviours, i have removed them all, and add a new IsNeedsUpdates, used only internally in the HLSParser to distinguish whether a representation needs an update of some thing (in this case for live updates)

CastagnaIT · 2024-01-22T08:35:33Z

src/common/AdaptiveStream.cpp

-      return !checkTime ||
-             (current_adp_->GetStreamType() != StreamType::VIDEO &&
-              current_adp_->GetStreamType() != StreamType::AUDIO) ||
-             SecondsSinceUpdate() < 1;


SecondsSinceUpdate uses (now removed) above and here should be the cause of the RAI buzz/frezze sound, by debugging this waitingForSegment method, was producing messed up return values

CastagnaIT · 2024-01-22T08:36:55Z

src/common/AdaptiveStream.cpp

+
+    // We can fall here when we are waiting for new segments, but the download it not started yet
+    // so return true only when checkTime is false, to avoid trigger EOS on sample reader and stop playback
+    return !checkTime;


This is the main cause because the Pluto TV stream was stopping playback (before was returning "false")

CastagnaIT · 2024-01-22T08:39:30Z

src/common/AdaptiveStream.cpp

+  // Disable representation only after stopped the worker
+  // otherwise if read some segments may invalidate this change
+  if (current_rep_)
+    current_rep_->SetIsEnabled(false);


Here the problem should be due to AdaptiveStream::ensureSegment
since force change SetIsEnabled on the representation and race condition may happens

src/common/Representation.h

CastagnaIT · 2024-01-22T08:42:51Z

src/parser/DASHTree.cpp

@@ -1552,7 +1552,6 @@ void adaptive::CDashTree::RefreshSegments(PLAYLIST::CPeriod* period,
 {
  if (adp->GetStreamType() == StreamType::VIDEO || adp->GetStreamType() == StreamType::AUDIO)
  {
-    m_updThread.ResetStartTime();


this should be a remnant of the old manifest update code, has no sense reset the worker thread here, isnt used when we update manifest at each new segment request "manually"

CastagnaIT · 2024-01-22T08:49:39Z

src/parser/HLSTree.cpp

-                                                                     PLAYLIST::CAdaptationSet* adp,
-                                                                     PLAYLIST::CRepresentation* rep,
-                                                                     bool update)
+bool adaptive::CHLSTree::PrepareRepresentation(PLAYLIST::CPeriod* period,


Here github show a big mess on code diff, there are no functional code changes on PrepareRepresentation, the only difference is that has been decoupled per functionality.
The old prepareRepresentation now has been splitted on more methods:

PrepareRepresentation: Act more or less as before

DownloadChildManifest: To download the child manifest only

ParseChildManifest: Parse the child manifest data

PrepareSegments: Update the segments (to adjust/set "current segment")

now ParseChildManifest could be reused in future to parse in right way the media playlist on CHLSTree::ParseManifest (see todo), not tested now, but a step forward to achieve it

CastagnaIT · 2024-01-22T08:53:37Z

@glennguy when you will have time try test this PR and review
i add some comments in code and also on github to try explain the code changes

glennguy

Thanks so much for this work and fixing some of the issues in adaptivestream.

glennguy · 2024-01-28T11:23:16Z

src/common/AdaptiveStream.cpp

+    //! @todo: THIS MUST BE CHANGED - !! BUG !!
+    //! this will broken/stop playback on live streams when adaptive stream change stream quality (so representation)
+    //! and there are no new segments because not immediately available (despite child manifest updated),
+    //! because we cant be ensure next segment without wait the appropriate timing...imo its not the right place here.
+    //! Can be reproduced with HLS live and by using "Stream selection" setting to "Test" by switching per 1 segment


yes the scenario makes sense why it fails. We could try a hacky workaround until something better comes along - assume when the next segment comes it will be the same size (usually is) and add that PTS to current segment's start PTS. Sound good or not?

glennguy · 2024-01-28T11:26:31Z

src/common/AdaptiveStream.cpp

+
+    // We can fall here when we are waiting for new segments, but the download it not started yet
+    // so return true only when checkTime is false, to avoid trigger EOS on sample reader and stop playback
+    return !checkTime;


CastagnaIT added Type: Fix non-breaking change which fixes an issue Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v21 Omega labels Jan 21, 2024

CastagnaIT force-pushed the fix_hls_man_new branch from fdab26d to a06883a Compare January 22, 2024 08:10

CastagnaIT commented Jan 22, 2024

View reviewed changes

src/common/Representation.h Outdated Show resolved Hide resolved

CastagnaIT commented Jan 22, 2024

View reviewed changes

CastagnaIT marked this pull request as ready for review January 22, 2024 08:52

CastagnaIT linked an issue Jan 22, 2024 that may be closed by this pull request

Subtitles from German live broadcasters are not displayed in Kodi 20 (Nexus) #1109

Closed

6 tasks

glennguy approved these changes Jan 28, 2024

View reviewed changes

[AdaptiveStream] Rework to improve HLS manifest updates

9296d63

CastagnaIT force-pushed the fix_hls_man_new branch from bddc3c0 to 9296d63 Compare January 28, 2024 12:50

CastagnaIT merged commit a3bc41d into xbmc:Omega Jan 28, 2024
10 checks passed

CastagnaIT deleted the fix_hls_man_new branch January 28, 2024 13:48

werimail mentioned this pull request Mar 30, 2024

Aborting playback in the event of commercial breaks or X-discontinuity with the inputstream.adaptive addon #1507

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AdaptiveStream] Rework to improve HLS manifest updates #1459

[AdaptiveStream] Rework to improve HLS manifest updates #1459

CastagnaIT commented Jan 21, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

glennguy Jan 28, 2024

CastagnaIT Jan 28, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

glennguy Jan 28, 2024

CastagnaIT Jan 22, 2024

CastagnaIT Jan 22, 2024

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT commented Jan 22, 2024

glennguy left a comment

glennguy Jan 28, 2024

glennguy Jan 28, 2024

[AdaptiveStream] Rework to improve HLS manifest updates #1459

[AdaptiveStream] Rework to improve HLS manifest updates #1459

Conversation

CastagnaIT commented Jan 21, 2024 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of change

Checklist:

CastagnaIT Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

glennguy Jan 28, 2024

Choose a reason for hiding this comment

CastagnaIT Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

glennguy Jan 28, 2024

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024

Choose a reason for hiding this comment

CastagnaIT Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

CastagnaIT commented Jan 22, 2024

glennguy left a comment

Choose a reason for hiding this comment

glennguy Jan 28, 2024

Choose a reason for hiding this comment

glennguy Jan 28, 2024

Choose a reason for hiding this comment

CastagnaIT commented Jan 21, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 28, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading

CastagnaIT Jan 22, 2024 •

edited

Loading