Closed Bug 1373537 Opened 7 years ago Closed 7 years ago

Videocontrols binding constructor triggers layout flushes

Categories

(Toolkit :: Video/Audio Controls, enhancement)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: ralin)

References

Details

Attachments

(3 files)

Bug 1373537 - WIP, remove calculations for clientWidth and instead use predetermined widths for controls, since in practice they don't change anyways.
59 bytes, text/x-review-board-request
Details
Bug 1373537 - Remove calculations for clientWidth and instead use predetermined widths for controls, since in practice they don't change anyways.
59 bytes, text/x-review-board-request
jaws
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1373537 - Fix dynamically add closed caption reftest against delay image decoding issue.
59 bytes, text/x-review-board-request
jaws
: review+
jcristau
: approval-mozilla-beta+
Details 
I came across this while profiling some stylo bits, but this seems like something that would be a general problem.  The videocontrols constructor gets clientWidth, which triggers a layout flush.  During pageload, this means we flush layout immediately after initial frame construction if there's video on the page.

Is there any way we can avoid flushing layout here?

The testcase is https://en.wikipedia.org/wiki/Barack_Obama fwiw (this is one of the testcases stylo is using to measure its stuff).
Component: Audio/Video → Audio/Video: Playback
This should be qf:p1 IMO. I actually discovered this a year ago, but was too busy to file, and then forgot about it. :-(

Here's a profile of loading the obama wikipedia page, filtered by ResolveStyleFor: http://bit.ly/2tbGPXU

If you count the big chunks, you'll notice that we style the entire document 9 times. We should fix that.
Component: Audio/Video: Playback → Video/Audio Controls
Product: Core → Toolkit
qf:p1 given the unnecessary layout flushing on all pages with <video> elements.

Jared, is this something you have time to fix? Looks like something around http://searchfox.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#380
Flags: needinfo?(jaws)
Whiteboard: [qf] → [qf:p1]
Seems like this could be a lazy getter instead?
I looked at this and think I have a solution for this, but I'm stuck on why this test is failing. I've put my WIP patch on this bug but I can't spend any more time on it right now. Anybody else should feel free to pick up my patch and either finish it or find a different way to fix it.

With this patch applied I don't see videocontrols.xml in the profiler when loading the Barack Obama wikipedia page.
Flags: needinfo?(jaws)
Added a WIP patch based on Jared's idea. I've tested on my Mac and it seems all the test are passed now, however, this patch still lack of specific pre-defined dimension for the platforms other than Mac since the length of label varies with different font types. I'll need to manually measure the width on Windows and Linux, and test with more cases to avoid potential regressions.
The mochitests look good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=207ebde482f6e55fdb316795b23c118fc772dd18, but the reftest are somehow failed by the fringes around throbber. I'll try to fix it or maybe add a fuzzy annotation if it's not a big deal.
> since the length of label varies with different font types

Can't it vary on Mac too, then?  Especially when users set accessibility prefs like min font size?
Thanks for the patch Ray. Could you test the scenario in comment 9?
Flags: needinfo?(ralin)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #9)
> > since the length of label varies with different font types
> 
> Can't it vary on Mac too, then?  Especially when users set accessibility
> prefs like min font size?

We set certain fonts for Mac: http://searchfox.org/mozilla-central/source/toolkit/themes/shared/media/videocontrols.css#329, so I guess the width should not be affected.
Flags: needinfo?(ralin)
That doesn't affect sizes.  And the user can still override that with user stylesheets, of course.
I thought the content inside binding won't apply any stylesheet from user scope. Though, I still couldn't figure what fails the reftest: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/cuEtp1EBTiS4m9sxwHLT9w/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1. The CC button does shows up, but no background-image displayed...I have no idea why would be this case, it's unlikely caused by my patch.
Looks like the reftest failure is intermittent https://treeherder.mozilla.org/#/jobs?repo=try&revision=b06b192770a58678c4bc5b67f22cc6c64583ba5e. 

Daniel, sorry to bother you. Since last time was you helped me at reftest, do you know what might intermittently cause the difference mentioned in comment 14? Thanks.
Flags: needinfo?(dholbert)
(In reply to Ray Lin[:ralin] from comment #15)
> Daniel, sorry to bother you. Since last time was you helped me at reftest,
> do you know what might intermittently cause the difference mentioned in
> comment 14? Thanks.

I don't.  If I load the testcase locally and call "doTest()", the "CC" seems to show up right away.

It's possible that the image takes an extra roundtrip through the event loop to render, for some reason.  You could test this theory by tweaking the reftest, to delay the "reftest-wait" class-removal using a setTimeout.

(If it were me, I'd probably try a 100ms setTimeout just as a diagnostic.  And if that helps, then I'd reduce it to a 0ms setTimeout, and hopefully that'll work as well.  As I recall, 0ms setTimeouts are allowed in reftests, though longer ones are frowned upon since the exact-number-dependence is fragile/flaky).
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #16)
> (In reply to Ray Lin[:ralin] from comment #15)
> > Daniel, sorry to bother you. Since last time was you helped me at reftest,
> > do you know what might intermittently cause the difference mentioned in
> > comment 14? Thanks.
> 
> I don't.  If I load the testcase locally and call "doTest()", the "CC" seems
> to show up right away.
> 
> It's possible that the image takes an extra roundtrip through the event loop
> to render, for some reason.  You could test this theory by tweaking the
> reftest, to delay the "reftest-wait" class-removal using a setTimeout.
> 
> (If it were me, I'd probably try a 100ms setTimeout just as a diagnostic. 
> And if that helps, then I'd reduce it to a 0ms setTimeout, and hopefully
> that'll work as well.  As I recall, 0ms setTimeouts are allowed in reftests,
> though longer ones are frowned upon since the exact-number-dependence is
> fragile/flaky).

Thank you so much, Daniel

setTimeout does help this case. The failure rate decreases as the delay time increases, I've tried several delay times(from 0ms ~ 2000ms), and it turns out the we should at least delay 500ms to get a stable outcome. My guess is that some of the events might no longer be blocked since we don't trigger style flushes unless size changed after patched, so the removal of "reftest-wait" comes sooner than before. Not 100% sure, but I found the event processing delay decreases via profiler.
Hi Jared,

Apart from avoiding the calculation of clientWidth with the pre-determined width in CSS, I've optimized the adjustments like removing some unnecessary adjustments for audio and preventing self-triggered reflow. Could you help me to review this patch. Thanks.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
(In reply to Ray Lin[:ralin] from comment #18)
> setTimeout does help this case. The failure rate decreases as the delay time
> increases, I've tried several delay times(from 0ms ~ 2000ms), and it turns
> out the we should at least delay 500ms to get a stable outcome.

That's too bad. :(  I would advise strongly against adding a 500ms setTimeout to pretty much any testcase.

A setTimeout with an arbitrary value like this will make this test *either* a source of randomorange, *or* (if it's high enough to avoid all possibility of randomorange) it'll just be an unfortunate compute-time-waster. (Each reftest normally takes a few milliseconds, and they'll only get faster as hardware improves in the future.  So it sucks to shackle a particular testcase to *always* require a particular large time value [especially on the order of seconds], for every reftest run on every treeherder run from now until forever.

The ideal workaround here is to have an event you can listen for.  There might not be an author-exposed event that you've got access to from a reftest, though... I'm not sure.

A slightly more painful (but definitely viable) alternative would be to write this test as a mochitest with WindowSnapshot.js, and take snapshots of the testcase on every "requestAnimationFrame" iteration, until you get a testcase-snapshot that matches the reference snapshot.  (And time out after some large amount of time.)  That would have the benefit of "passing quickly" for good builds.

Or if this is just an image being decoded (for the "CC") image, you could maybe have another <video> element in the page with "visibility:hidden" which does have the CC image displayed up-front, to trigger decoding earlier, so that the image is ready to paint immediately when the "real" part of the testcase needs it...
(In reply to Daniel Holbert [:dholbert] from comment #20)
> A setTimeout with an arbitrary value like this will make this test *either*
> a source of randomorange, *or* (if it's high enough to avoid all possibility
> of randomorange) it'll just be an unfortunate compute-time-waster. (Each
> reftest normally takes a few milliseconds, and they'll only get faster as
> hardware improves in the future.  So it sucks to shackle a particular
> testcase to *always* require a particular large time value [especially on
> the order of seconds], for every reftest run on every treeherder run from
> now until forever.
> 
> The ideal workaround here is to have an event you can listen for.  There
> might not be an author-exposed event that you've got access to from a
> reftest, though... I'm not sure.
> 
> A slightly more painful (but definitely viable) alternative would be to
> write this test as a mochitest with WindowSnapshot.js, and take snapshots of
> the testcase on every "requestAnimationFrame" iteration, until you get a
> testcase-snapshot that matches the reference snapshot.  (And time out after
> some large amount of time.)  That would have the benefit of "passing
> quickly" for good builds.
> 
Thanks! Learned a lot from your useful suggestions :D

> Or if this is just an image being decoded (for the "CC") image, you could
> maybe have another <video> element in the page with "visibility:hidden"
> which does have the CC image displayed up-front, to trigger decoding
> earlier, so that the image is ready to paint immediately when the "real"
> part of the testcase needs it...

I tried this approach and the result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9f0166c552c424a53c228872018ce2e0ac2429, thanks again.
Comment on attachment 8882098 [details]
Bug 1373537 - Fix dynamically add closed caption reftest against delay image decoding issue.

https://reviewboard.mozilla.org/r/153196/#review158516
Attachment #8882098 - Flags: review?(jaws) → review+
Comment on attachment 8880687 [details]
Bug 1373537 - Remove calculations for clientWidth and instead use predetermined widths for controls, since in practice they don't change anyways.

https://reviewboard.mozilla.org/r/152024/#review158522

r=me with the following issues fixed

::: toolkit/content/widgets/videocontrols.xml:1631
(Diff revision 4)
>          let win = doc.defaultView;
>          return doc.mozSyntheticDocument && win === win.top;
>        },
>  
>        controlBarMinHeight: 40,
> -      adjustedVideoSize: {},
> +      controlBarMinVisableHeight: 28,

spelling error, controlBarMinVisibleHeight instead of controlBarMinVisableHeight

::: toolkit/themes/shared/media/videocontrols.css:102
(Diff revision 4)
>  .playButton,
>  .muteButton,
>  .closedCaptionButton,
>  .fullscreenButton {
>    height: 100%;
> -  min-height: 30px;
> +  min-height: var(--playButton-width);

This should be --playButton-height

::: toolkit/themes/shared/media/videocontrols.css:380
(Diff revision 4)
>  }
>  
>  /* Overlay Play button */
>  .clickToPlay {
> -  min-width: 48px;
> -  min-height: 48px;
> +  min-width: var(--clickToPlay-width);
> +  min-height: var(--clickToPlay-width);

This should be --clickToPlay-height
Attachment #8880687 - Flags: review?(jaws) → review+
Issues fixed, thanks for the review.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4c14759189b
Remove calculations for clientWidth and instead use predetermined widths for controls, since in practice they don't change anyways. r=jaws
https://hg.mozilla.org/integration/autoland/rev/62f4ce5004d4
Fix dynamically add closed caption reftest against delay image decoding issue. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4c14759189b
https://hg.mozilla.org/mozilla-central/rev/62f4ce5004d4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1368639
Is this something we should consider uplifting to Beta in the interests of fixing bug 1367846?
Flags: needinfo?(ralin)
Comment on attachment 8880687 [details]
Bug 1373537 - Remove calculations for clientWidth and instead use predetermined widths for controls, since in practice they don't change anyways.

Approval Request Comment
[Feature/Bug causing the regression]: media controls' initial process
[User impact if declined]: 
1. highly increase the loading for layout computation.
2. the media controls would be invisible if the given height is smaller than 40px. See bug 1367846
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: please uplift "Bug 1373537 - Fix dynamically add closed caption reftest against delay image decoding issue." as well
[Is the change risky?]: medium 
[Why is the change risky/not risky?]: refactored whole size adjustment functions, but no major regression was reported in the last two weeks.
[String changes made/needed]: none

Thanks.
Flags: needinfo?(ralin)
Attachment #8880687 - Flags: approval-mozilla-beta?
Comment on attachment 8882098 [details]
Bug 1373537 - Fix dynamically add closed caption reftest against delay image decoding issue.

let's go ahead and land these for 55.0b9

fix a regression in media controls
Attachment #8882098 - Flags: approval-mozilla-beta+
Attachment #8880687 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thank you Julien, Ryan :D
Depends on: 1379030
(In reply to Ray Lin[:ralin] from comment #32)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on Ray Lin's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Depends on: 1382218
Depends on: 1391791
Depends on: 1610899
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: