Closed
Bug 1159579
Opened 9 years ago
Closed 9 years ago
Add Interval and IntervalSet
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
Details
Attachments
(2 files, 4 obsolete files)
34.51 KB,
patch
|
Details | Diff | Splinter Review | |
794 bytes,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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 : http://en.wikipedia.org/wiki/Interval_arithmetic) and have the concept of rounding and approximation. this would be used to abstract the dom::TimeRanges "tolerance" concept.
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8600745 -
Attachment is obsolete: true
Attachment #8600745 -
Flags: review?(matt.woodrow)
Comment 3•9 years ago
|
||
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 Intersects @@ +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+
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8600755 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Add unit test comparing intersection of normalized set vs non-normalized set
Assignee | ||
Updated•9 years ago
|
Attachment #8601839 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
I should have made the destructor virtual.
Attachment #8602428 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 8•9 years ago
|
||
Oh well, can't stringify a template argument unfortunately, so incompatible with MOZ_COUNT_CTOR
Attachment #8602442 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8602428 -
Attachment is obsolete: true
Attachment #8602428 -
Flags: review?(matt.woodrow)
Updated•9 years ago
|
Attachment #8602442 -
Flags: review?(matt.woodrow) → review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f4a02e7470a2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•