987076 - Construct HTMLMediaElement::mTextTrackManager lazily

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right now we construct a TextTrackManager for every HTMLMediaElement ... which goes on to create a bunch of objects of its own. This is unnecessary 99% of the time. Please fix it.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

I've removed the TextTrackManager's initialization from the MediaElement's ctor and added a getter that creates it if we don't have it yet. Functions on the MediaElement that are TextTrack specific now use this getter.

Comment 2

[Rick Eyre (:reyre)]

Attachment: v2: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

I've removed the TextTrackManager's initialization from the MediaElement's ctor and added a getter that creates it if we don't have it yet. The public functions exposed on the MediaElement's IDL now call this getter.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8395708 [details] [diff] [review]
v2: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

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

::: content/html/content/public/HTMLMediaElement.h
@@ +870,5 @@
>    void PopulatePendingTextTrackList();
>  
> +  // Gets a reference to the MediaElement's TextTrackManager. If the
> +  // MediaElement doesn't yet have one then it will create it.
> +  TextTrackManager* CreateOrGetTextTrackManager();

GetOrCreate is the more common ordering, so call it GetOrCreateTextTrackManager.

Comment 4

[Rick Eyre (:reyre)]

Attachment: v3: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

Thanks for review Robert.

Carrying forward r=roc.

Comment 5

[Rick Eyre (:reyre)]

Attachment: v4: Construct HTMLMediaElement::mTextTrackManager lazily r=roc

Forgot to update the commit message to reflect the new function name.

Carrying forward r=roc.

Comment 6

[Rick Eyre (:reyre)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d3f66aab13e2

Comment 7

[Rick Eyre (:reyre)]

Try looks green.

We also need to land this in Gecko 29 and 30 for bug 960224, correct Roc?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a509905f70

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/85a509905f70

Comment 10

[Rick Eyre (:reyre)]

Attachment: v1: Construct HTMLMediaElement::mTextTrackManager lazily r=roc (back-ported to FF29)

RyanVM pointed out that we should probably land this on Beta as well since it's landing in Aurora/B2G28. It should improve desktop as well.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 865407
User impact if declined: Adds an extra 50ms of load time to videos
Testing completed (on m-c, etc.): Completed on the base patch, not backported one.
Risk to taking this patch (and alternatives if risky): Will add 50ms of load time to pages with videos.
String or IDL/UUID changes made by this patch: N/A

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Rick, I don't see an uplift request for aurora.

Comment 12

[Rick Eyre (:reyre)]

[:sylvestre] from comment #11)
> Rick, I don't see an uplift request for aurora.

That work is being done in bug 960224 as it was the original bug for the regression. This bug was to get it into FF31, and 29 now since bug 960224 is only putting it into FF30.(In reply to Sylvestre Ledru

Comment 13

[Sylvestre Ledru [:Sylvestre]]

OK. Thanks for the information.

Can you evaluate the risk of approving this patch ?
For example, low, medium, high, thisisgoingtocrashfirefox, etc :)

Comment 14

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> OK. Thanks for the information.
> 
> Can you evaluate the risk of approving this patch ?
> For example, low, medium, high, thisisgoingtocrashfirefox, etc :)

I would say the risk of approving this patch is low. I'm 99% sure it won't crash Firefox, if it did I would be _very_ surprised. All the entry points into the code that is now being lazy loaded has guards on it and are confined to the MediaElement so even if there was an instance where we hadn't loaded it yet, but it was called, it wouldn't crash.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8397252 [details] [diff] [review]
v1: Construct HTMLMediaElement::mTextTrackManager lazily r=roc (back-ported to FF29)

OK. Thanks! Approving then.

Comment 16

[Rick Eyre (:reyre)]

No worries, thank you! :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ed874b0821b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6208bb83a93
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/d83f629b5daa