Closed Bug 1276403 Opened 4 years ago Closed 3 years ago

Opaque Region calculation problem in PostProcessLayers()

Categories

(Core :: Graphics, defect)

49 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- disabled
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

+++ 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.
Blocks: 1275680
No longer blocks: 1254897
No longer depends on: 1275680
Assignee: nobody → sotaro.ikeda.g
Summary: Garbled text within the flash element with HWA disabled → Opaque Region calculation problem in PostProcessLayers()
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
The patch addressed the problem of Bug 1275680 comment 0 when multi-process is off.
Attachment #8757563 - Flags: review?(matt.woodrow)
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.
Blocks: 1255703
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.
Attachment #8757563 - Flags: review?(matt.woodrow)
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
From comment 5, nsPluginFrame and nsNPAPIPluginInstance seems to have a problem.
Blocks: 1217665
Attachment #8757563 - Attachment is obsolete: true
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)
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
https://hg.mozilla.org/mozilla-central/rev/e35c83cf4a49
Status: NEW → RESOLVED
Closed: 4 years ago
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?
Flags: needinfo?(sotaro.ikeda.g)
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)
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)
(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)
(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)
Depends on: 1289859
This cause bug 1289859.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
No longer depends on: 1289859
You need to log in before you can comment on or make changes to this bug.