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

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1392498 - Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class.

If TimeUnits.h includes mozilla/dom/TimeRanges.h, then the build ends up
pulling in the Gecko DOM bindings, which pulls in a whole lot of JavaScript and
DOM bindings code. That makes it trickier to import GeckoMedia into Servo, and
makes Gecko's build slower, so move the code to convert TimeIntervals into
dom::TimeRanges.

Review commit: https://reviewboard.mozilla.org/r/170972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170972/

Comment 2

[Jean-Yves Avenard [:jya]]

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)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Include graph on TimeUnits.h without patch applied.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Include graph on TimeUnits.h *with* patch applied.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170972/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac74aef901ed
Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class. r=jya

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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