Fix incorrect segment position after manifest update #838

albertcsm · 2017-06-01T08:03:54Z

I observed quite a lot of assertion failure Assertion failed: SegmentReferences are incorrect for a multiple period DASH, and encountered some rare player stuck.

Before the stuck happen, there was console log that suggest the stuck is due to not being able to find segment from position number:
segment doest not exist: currentPeriod.startTime=1216.3484667 position=5

I found that the issue was caused by incorrect position number of segment references after segment merge on manifest update.

When the segment reference from the newer manifest get merged, the position number is left untouched. However, it may have changed due to removal of previous segments from the period. Please see the attached diagram, the segment number changed from 5 to 1 after manifest update. Consequently, any attempt to locate the next segment by position would fail.

After patching the player with corrected position number, the assert failures no longer occur.

The segments in my manifest are not aligning close to the period boundaries, so may be a corner case that other users may not see the same issue.

Please check my code diff, thanks.

…ange

googlebot · 2017-06-01T08:03:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

albertcsm · 2017-06-01T08:08:18Z

I signed it!

googlebot · 2017-06-01T08:08:21Z

CLAs look good, thanks!

joeyparrish

This makes sense to me, but I would like to see an additional test added to test/media/segment_index_unit.js to ensure we don't regress in the future. Thanks!

TheModMaker · 2017-06-05T17:21:19Z

Can you please provide an example manifest that reproduces your issue? We calculate the position based on the segment numbers in the manifest, which should not change. When there is a manifest update, the same segments should have the same segment number and media times. If you are removing segments, then you should update attributes like startNumber so we calculate the correct segment number.

If this is a bug on our part, then it will mess with other parts, like calculation of SegmentTemplate when using $Number$ , so this would not be the preferred fix.

TheModMaker · 2017-06-29T17:35:45Z

@albertcsm Can you provide an example manifest? Otherwise we are going to close the PR.

albertcsm · 2017-07-08T17:03:37Z

OK, I will provide two versions of the manifest that has segments removed for retention

albertcsm · 2017-07-12T12:55:20Z

I wasn't able to reproduce the issue at this moment, actually it is not easy to hit the issue.

Anyway, I attached the sample manifests from my encoder. The two manifests are consecutive snapshot of the manifest, with 4 seconds between them. Some segments from period 17 were removed in the second manifest.

manifest-5159.txt
manifest-5202.txt

TheModMaker · 2017-07-12T16:50:26Z

Your content is bad. The reason you are not seeing more problems is because you use $Time$ in the URL template; if you used $Number$ , more problems would appear.

The encoder is removing segments from the list without updating the startNumber attribute. This means that the segment numbers of the segments in the update don't match their segment numbers in the original. Your encoder should be setting the startNumber attribute so we can calculate the segment number correctly.

For example, in the audio stream, the first segment initially starts at 7888012920. Since there is no startNumber, it has a segment number of 1. Then the next segment starts at 7908066253 and has a segment number of 2. Then in the next update, the first segment listed starts at 7908066253, so it has a segment number of 1. A segment must have the same segment number for the whole presentation.

albertcsm · 2017-07-13T09:46:19Z

@TheModMaker I am not sure if startNumber is really required for $Time$ based mpd. I see that another sample asset in Shaka (Wowza / Big Buck Bunny (Live)) also removes segments from MPD in the same way, and there is also no startNumber.

big-buck-bunny-1.txt
big-buck-bunny-2.txt

TheModMaker

I forgot that we are supporting this use-case for other content. Sory for my confusion.

Also please add a unit test for this.

TheModMaker · 2017-07-13T17:55:14Z

lib/media/segment_index.js

-        newReferences.push(r2);
+        var r = new shaka.media.SegmentReference(r1.position,
+            r2.startTime, r2.endTime, r2.getUris, r2.startByte, r2.endByte);
+        newReferences.push(r);


I think it could be implemented differently. If you just increment i (not j), then the last segment will be treated as new and will be appended with the logic below (I think).

If you do it that way, I would suggest moving the check to the higher else-if chain.

@TheModMaker, I don't think I understand you. If we're processing both a new and old ref, and replacing one with the other, why wouldn't we increment the indices on both arrays?

Because if there are leftover new refs after this loop finishes, the code below will append them while adjusting the position numbers correctly (see line 165+ below). So if we increment i (ignore old ref), the last ref will be treated as a new ref and that code will handle the renumbering.

I am also suggesting that you don't push anything, just increment i. For example:

if (r1.startTime < r2.startTime) { ... } else if (r1.startTime > r2.startTime) { ... } else if (Math.abs(r1.endTime - r2.endTime) > 0.1) { // When a period is changed... goog.asserts.assert(...); i++; // r2 will be pushed as a new ref by the code below. } else { newReferences.push(r1); i++; j++; }

But maybe this is too subtle, the old code is fine if that is better.

albertcsm · 2017-07-14T03:51:00Z

I have added a unit test, please check, thanks.

TheModMaker

Looks good to me, just two nits.

I believe you marked the files as executable, they should be normal text files. This can be fixed with:

git add --chmod=-x lib/media/segment_index.js test/media/segment_index.js
git checkout -- lib/media/segment_index.js test/media/segment_index.js

TheModMaker · 2017-07-17T16:58:48Z

test/media/segment_index_unit.js

@@ -255,6 +255,29 @@ describe('SegmentIndex', /** @suppress {accessControls} */ function() {
      expect(index1.references_[1]).toEqual(references2[0]);
      expect(index1.references_[2]).toEqual(references2[1]);
    });
+
+    it('last live stream reference with corrected position', function() {


Also add a comment here with the URL to your PR.

albertcsm · 2017-07-18T09:06:52Z

@TheModMaker Oh yes, thanks for pointing out, I was making the change over a mounted folder on a Windows desktop... I have fixed it and added the PR link.

@TheModMaker

Letting @TheModMaker review and approve

joeyparrish · 2017-07-24T19:32:13Z

I'm running tests right now. Assuming they pass, we'll wait for final approval from @TheModMaker. Thanks!

shaka-bot · 2017-07-24T19:38:36Z

All tests passed!

joeyparrish · 2017-07-25T13:07:53Z

The fix has been cherry-picked for v2.1.6.

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

fix segment reference updated with incorrect position after period ch…

fcd7800

…ange

albertcsm changed the title ~~fix segment reference updated with incorrect position after period ch…~~ Fix incorrect segment position after manifest update Jun 1, 2017

joeyparrish previously requested changes Jun 5, 2017

View reviewed changes

joeyparrish assigned joeyparrish and TheModMaker and unassigned joeyparrish Jun 5, 2017

TheModMaker added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jun 12, 2017

TheModMaker added status: bad content Caused by invalid, broken, or unsupported content and removed status: waiting on response Waiting on a response from the reporter(s) of the issue labels Jul 12, 2017

albertcsm closed this Jul 13, 2017

albertcsm reopened this Jul 13, 2017

TheModMaker suggested changes Jul 13, 2017

View reviewed changes

TheModMaker added type: bug Something isn't working correctly and removed status: bad content Caused by invalid, broken, or unsupported content labels Jul 13, 2017

add unit test for corrected segment position in segment merge

f8509cd

TheModMaker suggested changes Jul 17, 2017

View reviewed changes

joeyparrish added this to the v2.2.0 milestone Jul 17, 2017

albertcsm added 2 commits July 18, 2017 07:17

fix file mode

546f34f

add comment and link to pull request

23e2d49

albertcsm closed this Jul 18, 2017

albertcsm reopened this Jul 18, 2017

joeyparrish approved these changes Jul 24, 2017

View reviewed changes

TheModMaker merged commit d80a2cd into shaka-project:master Jul 24, 2017

joeyparrish pushed a commit that referenced this pull request Jul 25, 2017

Fix incorrect segment position after manifest update (#838)

c00f1c8

hdodev mentioned this pull request Feb 20, 2018

Stream interrupted playback with Live DASH Halo pakager using Shaka 2.3.0 version #1311

Closed

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect segment position after manifest update #838

Fix incorrect segment position after manifest update #838

albertcsm commented Jun 1, 2017

googlebot commented Jun 1, 2017

albertcsm commented Jun 1, 2017

googlebot commented Jun 1, 2017

joeyparrish left a comment

TheModMaker commented Jun 5, 2017

TheModMaker commented Jun 29, 2017

albertcsm commented Jul 8, 2017

albertcsm commented Jul 12, 2017

TheModMaker commented Jul 12, 2017

albertcsm commented Jul 13, 2017

TheModMaker left a comment

TheModMaker Jul 13, 2017

joeyparrish Jul 13, 2017

TheModMaker Jul 13, 2017

albertcsm commented Jul 14, 2017

TheModMaker left a comment

TheModMaker Jul 17, 2017

albertcsm commented Jul 18, 2017

joeyparrish commented Jul 24, 2017

shaka-bot commented Jul 24, 2017

joeyparrish commented Jul 25, 2017

Fix incorrect segment position after manifest update #838

Fix incorrect segment position after manifest update #838

Conversation

albertcsm commented Jun 1, 2017

googlebot commented Jun 1, 2017

albertcsm commented Jun 1, 2017

googlebot commented Jun 1, 2017

joeyparrish left a comment

Choose a reason for hiding this comment

TheModMaker commented Jun 5, 2017

TheModMaker commented Jun 29, 2017

albertcsm commented Jul 8, 2017

albertcsm commented Jul 12, 2017

TheModMaker commented Jul 12, 2017

albertcsm commented Jul 13, 2017

TheModMaker left a comment

Choose a reason for hiding this comment

TheModMaker Jul 13, 2017

Choose a reason for hiding this comment

joeyparrish Jul 13, 2017

Choose a reason for hiding this comment

TheModMaker Jul 13, 2017

Choose a reason for hiding this comment

albertcsm commented Jul 14, 2017

TheModMaker left a comment

Choose a reason for hiding this comment

TheModMaker Jul 17, 2017

Choose a reason for hiding this comment

albertcsm commented Jul 18, 2017

joeyparrish commented Jul 24, 2017

shaka-bot commented Jul 24, 2017

joeyparrish commented Jul 25, 2017