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

Updates for ATSC-DASH type of content (2nd) #1100

Merged

Conversation

wzia
Copy link

@wzia wzia commented Jan 31, 2016

Purpose is to provide update(s) to support ATSC-DASH type of content with a few characteristics:

Dynamic content with or without updates.
Playback from a local cache (high bandwidth link, low latency)
Playing from close to AST.
Playing close to live edge.
Small initial playback delay.
Transitioning over period boundaries.
...

…troller.onLiveEdgeSearchCompleted fall after the segmentAvailabilityRange.start
@wzia
Copy link
Author

wzia commented Jan 31, 2016

Description of: waqarz@057cb3f : the issue is created when trying to play content very soon after AST (less than LiveDelayFragmentCount). Previously, postponeUpdate was only being done when there were no segments available. As in the case (very soon after AST), segments are indeed available, so we don't suspend updates. As a result, we immediately start asking for segments before availability window in ScheduleController.onLiveEdgeSearchCompleted (startTime < segmentAvailabilityRange.start) and the code crashes. Hence we must wait till startTime >= segmentAvailabilityRange.start, as implemented here.

Test results:
I have used a few vectors to do regression testing, if there are other procedures, please point to me. If any other vectors shows an issue, please point to that too. All the other commits after the first one have been tested using same procedure unless if mentioned otherwise.
Dash.js_testing.xlsx

image

@davemevans
Copy link
Contributor

This (starting correctly when AST is in the future) used to work pre-refactor and there has been a regression.

I'm not sure this is the correct way to fix this.

@waqarz
Copy link
Contributor

waqarz commented Feb 1, 2016

This (starting correctly when AST is in the future)

This is NOT a fix when AST is in future, but is in recent past (less than LiveDelayFragmentCount)

I'm not sure this is the correct way to fix this.

@bcrddave What is the issue with this fix?

@davemevans
Copy link
Contributor

if (r.adaptation.period.mpd.manifest.type == 'dynamic') will almost always be true when dealing with streams with AST.

How will else if (e.error && e.error.code === DashHandler.SEGMENTS_UNAVAILABLE_ERROR_CODE) be reached to take availabilityDelay in to account?

@waqarz
Copy link
Contributor

waqarz commented Feb 1, 2016

Right, the section with else if ... can be removed. I'll review this. Thanks for pointing out.

@dsparacio
Copy link
Contributor

Hello @waqarz. How do you feel about us pushing this to the next release 2.1. We are late on 2.0 and trying to just fix critical bugs from the milestone list. We are trying to limit the PRs at this point. I can pull this in if you are absolutely confident that it will not break anything else but would prefer to push out to 2.1.

@waqarz
Copy link
Contributor

waqarz commented Feb 1, 2016

@AkamaiDASH OK, we can plan for 2.1!

@dsparacio
Copy link
Contributor

@waqarz , OK we will make sure this gets in first thing after we tag the release. It will be pulled into Dev and may need a rebase to not conflict which I can help with if needed.

@dsparacio dsparacio added this to the 2.1.0 milestone Feb 1, 2016
@dsparacio dsparacio self-assigned this Feb 1, 2016
@dsparacio
Copy link
Contributor

@waqarz and @bbcrddave , To be clear I am OK with this in 2.0 if you guys are and feel comfortable with the change. But @bbcrddave, seems like there is something broken in 2.0 regarding AST in the future. Is this being tracked anywhere?

@davemevans
Copy link
Contributor

I'm not sure if it's broken or not. In previous versions we have successfully deployed dash.js to play streams which have not yet started and, once AST has been reached, started with the correct behaviour around AST so I am surprised.

Additionally I have a concern with this particular PR which has been noted above.

I need to test the behaviour but don't have any assets right now. Will add a bug to the tracker if needs be once testing complete if there is a problem.

@waqarz
Copy link
Contributor

waqarz commented Feb 2, 2016

@bbcrddave , @AkamaiDASH
I think I owe some explanation for this part of PR, in the following:

What this PR is for?

This is for potentially multiple updates that would ensure support for ATSC-DASH type of content.
The issue we are discussing just above: Playing Close to AST, is one part of such updates.

What the current issue "Playing Close to AST" is?

The issue is that if you play at AST <= time < (AST + LiveDelayFragmentCount), the player will crash. Note that this case in not when AST is in the future (which may indeed be working), but the AST is in recent past (the upper bound is playing < (AST + LiveDelayFragmentCount), as written above.

Can this issue be seen?

I have setup a service with 1 sec segments to show this: http://54.72.87.160/DynamicEmulator/DynamicEmulation_v2/Process_mpd.php?astoffset=10&type=mpd

  • When you access this service, it will set the AST to be 2 seconds in the past from "now", which is less than the current LiveDelayFragmentCount (4 fragments, 1 second segments = 4 sec)
  • astoffset=x ; x > 0 is a configurable parameter which you can use to trim the MPD to test various playback start times in the MPD.

I have set dev version SHA 33115d5 from 29.1. here to test this URL:
http://54.72.87.160/dash.js/dash.js_es/samples/dash-if-reference-player/index.html

i.e. click this to see the issue in play, access this URL:
http://54.72.87.160/dash.js/dash.js_es/samples/dash-if-reference-player/index.html?url=http://54.72.87.160/DynamicEmulator/DynamicEmulation_v2/Process_mpd.php?astoffset=10&type=mpd

What is the reason for this issue:

postponeUpdate is only being done when there were no segments available (only when error DashHandler.SEGMENTS_UNAVAILABLE_ERROR_CODE is raised). As in the case (very soon after AST), segments are indeed available, so there is no such error and we don't suspend updates. As a result, we immediately start asking for segments before availability window in ScheduleController.onLiveEdgeSearchCompleted (startTime < segmentAvailabilityRange.start) and the code crashes.

What is the fix:

The approach I have used is not to rely on DashHandler.SEGMENTS_UNAVAILABLE_ERROR_CODE, but to wait till the live edge falls after the segment availability start.
I have deployed the fixed code here to repeat the test above: http://54.72.87.160/dash.js/dash.js_wz/samples/dash-if-reference-player/index.html?url=http://54.72.87.160/DynamicEmulator/DynamicEmulation_v2/Process_mpd.php?astoffset=10&type=mpd

How stable is this fix? What testing has been done?

URLs tested and description as provided above (in addition to the service above):

image

Test process:

image

If there are more tests to be done, please tell.

Is this PR urgent to be pulled

No, I don't have any urgency.

"else if (e.error && e.error.code === DashHandler.SEGMENTS_UNAVAILABLE_ERROR_CODE)"

was redundant, hence removed. This will have no regression.
After removing this section, import DashHandler from '../DashHandler.js' was not needed, so removed.
@dsparacio
Copy link
Contributor

@waqarz thanks for the details. Really helpful. I am going to pull PR local and test a few things. @bbcrddave please do the same if you concerns and time to test. I want to pull this in If i do not see issue with other live edge scenarios. I will wait until thursday morning to pull in. No conflicts should come be raised with this PR so should be easy to merge.

@davemevans
Copy link
Contributor

I have had a chance to test this PR and am happy that, in itself, it works and there is no regression, so I would love to see it merged.

However, whilst testing this I have seen some issues around streams with AST in the future which appear to be regression. I will open separate issue/PR for these.

@dsparacio
Copy link
Contributor

@bbcrddave thanks for checking this out. I am going to pull in as well as your PR that fixes what you mention above. Again thanks!

dsparacio pushed a commit that referenced this pull request Feb 4, 2016
Updates for ATSC-DASH type of content (2nd)
@dsparacio dsparacio merged commit 915875d into Dash-Industry-Forum:development Feb 4, 2016
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.

4 participants