Closed Bug 1159579 Opened 5 years ago Closed 5 years ago

Add Interval and IntervalSet


(Core :: Audio/Video, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: jya, Assigned: jya)



(2 files, 4 obsolete files)

We need a way to represent intervals and a set of intervals.

An interval can be of any types (which of course make sense in the context of an interval of course).

IntervalSet<TimeUnit> would be closely similar to the current dom::TimeRanges object.

IntervalSet would contain an ordered list of Intervals.

An Interval could provide basic interval arithmetic operators (as per interval arithmetic : and have the concept of rounding and approximation.

this would be used to abstract the dom::TimeRanges "tolerance" concept.
Add IntervalSet and TimeIntervals object. Ideally, I would have liked to enable implicit copy constructors so we could do declarations like TimeIntervals blah = dom::TimeRanges*. However, that cause failure in the static analyzer on 10.6 even with marking the constructor MOZ_IMPLICIT.
Attachment #8600745 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Mark TimeIntervals constructor implicit so we can use syntax TimeIntervals i = ... ; just like we could do IntervalSet<T> i = ...
Attachment #8600755 - Flags: review?(matt.woodrow)
Attachment #8600745 - Attachment is obsolete: true
Attachment #8600745 - Flags: review?(matt.woodrow)
Comment on attachment 8600755 [details] [diff] [review]
Add Interval and IntervalSet objects

Review of attachment 8600755 [details] [diff] [review]:

::: dom/media/Intervals.h
@@ +79,5 @@
> +    return *this;
> +  }
> +
> +  // Basic interval arithmetic operator definition.
> +  SelfType operator+ (const SelfType& aOther) const

Do we need operator+/- for anything?

It seems like a weird operator for an interval, I'm not sure where it would be useful. For the values of T that I'm used to, it seems to be akin to adding two TimeStamps together, which doesn't make much sense.

@@ +125,5 @@
> +  {
> +    return mStart <= aOther.mStart && aOther.mEnd <= mEnd;
> +  }
> +
> +  bool Intersect(const SelfType& aOther) const


@@ +324,5 @@
> +    for (; i < mIntervals.Length() && j < other.Length();) {
> +      if (mIntervals[i].Intersect(other[j])) {
> +        intersection.AppendElement(mIntervals[i].Intersection(other[j]));
> +      }
> +      if (mIntervals[i].mEnd < other[j].mEnd) {

This requires both IntervalSets to be normalized to work correctly. Can you add a (debug only?) way of checking that this is true.
Attachment #8600755 - Flags: review?(matt.woodrow) → review+
carrying r+ (Intersect -> Intersects). Add two unit tests showing that you don't have to normalize the intervalsets before calculating the intersections. First one takes two normalized IntervalSet and calculate all the permutations possible (so 3! * 4! = 144) ; then calculates the intersections of the results and compare with the reference one.
2nd unit test takes two non-normalized array and redo the above. The output is always a normalized intervalset and always the same one.
Attachment #8600755 - Attachment is obsolete: true
Add unit test comparing intersection of normalized set vs non-normalized set
Attachment #8601839 - Attachment is obsolete: true
I should have made the destructor virtual.
Attachment #8602428 - Flags: review?(matt.woodrow)
Oh well, can't stringify a template argument unfortunately, so incompatible with MOZ_COUNT_CTOR
Attachment #8602442 - Flags: review?(matt.woodrow)
Attachment #8602428 - Attachment is obsolete: true
Attachment #8602428 - Flags: review?(matt.woodrow)
Attachment #8602442 - Flags: review?(matt.woodrow) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.