Closed Bug 1199371 Opened 5 years ago Closed 4 years ago

Don't create TextureClients for video when the video isn't visible

Categories

(Core :: Graphics: Layers, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

Attached patch image-bridge-idle (obsolete) — Splinter Review
Playing youtube videos in background tabs spends a lot of CPU time uploading to the GPU.

Skipping this seems like the right thing to do.
Attachment #8653623 - Flags: review?(nical.bugzilla)
Comment on attachment 8653623 [details] [diff] [review]
image-bridge-idle

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

A comment explaining that we keep track of whether the ImageContainer is "attached" to avoid sending TextureClients that won't be displayed would be very nice.
Attachment #8653623 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/446055bf8ee4
https://hg.mozilla.org/mozilla-central/rev/aef5095f71da
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
sorry had to back this out since this might have made OS X reftest failung more frequently in https://treeherder.mozilla.org/logviewer.html#?job_id=13639839&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(matt.woodrow)
Resolution: FIXED → ---
We need to synchronously send the message to the image bridge thread, otherwise we have a race condition where the main thread can schedule a composite before the ImageContainer gets updated.
Attachment #8653623 - Attachment is obsolete: true
Flags: needinfo?(matt.woodrow)
Attachment #8659919 - Flags: review?(nical.bugzilla)
Comment on attachment 8659919 [details] [diff] [review]
Image Bridge Idle

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

That's a bit sad (having to rely on more synchronous communication). Can't we do this only for stuff that comes from a main-thread transaction ? we have ImageContainer::SetCurrentImageInTransaction.
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> We need to synchronously send the message to the image bridge thread,
> otherwise we have a race condition where the main thread can schedule a
> composite before the ImageContainer gets updated.

Isn't this in fact a problem with the test rather than gecko ? ImageBridge is for things which don't require to be in sync with layer transactions. If we want to make the test work we should fix the test rather than add synchronous roundtrips on gecko's busiest thread.
Comment on attachment 8659919 [details] [diff] [review]
Image Bridge Idle

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

Scratch my last comment, I misunderstood the a crucial part of the patch in my review and Matt made a valid point about the first paint being synchronous so that there is content in the first composition after an image layer has been made visible.
Attachment #8659919 - Flags: review?(nical.bugzilla) → review+
Matt, did the patch in comment 10 ever get landed?
Flags: needinfo?(matt.woodrow)
Whiteboard: [gfx-noted]
Nope. I don't think we need it any more though since we're working on disabling the decoder for videos that aren't visible.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.