1138260 - Add a typed Microseconds class and use it for TrackBuffer::RangeRemoval

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P5)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: typed-microseconds

This is a start towards bug 1065715, and fixes a bug where we call ConvertToByteOffset with microseconds.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8571127 [details] [diff] [review]
typed-microseconds

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

::: dom/media/TimeUnits.h
@@ +10,5 @@
> +#include "VideoUtils.h"
> +#include "mozilla/FloatingPoint.h"
> +
> +namespace mozilla {
> +namespace media {

Note that I'm adding a new namespace here (which is generally avoided), because the classes in here are pretty likely to have naming collisions with other modules, and we already have a Microseconds typedef.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8571127 [details] [diff] [review]
typed-microseconds

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

There are hundreds more of those...

Would be unfortunate that it's only use in TrackBuffer::RangeRemoval

::: dom/media/mediasource/TrackBuffer.cpp
@@ +1032,4 @@
>          // It will be entirely emptied, can clear all data.
>          decoders[i]->GetResource()->EvictAll();
>        } else {
> +        decoders[i]->Trim(aStart.mValue);

you opened pandora's box ! I think you know must go over all of the code wherever int64_t is used and make them take a media::Microseconds instead.

Comment 3

[Matt Woodrow (:mattwoodrow)]

The plan is to convert all the code, I'll do bits and pieces as boredom allows.

Having the class in means that we can require all new code and refactorings to use it, so the problem stops getting worse.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8571127 [details] [diff] [review]
typed-microseconds

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

::: dom/media/TimeUnits.h
@@ +27,5 @@
> +  }
> +
> +  static Microseconds FromSeconds(double aValue) {
> +    if (IsInfinite(aValue)) {
> +      return Microseconds(INT64_MAX);

What about -Inf?

Check for NaN, too.

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Matthew Gregan [:kinetik] from comment #4)
> Comment on attachment 8571127 [details] [diff] [review]
> typed-microseconds
> 
> Review of attachment 8571127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/TimeUnits.h
> @@ +27,5 @@
> > +  }
> > +
> > +  static Microseconds FromSeconds(double aValue) {
> > +    if (IsInfinite(aValue)) {
> > +      return Microseconds(INT64_MAX);
> 
> What about -Inf?
> 
> Check for NaN, too.

Any preference on how you want to handle NaN? Assert, 0, INT64_MAX?

Comment 6

[Jean-Yves Avenard [:jya]]

should use 0 IMHO.

Or alternatively, have like the CheckedInt<Microseconds> used in few places, have a IsValid() method.

All places where the time could be NaN would have been handled already and this couldn't should be called, so even MOZ_ASSERT is appropriate.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8571127 [details] [diff] [review]
typed-microseconds

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

::: dom/media/TimeUnits.h
@@ +29,5 @@
> +  static Microseconds FromSeconds(double aValue) {
> +    if (IsInfinite(aValue)) {
> +      return Microseconds(INT64_MAX);
> +    }
> +    return Microseconds(int64_t(aValue * USECS_PER_S));

this will cause a problem when aValue is greater than INT64_MAX / USECS_PER_SEC and potentially returns a negative value

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Any preference on how you want to handle NaN? Assert, 0, INT64_MAX?

I think we'd want to assert, the number of places that a NaN can legitimately appear is pretty limited and only at the DOM interface boundary IIRC.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: typed-microseconds v2

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: typed-microseconds v2

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08d7a1a55ba

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d08d7a1a55ba

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8571636 [details] [diff] [review]
typed-microseconds v2

This should have been uplifted to 38 :(

Approval Request Comment
[Feature/regressing bug #]:1138260
[User impact if declined]: Will prevent some EME partner site to work properly. Invalid implementation of the range removal algorithm
[Describe test coverage new/current, TreeHerder]:been in m-c for a month.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8571636 [details] [diff] [review]
typed-microseconds v2

Aurora is now 39.
Taking it for 38. It should be in beta 2.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/67259acf0c1e