Closed Bug 1289859 Opened 8 years ago Closed 8 years ago

Flash Player beta23.0.0.111 does not work in windowed and opaque cases (but 22.0.0.209 works properly)

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Version:
49 Branch
Platform:
x86
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: alice0775, Assigned: sotaro)

Details

Attachments

(3 files)

patch - Update PostProcessLayers() as to handle plugin quirk
7.30 KB, patch
Details | Diff | Splinter Review
no-opaque-container
1.14 KB, patch
Details | Diff | Splinter Review
patch - no-opaque-container with backout of Bug 1276403
1.46 KB, patch
Details | Diff | Splinter Review 
Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebd239b4cbfb8eff833560f72b163275ac4026d3
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160727004019

The problem does not happens on Firefox48.0 with/without e10s.
The problem happens on 49.0a2 and 50.0a1 with/without e10s.

*Flash Player 22.0.0.209 works without the problem.

Pushlog:


Steps To Reproduce:
1. Install Flash Player beta23.0.0.111
2. Open http://emk.name/test/swftxt.html
3. Try to type something with keyboard

Actual Results:
Flash Player beta23.0.0.111 is not shown if windowed and opaque property is defined

Expected Results:
All ok


And another site is also affected( http://www.adobe.com/software/flash/about/ ).
The version information box is empty.
Pushlpg:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c605b6253cdb354a69de5d31e8146b3dbdcf93bb&tochange=e35c83cf4a498271a7b4ac24acc373e77215e4fa

Triggered by: e35c83cf4a49	Sotaro Ikeda — Bug 1276403 - Fix nsPluginFrame::IsOpaque() r=mattwoodrow
Blocks: 1276403
Flags: needinfo?(sotaro.ikeda.g)
Keywords: regression
Assignee: nobody → sotaro.ikeda.g
Hmm, even when UseAsyncRendering() is true, there is a case that async rendering is not used.
Flags: needinfo?(sotaro.ikeda.g)
Do we need to check AsyncDrawingSupport flag? (See bug 1283274.)
It has become a can reproduce the bug 1275680 with hwa enabled after bug 1276403.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> Do we need to check AsyncDrawingSupport flag? (See bug 1283274.)

I locally tested AsyncDrawingSupport flag. The flag was always true when Flash Player beta23.0.0.111 is used. So, the flag could not be used for check if AsyncRendering is used.
(In reply to Alice0775 White from comment #0)
> Steps To Reproduce:
> 1. Install Flash Player beta23.0.0.111
> 2. Open http://emk.name/test/swftxt.html

When the url was shown, async rendering was actually used only on transparent mode.
nsPluginFrame needs to be opaque even when UseAsyncRendering() is true, because AsyncRendering might not be used. It seems better to revert bug 1276403 and add workaround to LayerManagerComposite.

bug 1276403 happened because layer of nsPluginFrame is created as opaque but its content could be non-opaque. If it happens, we need not to skip cleaning the region before compositing, because we skip cleaning the opaque region to improve performance.
Attachment #8775565 - Flags: review?(matt.woodrow)
[Tracking Requested - why for this release]:
should be uplifted with bug 1283274
tracking-firefox49: --- → ?
Comment on attachment 8775565 [details] [diff] [review]
patch - Update PostProcessLayers() as to handle plugin quirk

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

Why do we need to propagate the clear region up to the root before subtracting it from the opaque? Why can't we just do that immediately at the current layer level?
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> -----------------------------------------------------------------
> 
> Why do we need to propagate the clear region up to the root before
> subtracting it from the opaque? Why can't we just do that immediately at the
> current layer level?

ContainerLayer of the plugin layer owner is also set as opaque in this case. Therefore we needs to subtract the clear region from the opaque after all of the plugin layers are calculated.
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Therefore we needs to subtract the clear region from the opaque after all of
> the plugin layers are calculated.

Correction:
Therefore we needs to subtract the clear region from the opaque after all of its owner container layers' opaque regions are calculated.
Similar comment is in Bug 1276403  Comment 5.
Can we instead just ignore CONTENT_OPAQUE on ContainerLayers and only use it for leaves?
Yes, it was done in the first patch in Bug 1276403.
I meant make Layer::IsOpaque return false for all ContainerLayers so that we never add to localOpaque for ContainerLayers, only for leaf layers.
Does this work?
(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Created attachment 8775823 [details] [diff] [review]
> no-opaque-container
> 
> Does this work?

With backout of Bug 1276403, it works. But I am concerning about the performance compared to attachment 8775565 [details] [diff] [review].

When attachment 8775565 [details] [diff] [review], Bug 1275680 Comment 0 was addressed both e10s and non-e10s. But with Bug 1276403 it was addressed only on non-e10s. Somehow, Bug 1275680 Comment 0 did not addressed because of flash side problem. I saw it often during investigating Bug 1276403. I suspect that it could be related to composition performance.
Why would performance be worse?

If the ContainerLayer is actually opaque, then we should still figure that out because we visit all the children and add them to the opaque region.
(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> Why would performance be worse?
> 
> If the ContainerLayer is actually opaque, then we should still figure that
> out because we visit all the children and add them to the opaque region.

With attachment 8775840 [details] [diff] [review] and e10s is enabled, a layer of chrome process is composited as a background of the plugin's layers. The plugin's layers is created as opaque in content process, therefore, there is no visible region of other layers under plugin's layer.

This does not happen during non-e10s. The plugin's layers is created as opaque and there is no visible region of other layers under plugin's layer.
That makes it sound like we need to treat plugins as transparent in the content process if there is any chance they will be transparent during compositing.
It was done in Bug 1276403 and caused this bug, because there seems no good way to know when plugin async rendering is actually used.
When async rendering is not used and non-transparent mode, nsPluginFrame needs to be opaque.
Why can't we know if async rendering is being used?

(reply to Sotaro Ikeda [:sotaro] from comment #25)
> When async rendering is not used and non-transparent mode, nsPluginFrame
> needs to be opaque.

Why is this? It doesn't really matter *too* much if we mark a layer as transparent when it isn't really does it?
Attachment #8775565 - Flags: review?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> 
> Why is this? It doesn't really matter *too* much if we mark a layer as
> transparent when it isn't really does it?

Sorry, my analysis was wrong. When beta23.0.0.111 is used, links of comment uses async rendering.

When wmode was windowed or opaque mode, the plugin seems not render black color. Instead, it seems to render as transparent color and then background color is used as black color.
http://emk.name/test/swftxt.html in comment 0 case, when I changed background color of <body> to black via Inspector, the text was rendered correctly. When I changed the background color to red, texts of windowed or opaque mode changed to red.
When nsPluginFrame is opaque, plugin's background color becomes default clear color(#000000), then black text is shown as black.
:mattwoodrow, from Comment 28. The problem seems to be caused by beta23.0.0.111. Is it better to be addressed by adobe side? Or should we add workaround for it?
Flags: needinfo?(matt.woodrow)
(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> When nsPluginFrame is opaque, plugin's background color becomes default
> clear color(#000000),

Because there is no background layer under it. Then DrawTarget::ClearRect() in the following becomes background color of the plugin's layer.
  https://dxr.mozilla.org/mozilla-central/source/widget/CompositorWidget.cpp#51
(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> :mattwoodrow, from Comment 28. The problem seems to be caused by
> beta23.0.0.111. Is it better to be addressed by adobe side? Or should we add
> workaround for it?

Ideally we'd get them to fix it.

(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> (In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > When nsPluginFrame is opaque, plugin's background color becomes default
> > clear color(#000000),
> 
> Because there is no background layer under it. Then DrawTarget::ClearRect()
> in the following becomes background color of the plugin's layer.
>  
> https://dxr.mozilla.org/mozilla-central/source/widget/CompositorWidget.cpp#51

This makes sense I think. If layout thinks the plugin is going to be opaque, then it won't render any content behind the plugin. If the plugin is actually transparent, then we're going to end up showing the ClearRect() colour through when we shouldn't.
Flags: needinfo?(matt.woodrow)
:masayuki, do you know how to ask adobe to address comment 27?
Flags: needinfo?(masayuki)
I emailed you since it includes their email addresses.
Flags: needinfo?(masayuki)
Thanks a lot!
I heard that this bug is already addressed internally in adobe and a fix will be in next beta release.
The problem seems to be fixed in Flash Player beta 23.0.0.126.
Yes, I verified that Flash Player beta 23.0.0.126 fixed the problem.
No longer blocks: 1276403
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: unaffected → ---
status-firefox48: unaffected → ---
status-firefox49: affected → ---
status-firefox50: affected → ---
tracking-firefox49: ? → ---
Keywords: regression
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: