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

Implement duration change for MPD Validity Expiration events #2304

Merged
merged 3 commits into from
Jan 22, 2018

Conversation

robertbryer
Copy link
Contributor

Implements the event duration portion of the spec for emsg MPD Validity Expiration events, so the event duration field can be used to set the remaining duration of a live stream. It also allows a stream to be ended at the event time when presentation_time_delta and event_duration are 0.

It does not implement the checking of the message_data field or the presentation_time_delta field to verify that the reloaded manifest has the correct validity

@@ -181,7 +205,9 @@ function EventController() {
activeEvents[eventId] = curr;
}
if (curr.eventStream.schemeIdUri == MPD_RELOAD_SCHEME && curr.eventStream.value == MPD_RELOAD_VALUE) {
refreshManifest();
if (event.duration !== 0 || event.presentationTimeDelta !== 0) { //If both are set to zero, it indicates the media is over at this point. Don't reload the manifest.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertbryer, I think this should be:
if (curr.duration !== 0 || curr.presentationTimeDelta !== 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, dumb mistake, thanks.

@epiclabsDASH
Copy link
Contributor

It also causes a weird effect in ad-insertion/inband example. First period plays just for 5-6 seconds instead of 20 and because duration is updated during playback, seeking bar reacts differently.

@robertbryer
Copy link
Contributor Author

For the ad-insertion inband example, I find this as the event

"{"duration":1,
"presentationTime":5,
"id":1,
 ......,
"presentationTimeDelta":1}"

So the first period should have a duration of 6; at least until the new manifest comes in and says otherwise; so I think there's something about the manifest refresh that's not working here - I'll take a look and see what's going on.

@epiclabsDASH
Copy link
Contributor

Ok, looking at the emsg box, event_duration is set to 1. That seems to be wrong and the reason behind not playing the full asset after your changes being applied.

I would say event_duration parameter of emsg box should be 20. @dsilhavy, as the one that added the inband example, what do you think?

@epiclabsDASH
Copy link
Contributor

Let's wait for @dsilhavy comments before merging. I think the PR is ready but prefer don't break ad insertion examples, even although seems to be a problem with the content not with this PR.

@robertbryer
Copy link
Contributor Author

Yeah, that seems like a good idea. I think because the event duration is non-zero, this would be allowed, and we should probably not end the stream until the manifest refresh has completed. But, this might be a bit of an edge case to implement, and we might want to just accept it as a limitation until we implement the behaviour of the other fields of the event message.

@epiclabsDASH epiclabsDASH modified the milestones: v2.6.4, v2.6.5 Nov 29, 2017
@epiclabsDASH
Copy link
Contributor

@dsilhavy, do you have any feedback to share?
Thanks!

@dsilhavy
Copy link
Collaborator

Sorry I just saw this issue, I will try to take a look at it this week. Please feel free to merge your changes if this is a content problem. We will change the content accordingly if it has been created incorrectly.

@epiclabsDASH
Copy link
Contributor

Ok. I will wait for your feedback before merging. I prefer don't breaking any of the current samples.

@dsilhavy
Copy link
Collaborator

Hi guys,
thanks for sharing this issue. I agree that the event duration in the content is wrong. So the spec states:
"The event duration expresses the remaining duration of Media Presentation from the event time. If the event duration is 0, Media Presentation ends at the event time. If 0xFFFF, the media presentation duration is unknown."

In our case this means that the stream stops after 6 seconds.
From my understanding the event duration should be set either set to 15 seconds because we start the new period then, or to 35 seconds, because that is the total media duration. To be save we might set the duration to 0xFFFF.
What are your opinions on that?

Best
Daniel

@davemevans
Copy link
Contributor

IIRC, this content also needed the message_data updated to include the relevant MPD@publishTime? (It's been a while since I looked and that may have happened already).

@epiclabsDASH
Copy link
Contributor

My understanding is it should be 15 seconds.

In the practice, it would be easier to use 0xFFFF and "inherit" the duration from what is already declared in the manifest. However, I am not sure if "unknown duration" should be interpreted as "use what is declared in the manifest" or "unknown, so continue requesting segments without an end". I guess it is the second option. Probably @bbcrddave or @sandersaares have a more accurate view about this.

@sandersaares
Copy link
Member

I can try to comment but is there some place I can download this test content you reference as a zip or something like that? Or can you point out the segment containing the message in question?

@davemevans
Copy link
Contributor

davemevans commented Dec 15, 2017

Since the sample in question is really a contrived example of a live stream with ad insertion, and the DASH event is used to trigger merely an MPD update rather than end of stream, I think the correct value for the event duration in the emsg in the first part of the content (ie before the ad) is either 0xFFFF or 35 seconds.

I believe the presentation_time_delta also needs to be modified to be 15 seconds because that is the remaining duration described by the current MPD. Remember that this particular DASH event schema/value describes MPD validity - and the MPD remains valid until the end of the content in question.

@epiclabsDASH
Copy link
Contributor

@sandersaares, content is the one accessed from this sample: http://reference.dashif.org/dash.js/v2.6.4/samples/ad-insertion/inband.html

Url of the mpd has a query string parameter, which is different per user, and that is used to return one mpd content or other depending if it was the first mpd request or the second one.

@sandersaares
Copy link
Member

Okay, I see it.

I agree with @bbcrddave on all 3 points:

  • that an 0xFFFF duration is the proper value because this event is not indicating an end to the presentation.
  • that message_data must contain the PublishTime of the manifest to which the message applies to, instead of the arbitrary (and incomplete) XML blob currently in there.
  • that presentation_time_delta should be 15 (placing the event at 5+15=20 seconds, end of the first MPD)

@dsilhavy
Copy link
Collaborator

Thanks,
I agree on the duration and the message_data. One question regarding the presentation_time_delta. By setting this to 15, the event is fired at 20 seconds right? Why cant we do that earlier to give the player enough time to prebuffer segments of the new period? Also if we fire this event exactly at 20 seconds we might run into a scenario in which the player already stopped the playback because the first period has finished?

@davemevans
Copy link
Contributor

The problem comes from the fact that MPD validity expiration events are a bit of a hack on the generic event messaging system - the fields don't have the same meaning.

However, like generic event, having the events appear in the stream early does give players the opportunity to acquire the new manifest and, assuming the manifest has a publish time describing the future and the content is available, begin grabbing the content, preparing for the switch etc, or at least be aware that the content is being extended and be prepared for that.

Unfortunately, the current implementation is not capable of achieving this because it does not consider the special case of MPD validity expiration.

@dsilhavy
Copy link
Collaborator

Ok thanks Dave. I just looked at the example from the DASH spec again, and they also set the presentation_time_delta in a way that the ept of the segment and the value for the ptd match the start time of the new period.
@epiclabsDASH We will change our content as discussed. Unfortunately this might not be possible this year. Not sure if you want to hold back this pull request until the content is changed or if you merge it now.

@epiclabsDASH
Copy link
Contributor

Yep, I think makes more sense waiting. There is no issue reported that is blocked because this PR and, although there is an issue in the content, prefer to don't break the sample.

@robertbryer, out of my knowledge, is this PR blocking anything in your side?

@robertbryer
Copy link
Contributor Author

Been using this already in production for a while - nothing I need merged in urgently

@epiclabsDASH epiclabsDASH modified the milestones: v2.6.5, v2.6.6 Dec 15, 2017
@epiclabsDASH
Copy link
Contributor

@dsilhavy, any news regarding when the content could be changed?

Many thanks in advance

@dsilhavy
Copy link
Collaborator

@epiclabsDASH We are on it, should be ready within the next week.

@epiclabsDASH epiclabsDASH merged commit e0c1348 into Dash-Industry-Forum:development Jan 22, 2018
@dsilhavy
Copy link
Collaborator

@epiclabsDASH We have created new content but I have not had the chance to test it against the changes. I will let you know as soon as I have results

@epiclabsDASH
Copy link
Contributor

Great, many thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants