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

RESOLVED FIXED in Firefox 38

Status

()

P5
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

29 Branch
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Posted patch typed-microseconds (obsolete) — Splinter Review
This is a start towards bug 1065715, and fixes a bug where we call ConvertToByteOffset with microseconds.
Attachment #8571127 - Flags: review?(kinetik)
(Assignee)

Updated

4 years ago
Blocks: 778617
(Assignee)

Updated

4 years ago
Attachment #8571127 - Flags: review?(jyavenard)
(Assignee)

Comment 1

4 years ago
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 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.
Attachment #8571127 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 3

4 years ago
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 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.
Attachment #8571127 - Flags: review?(kinetik) → review+
(Assignee)

Comment 5

4 years ago
(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?
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 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
(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.

Updated

4 years ago
Priority: -- → P5
(Assignee)

Comment 9

4 years ago
Posted patch typed-microseconds v2 (obsolete) — Splinter Review
Attachment #8571127 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Attachment #8571635 - Attachment is obsolete: true
Blocks: 1096089
https://hg.mozilla.org/mozilla-central/rev/d08d7a1a55ba
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
Attachment #8571636 - Flags: approval-mozilla-beta?
Attachment #8571636 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
Comment on attachment 8571636 [details] [diff] [review]
typed-microseconds v2

Aurora is now 39.
Taking it for 38. It should be in beta 2.
Attachment #8571636 - Flags: approval-mozilla-beta?
Attachment #8571636 - Flags: approval-mozilla-beta+
Attachment #8571636 - Flags: approval-mozilla-aurora?
Attachment #8571636 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.