TimeUnits.h include graph pulls in DOM bindings and JavaScript via dom::TimeRanges

RESOLVED FIXED in Firefox 57

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

55 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

6 months ago
TimeUnits.h defines a TimeIntervals class, which defines operators to convert to and from dom::TimeRanges. This means TimeUnits.h #includes "mozilla/dom/TimeRanges.h", and since dom::TimeRanges is a DOM binding object that means it needs to include "nsWrapperCache.h" which includes DOM bindings stuff which eventually ends up pulling in a whole lot of JavaScript definitions and cycle collection stuff. This greatly increases the include graph, so makes importing TimeUnits.h into Servo much harder, and will also increase the C++ build time.

It would greatly reduce the amount of stuff I need to import into Servo for media playback if I can move the TimeIntervals to dom::TimeRanges out of TimeUnits.h and into TimeRanges.h.

This also separates the concerns neatly; the code to convert the Gecko media TimeIntervals to Gecko DOM bindings is in Gecko's DOM bindings, and Servo will likely need something equivalent for its own DOM bindings.
Comment hidden (mozreview-request)

Comment 2

6 months ago
mozreview-review
Comment on attachment 8899675 [details]
Bug 1392498 - Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class.

https://reviewboard.mozilla.org/r/170972/#review176204

::: dom/html/HTMLMediaElement.cpp:2870
(Diff revision 1)
>  already_AddRefed<TimeRanges>
>  HTMLMediaElement::Seekable() const
>  {
> -  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()));
> -  if (mDecoder) {
> -    mDecoder->GetSeekable().ToTimeRanges(ranges);
> +  media::TimeIntervals seekable =
> +    mDecoder ? mDecoder->GetSeekable() : media::TimeIntervals();
> +  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), seekable);

we're in the dom namespace here, there seems to be a lack of consistency in this code, where sometimes you see dom::TimeRanges and other times TimeRanges only.

Can you make it consistent everywhere please?

::: dom/html/HTMLMediaElement.cpp
(Diff revision 1)
>  HTMLMediaElement::Buffered() const
>  {
> -  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()));
> -  if (mDecoder) {
> -    media::TimeIntervals buffered = mDecoder->GetBuffered();
> +  media::TimeIntervals buffered =
> +    mDecoder ? mDecoder->GetBuffered() : media::TimeIntervals();
> +  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), buffered);
> -    if (!buffered.IsInvalid()) {

the code isn't strictly equivalent, there's no check for IsInvalid()

Though I think this was a bogus test, one that would always be true.
AFAIK, only the OggReader could generate invalid buffer ranges, and is no longer there

::: dom/html/TimeRanges.h:60
(Diff revision 1)
>    void Union(const TimeRanges* aOtherRanges, double aTolerance);
>  
>    // Mutate this TimeRange to be the intersection of this and aOtherRanges.
>    void Intersection(const TimeRanges* aOtherRanges);
>  
> -  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +  JSObject* WrapObject(JSContext* aCx,

this seems to be out of scope for the title of this change...

So please, either create a new bug, or amend the commit comment describing this change.

(though it was silly to make those members virtual, when the TimeRanges class is declared final)

::: dom/html/TimeRanges.h:70
(Diff revision 1)
>    uint32_t Length() const
>    {
>      return mRanges.Length();
>    }
>  
> -  virtual double Start(uint32_t aIndex, ErrorResult& aRv);
> +  double Start(uint32_t aIndex, ErrorResult& aRv) const;

same here

::: dom/html/TimeRanges.cpp:42
(Diff revision 1)
> +  : TimeRanges(aParent)
> +{
> +  if (aTimeIntervals.IsInvalid()) {
> +    return;
> +  }
> +  for (media::TimeIntervals::IndexType i = 0; i < aTimeIntervals.Length();

you could use the iterators there instead.

for (const media::TimeInterval& interval : aTimeIntervals) {
 Add(interval.mStart.ToSeconds(), interval.mEnd.ToSeconds());
}

::: dom/media/gtest/TestIntervalSet.cpp
(Diff revision 1)
> -  media::TimeIntervals i2 = media::TimeIntervals::FromTimeRanges(tr);
> -  CheckTimeRanges(tr, i2);
> +  RefPtr<dom::TimeRanges> tr2 = new dom::TimeRanges(tr->ToTimeIntervals());
> +  CheckTimeRanges(tr2, i1);
> -
> -  media::TimeIntervals i3;
> -  // operator=(TimeRanges*)
> -  i3 = tr;

still think you should have gtest for the operators and implement those operators. Or indicate as such in the commit comment

::: dom/media/mediasource/SourceBuffer.cpp:117
(Diff revision 1)
>    }
>    bool rangeChanged = true;
>    media::TimeIntervals intersection = mTrackBuffersManager->Buffered();
>    MSE_DEBUGV("intersection=%s", DumpTimeRanges(intersection).get());
>    if (mBuffered) {
> -    media::TimeIntervals currentValue(mBuffered);
> +    media::TimeIntervals currentValue(mBuffered->ToTimeIntervals());

would have been nicer with an operator=(dom::TimeRanges)
Attachment #8899675 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 3

6 months ago
Created attachment 8899780 [details]
Include graph on TimeUnits.h without patch applied.
(Assignee)

Comment 4

6 months ago
Created attachment 8899781 [details]
Include graph on TimeUnits.h *with* patch applied.
(Assignee)

Comment 5

6 months ago
Thanks for the fast review. I've uploaded visualizations of the before/after snapshot of the include graphs, to help make the impact of the change here clear.
(Assignee)

Updated

6 months ago
Attachment #8899780 - Attachment description: Inclulde graph on TimeUnits.h without patch applied. → Include graph on TimeUnits.h without patch applied.
(Assignee)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8899675 [details]
Bug 1392498 - Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class.

https://reviewboard.mozilla.org/r/170972/#review178338

::: dom/html/HTMLMediaElement.cpp
(Diff revision 1)
>  HTMLMediaElement::Buffered() const
>  {
> -  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()));
> -  if (mDecoder) {
> -    media::TimeIntervals buffered = mDecoder->GetBuffered();
> +  media::TimeIntervals buffered =
> +    mDecoder ? mDecoder->GetBuffered() : media::TimeIntervals();
> +  RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), buffered);
> -    if (!buffered.IsInvalid()) {

I moved the IsInvalid() check into the TimeRanges constructor, so I think the code is equivalent. We'll still generate an empty TimeRanges here in the invalid case.

::: dom/html/TimeRanges.h:70
(Diff revision 1)
>    uint32_t Length() const
>    {
>      return mRanges.Length();
>    }
>  
> -  virtual double Start(uint32_t aIndex, ErrorResult& aRv);
> +  double Start(uint32_t aIndex, ErrorResult& aRv) const;

This needed to become const for ToTimeIntervals() to be const. Otherwise ToTimeIntervals() would need to be non-const, which is yucky.

::: dom/media/gtest/TestIntervalSet.cpp
(Diff revision 1)
> -  media::TimeIntervals i2 = media::TimeIntervals::FromTimeRanges(tr);
> -  CheckTimeRanges(tr, i2);
> +  RefPtr<dom::TimeRanges> tr2 = new dom::TimeRanges(tr->ToTimeIntervals());
> +  CheckTimeRanges(tr2, i1);
> -
> -  media::TimeIntervals i3;
> -  // operator=(TimeRanges*)
> -  i3 = tr;

Sorry, these operators greatly complicate reusing this code outside of Gecko.

::: dom/media/mediasource/SourceBuffer.cpp:117
(Diff revision 1)
>    }
>    bool rangeChanged = true;
>    media::TimeIntervals intersection = mTrackBuffersManager->Buffered();
>    MSE_DEBUGV("intersection=%s", DumpTimeRanges(intersection).get());
>    if (mBuffered) {
> -    media::TimeIntervals currentValue(mBuffered);
> +    media::TimeIntervals currentValue(mBuffered->ToTimeIntervals());

Sorry, these operators greatly complicate reusing this code outside of Gecko.
Comment hidden (mozreview-request)

Comment 8

6 months ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac74aef901ed
Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class. r=jya
https://hg.mozilla.org/mozilla-central/rev/ac74aef901ed
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.