GNOME Bugzilla – Bug 751792
dashdemux: incorrect segment duration for a segment list
Last modified: 2016-04-05 16:05:40 UTC
If a segment list contains multiple segments, the gst_mpd_client_setup_representation function will try to limit the duration of the last segment so that it ends on the period end: if (last_media_segment && GST_CLOCK_TIME_IS_VALID (PeriodEnd)) { if (last_media_segment->start + last_media_segment->duration > PeriodEnd) { last_media_segment->duration = PeriodEnd - last_media_segment->start; GST_LOG ("Fixed duration of last segment: %" GST_TIME_FORMAT, GST_TIME_ARGS (last_media_segment->duration)); } GST_LOG ("Built a list of %d segments", last_media_segment->number); } The problem is that the limitation is attempted only for the last segment. More, if the period end is before the last segment start, the last segment will get an overflowed duration. For example, consider a list of 2 segments, each of 25s duration, but a period of only 20 seconds. The first segment will end up with a duration of 25 and the second with a duration of 18446744068709551616. I don't know what should be the correct approach if period duration is much smaller than the sum of segments. I propose that the gst_mpd_client_setup_representation function to check each segment before it is being added to the stream list (by gst_mpd_client_add_media_segment function). If the segment would end after period's end, we reduce the segment duration and we refuse to add any more segments to the stream (so that period duration is trusted more than the segment list). For the above example, we end up with a single segment of 20s duration.
Created attachment 306543 [details] [review] unit test reproducing the problem attached a patch containing a unit test that reproduces the problem
From the spec (5.3.2.1): "For any regular Period the following holds: PeriodStart reflects the actual time that should elapse after playing the media of all prior Periods in this media Presentation relative to the PeriodStart time of the first Period in the Media Presentation. The Period extends until the PeriodStart of the next Period, or until the end of the Media Presentation in the case of the last Period. More specifically, the difference between the PeriodStart time of a Period and either the PeriodStart time of the following Period, if this is not the last Period, or the value of the MPD@mediaPresentationDuration if this is the last one, is the presentation duration in Media Presentation time of the media content represented by the Representations in this Period." It seems that the media inside a period has to be limited by the next period's start or the total mpd duration. So IMHO we should restrict the duration/segments to the period's length.
Review of attachment 306543 [details] [review]: I'm looking at fixing this, and understand what the test tests, as my first patch fails the test case. ::: tests/check/elements/dash_mpd.c @@ +2232,3 @@ + /* + * Period duration is 30 seconds + * Period start is 10 seconds. Thus, period duration is 20 seconds. Here, the first duration is "end time" (so start at 10 and end at 30 yields 20 duration) ? @@ +2249,3 @@ + " <SegmentList duration=\"25\">" + " <SegmentURL" + " media=\"TestMedia0\" mediaRange=\"10-20\"" This line means this segment runs from 10 seconds to 20 seconds, right ?
For anything related to durations and timestamps in multi-period mpds, check #754222 first. The code in GIT is broken.
Review of attachment 306543 [details] [review]: ::: tests/check/elements/dash_mpd.c @@ +2232,3 @@ + /* + * Period duration is 30 seconds + * Period start is 10 seconds. Thus, period duration is 20 seconds. That's right @@ +2249,3 @@ + " <SegmentList duration=\"25\">" + " <SegmentURL" + " media=\"TestMedia0\" mediaRange=\"10-20\"" those are bytes. Media segment is the part between bytes 10 and 20 in the TestMedia0 file
(In reply to Vincent Penquerc'h from comment #3) > Review of attachment 306543 [details] [review] [review]: > > I'm looking at fixing this, and understand what the test tests, > as my first patch fails the test case. > The test proves that if you have a period of 20s and you want to squeeze into it 2 segments of 25s each, the code will fail to detect this and you will end up with a segment with overflowed duration. The implementation only checks the last segment in the list and is able to detect the problem if only the last segment end is greater than the period end. But if the second to last segment (first segment in the test) has an end time greater than the period end, the code fails to detect and correct it.
Thanks, I meant that in more details, since the test I made used that understanding about the test, and it still came out wrong. But slomo's comment probably explains why.
Slomo's comment applies to multiple periods. In this case we have only one. I don't think this is impacted by #754222
There may be something I'm missing, as I'm really unclear about the base all these durations and timestamps are from, but I think this should be changed in the unit test: - expectedDuration = duration_to_ms (0, 0, 0, 0, 0, 25, 0); + expectedDuration = duration_to_ms (0, 0, 0, 0, 0, 20, 0); This is because the first segment should be clipped (assuming it's meant to start at the start of the period (which starts at 10) and not at the same time as the period's time base (10 seconds before)). The comment also says the first segment is supposed to be 20 seconds: * We expect first segment to have a duration of 20 seconds (limited by the period) Second, the second segment should have: - assert_equals_uint64 (fragment.duration, expectedDuration * GST_MSECOND); + assert_equals_uint64 (fragment.duration, 0); Since it gets completely clipped. As for its starting point, I'm not sure what it should be, as it's entirely clipped out. With those changes, the test passes with my patch (except the second's segment's start value, which I'm not sure what makes most sense there).
(In reply to Vincent Penquerc'h from comment #9) > There may be something I'm missing, as I'm really unclear about the base all > these durations and timestamps are from, but I think this should be changed > in the unit test: > > - expectedDuration = duration_to_ms (0, 0, 0, 0, 0, 25, 0); > + expectedDuration = duration_to_ms (0, 0, 0, 0, 0, 20, 0); > > This is because the first segment should be clipped (assuming it's meant to > start at the start of the period (which starts at 10) and not at the same > time as the period's time base (10 seconds before)). The comment also says > the first segment is supposed to be 20 seconds: > > * We expect first segment to have a duration of 20 seconds (limited by > the period) > You are correct, the first segment should have a 20ms duration. > Second, the second segment should have: > > - assert_equals_uint64 (fragment.duration, expectedDuration * GST_MSECOND); > + assert_equals_uint64 (fragment.duration, 0); > > Since it gets completely clipped. > As for its starting point, I'm not sure what it should be, as it's entirely > clipped out. I think the second segment should not exist at all because it is completely clipped. The test should check that only 1 segment is present. The implementation should refuse to add segments to list if their start is greater than period end.
Created attachment 311054 [details] [review] modified unit test This is a suggested modified unit test, with fixed duration for the first segment, and discarding the second segment.
Created attachment 311055 [details] [review] clamp segment durations to each other and period end And a patch to get the test to pass.
Review of attachment 311054 [details] [review]: looks OK
(In reply to Florin Apostol from comment #8) > Slomo's comment applies to multiple periods. In this case we have only one. > I don't think this is impacted by #754222 No it also applies to single periods that have a start time not equal to 0. See my patches, especially the commit messages with the explanations and the unit test patches ;) Every period will start with a timestamp (pts) of 0, independent of it's start time. The start time is the stream time in the complete MPD composition.
Review of attachment 311055 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3408,3 @@ + GstMediaSegment *next_segment = + g_ptr_array_index (stream->segments, n + 1); + if (next_segment && next_segment->start < PeriodEnd) how could media_segment->start + media_segment->duration > PeriodEnd but next_segment->start < PeriodEnd ? Isn't it guaranteed that segment end < next segment start? If not, then we have overlapping segments.
(In reply to Florin Apostol from comment #15) > Review of attachment 311055 [details] [review] [review]: > > ::: ext/dash/gstmpdparser.c > @@ +3408,3 @@ > + GstMediaSegment *next_segment = > + g_ptr_array_index (stream->segments, n + 1); > + if (next_segment && next_segment->start < PeriodEnd) > > how could media_segment->start + media_segment->duration > PeriodEnd but > next_segment->start < PeriodEnd ? > Isn't it guaranteed that segment end < next segment start? If not, then we > have overlapping segments. We're trying to fix up segments that don't fit in the declared period in the first place. If we assume these could be wrong, then it seems fair not to assume those can't be wrong either. I'm still not very comfortable with how this (periods, segments, media durations) all fits together though, so maybe this makes less sense for those to be wrong.
(In reply to Vincent Penquerc'h from comment #16) > (In reply to Florin Apostol from comment #15) > > Review of attachment 311055 [details] [review] [review] [review]: > > > > ::: ext/dash/gstmpdparser.c > > @@ +3408,3 @@ > > + GstMediaSegment *next_segment = > > + g_ptr_array_index (stream->segments, n + 1); > > + if (next_segment && next_segment->start < PeriodEnd) > > > > how could media_segment->start + media_segment->duration > PeriodEnd but > > next_segment->start < PeriodEnd ? > > Isn't it guaranteed that segment end < next segment start? If not, then we > > have overlapping segments. > > We're trying to fix up segments that don't fit in the declared period in the > first place. If we assume these could be wrong, then it seems fair not to > assume those can't be wrong either. I'm still not very comfortable with how > this (periods, segments, media durations) all fits together though, so maybe > this makes less sense for those to be wrong. We should aim to detect any possible inconsistencies in the mpd manifest. In order to implement these checks, I would have changed the gst_mpd_client_add_media_segment function or made the checks before calling it. There, we could have checked overlaps between segments and conflicts between segment end and period end. It is more natural to check before adding to list than to add to list and later remove from it. We still should add checks for overlaps between segments and those are more easily done before adding to list than later.
It seems simpler this way. Adding the checks as the list is built makes the adding code more complex, especially as you need to know the start time of the next segment to know whether to add the first segment. Besides, the removal of clipped segments should happen only exceptionally.
Created attachment 311123 [details] [review] unit test reproducing the problem As a git patch, plus obsoleting previous versions.
Review of attachment 311123 [details] [review]: looks ok
(In reply to Vincent Penquerc'h from comment #18) > It seems simpler this way. Adding the checks as the list is built makes the > adding code more complex, especially as you need to know the start time of > the next segment to know whether to add the first segment. Besides, the > removal of clipped segments should happen only exceptionally. My idea is to check each segment against the previous one added to the list, not against the next one: before adding a segment to segment list, I would have checked it against the period end and clipped if necessary. Also, I would have checked S->t against start_time (that is, check each segment start against the previous segment end). You still need to add this check somewhere, otherwise a wrong S->t value will go undetected and mess up the segment list.
The standard says that a segment's duration must be clipped to the next segment's start time though, so I think this would not be enough. Unless you are saying reject the second segment if its start time is before the start time of the first ? That'd work too, though the standard seems to lean more towards removing the first. In any case, I prefer keeping the code in two passes (building a list, then sanity checking it), regardless of the change in logic that we may end up with.
ok, let's do it in 2 passes.
Ping
Looking at what still needs doing with this, I do not understand that comment: "I would have checked S->t against start_time (that is, check each segment start against the previous segment end).". From reading the code, S->t is a start time of a segment. Did you mean something else ? There are so many different times here that it's all really confusing.
That was my approach to do the validation before adding to list. But as we agreed to do the validation at the end, you can ignore that comment. The patch is good. It solves the problem. I don't think we need to do anything else here.
There were a couple failures in the unit test. One of them was fixed by subtracting PeriodStart to PeriodEnd, as was changed in the context since the patch was made. The other I'm less sure about: https://bugzilla.gnome.org/show_bug.cgi?id=754222 changed start times, and this causes the start time of dash_mpdparser_multipleSegmentURL to return 0, not the "expected" 10000000000. I think the unit test should change to expect 0 here. If you agree, I'll do that and push both patches.
Created attachment 325343 [details] [review] clamp segment durations to each other and period end
Review of attachment 311123 [details] [review]: looks fine
Review of attachment 325343 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +4090,3 @@ + } + last_media_segment = + g_ptr_array_index (stream->segments, stream->segments->len - 1); the list can become empty so this will make an illegal memory access. Check the len before calling g_ptr_array_index
Created attachment 325391 [details] [review] clamp segment durations to each other and period end
Created attachment 325404 [details] [review] unit test reproducing the problem I changed the expected timestamp as per my previous comment. I think it is correct, and I'll be pushing this later today if there's no further comment.
commit 8ac261841c86aecc80e0bb0d3ab6bd725bc39bb4 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Apr 1 14:36:15 2016 +0100 mpdparser: clamp segment durations to each other and period end 5.3.2.1 in the spec. https://bugzilla.gnome.org/show_bug.cgi?id=751792 commit 7d8cc26c9166e3548d3603123e301add79dc130f Author: Florin Apostol <florin.apostol@oregan.net> Date: Thu Sep 10 13:22:58 2015 +0100 dashdemux: unit testing reproducing segment duration overflow unit test reproducing https://bugzilla.gnome.org/show_bug.cgi?id=751792 With minor changes by Vincent Penquerc'h.