Closed
Bug 1276403
Opened 5 years ago
Closed 5 years ago
Opaque Region calculation problem in PostProcessLayers()
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
(Keywords: regression, Whiteboard: [gfx-noted])
Attachments
(2 files, 1 obsolete file)
7.23 KB,
text/plain
|
Details | |
586 bytes,
patch
|
mattwoodrow
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1275680 +++ Created based on Bug 1275680 comment 25. Handle a problem of Bug 1275680 comment 0 that is caused by Layer invalidation/composition problem.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•5 years ago
|
Summary: Garbled text within the flash element with HWA disabled → Opaque Region calculation problem in PostProcessLayers()
Assignee | ||
Comment 1•5 years ago
|
||
From analysis, LayerManagerComposite::PostProcessLayers() seemed to calculate opaque region incorrectly. When the flash video area is also handled as opque area. The area is not cleaned CompositorWidgetProxy::GetBackBufferDrawTarget(). https://dxr.mozilla.org/mozilla-central/source/widget/CompositorWidgetProxy.cpp#49
Assignee | ||
Comment 2•5 years ago
|
||
The patch addressed the problem of Bug 1275680 comment 0 when multi-process is off.
Assignee | ||
Updated•5 years ago
|
Attachment #8757563 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 3•5 years ago
|
||
Bug 1275680 was originally noticed as regression of bug 1254897. Bug 1254897 fix is in mozilla48. Until the fix, BasicCompositor with back buffer always reallocate the back buffer. It caused same effect to clean the whole buck buffer every composition. Therefore this bug was hidden until Bug 1254897 fix.
Comment 4•5 years ago
|
||
Comment on attachment 8757563 [details] [diff] [review] patch - Do not count FullyRenderedRegion() of ContainerLayer and RefLayer as opaque region Review of attachment 8757563 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for taking a while to get to this. Can you explain more about why we need to do this? If the ContainerLayer has the CONTENT_OPAQUE flag, then adding its visible region to the opaque region seems like the correct thing to do. Are we setting this flag when it's not true? This seems like it'll stop opaque regions being propagated up the layer tree and reduce the effectiveness of the occlusion computation. Do you have a layer tree dump from the broken case? ::: gfx/layers/composite/LayerManagerComposite.cpp @@ +353,5 @@ > !aLayer->HasMaskLayers() && > aLayer->IsOpaqueForVisibility()) { > + if (aLayer->IsOpaque() && > + !aLayer->AsContainerLayer() && > + !aLayer->AsRefLayer()) { RefLayer is a descendant of ContainerLayer, so we don't need to check for it explicitly.
Assignee | ||
Updated•5 years ago
|
Attachment #8757563 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 5•5 years ago
|
||
From the layer dump, ImageLayer is transparent and there is no opaque layer under the ImageLayer except ContainerLayer. Then I looked into nsPluginFrame. nsPluginFrame::IsOpaque() returned true and the ImageLayer has Layer::CONTENT_OPAQUE flag. The flag did not in the layer dump be case ImageLayerComposite::IsOpaque() return false. It judge the opacity based on current content. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageLayerComposite.cpp#158
Assignee | ||
Comment 6•5 years ago
|
||
From comment 5, nsPluginFrame and nsNPAPIPluginInstance seems to have a problem.
Assignee | ||
Updated•5 years ago
|
Attachment #8757563 -
Attachment is obsolete: true
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
Comment on attachment 8759644 [details] [diff] [review] patch - Fix nsPluginFrame::IsOpaque() Root cause seems nsPluginFrame::IsOpaque(). By applying the patch, PostProcessLayers() problem was addressed.
Attachment #8759644 -
Flags: review?(matt.woodrow)
Updated•5 years ago
|
Attachment #8759644 -
Flags: review?(matt.woodrow) → review+
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e35c83cf4a49 Fix nsPluginFrame::IsOpaque() r=mattwoodrow
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e35c83cf4a49
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Marking as affected for 48 as well based on comment 3. Is this something suitable for uplift to 49 or 48, or do you think the fix should just ride with Firefox 50?
status-firefox48:
--- → affected
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 12•5 years ago
|
||
The bug happens only when async plugin drawing is enabled. It is disabled on release and beta by Bug 1246311. Then the bug does not affect to 48(beta) now.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 13•5 years ago
|
||
Comment on attachment 8759644 [details] [diff] [review] patch - Fix nsPluginFrame::IsOpaque() Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: flush content might not be rendered correctly. [Describe test coverage new/current, TreeHerder]: locally tested. [Risks and why]: low [String/UUID change made/needed]: none
Attachment #8759644 -
Flags: approval-mozilla-aurora?
Comment on attachment 8759644 [details] [diff] [review] patch - Fix nsPluginFrame::IsOpaque() Fix for flash crashing/weirdness issue, let's uplift to 49 aurora.
Attachment #8759644 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
sotaro, at what point will we know if we're ready to re-enable async drawing on beta/release? Is that a goal for a future release? If so, maybe there is something relman or QA can do in support.
Flags: needinfo?(sotaro.ikeda.g)
Comment 16•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/33b117179d49
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15) > sotaro, at what point will we know if we're ready to re-enable async drawing > on beta/release? Is that a goal for a future release? If so, maybe there is > something relman or QA can do in support. :dvander is better anserwer than me. :dvander, can you answer the question?
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(dvander)
(In reply to Sotaro Ikeda [:sotaro] from comment #17) > (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15) > > sotaro, at what point will we know if we're ready to re-enable async drawing > > on beta/release? Is that a goal for a future release? If so, maybe there is > > something relman or QA can do in support. > > :dvander is better anserwer than me. > > :dvander, can you answer the question? I don't think we're going to enable this on beta/release for 48 - but nightly/aurora might be okay. Last I heard Adobe still has work to do before they ship direct drawing support on their release channel.
Flags: needinfo?(dvander) → needinfo?(blassey.bugs)
Comment 19•5 years ago
|
||
(In reply to David Anderson [:dvander] from comment #18) > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15) > > > sotaro, at what point will we know if we're ready to re-enable async drawing > > > on beta/release? Is that a goal for a future release? If so, maybe there is > > > something relman or QA can do in support. > > > > :dvander is better anserwer than me. > > > > :dvander, can you answer the question? > > I don't think we're going to enable this on beta/release for 48 - but > nightly/aurora might be okay. Last I heard Adobe still has work to do before > they ship direct drawing support on their release channel. We should target 48 for this
Flags: needinfo?(blassey.bugs)
Assignee | ||
Comment 20•5 years ago
|
||
This cause bug 1289859.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•5 years ago
|
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•