Last Comment Bug 657874 - Flash movie with w_mode shows wrong color on a screen with xBGR visual
: Flash movie with w_mode shows wrong color on a screen with xBGR visual
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla7
Assigned To: Ginn Chen
:
Mentors:
Depends on:
Blocks: 556487 626602
  Show dependency treegraph
 
Reported: 2011-05-18 00:36 PDT by Ginn Chen
Modified: 2011-06-01 19:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
transparent case screenshot after patching PluginInstanceChild.cpp#l2728 (32.85 KB, image/png)
2011-05-20 00:57 PDT, Ginn Chen
no flags Details
patch (5.31 KB, patch)
2011-05-26 23:49 PDT, Ginn Chen
karlt: review+
Details | Diff | Review
patch to commit (5.43 KB, patch)
2011-05-31 20:55 PDT, Ginn Chen
karlt: review+
Details | Diff | Review

Description Ginn Chen 2011-05-18 00:36:32 PDT
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 (ni?:karlt) 2011-05-18 02:12:01 PDT
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 2011-05-18 18:34:53 PDT
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 (ni?:karlt) 2011-05-18 18:58:56 PDT
Quite possibly, but we generally do workarounds for Flash PLayer bugs.
Comment 4 Ginn Chen 2011-05-18 19:58:10 PDT
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 (ni?:karlt) 2011-05-18 20:11:44 PDT
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 2011-05-18 20:38:48 PDT
Changed
visual = defaultVisual;
to
visual = gdk_x11_visual_get_xvisual(gdk_rgb_get_visual());

same result.
Comment 7 Karl Tomlinson (ni?:karlt) 2011-05-18 21:17:59 PDT
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 2011-05-19 00:43:04 PDT
Actually, gdk_x11_visual_get_xvisual(gdk_rgb_get_visual()) and DefaultVisualOfScreen(screen) are both xBGR for my screen.
Comment 9 Karl Tomlinson (ni?:karlt) 2011-05-19 01:12:58 PDT
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 2011-05-19 01:44:22 PDT
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 2011-05-19 01:56:57 PDT
(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 (ni?:karlt) 2011-05-19 04:11:29 PDT
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 2011-05-20 00:57:58 PDT
Created attachment 533902 [details]
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 (ni?:karlt) 2011-05-22 16:30:18 PDT
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 2011-05-23 01:44:19 PDT
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 (ni?:karlt) 2011-05-23 15:02:59 PDT
(In reply to comment #15)
> Why?

I can't explain that.  Does it render appropriately in both situations?
Comment 17 Ginn Chen 2011-05-23 18:40:56 PDT
(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 2011-05-24 01:36:05 PDT
(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 (ni?:karlt) 2011-05-24 20:42:09 PDT
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 2011-05-26 23:49:26 PDT
Created attachment 535567 [details] [diff] [review]
patch
Comment 21 Karl Tomlinson (ni?:karlt) 2011-05-31 20:15:54 PDT
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 2011-05-31 20:55:09 PDT
Created attachment 536515 [details] [diff] [review]
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 (ni?:karlt) 2011-05-31 21:03:42 PDT
(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 (ni?:karlt) 2011-05-31 21:05:33 PDT
Comment on attachment 536515 [details] [diff] [review]
patch to commit

Thank you for sorting this out!
Comment 25 Ginn Chen 2011-05-31 21:14:35 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.