1306698 - When resize content/window, image under the flash plug-in is briefly display

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Alice0775 White]

Attachment: testcase

Visually not good.

Steps To reproduce:
1. Reduce the size of the window
2. Open the attached testcase
3. Double click on title bar to toggle window sizemode
   OR
   Toggle sidebar (Ctrl+B)
4. Repeat step.3

Actual Results:
Image under the flash plug-in is briefly display

Expected results:
Image should not be display

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=02c69f4f896255189ce2f9f4e0d875e383bcfbd7&tochange=a1bd47d76f71162534090485acc57866dcd55eef

Triggered by: a1bd47d76f71	David Anderson — Enable direct plugin drawing by default. (bug 1229961 part 2, r=aklotz)

Comment 2

[Benjamin Smedberg]

dvander, what's the right component for this?

Comment 3

[u554753]

I can reproduce with the following specs:
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Comment 4

[Alice0775 White]

The problem occur on AbemaTV(Japan only).

STR
1. Open https://abema.tv/
2. Click on a thumbnail
   ( Direct link https://abema.tv/now-on-air/space-shower )
3. Toggle Sidebar (Ctrl+B)

AR:
The video flickers. image under the video is briefly display

Comment 5

[u554753]

More details from Comment 3

Reproducible on Windows 10
CPU             Intel Core i7-4790 CPU @ 3.60GHz
RAM             16.0 GB
Graphics        NVIDIA GeForce GTX 745
Version 	49.0.2
Build ID 	20161019084923
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Shockwave Flash:23.0.0.162
Aynchronous Rendering Plugin - true
e10s - Disabled

Comment 6

[Jim Mathies [:jimm]]

Hey Milan, this bug is easy to reproduce using fullscreen and the test case posted. The bug reproduces the clipping issue with other content.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

It sounds like bug 1312242 could be where we describe the root cause and point to a solution, so I'll continue some conversation in there.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

We shipped 50.0b10 with dom.ipc.plugins.asyncdrawing.enabled set to false. With that default 50 should be unaffected. This is now a wontfix (in case that perf is flipped to true).

Comment 9

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #7)
> It sounds like bug 1312242 could be where we describe the root cause and
> point to a solution, so I'll continue some conversation in there.

I don't think this is the transparent background color bug, this is the layout related z-level clipping bug. I'll try to find someone in layout to take a look.

Comment 10

[Jim Mathies [:jimm]]

Hey Jet, do you have someone who can look at this? It looks layout related, some sort of clipping problem maybe?

There are prefs details for testing on the wiki - 
https://wiki.mozilla.org/Plugins/Async_Drawing_Triage

Comment 11

[Jet Villegas (inactive)]

Matt: can you have a look at the stacking order of things here (with and without async plugin drawing?) I don't recall messing with that when we enabled the feature, but I could be wrong. Thx!

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Dump of plugin output

This also looks like a flash bug.

Attached is a dump of the first surface output by the plugin (to NPN_SetCurrentAsyncSurface) after a resize.

The size of the surface is 2298x1193, but you can see that strips along the top/bottom are left transparent.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Dump of plugin output

Sorry, attached the wrong frame. This one should be visibly incorrect.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Created attachment 8806994 [details]
> Dump of plugin output
> 
> Sorry, attached the wrong frame. This one should be visibly incorrect.

Ok, so I see the entire background firefox logo image flash in and out during the resize. You're thinking this is some sort of transparency bug in flash? Any chance you could post your frame dump code so I could take a look at a longer series of frames?

Comment 15

[Jim Mathies [:jimm]]

Maybe we fail to paint the plugin bits on certain frames during the resize?

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> > Created attachment 8806994 [details]
> > Dump of plugin output
> > 
> > Sorry, attached the wrong frame. This one should be visibly incorrect.
> 
> Ok, so I see the entire background firefox logo image flash in and out
> during the resize. You're thinking this is some sort of transparency bug in
> flash? Any chance you could post your frame dump code so I could take a look
> at a longer series of frames?

Interesting, I only see parts of it flash visible. The attached png has transparent areas (which show as grey in firefox), and those are where I see the firefox logo.

I'll grab the patch off my other machine soon, but it's basically just making a call to gfxUtils::WriteAsPNG from PluginInstanceParent::RecvShowDirectBitmap (using 'source' and a unique filename).


(In reply to Jim Mathies [:jimm] from comment #15)
> Maybe we fail to paint the plugin bits on certain frames during the resize?

It doesn't look like that to me. Enabling layer borders and watching the transition in slow motion (using a screen recording) definitely shows us trying to paint the full plugin rectangle every time.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: dump-plugin-surface

Comment 18

[Matt Woodrow (:mattwoodrow)]

Having a bit of a think about possible workarounds:

If we could detect when we expect the plugin to be outputting an opaque surface, then we could manually overwrite any transparent pixels we find without an 'appropriate' opaque color.

Differentiating between invalid transparent pixels and intentional ones could be really difficult though, I'm not sure how we'd know which is which.

Picking the right color could be hard too, and might end up looking worse than the current bug.

We could also consider ignoring the first N frames from the plugin after a size transition, and just present the last frame (scaled to fit in the new size). Would need to handle the case where the plugin stops animating before we consider the incoming frames to be 'valid'.

Just leaving this bug as is until the Flash team can fix it doesn't seem that bad to me.

Comment 19

[David Parks [:handyman]]

Attachment: WIP: Hack to eliminate junk frame

Poking around a bit, I agree with Matt... and Jim.  I'm pretty sure this is two bugs.

Its a bit easier to see in debug builds, due to performance, but, following the jump to fullscreen, we are pretty much always showing a frame without the plugin (what jimm reported in comment 14), followed by a frame with a variable amount of the view as transparent junk (as mattwoodrow says in comment 16).

This patch is a silly hack to remove the junk frame.  For me (at least in my debug build), the difference is rather clear.  It looks like tearing due to Flash reading a texture that is being rendered, or something similar.  I agree with Matt that we'll need Adobe to resolve that piece.  I can confirm that Firefox isn't sending bad clip regions or window sizes or anything.  Just the (one) straightforward SetAsyncWindow call to update the position/size.

Now, the full frame of the Firefox logo is a different story.  I don't yet see how/if we ever dealt with the issue but the problem appears to be that we allow the compositor to swap buffers without drawing the plugin.  From what I see so far, we might be pushing the plugin render when it is available... and the first frame comes long before the plugin process has sent one.  I've only done this with the debugger -- I see it clearly with judiciously-timed breakpoints -- by breaking on the AsyncSetWindow call in the plugin process, then on the next RecvShowDirectBitmap call in content (which is where the first plugin frame of the new size is blitted).  At this point the browser will already be showing the Firefox logo frame, having not waited for the plugin to update.  We may have a mechanism in place for this that is just failing but I haven't found it yet.

Comment 20

[David Parks [:handyman]]

Attachment: WIP: Hack to eliminate junk frame AND missing frame

TL;DR: This patch adds code that delays the clearing of the plugin's mImageContainer until a new frame is available.

When coupled with the prior torn-frame "fix", the visual glitches vanish.  The reason is that the renderer can and does use the old rendering while it waits for the resized update.

The ClearAllFrames call is happening because Flash has told gecko to "unset" the old non-fullscreen frame (e.g. NPN_SetCurrentAsyncSurface(NULL)) but hasn't drawn new ones to show yet.  So render calls have nothing to show.

A bad idea would be to wait for Flash to call NPN_FinalizeAsyncSurface before we ClearAllFrames, which is essentially what happens if you just comment out the ClearAllFrames call.  The issue with this is that this will change the memory behavior of the plugin.  In our example, the finalize call doesn't happen until you leave the page and any window resize creates new buffers that will just accumulate.  So this won't work.

Instead, I record the need for a ClearAllFrames call whenever NPN_SetCurrentAsyncSurface(NULL) is called and, on the subsequent NPN_SetCurrentAsyncSurface(newFrame) call, I issue the recorded ClearAllFrames call.  If Flash's behavior usually looks like this, as I expect, then this is a minor and brief increase in the memory usage of the plugin.

Comment 21

[David Parks [:handyman]]

Attachment: WIP: Delay plugin's request to ClearAllFrames until a new frame is available

This patch is spelled out in comment 20.  The spec for the relevant NPAPI methods is here:
https://wiki.mozilla.org/NPAPI:AsyncDrawing#General_Async_Drawing_Mechanics

It doesn't say anything about the allocation/deallocation of memory so the new behavior still fits.

Comment 22

[David Parks [:handyman]]

Attachment: WIP: Hack to eliminate junk frame

This is, again, the hack that removes the torn frame by detecting when Flash draws only a partial dirty rect as a first frame (the first frame is by definition entirely dirty).  Note that the dirty rect provided by Flash has nothing in common with the region of the frame that was rendered... its just wrong.

The behavior is spelled out in comment 19.  We will need Adobe to address the root of the bug.  NIing jimm for that.

Comment 23

[David Parks [:handyman]]

Attachment: Remove ASSERT that forbids Direct Drawing with async plugin rendering

derp.

(This is triggered in debug builds by the STR in this bug.)

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8824549 [details] [diff] [review]
WIP: Delay plugin's request to ClearAllFrames until a new frame is available

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

Isn't this racy?

The implementation of NPN_SetCurrentAsyncSurface does a sync call back to the browser process so that when it returns to plugin code, we are guaranteeing that the browser won't be reading from the surface.

Making this asynchronous could result in the plugin trying to write new data to the surface while we're still reading from it.

I suspect that this only happens when the plugin is planning on deleting the surface (NPN_FinalizeAsyncSurface) so the race is unlikely to happen in practice, but I'm not sure if we can rely on this.

Shouldn't Flash instead just be creating the new surface and changing the current surface to that, rather than transitioning via a nullptr surface?

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +775,5 @@
>          return IPC_FAIL_NO_REASON(this);
>      }
>  
> +#ifdef OS_WIN
> +    if (mClearImagesOnNextFrame) {

We might want to process this if we get RecvFinalizeDXGISurface too.

Comment 25

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

Comment on attachment 8824568 [details] [diff] [review]
Remove ASSERT that forbids Direct Drawing with async plugin rendering

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

The code following this assert is all related to the "async" drawing model, which has all sorts of state that probably conflicts with direct drawing. They weren't designed to be mixed, so we should figure out why we're hitting the assert instead of removing it.

It looks like ShowPluginFrame can only be called from InvalidateRectDelayed, which is a task posted in AsyncShowPluginFrame [1]. We guard that IsDirectDrawing is false before posting the task. It might be possible that IsDirectDrawing is false before posting the task, but true when the task runs. If so we should just duplicate the check in InvalidateRectDelayed.

[1] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/dom/plugins/ipc/PluginInstanceChild.cpp#4091

Comment 26

[David Parks [:handyman]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> Comment on attachment 8824549 [details] [diff] [review]
>
> Isn't this racy?
> 

Err ...yes.  You have a point that the plugin can legitimately write to the surface while I am showing the cached image, which would be bad.  Altering the way Flash posts the surface would be less hacky.  NIing jimm (again) so he can also pass this on to Adobe.

----

To be clear, Flash is doing something like:

[current surface is the pre-fullscreen oldSurface]
SetCurrentAsyncSurface(null)   -- deletes oldSurface's buffer
RenderOffscreenSurface(newSurface)
SetCurrentAsyncSurface(newSurface)
FinalizeAsyncSurface(oldSurface)

and we need it to do:

[current surface is the pre-fullscreen oldSurface]
RenderOffscreenSurface(newSurface)
SetCurrentAsyncSurface(newSurface)
FinalizeAsyncSurface(oldSurface)   -- deletes oldSurface's buffer

...skipping the SetCurrentAsyncSurface(null) call, guaranteeing the current surface is never null.

Comment 27

[David Parks [:handyman]]

Attachment: Check for async switch to Direct Drawing when plugin rendering on InvalidateRect

Good call on the async mode update.  This patch also avoids the assert.

Comment 28

[Jim Mathies [:jimm]]

emailed adobe, will post back once I know more. Lets keep working toward a landable fix on our side.

Comment 29

[David Parks [:handyman]]

We are submitting the one patch: "Check for async switch to Direct Drawing when plugin rendering on InvalidateRect" and leave_open this bug to track Adobe's progress on the remaining issues.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6574c2d7611
Make sure that async changes to the plugin render mode do not cause AsyncShowPluginFrame to run when doing direct drawing. r=dvander

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Jim Mathies [:jimm]]

fixed by latest (25) flash release.

Comment 33

[Jim Mathies [:jimm]]

I'm still seeing a little bit of flicker here. David can you take a look, is there something we can land to address any remaining issues?

Comment 34

[David Parks [:handyman]]

Jimm, you may not be hallucinating as there is a real issue but I'm not certain its related to this buggy visual.

Of the two issues in this bug, the torn frame issue seems to have been fixed.  However, the other issue of surface destruction order, which seemed to be fixed in some WIP Flash builds, has returned in a new incarnation.  I'm not convinced it can cause a bad frame but its definitely incorrect.  The issue is the one that Matt and I cover in comment 24 and comment 26.

In comment 26, I said:

> To be clear, Flash is doing something like:
> 
> [current surface is the pre-fullscreen oldSurface]
> SetCurrentAsyncSurface(null)   -- deletes oldSurface's buffer
> RenderOffscreenSurface(newSurface)
> SetCurrentAsyncSurface(newSurface)
> FinalizeAsyncSurface(oldSurface)
> 
> and we need it to do:
> 
> [current surface is the pre-fullscreen oldSurface]
> RenderOffscreenSurface(newSurface)
> SetCurrentAsyncSurface(newSurface)
> FinalizeAsyncSurface(oldSurface)   -- deletes oldSurface's buffer
> 
> ...skipping the SetCurrentAsyncSurface(null) call, guaranteeing the current
> surface is never null.

By logging messages in the debugger, I see that it is now doing this:

[current surface is the pre-fullscreen oldSurface]
FinalizeAsyncSurface(oldSurface)  -- current surface is now nothing so you get background
InitAsyncSurface(newSurface)
RenderOffscreenSurface(newSurface)
SetCurrentAsyncSurface(newSurface)   -- finally, the new image is ready

...so any rendering that happens between the first and last line is "bad".  They are too eager to finalize the old surface.

(The location of the 'InitAsyncSurface' call isn't too big a deal but since it obviously needs to happen before the Render and SetCurrent calls, and because its moved to a position where that will no longer be true when they are moved about the Finalize call, its another thing that needs to be fixed.)

Additionally, this is easy to see with a debug build of Firefox because an ASSERT in the problematic FinalizeAsyncSurface call crashes the plugin. [1]  This alone is good reason to fix the bug.  I removed the ASSERT to conduct my tests.

I'm not sure this leads to the same missing frame, however, because I'm not seeing the call to clear the image cache that would leave the plugin with nothing to show.  It is specifically this call to clear the cache that the WIP patch in this bug uses to hack around the issue.  Since the 'cache clear' call doesn't seem to happen, I don't think that hack will work with the new bug. OTOH, if there really is a flicker then it must be invalidating the cached image somehow.

I do see an ugly layout on my way to fullscreen but that can't be Flash's fault -- thats going to be Gecko going fullscreen with the old layout for the old display size.  I see that on every web page.

I tested this with the Flash debug plugin but it is the latest version (25.0.0.127) so presumably that makes no difference.

[1] https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/dom/plugins/ipc/PluginInstanceChild.cpp#2914

Comment 35

[Stefan [:StefanG_QA]]

Attachment: When resize content or window, image under the flash plug-in is briefly display.png

The issue is still reproducible with the latest Nightly 32bits build on Windows 10 using the latest Flash beta build 26.0.0.123.

Please see the attached screenshot showing the issue.

Comment 36

[Chris Peterson [:cpeterson]]

I can reproduce this Flash rendering problems with this testcase on 64-bit Firefox 53 and Nightly 55 when async drawing is disabled. I'm not able to reproduce the problem with 32-bit Firefox Beta 54, with or without async drawing, on my machine. I am using Flash beta version 26.0.0.123, too.

Comment 37

[Jim Mathies [:jimm]]

Tracy, could you do some win10 testing please, see if you can make sense of this.

Repo summary:

Fx 56 x86, Win10, flash beta, async drawing enabled
Fx 56 x86, Win10, flash beta, async drawing disabled
Fx 53 x86, Win10, flash beta, async drawing disabled

Can't repo summary:

Fx 54 x86, Win10, flash beta, async drawing enabled
Fx 54 x86, Win10, flash beta, async drawing disabled
Fx 56 x86, Win7, flash release, async drawing enabled
Fx 56 x64, Win7, flash release, async drawing enabled

STR:

1) open test case in a desktop window
2) let test case fully load
3) resize the window

Comment 38

[Tracy Walker [:tracy]]

Adobe just put out a new version of Flash player for release (26.0.0.126) that is higher than the available beta version (26.0.0.123). Results as follows:

Firefox 54.0  release, 64bit, Win10, Flash Beta (26.0.0.123), asyncDrawing enabled - WFM
Firefox 54.0  release, 64bit, Win10, Flash Beta (26.0.0.123), asyncDrawing disabled - WFM (paint issue in dancer animation)
Nightly 56 (20170613), 64bit, Win10, Flash Beta (26.0.0.123), asyncDrawing enabled - WFM
Nightly 56 (20170613), 64bit, Win10, Flash Beta (26.0.0.123), asyncDrawing disabled - WFM (paint issue in dancer animation)

Firefox 54.0  release, 32bit, Win10, Flash Beta (26.0.0.123), asyncDrawing enabled - still affected by this bug
Firefox 54.0  release, 32bit, Win10, Flash Beta (26.0.0.123), asyncDrawing disabled - WFM
Nightly 56 (20170613), 32bit, Win10, Flash Beta (26.0.0.123), asyncDrawing enabled - testcase causes browser hang; no dancer
Nightly 56 (20170613), 32bit, Win10, Flash Beta (26.0.0.123), asyncDrawing disabled - testcase causes browser hang; no dancer

Firefox 54.0  release, 64bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing enabled - WFM
Firefox 54.0  release, 64bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing disabled - WFM (paint issue in dancer animation)
Nightly 56 (20170613), 64bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing enabled - WFM
Nightly 56 (20170613), 64bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing disabled - WFM (paint issue in dancer animation)

Firefox 54.0  release, 32bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing enabled - still affected by this bug
Firefox 54.0  release, 32bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing disabled - WFM
Nightly 56 (20170613), 32bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing enabled - testcase causes browser hang; no dancer
Nightly 56 (20170613), 32bit, Win10, Flash Rel. (26.0.0.126), asyncDrawing disabled - testcase causes browser hang; no dancer

In short: 
- 32bit with asyncDrawing enabled is still affected by this bug in Firefox 54.  
- There is a different bug in paint of the dancer automation with asyncDrawing disabled
- Nightly is suffering from a regression causing a temporary hang and never displaying the dancer animation of the testcase.

Comment 39

[Tracy Walker [:tracy]]

> - Nightly is suffering from a regression causing a temporary hang and never
> displaying the dancer animation of the testcase.

^^^^ is a 32bit only regression.

Comment 40

[BMO Automation]

Resolving as wont fix, plugin support deprecated in Firefox 85.