1276403 - Opaque Region calculation problem in PostProcessLayers()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ 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.

Comment 1

[Sotaro Ikeda [:sotaro]]

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

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Do not count FullyRenderedRegion() of ContainerLayer and RefLayer as opaque region

The patch addressed the problem of Bug 1275680 comment 0 when multi-process is off.

Comment 3

[Sotaro Ikeda [:sotaro]]

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

[Matt Woodrow (:mattwoodrow)]

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.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: layer dump when the problem happen

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

Comment 6

[Sotaro Ikeda [:sotaro]]

From comment 5, nsPluginFrame and nsNPAPIPluginInstance seems to have a problem.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix nsPluginFrame::IsOpaque()

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8759644 [details] [diff] [review]
patch - Fix nsPluginFrame::IsOpaque()

Root cause seems nsPluginFrame::IsOpaque(). By applying the patch, PostProcessLayers() problem was addressed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e35c83cf4a49
Fix nsPluginFrame::IsOpaque() r=mattwoodrow

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e35c83cf4a49

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 12

[Sotaro Ikeda [:sotaro]]

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.

Comment 13

[Sotaro Ikeda [:sotaro]]

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

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8759644 [details] [diff] [review]
patch - Fix nsPluginFrame::IsOpaque()

Fix for flash crashing/weirdness issue, let's uplift to 49 aurora.

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33b117179d49

Comment 17

[Sotaro Ikeda [:sotaro]]

(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?

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

(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.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 20

[Sotaro Ikeda [:sotaro]]

This cause bug 1289859.