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

Bugfix live edge calculation #3201

Conversation

dsilhavy
Copy link
Collaborator

This PR changes the priority of the live delay calculation to the following descending order

  1. If low latency enabled set delay to 0
  2. If live delay was set by the user use this value
  3. If live delay fragment count was set by the user use this value
  4. If r parameter is present in the url use this parameter
  5. If suggestedPresentationDelay in included in the MPD use this value
  6. If fragment duration is available use 4*fragment duration
  7. Otherwise use minBufferTime*2

This PR also related to a fix applied regarding the suggestedPresentationDelay : #3199

@dsilhavy dsilhavy added this to the 3.1.0 milestone Mar 17, 2020
@dsilhavy
Copy link
Collaborator Author

@bbert @nicosang Any objections/thoughts from your side?

@wilaw
Copy link
Member

wilaw commented Mar 17, 2020

Several comments here:

  1. Is liveDelay the distance behind the live point at which the player tried to load data, or play data? If play, then a liveDelay of 0 is irrational, since that would imply a zero buffer.

  2. If the user sets a low live delay value (such as < 2x segment duration), should not that trigger low latency behavior? With current hierarchy, if user sets lowlatencyMode true and then a live delay of 2s, that live delay value is ignored? The whole notion of a manual setting of low latency mode seems contrary to the other operations of dash.js . I am wondering if it arose as a quick response to enable testing but has now been cemented in to the APIs. Really there should be only one input here which is a target live delay. The player should then decide internally what "mode" it needs to achieve that target?

  3. What about DVB and DASH IF low latency ServiceDescription element, which defines a latency target. Where does that value get injected in the hierarchy?

   <ServiceDescription id="0">
      <Latency target="3000" referenceId="1"/>
   </ServiceDescription>
  1. Item [6] sets the live delay at 4x segment duration. But then item [7] relaxes this to just 2x minBufferTime. The value of minBufferTime is usually at most one segment duration (it is the time duration over which the signaled average bitrate is true) so this value would end up being half the prior value. Why not make [6] be 4x minBufferTime?

@dsilhavy
Copy link
Collaborator Author

@wilaw Thanks for the input

For means of comparison the current hierarchy:

  1. If user enables suggestedPresentationDelay(off by default) and value is defined in the MPD use it (was never used because of the bug fixed in [Bug] SuggestedPresentationDelay is ignored because of wrong referenc… #3199 )
  2. If low latency is enabled set delay to 0
  3. If live delay was set by the user use this value
  4. If r parameter is present in the url use this parameter
  5. If fragment duration is know set delay to fragmentDuration * liveDelayFragmentCount (default is set to 4)
  6. Otherwise use minBufferTime * 2

Regarding your input:

  1. live delay is the point at which the player tries to find a valid segment. Leaving UTC synchronization aside player tries to find a segment with Now(DvrWindowEnd) - delay. This is correct in my opinion as the effective time shift buffer should be equal to TimeShiftBuffer - PresentationDelay.

  2. The concrete live delay in low latency mode is handled at a different point in the code. In the hierarchy defined above the delay will be 0. However, the player will use the starttime of the requested segment and the live delay to set the explicit playback time.
    const liveStartTime = request.duration < mediaPlayerModel.getLiveDelay() ? request.startTime : request.startTime + request.duration - mediaPlayerModel.getLiveDelay();
    So for instance if the starttime of the segment is 16 and we have 8 second segments with 1sec chunks and a target live delay of 4 seconds: liveStartTime will be 20 and not 16 like it is for classic segments.
    Nevertheless, I agree that the checkbox for "enable low latency mode" is redundant, we could work with the live delay and derive the mode of operation from there.

  3. DVB Service descriptors will update the internal settings of the player accordingly. To be precise, lowlatency mode is activated and livedelay is set to 3seconds in your example. Then the mechanism described in 2) is used.

  4. Agreed, we can do minBufferTime*4. I saw some Elemental Streams for which the minBufferTime was quite large though(30seconds compared to 6second segments). See Live MultiPeriod $Time$+SegmentTimeline stalls at period boundary #2946

To sum up: I did not want to touch the existing live delay logic in dash.js at this point. The goal was to prioritize in a different way than we do today (despite obvious bugfixes like #3199). In my opinion the MPD@suggestedPresentationDelay should not be disabled by default. However, concrete values provided by the user should always have priority over MPD values.

@bbert
Copy link
Contributor

bbert commented Mar 20, 2020

As far as I understand I have no objection for this part.
Juste one remark, I would initialize default setting values for which we expect a number to NaN instead of null

@dsilhavy dsilhavy merged commit 12fafca into Dash-Industry-Forum:development Mar 23, 2020
@dsilhavy dsilhavy deleted the bugfix-liveEdgeCalculation branch June 26, 2021 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants