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

Flatten periods and SegmentTemplate internally #1339

Closed
joeyparrish opened this issue Mar 6, 2018 · 5 comments
Closed

Flatten periods and SegmentTemplate internally #1339

joeyparrish opened this issue Mar 6, 2018 · 5 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@joeyparrish
Copy link
Member

joeyparrish commented Mar 6, 2018

Today, some of our most complex and subtle code is around period transitions. Player and StreamingEngine both have to manage a bunch of corner cases around transition points. The Player may be showing the set of tracks from period 1, while StreamingEngine and AbrManager are dealing with a different set of tracks from period 2.

StreamingEngine has complex code to manage period transitions and synchronize them across stream types (audio, video, text). The only thing that really needs to change between periods is the timestampOffset, append window, and init segment.

If we internally flatten out multi-period DASH, we can have a single list of segments that cross period boundaries. Each segment reference could contain its own append window, timestamp offset, and init segment reference. If the init segment changes between segment N and N+1, StreamingEngine could just fetch and append that init segment before the corresponding media segment. If the append window or timestamp offset change from one segment to the next, StreamingEngine could just pass those parameters to MediaSourceEngine before fetching and appending the next segments. StreamingEngine then would need no period-related state.

A consequence of flattening periods is that we would also have a consistent set of tracks. No manual selection would get undone at a period boundary. There would be no need to ask AbrManager to make choices at period boundaries. If representations in different periods were ill-aligned, these new tracks would map out multiple paths through those representations based on what would be chosen at different bandwidth levels. This mapping would be the most difficult part of the system to get right, but it would be isolated to the DASH implementation.

After this transition, our internal manifest structure would look more like HLS than DASH, which would enable us to then implement features like HLS discontinuities.

Much of this was originally proposed by @vaage internally.

This will unblock #1335, #1308, and #892. It may simplify #999 and #1307. It would fix #856 and #1543.

@joeyparrish joeyparrish added the type: enhancement New feature or request label Mar 6, 2018
@joeyparrish joeyparrish self-assigned this Mar 6, 2018
@TheModMaker TheModMaker added this to the v2.5 milestone Mar 10, 2018
@joeyparrish joeyparrish modified the milestones: v2.5, v2.6 Jul 23, 2018
shaka-bot pushed a commit that referenced this issue Jul 8, 2019
This replaces find/get callbacks in Stream with a SegmentIndex.  With
the exception of DASH's SegmentTemplate+duration, all manifests were
already backed by SegmentIndex.  Now, all manifests are backed by
SegmentIndex.  This will simplify Period-flattening, in which all
tracks will be represented by a list of segments, some of which come
from different Periods.

The SegmentIndex in Stream will not be created until
createSegmentIndex is called.  Prior to this change, the find/get
callbacks could be invoked without createSegmentIndex() in some cases
(excluding DASH's SegmentBase), which some lazy tests took advantage
of.  Now that find/get are methods on SegmentIndex, createSegmentIndex
must be called in all cases.  The tests have been updated accordingly.

Making SegmentIndex generation async in all cases exposed some issues
with the parser context being modified in-place between one
Representation and the next.  So the parser now makes a shallow copy
of the context before it is bound into an async callback.

To facilitate updating the SegmentIndex for SegmentTemplate+duration
content, SegmentIndex now has a method to update its list on a timer.
Once per segment duration, the index will be updated to add and remove
SegmentReferences.

The initial expansion of SegmentTemplate+duration will be limited to a
relatively small number of segments, to avoid excessive CPU or memory
consumption.  This defaults to 1000 segments, but is configurable.

Issue #1339

Change-Id: I99c007b1096c3b396d04a729750cd7b743cb899d
shaka-bot pushed a commit that referenced this issue Jul 9, 2019
This was broken in "Replace find/get callbacks with SegmentIndex"
because automated tests for SegmentBase did not cover multiple
Representations with different index/init ranges.  Once the parsing
became async in the previous change, the index information was being
pulled from the last Representation parsed, rather than any specific
one.

This fixes the issue by making a shallow copy of the parsing context,
as was already done for SegmentTemplate.

SegmentList is synchronous, so it was not affected.

Issue #1339

Change-Id: I09c606c464b49c99d35d1031734b64872cfa6599
shaka-bot pushed a commit that referenced this issue Jul 9, 2019
To prepare for an upcoming change to the format of the data stored in
IndexedDB, refactor the storage cells to remove common functionality.
This will make it easier to add a third cell type.

The format will change as we move fields in the manifest structure and
ultimately flatten out periods.

Issue #1339

Change-Id: Ibaeb35596180b2e2362b4d53734019599d6fff77
AnteWall pushed a commit to AnteWall/shaka-player that referenced this issue Jul 17, 2019
This replaces find/get callbacks in Stream with a SegmentIndex.  With
the exception of DASH's SegmentTemplate+duration, all manifests were
already backed by SegmentIndex.  Now, all manifests are backed by
SegmentIndex.  This will simplify Period-flattening, in which all
tracks will be represented by a list of segments, some of which come
from different Periods.

The SegmentIndex in Stream will not be created until
createSegmentIndex is called.  Prior to this change, the find/get
callbacks could be invoked without createSegmentIndex() in some cases
(excluding DASH's SegmentBase), which some lazy tests took advantage
of.  Now that find/get are methods on SegmentIndex, createSegmentIndex
must be called in all cases.  The tests have been updated accordingly.

Making SegmentIndex generation async in all cases exposed some issues
with the parser context being modified in-place between one
Representation and the next.  So the parser now makes a shallow copy
of the context before it is bound into an async callback.

To facilitate updating the SegmentIndex for SegmentTemplate+duration
content, SegmentIndex now has a method to update its list on a timer.
Once per segment duration, the index will be updated to add and remove
SegmentReferences.

The initial expansion of SegmentTemplate+duration will be limited to a
relatively small number of segments, to avoid excessive CPU or memory
consumption.  This defaults to 1000 segments, but is configurable.

Issue shaka-project#1339

Change-Id: I99c007b1096c3b396d04a729750cd7b743cb899d
AnteWall pushed a commit to AnteWall/shaka-player that referenced this issue Jul 17, 2019
This was broken in "Replace find/get callbacks with SegmentIndex"
because automated tests for SegmentBase did not cover multiple
Representations with different index/init ranges.  Once the parsing
became async in the previous change, the index information was being
pulled from the last Representation parsed, rather than any specific
one.

This fixes the issue by making a shallow copy of the parsing context,
as was already done for SegmentTemplate.

SegmentList is synchronous, so it was not affected.

Issue shaka-project#1339

Change-Id: I09c606c464b49c99d35d1031734b64872cfa6599
AnteWall pushed a commit to AnteWall/shaka-player that referenced this issue Jul 17, 2019
To prepare for an upcoming change to the format of the data stored in
IndexedDB, refactor the storage cells to remove common functionality.
This will make it easier to add a third cell type.

The format will change as we move fields in the manifest structure and
ultimately flatten out periods.

Issue shaka-project#1339

Change-Id: Ibaeb35596180b2e2362b4d53734019599d6fff77
shaka-bot pushed a commit that referenced this issue Aug 1, 2019
To prepare for flattening out the manifest structure to remove
periods, this change moves initSegmentReference and
presentationTimeOffset fields into the SegmentReference object.  This
way, the segments on either side of a period transition or HLS
discontinuity can have different offsets or init segments, eventually
allowing us to create a single array of SegmentReferences for
multi-period content.

Issue #1339

Change-Id: Ic7eff0483789644881247ecf8044c5fb6a48f0e6
shaka-bot pushed a commit that referenced this issue Nov 5, 2019
In StreamingEngine, rather than wait to enable ABR after indexes have
been created for all streams, create each stream's SegmentIndex
on-demand as needed during playback.  This means ABR can be enabled
much more quickly, and also eliminates some complexity from
StreamingEngine's startup sequence.

This required several test changes, since many of our tests were
accidentally structured to depend on certain operations either being
synchronous or happening early during startup.

Issue #1339 (flatten periods)
Issue #892 (refactor StreamingEngine)

Change-Id: I4bc1d0cdf9022aad14a008accf0aac37c870a83f
shaka-bot pushed a commit that referenced this issue Nov 6, 2019
Instead of making many internal methods async to accomodate
createSegmentIndex being called lazily, just call createSegmentIndex
during the update cycle instead.  This greatly simplifies things and
allows me to revert some of the changes I made in the earlier commit.

Issue #1339 (flatten periods)
Issue #892 (refactor StreamingEngine)

Change-Id: I72be8e88f0cf8b04b63d3cda129fa38cef727c0f
@joeyparrish
Copy link
Member Author

Note to self: make sure DASH manifest updates are as efficient as possible in the new flattened structure. For example, when updating a SegmentTimeline, don't parse the entire thing and merge indexes if we can start parsing after the last segment from the previous manifest update. This could help a lot on slow devices.

shaka-bot pushed a commit that referenced this issue Nov 7, 2019
Because aborting requires knowledge of the new stream, this process
must be done asynchronously.  This makes the abort logic async, and
checks carefully for any stream or operation changes during the
process.

Issue #1339 (flatten periods)
Issue #892 (refactor StreamingEngine)

Change-Id: Ic187676eeca907603efeb0ffa11855b9af2fc5ca
shaka-bot pushed a commit that referenced this issue Dec 19, 2019
There were fields in SegmentReference which were not being examined
when instances are compared by jasmine.  Now, the comparison is more
general and will continue to work when we change the structure.

This relates to the issues below because the next changes for those
issues will change the fields in SegmentReference.

Issue #1339 (flatten periods)
Issue #892 (refactor StreamingEngine)

Change-Id: Ib7cd3d3cadeb0e58efd70964c082219b1c097fad
shaka-bot pushed a commit that referenced this issue Feb 14, 2020
This will make it easier to ensure conversions are done correctly,
which will be increasingly important as we change the database format
as part of #1339 (period flattening).

Change-Id: I1114b1a9a0d341589f7f1026ec23ad93af38a6b0
@joeyparrish
Copy link
Member Author

As part of selecting which AdaptationSet/Representation to map to tracks, we should look at Period-connectivity. See DASH 5.3.2.4 and DASH Timing Guidelines section 12.

Having looked more closely at what period-connectivity offers, it may be more complex to use it at this point. It does not indicate the correct transitions from one period to the next on all cases. Rather, it indicates some correct transitions between some periods, but not necessarily covering all streams, and not necessarily for contiguous periods. For example, the diagrams in those guidelines show period 1 connected to period 3, and only for a subset of streams.

So using this information for period flattening would be complex.

Further, it seems that the purpose of period-connectivity information is to indicate that the init segments for certain streams are equivalent for decoder initialization. While this would be a potential optimization, in that we could sometimes avoid fetching an init segment when we change periods, it seems to me that it would be a minor one. And a general period flattening algorithm would naturally connect streams with the same container, codec, and resolution, which are the things that must be in common to reuse the init segment and therefore to have period-connectivity.

So I would expect to wind up with the same connections as we would if we had used the period-connectivity info, but without the optimization of reusing init segments across periods. We may choose to implement that optimization at a later time.

shaka-bot pushed a commit that referenced this issue Mar 3, 2020
Segment times and positions are no longer period-relative.

Issue #1339

Change-Id: If2b1b7d56ac49d50439598403f681a2da88ce605
shaka-bot pushed a commit that referenced this issue Mar 5, 2020
As part of Period-flattening, I'm trying to reduce our dependence on
the "position" field of SegmentReference.  If it can be eliminated, we
can more easily concatenate Arrays of SegmentReferences without
modifying them.

The HLS parser uses positions to find previously-resolved segment
times when refreshing a live playlist.  What we're really doing is
using HLS media sequence numbers, since we use those to assign the
segment positions in the first place.

This change makes the use of media sequence numbers more explicit, and
adds a per-Stream map from media sequence number to segment start
time.

The only other users of segment positions are StreamingEngine and
various tests.

Issue #1339 (period-flattening)

Change-Id: If970ed2c8722ed5779a51349ca2e64208d78130d
@avelad
Copy link
Collaborator

avelad commented Mar 6, 2020

I think that point 5.3.2.2 of https://dashif.org/docs/DASH-IF-IOP-v4.3.pdf could be useful. what do you think?


AssetIdentifier descriptors identify the asset to which a Period belongs. This can be used
for implementation of client functionality that depends on distinguishing between ads and main
content (e.g. progress bar).
Periods with same AssetIdentifier should have identical Adaptation Sets, Initialization
Segments and same DRM information (i.e., DRM systems, licenses). This allows reuse of at least
some initialization data across periods of the same asset, and ensures seamless continuation of
playback if inserted periods have zero duration. Period continuity or connectivity should be sig-
naled, if the content obeys the rules.

shaka-bot pushed a commit that referenced this issue Mar 11, 2020
As part of Period-flattening, I'm trying to reduce our dependence on
the "position" field of SegmentReference.  If it can be eliminated, we
can more easily concatenate Arrays of SegmentReferences without
modifying them.

There has long been a comment at the top of SegmentIndex's merge
method stating that we only ever extend the references, but never
interleave them.  The code, however, is structured in such a way that
it could interleave them.  This could cause the offset of a given
SegmentReference in the Array to change, which would be counter to the
comment about only ever extending the list.

This change simplifies merge() so that it can only ever extend the
array of SegmentReferences.  This will guarantee that their offset
within the Array will not change during the merge operation.  This, in
turn, enables further SegmentIndex changes to move "next" segment
tracking out of StreamingEngine (where it is based on the "position"
field of SegmentReference) and into SegmentIndex (where it could be
based on offset into the Array of references).

It removes one test related to PR #838.  This test was about our
ability to update the position of the final segment in a list.  This
doesn't seem to make a lot of sense, and we're going to stop relying
on segment position anyway.

Issue #1339 (period-flattening)

Change-Id: I2067e2266cf2d02c0e6350d6b57d74f9ed1b27d3
shaka-bot pushed a commit that referenced this issue Mar 11, 2020
As part of Period-flattening, I'm trying to reduce our dependence on
the "position" field of SegmentReference.  If it can be eliminated, we
can more easily concatenate Arrays of SegmentReferences without
modifying them.

SegmentIndex can now track the last reference you asked for and
iterate through the list of references.  This means we don't need the
"position" field of SegmentReference, which means we don't need to
know positions in advance or globally.  StreamingEngine will no longer
use position to request segments.

The old methods find(time):position and get(position):SegmentReference
have been replaced with seek(time), current(), and next(), all of
which return a SegmentReference and maintain an internal pointer to
the "current" reference.  Care has been taken to maintain that pointer
during the evict() and fit() operations.  Recent changes to merge()
made sure that the pointer does not need to change during that
operation.

All test updates are related to the SegmentIndex API change, not
changing expectations or behavior.

Issue #892 (refactor StreamingEngine)
Issue #1339 (period flattening)

Change-Id: I1682dcc2dd625c6e390711538e46d31e6eb6cea8
joeyparrish added a commit that referenced this issue Mar 11, 2020
To prepare for an upcoming change to the format of the data stored in
IndexedDB, refactor the storage cells to remove common functionality.
This will make it easier to add a third cell type.

The format will change as we move fields in the manifest structure and
ultimately flatten out periods.

Issue #1339

Change-Id: Ibaeb35596180b2e2362b4d53734019599d6fff77
joeyparrish added a commit that referenced this issue Mar 11, 2020
This will make it easier to ensure conversions are done correctly,
which will be increasingly important as we change the database format
as part of #1339 (period flattening).

Backported to v2.5.x

Change-Id: I1114b1a9a0d341589f7f1026ec23ad93af38a6b0
shaka-bot pushed a commit that referenced this issue Mar 23, 2020
This reverts commit 235e4e1.

The effort to remove SegmentReference's position field will be handled
in a different way.

Issue #892 (refactor StreamingEngine)
Issue #1339 (period flattening)

Change-Id: I62b115137abc89f498b30467e574b0401dcad05d
shaka-bot pushed a commit that referenced this issue Mar 23, 2020
As part of Period-flattening, I'm trying to remove our dependence on
the "position" field of SegmentReference.  With that eliminated, we
can more easily concatenate Arrays of SegmentReferences without
modifying them.

 - Make SegmentIndex iterable
 - Add specialized seek() and current() methods to SegmentIterator
 - Remove position from SegmentReference
 - Make positions in SegmentIndex API stable without field in
   reference
 - Remove brittle hard-coded positions in tests (except SegmentIndex
   tests, where they would be hard to avoid in testing methods
   separately)
 - Use SegmentIterator in StreamingEngine to track the next segment
   between switches

Issue #892 (refactor StreamingEngine)
Issue #1339 (period flattening)

Change-Id: I666cc21249c34ee6cbc138a59109d9f1159fa127
shaka-bot pushed a commit that referenced this issue Mar 23, 2020
SegmentIndex has had a destroy() method for a long time, but it has
never been called.  Now that SegmentIndex has a timer which adds new
references for DASH SegmentTemplate live streams, it is more important
than ever to properly stop this active part of SegmentIndex.

This change replaces async destroy() with synchronous release() and
calls it from Player when the manifest is being disposed of.  This
will ensure that SegmentIndexes don't grow out of control after
content is unloaded.

This would not have affected v2.5, since we didn't have this
timer-driven growth of DASH SegmentTemplate live streams in that
release branch.

Related to #1339 (fixes issues introduced for period flattening)

Change-Id: I419a06a62eaa507d92132e20d4cc2d6e45c83ff2
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
This removes periods from the internal manifest structure and cleans
up code and tests accordingly.  This leaves us unable to play
multi-period DASH & offline streams until the main period-flattening
algorithm is completed in shaka.util.Periods.

Three test cases have been disabled for the moment.

Multi-period playback will be restored in a smaller, more focused
follow-up commit, with disabled tests re-enabled.

Issue #1339 (flatten periods)
Issue #1698 (rapid period transitions issue)
Issue #856 (audio change causes bitrate change)
Closes #892 (refactor StreamingEngine)

Change-Id: I0cbf3b56bfdb51add15229df323b902f0b2e643a
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
Period-flattening will concatenate Stream objects, so this information
should be available per-Stream instead of at the Variant level.

Issue #1339

Change-Id: I96195fea48cab1e4a349b2ab0b16064a443e928a
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
This brings the field name in line with the Stream objects from the
manifest types, allowing for more general processing of Stream and
StreamDB for period-flattening.

Issue #1339

Change-Id: Ic5d0e5d69a6560d475a19f5d3ecb0b1b40b8d271
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
When period-flattening combines Streams, key ID arrays would get very
long with duplicates.

This changes keyIds in the manifest and offline structures from Array
to Set.

Issue #1339

Change-Id: I003d23e567efafa771ecd2ad597900181604ad18
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
This will be used in period-flattening, where we will need to create
segment indexes before combining Streams.  Although the operation
would be synchronous for DB types, the general interface will need to
be async.

Issue #1339

Change-Id: Ibedb267118462cbaf3ca599ab6a67c8f355965f6
shaka-bot pushed a commit that referenced this issue Apr 9, 2020
In these tests, SegmentBase referred to init segments that did not
exist.  In period-flattening, we will be fetching segment indexes in
advance during parsing, so these bogus init segments would be fetched
and cause an exception.  Using SegmentTemplate avoids this for tests
are not concerned with SegmentBase specifically.

Issue #1339

Change-Id: I54990e95480dfbb59154fca72f12d277eca25701
Tracks & manifest refactor automation moved this from Actionable to Done Apr 9, 2020
@shaka-project shaka-project locked and limited conversation to collaborators Jun 8, 2020
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

5 participants