1163445 - Replace usage of dom::TimeRanges with TimeIntervals object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This will be required for using the state-mirroring machinery introduced in bug 1144481

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Keep IntervalSet normalized

Keep IntervalSet normalized. Update unit tests. Keeping it normalized in 99% of our usage will add no overhead.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Add TimeUnit infinity concept

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).

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Keep IntervalSet normalized

Keep IntervalSet normalized. Update unit tests. Keeping it normalized in 99% of our usage will add no overhead.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Add TimeUnit infinity concept

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).

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add IntervalSet::Contains(Interval) method

Add IntervalSet::Contains(Interval) method. Missed that one

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add IntervalSet::Contains(Interval) method

Wrong one :(

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Add IntervalSet::SetFuzz method

Add IntervalSet::SetFuzz method; we'll make use of it with MediaSource

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Add IntervalSet::SetFuzz method

Add IntervalSet::SetFuzz method; we'll make use of it with MediaSource. Fix constness I just noticed

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Replace dom::TimeRanges with TimeIntervals object

Replace TimeRanges with TimeIntervals in all media code. TimeRanges is now only used to return objects to the JS

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Update webref test

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.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8606724 [details] [diff] [review]
Part6. Update webref test

Actually, that one is for bug 1163485.

Comment 12

[Matt Woodrow (:mattwoodrow)]

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.

Comment 13

[Jean-Yves Avenard [:jya]]

(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.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Add TimeUnit infinity concept

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c86cc335c31
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71d9f13a37d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc62c82bb10
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe9422de17b
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a7f0fda98b

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c86cc335c31
https://hg.mozilla.org/mozilla-central/rev/d71d9f13a37d
https://hg.mozilla.org/mozilla-central/rev/7cc62c82bb10
https://hg.mozilla.org/mozilla-central/rev/0fe9422de17b
https://hg.mozilla.org/mozilla-central/rev/24a7f0fda98b