657874 - Flash movie with w_mode shows wrong color on a screen with xBGR visual

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ginn Chen]

On an X screen which default visual is xBGR, start Firefox 4.0 or remote Firefox 4.0 to the display,
Open
http://www.adobe.com/devnet/flash/articles/vidtemplate_corppreso/_jcr_content/articlecontentAdobe/swfmodal.content.html

The color is wrong, blue should be red, red should be blue.


It doesn't happen with Flash movie without w_mode
It doesn't happen if ipc is disabled.
It doesn't happen with Firefox 3.6.x or Firefox 4.0 b6.

It looks like the problem is caused by Bug 556487.

Any idea where should we do the xBGR <-> xRGB conversion?

Comment 1

[Karl Tomlinson (:karlt)]

Something may have changed the visual of the surface passed to flash from an RGB visual (gdk_rgb_get_visual()) to the default visual.

Does the same problem happen with opaque and transparent plugins?
(See http://www.communitymx.com/content/source/E5141/wmodeopaque.htm)

Comment 2

[Ginn Chen]

It happens with both opague and transparent.

I'm afraid Flash may not read the visual of the surface, instead it reads the default visual of the screen.
(If so, is it a bug of Flashplayer?)

Comment 3

[Karl Tomlinson (:karlt)]

Quite possibly, but we generally do workarounds for Flash PLayer bugs.

Comment 4

[Ginn Chen]

How is the surface transferred to Flashplayer?

I've confirmed Flashplayer is using gdk_visual_get_system() to determine color masks.

Comment 5

[Karl Tomlinson (:karlt)]

This is intended to pass a surface with default visual to Flash Player:
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2403

(This should be a Flash Player only quirk.  Other plugins are not fussy about visuals.)

However, it may be worth experimenting with passing a surface with an rgb visual (from gdk_rgb_get_visual probably), here.

Comment 6

[Ginn Chen]

Changed
visual = defaultVisual;
to
visual = gdk_x11_visual_get_xvisual(gdk_rgb_get_visual());

same result.

Comment 7

[Karl Tomlinson (:karlt)]

Are you sure that path was hit?
As an experiment, I'd try changing
Visual* defaultVisual = DefaultVisualOfScreen(screen);
to
Visual* defaultVisual = gdk_x11_visual_get_xvisual(gdk_rgb_get_visual());

Comment 8

[Ginn Chen]

Actually, gdk_x11_visual_get_xvisual(gdk_rgb_get_visual()) and DefaultVisualOfScreen(screen) are both xBGR for my screen.

Comment 9

[Karl Tomlinson (:karlt)]

Oh.  Hmm.

Can you use nightlies to confirm Bug 556487 as the trigger for the regression?
That didn't actually take affect until bug 598112 landed a few days later:
http://hg.mozilla.org/mozilla-central/rev/94b9c4cb2312

Comment 10

[Ginn Chen]

plugin reftest also fails with x-test plugin with ipc enabled.

So, probably we need to fix it inside Firefox.

Failed cases are:
/modules/plugin/test/reftest/plugin-sanity.html | image comparison (==)
/modules/plugin/test/reftest/plugin-busy-alpha-zindex.html | image comparison (==)
/modules/plugin/test/reftest/plugin-background.html | image comparison (==)
/modules/plugin/test/reftest/plugin-background-2-step.html | image comparison (==)

Comment 11

[Ginn Chen]

(In reply to comment #9)
> Oh.  Hmm.
> 
> Can you use nightlies to confirm Bug 556487 as the trigger for the
> regression?
> That didn't actually take affect until bug 598112 landed a few days later:
> http://hg.mozilla.org/mozilla-central/rev/94b9c4cb2312

I just did a quick test with Flash.
I found the regression of opaque wmode is between b6 and b7, but the regression of transparent wmode is between b11 and b12.

I'll test with nightlies tomorrow.

Comment 12

[Karl Tomlinson (:karlt)]

mHelperSurface is not being used when it should be.
Looks like this condition is wrong:
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2728

(renderSurface->GetType() != gfxASurface::SurfaceTypeXlib) there can be replaced with (mHelperSurface), if you trust the logic here:
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2516

As a further improvement mCurrentSurface should be created with the default visual here when !mIsTransparent || mBackground
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2359
That will mean that mHelperSurface is usually not required.

Comment 13

[Ginn Chen]

Attachment: transparent case screenshot after patching PluginInstanceChild.cpp#l2728

The opaque regression is between changeset 54458 (Sep.21) and 54500 (Sep.22).
Bug 598112 was landed on that day.

The transparent regression is between changeset 62652 (Feb.16) and 62729 (Feb.17).
Bug 626602 was landed on that day.

Change (renderSurface->GetType() != gfxASurface::SurfaceTypeXlib) to (mHelperSurface)
fixed the opaque case, but the transparent case is totally broken.
See screenshot.

Thanks!

Comment 14

[Karl Tomlinson (:karlt)]

mBackground should be getting copied to renderSurface (mHelper surface when that exists) instead of mCurrentSurface as done here:
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2937

However, if mCurrentSurface were created with the default visual (comment 12) there would be no need for mHelperSurface in that situation and so that it would not be necessary to consider mHelperSurface when copying the background.

Comment 15

[Ginn Chen]

For http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2937
mHelperSurface ? mHelperSurface : mCurrentSurface;
will fix the problem for transparent.
(I didn't change
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l2359)

But if I run it on a xRGB screen, I found:
1) 
Open Firefox with http://www.communitymx.com/content/source/E5141/wmodetrans.htm
Close Firefox cleanly.
Restart Firefox, open http://www.communitymx.com/content/source/E5141/wmodetrans.htm

mHelperSurface is 0.

2)
Open Firefox with http://www.communitymx.com/content/source/E5141/wmodetrans.htm
Kill Firefox.
Restart Firefox, open http://www.communitymx.com/content/source/E5141/wmodetrans.htm

mHelperSurface is not 0.

Why?

BTW: Firefox would crash if RENDER extension is absent, I will file another bug.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> Why?

I can't explain that.  Does it render appropriately in both situations?

Comment 17

[Ginn Chen]

(In reply to comment #16)
> (In reply to comment #15)
> > Why?
> 
> I can't explain that.  Does it render appropriately in both situations?

Yes.

Comment 18

[Ginn Chen]

(In reply to comment #15)
It's about timing.

At first, mHelperSurface is created because mCurrentSurface is CONTENT_COLOR_ALPHA (visual is 32bits depth) and default visual is 24bits depth.

For case 2)
We do SwapSurfaces(), mCurrentSurface is nil now.
Then we get PluginInstanceChild::RecvUpdateBackground(), because mCurrentSurface is nil, we create mCurrentSurface with CONTENT_COLOR.
We will drop outdated back surface on SwapSurfaces().
But mHelperSurface is always there.

For case 1)
We create mCurrentSurface with CONTENT_COLOR_ALPHA before we get RecvUpdateBackground().
When we get RecvUpdateBackground(), mCurrentSurface is not nil.
Because (mBackground && gfxSurface::CONTENT_COLOR != mCurrentSurface->GetContentType()), we do ClearCurrentSurface(), mHelperSurface is gone.

So, it looks like we should drop mHelperSurface at some point.

Comment 19

[Karl Tomlinson (:karlt)]

Thanks for the analysis.  I assume that is with an xRGB default visual (as otherwise mHelperSurface would always be needed).

SwapSurfaces should use ClearCurrentSurface() instead of its own code here:
http://hg.mozilla.org/mozilla-central/annotate/1f3777d4ed8b/dom/plugins/ipc/PluginInstanceChild.cpp#l3334

That will correctly clear mHelperSurface when the size or contenttype changes.

There's still the issue of mHelperSurface not being cleared when mCurrentSurface becomes null because there was no mBackSurface in SwapSurfaces.  Currently, if mHelperSurface is still useful, we create it again (unnecessarily).  I guess we could check the size in MaybeCreatePlatformHelperSurface, only recreating when it's different, and clear mHelperSurface there if it is not needed.  I'd be just as happy, though, with a simple solution of clearing mHelperSurface in SwapSurfaces when mCurrentSurface becomes null from the swap.

Comment 20

[Ginn Chen]

Attachment: patch

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 535567 [details] [diff] [review]
patch

>     case NPNVSupportsWindowless:
>-#if defined(OS_LINUX) || defined(OS_WIN)
>+#if defined(OS_LINUX) || defined(OS_SOALRIS) || defined(OS_WIN)
>         *((NPBool*)aValue) = true;
> #else
>         *((NPBool*)aValue) = false;
> #endif

This should be "#if defined(OS_LINUX) || defined(MOZ_X11) || defined(OS_WIN)".
(OS_LINUX is retained for ANDROID.)

>-#if defined(OS_LINUX)
>+#if defined(OS_LINUX) || defined(OS_SOLARIS)
>     case NPNVSupportsXEmbedBool:
>         *((NPBool*)aValue) = true;
>         return NPERR_NO_ERROR;
> 
>     case NPNVToolkit:
>         *((NPNToolkitType*)aValue) = NPNVGtk2;
>         return NPERR_NO_ERROR;

And this should be just "#if defined(MOZ_X11)".

>+        if (!mIsTransparent  || mBackground) {
>+            Visual* defaultVisual = DefaultVisualOfScreen(screen);
>+            mCurrentSurface =
>+                gfxXlibSurface::Create(screen, defaultVisual,
>+                                       gfxIntSize(mWindow.width,
>+                                                  mWindow.height));
>+            return mCurrentSurface != nsnull;
>+        }
>+
>         XRenderPictFormat* xfmt = gfxXlibSurface::FindRenderFormat(dpy, format);
>         if (!xfmt) {
>             NS_ERROR("Need X falback surface, but FindRenderFormat failed");
>             return false;
>         }

Now that we know the desired format here, can you update the initialization of xfmt to the following, please?

XRenderPictFormat* xfmt = XRenderFindStandardFormat(dpy, PictStandardARGB32);

(mHelperSurface can still end up getting used unnecessarily in case 2 of comment 18, but that need not be addressed here.)

Comment 22

[Ginn Chen]

Attachment: patch to commit

I should have removed the OS_SOLARIS stuff because it doesn't belong to this bug.
Since you gave comments for them, I'll leave it here.

> (mHelperSurface can still end up getting used unnecessarily in case 2 of
> comment 18, but that need not be addressed here.)
We'll clear mHelperSurface, if (mCurrentSurface->GetContentType() != mBackSurface->GetContentType()).

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to comment #22)
> We'll clear mHelperSurface, if (mCurrentSurface->GetContentType() !=
> mBackSurface->GetContentType()).

That doesn't happen on the first swap when mCurrentSurface is NULL, but I guess it does happen on the next swap.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 536515 [details] [diff] [review]
patch to commit

Thank you for sorting this out!

Comment 25

[Ginn Chen]

(In reply to comment #23)
> (In reply to comment #22)
> > We'll clear mHelperSurface, if (mCurrentSurface->GetContentType() !=
> > mBackSurface->GetContentType()).
> 
> That doesn't happen on the first swap when mCurrentSurface is NULL, but I
> guess it does happen on the next swap.

That's right.

Comment 26

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/9a6c139a4e58