fix(dash): correct period position calculation

[romsil]

Description

Fixes #3717

We have encountered fatal playback failure due to phantom media segment requests caused by small floating points being ceiled in Shaka. This PR fixes the issue by limiting the argument passed to Math.ceil().

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update

Checklist:

• I have signed the Google CLA https://cla.developers.google.com
• My code follows the style guidelines of this project
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have verified my change on multiple browsers on different platforms
• I have run ./build/all.py and the build passes
• I have run ./build/test.py and all tests pass

[TheModMaker]

I don't think this is the correct fix. toFixed will convert the number to a string, then ceil will just convert it back to a number. A better fix would be to account for rounding errors when determining when to end. That would probably be in StreamingEngine.

[romsil]

@TheModMaker Thanks for the review.
As detailed in #3717, for the period and audio track declaration below:

<Period duration="PT30.720000S" id="a-2">
      <AssetIdentifier schemeIdUri="urn:com:uplynk:ad-asset-id" value="d4bfa4522c044a968fc4f2a0d944340f" />
      <AdaptationSet audioSamplingRate="48000" contentType="audio" group="1" lang="en" mimeType="audio/mp4" segmentAlignment="true" startWithSAP="1">
         <Role schemeIdUri="urn:mpeg:dash:role:2011" value="main" />
         <Representation bandwidth="127878" codecs="mp4a.40.5" id="v0">
            <AudioChannelConfiguration schemeIdUri="urn:mpeg:dash:23003:3:audio_channel_configuration:2011" value="2" />
            <BaseURL>https://RedactedCDNUrl/slices/d4b/4f59ea65df184a2580ad8fc80d3eaa32/d4bfa4522c044a968fc4f2a0d944340f/</BaseURL>
            <SegmentTemplate duration="184320" initialization="https://RedactedCDNUrl/slices/d4b/4f59ea65df184a2580ad8fc80d3eaa32/d4bfa4522c044a968fc4f2a0d944340f/TK_init.mp4" media="TK_$Number%08d$.m4f" startNumber="0" timescale="90000" />
         </Representation>
      </AdaptationSet>
...

, and from the code:

const availablePeriodPositions = [
        Math.ceil(availablePeriodTimes[0] / segmentDuration),
        Math.ceil(availablePeriodTimes[1] / segmentDuration) - 1,
];

we get the following values on Chrome:

availablePeriodTimes[1] = 30.720000000000027
segmentDuration = 2.048
availablePeriodTimes[1] / segmentDuration = 15.000000000000012
Math.ceil(availablePeriodTimes[1] / segmentDuration) = 16

After the fix, we get:

Math.ceil(availablePeriodTimes[1] / segmentDuration).toFixed(10) = 15

15 segments is the correct value since the declared period duration is PT30.720000S.

We tried other solutions, but this one-line change is the least intrusive since we're basically just disregarding anything after the 10th decimal place when getting the number of media segments in a period.

[TheModMaker]

It would be fine to have segments in the index that don't exist in this case. I think the problem is with the below line where we determine if we have buffered to the end. If we change this to account for a small epsilon, we won't download the segment and end at the correct segment.

https://github.com/google/shaka-player/blob/b8b72a900d155ef085e72e62b5da697d67990c04/lib/media/streaming_engine.js#L991

[romsil]

Hello @TheModMaker,
Thanks for the review. I can do the suggested change but we won't be able test it since we have already applied our patch and filtered our assets.

The change I have added in this PR however have been thoroughly tested.
Can we have this merged as it is?

[joeyparrish]

Sorry, we will not be merging this PR. We do not believe that this is the correct fix.

The issues we have with this fix are:

1. you convert a number to string before implicitly converting it back to a number again
2. the rounding that you're applying seems arbitrary - it would also give you the same result if availablePeriodTimes[1] were truncated to 10 decimals instead

I agree with @TheModMaker. We should fix this in StreamingEngine instead. I'll make his suggested fix and create a regression test, then get it up for review as soon as possible.