Closed Bug 1163445 Opened 4 years ago Closed 4 years ago

Replace usage of dom::TimeRanges with TimeIntervals object

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 6 obsolete files)

19.50 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.11 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.59 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
108.55 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.27 KB, patch
Details | Diff | Splinter Review
This will be required for using the state-mirroring machinery introduced in bug 1144481
Depends on: 1163453
Keep IntervalSet normalized. Update unit tests. Keeping it normalized in 99% of our usage will add no overhead.
Attachment #8606068 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attached patch Add TimeUnit infinity concept (obsolete) — Splinter Review
Add an TimeUnit::Infinite() object. As TimeRanges use doubles, it can store infinity, and this is a required time interval for media with inifnite duration (such as a stream). In order to save storing an extra bool, we use INT64_MAX as meaning infinity (similar as what the MediaDecoderStateMachine use).
Attachment #8606069 - Flags: review?(matt.woodrow)
Keep IntervalSet normalized. Update unit tests. Keeping it normalized in 99% of our usage will add no overhead.
Attachment #8606070 - Flags: review?(matt.woodrow)
Attachment #8606068 - Attachment is obsolete: true
Attachment #8606068 - Flags: review?(matt.woodrow)
Add an TimeUnit::Infinite() object. As TimeRanges use doubles, it can store infinity, and this is a required time interval for media with inifnite duration (such as a stream). In order to save storing an extra bool, we use INT64_MAX as meaning infinity (similar as what the MediaDecoderStateMachine use).
Attachment #8606071 - Flags: review?(matt.woodrow)
Attachment #8606069 - Attachment is obsolete: true
Attachment #8606069 - Flags: review?(matt.woodrow)
Add IntervalSet::Contains(Interval) method. Missed that one
Attachment #8606072 - Flags: review?(matt.woodrow)
Wrong one :(
Attachment #8606073 - Flags: review?(matt.woodrow)
Attachment #8606072 - Attachment is obsolete: true
Attachment #8606072 - Flags: review?(matt.woodrow)
Add IntervalSet::SetFuzz method; we'll make use of it with MediaSource
Attachment #8606074 - Flags: review?(matt.woodrow)
Add IntervalSet::SetFuzz method; we'll make use of it with MediaSource. Fix constness I just noticed
Attachment #8606078 - Flags: review?(matt.woodrow)
Attachment #8606074 - Attachment is obsolete: true
Attachment #8606074 - Flags: review?(matt.woodrow)
Replace TimeRanges with TimeIntervals in all media code. TimeRanges is now only used to return objects to the JS
Attachment #8606124 - Flags: review?(matt.woodrow)
Attached patch Part6. Update webref test (obsolete) — Splinter Review
The test now almost always succeed. However, I do get the occasional timeout every once in a while. May have to completely disable this test.
Attachment #8606724 - Flags: review?(karlt)
Attachment #8606724 - Flags: review?(karlt) → review+
Comment on attachment 8606724 [details] [diff] [review]
Part6. Update webref test

Actually, that one is for bug 1163485.
Attachment #8606724 - Attachment is obsolete: true
Attachment #8606070 - Flags: review?(matt.woodrow) → review+
Attachment #8606071 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8606071 [details] [diff] [review]
Part2. Add TimeUnit infinity concept

Review of attachment 8606071 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/TimeUnits.h
@@ +133,4 @@
>      return TimeUnit(mValue + aOther.mValue);
>    }
>    TimeUnit operator - (const TimeUnit& aOther) const {
> +    if (IsInfinite() && !aOther.IsInfinite()) {

What if !IsInfinite() && aOther.IsInfinite()? I'm not even sure what the right behaviour is there.
Attachment #8606073 - Flags: review?(matt.woodrow) → review+
Attachment #8606078 - Flags: review?(matt.woodrow) → review+
Attachment #8606124 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Comment on attachment 8606071 [details] [diff] [review]
> Part2. Add TimeUnit infinity concept
> 
> Review of attachment 8606071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/TimeUnits.h
> @@ +133,4 @@
> >      return TimeUnit(mValue + aOther.mValue);
> >    }
> >    TimeUnit operator - (const TimeUnit& aOther) const {
> > +    if (IsInfinite() && !aOther.IsInfinite()) {
> 
> What if !IsInfinite() && aOther.IsInfinite()? I'm not even sure what the
> right behaviour is there.

nonInfinite - infinite = - infinite. I think we should just assert.

I had thought of keeping a float alongside the int64_t mValue ; just so it would take care of all the oo / NaN etc... 
may be easier that way, though we would increase the size of TimeUnit by 4 bytes. which is why i didn't use a bool in the first place to indicate oo.
Blocks: 1165773
Carrying r+, assert should attempting to permorm infinity calculations. It should never be used in the current code. https://treeherder.mozilla.org/#/jobs?repo=try&revision=985297f63a88
Attachment #8606071 - Attachment is obsolete: true
Depends on: 1182418
You need to log in before you can comment on or make changes to this bug.