1374875 - Replace the nsRange HTMLMediaElement::mSourcePointer with something faster

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

It looks like every HTMLMediaElement (<audio> and <video> I presume) creates
a nsRange in case there are <source> children:
https://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/html/HTMLMediaElement.cpp#6362
it seems this nsRange exist for the lifetime of the HTMLMediaElement.

nsRange is a mutation observer of its root node though, which slows down
DOM changes, so I'd like to get rid of this nsRange if possible.

I think the reason for using a nsRange here is because the spec requires
the "pointer" to be updated in response to DOM changes:
https://dev.w3.org/html5/spec-preview/media-elements.html#concept-media-load-algorithm
"As nodes are inserted and removed into the media element,
/pointer/ must be updated as follows [...]"

Using a nsRange for this purpose seems like overkill, we can just
let the media element observe itself to detect such changes.

Here's a WIP patch for doing that:
https://hg.mozilla.org/try/rev/889f13632c88

Looks green so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f4158d2e9d249986863289f8e87b2d9e92cf4f

Chris, do you think something simpler like this could work?

Comment 1

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

(In reply to Mats Palmgren (:mats) from comment #0)
> It looks like every HTMLMediaElement (<audio> and <video> I presume) creates
> a nsRange in case there are <source> children:
> https://searchfox.org/mozilla-central/rev/
> 7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/html/HTMLMediaElement.cpp#6362
> it seems this nsRange exist for the lifetime of the HTMLMediaElement.
> 
> nsRange is a mutation observer of its root node though, which slows down
> DOM changes, so I'd like to get rid of this nsRange if possible.

You make a very convincing argument. :)

 
> I think the reason for using a nsRange here is because the spec requires
> the "pointer" to be updated in response to DOM changes:
> https://dev.w3.org/html5/spec-preview/media-elements.html#concept-media-load-
> algorithm
> "As nodes are inserted and removed into the media element,
> /pointer/ must be updated as follows [...]"

Yes, that matches my recollection.


> Using a nsRange for this purpose seems like overkill, we can just
> let the media element observe itself to detect such changes.
> 
> Here's a WIP patch for doing that:
> https://hg.mozilla.org/try/rev/889f13632c88
> 
> Looks green so far:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=43f4158d2e9d249986863289f8e87b2d9e92cf4f
> 
> Chris, do you think something simpler like this could work?

I think https://searchfox.org/mozilla-central/source/dom/media/test/test_media_selection.html should exercise the code changed here. If what you have done here didn't work, that test should have failed.

So provided we still observe mutations to an HTMLMediaElement's <source> children while the async load algorithm is running, as appears to be happening, then we should be good here. Ship it!

Comment 2

[Mats Palmgren (inactive)]

Attachment: Use simpler mutation observer than nsRange for media elements

Yeah, this should notify all changes in the media element subtree,
and I think we can ignore all notifications except insert/remove
(as the spec points out), and all of those that isn't a direct child
(aContainer != this).

The test_media_selection.html test pass locally and in the Try run.

BTW, this also removes the only use of SetEnableGravitationOnElementRemoval
so I'll remove that nsRange method in a follow-up bug.

Comment 3

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

Comment on attachment 8880004 [details] [diff] [review]
Use simpler mutation observer than nsRange for media elements

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

Great, thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/463d13ce3e83
Use simpler mutation observer than nsRange for media elements.  r=cpearce

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/463d13ce3e83

Comment 6

[Cătălin Badea (:catalinb)]

Can we make mSourcePointer a node pointer instead of an integer offset?
I'm working on a set of patches for bug 1330970 and providing an index on mutation notifications is going to
be expensive. Also, GetChildAt will be slower. Keeping a node reference around and using GetNextSibling()
will help avoid the perf hit.

Comment 7

[Cătălin Badea (:catalinb)]

Ah, linked the wrong bug. It's bug 651120.

Comment 8

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

(In reply to Cătălin Badea (:catalinb) from comment #6)
> Can we make mSourcePointer a node pointer instead of an integer offset?
> I'm working on a set of patches for bug 1330970 and providing an index on
> mutation notifications is going to
> be expensive.


The key thing we have to maintain here is that we need to ensure that the asynchronous HTMLMediaElement load algorithm works if HTMLSourceElement children are removed or added while it the load algorithm is running. So provided we could recover from the node being pointed to by the node pointer being removed, and provided we'd traverse nodes appended as children to the media element while the algorithm is running, I don't have a problem with this change being made.


> Also, GetChildAt will be slower. Keeping a node reference
> around and using GetNextSibling()
> will help avoid the perf hit.

Will your change affect the performance of any web facing APIs?

Comment 9

[Cătălin Badea (:catalinb)]

(In reply to Chris Pearce (:cpearce) from comment #8)
> (In reply to Cătălin Badea (:catalinb) from comment #6)
> > Can we make mSourcePointer a node pointer instead of an integer offset?
> > I'm working on a set of patches for bug 1330970 and providing an index on
> > mutation notifications is going to
> > be expensive.
> 
> 
> The key thing we have to maintain here is that we need to ensure that the
> asynchronous HTMLMediaElement load algorithm works if HTMLSourceElement
> children are removed or added while it the load algorithm is running. So
> provided we could recover from the node being pointed to by the node pointer
> being removed, and provided we'd traverse nodes appended as children to the
> media element while the algorithm is running, I don't have a problem with
> this change being made.
> 
> 
> > Also, GetChildAt will be slower. Keeping a node reference
> > around and using GetNextSibling()
> > will help avoid the perf hit.
> 
> Will your change affect the performance of any web facing APIs?
nsInode::GetChildAt becomes O(n) and IndexOf won't benefit from the same cache as before.
I'm working on fixing these issues by removing their usage as much as possible and using
caching where possible. On the other side we gain on basic tree operations such as InsertBefore and RemoveChild.