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

RESOLVED FIXED in mozilla7

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ginn Chen, Assigned: Ginn Chen)

Tracking

unspecified
mozilla7
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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?
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)
(Assignee)

Comment 2

6 years ago
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?)
Quite possibly, but we generally do workarounds for Flash PLayer bugs.
(Assignee)

Comment 4

6 years ago
How is the surface transferred to Flashplayer?

I've confirmed Flashplayer is using gdk_visual_get_system() to determine color masks.
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.
(Assignee)

Comment 6

6 years ago
Changed
visual = defaultVisual;
to
visual = gdk_x11_visual_get_xvisual(gdk_rgb_get_visual());

same result.
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());
(Assignee)

Comment 8

6 years ago
Actually, gdk_x11_visual_get_xvisual(gdk_rgb_get_visual()) and DefaultVisualOfScreen(screen) are both xBGR for my screen.
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
(Assignee)

Comment 10

6 years ago
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 (==)
(Assignee)

Comment 11

6 years ago
(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.
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.
(Assignee)

Comment 13

6 years ago
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!
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.
Blocks: 626602
(Assignee)

Comment 15

6 years ago
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.
(In reply to comment #15)
> Why?

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

Comment 17

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

Yes.
(Assignee)

Comment 18

6 years ago
(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.
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.
(Assignee)

Comment 20

6 years ago
Created attachment 535567 [details] [diff] [review]
patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #535567 - Flags: review?(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.)
Attachment #535567 - Flags: review?(karlt) → review+
(Assignee)

Comment 22

6 years ago
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()).
Attachment #535567 - Attachment is obsolete: true
Attachment #536515 - Flags: review?(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 on attachment 536515 [details] [diff] [review]
patch to commit

Thank you for sorting this out!
Attachment #536515 - Flags: review?(karlt) → review+
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Comment 26

6 years ago
http://hg.mozilla.org/mozilla-central/rev/9a6c139a4e58
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.