Closed Bug 1231564 Opened 5 years ago Closed 5 years ago

Playing youtube videos cause 60fps paint rate

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- ?
firefox44 --- affected
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: jrmuizel, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I see this on OS X Nightly with e10s
Component: Audio/Video → Audio/Video: Playback
I can see this on Windows7.
https://hg.mozilla.org/mozilla-central/rev/40b58759c962bf1a8dbf8b56a7227778dfdcfa50
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 ID:20151209030228


Steps to reproduce:
1. Set layers.acceleration.draw-fps = true
2. Open http://www.youtube.com/watch?v=BH0BNbwqNK8&webm=1&html5=1

Actual Results:
Counter displays around 60

Expected Results:
Counter displays around 45

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8eb31a08c5b66fb68e949cb47857a124261fc742&tochange=fbe8ec51aa386356cff2198d92a6e9bd5e8d6906

Triggered by: fbe8ec51aa38	David Anderson — Compute the compositor's damage region before composites, rather than layers updates. (bug 1217560, r=mattwoodrow)
Sorry spam, I am not sure the behavior of comment#1 is regression or not.
Please ignore comment#1, if I have misunderstood this bug.
Please provide better STR (or confirm Alice's findings from comment 1) if you want help tracking this down.
Flags: needinfo?(jmuizelaar)
Blocks: 1195790
Assignee: nobody → matt.woodrow
Flags: needinfo?(jmuizelaar)
Compositing ImageHosts updates the mLastFrameID variable, which we cache in LayerTreeInvalidation.

Taking the snapshot before we composite means that we store the value of the previous frame, and will then invalidate even if the image hasn't changed.
Attachment #8699787 - Flags: review?(dvander)
mInvalidRegion is only read on the following composite, so we always ended up with an invalid region and forced 60fps compositing with video playing.
Attachment #8699789 - Flags: review?(bgirard)
This makes quite a big difference to our YouTube power consumption.
Attachment #8699789 - Flags: review?(bgirard) → review+
Comment on attachment 8699787 [details] [diff] [review]
Don't take the snapshot until after compositing

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

r=me could you extend the comment with the same explanation?
Attachment #8699787 - Flags: review?(dvander) → review+
Is this potentially related to the excessive power consumption?
Matt, is this bug a regression from dvander's bug 1195790, after all? That change is in FF 44. Are your patches safe enough to uplift to 44 (Beta)?
Flags: needinfo?(matt.woodrow)
Priority: -- → P1
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> Is this potentially related to the excessive power consumption?

Yes, it is definitely related.

(In reply to Chris Peterson [:cpeterson] from comment #9)
> Matt, is this bug a regression from dvander's bug 1195790, after all? That
> change is in FF 44. Are your patches safe enough to uplift to 44 (Beta)?

Yeah, it should be fairly low risk. I'd like to let it sit on Nightly for a bit, and then we should aim to uplift where we can.
Flags: needinfo?(matt.woodrow)
https://hg.mozilla.org/mozilla-central/rev/f9637decbe5d
https://hg.mozilla.org/mozilla-central/rev/bccb7856b4c0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.