Closed Bug 613449 Opened 9 years ago Closed 9 years ago

DWM fails to composite plugin windows correctly when they're over glass margins

Categories

(Core :: Widget: Win32, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

See bug 590568 comments 93 to 104.

The standalone test app in comment #104 clearly shows that the DWM fails hard where a child window is over the glass margins. This causes at least two visible bugs in Firefox:

a) https://bugzilla.mozilla.org/attachment.cgi?id=491440
When a windowed plugin is at the edge of our window, you get a couple of rows or columns of weird pixels. I'm pretty sure this is due to the plugin being over the two-pixel margin we add for borderless glass:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#2526

b) https://bug590568.bugzilla.mozilla.org/attachment.cgi?id=491442
When you make the window very short, the entire plugin becomes subject to "the effect". I think here our "find the largest rectangle in the opaque region" analysis is choosing the URL bar as the largest rectangle and extending the glass margin over the entire content area. Probably because the 1px bottom-border of the URL bar is not marked as opaque by display list analysis, so our opaque region is split into two separate pieces.

Problem (b) is fixable by enhancing display list analysis a bit more so we know that the URL bar and the content area form a contiguous opaque rect. Or, we could modify the way we find the largest opaque rectangle to ensure it always includes all windowed plugins (if possible). I'll probably go with the display list enhancement since it should also improve performance (smaller glass margins).

Problem (a) may not be completely fixable. That extra margin was added in bug 554982. I'm not sure why 2px was chosen ... can we make it 1px? I assume we know of no other way to hide the DWM border? It's not the end of the world if we can't fix it.
Ah, I see now that with only 1px of margin some of the DWM's border does indeed leak out.
OK, so as suggested obliquely by some DWM documentation, the problem is that GDI drawing into the child window is not setting the alpha values of the child window pixels correctly. If I modify D3DTest.cpp to use AlphaBlend to draw into the child window, the problem goes away.
It seems to be possible to "fix up" window alpha values after they've been painted. This code shows how, see ChildMsgProc. But it's horrible, we have to copy the contents of the window to a DIB, set all the alpha values in the DIB to 255, then AlphaBlend the contents of the DIB back to the window. Blech.

It's probably not worth doing this just to fix part (a) of this bug.
Embedding the window inside (on top of) another child window that we paint with AlphaBlend doesn't seem to work. It seems like the DWM uses something like OPERATOR_SOURCE to composite the child windows into the parent. It seems crazy.
The approach in comment #3 could perform OK since we only have to do the fixup for the part of the child window that overlaps the glass borders, and typically those borders will only be 2 pixels wide.

But trapping WM_PAINT for all the child windows for the plugin would be really painful. Maybe we could use Windows hooks or something, but it would fall apart if more processes than the browse and the plugin-container are involved. I don't want to go down this road.
Especially since so far I'm not aware of any user-filed bugs on this issue. Flash is probably almost always drawing its window with correct alpha values.
This also moves NS_OPPOSITE_SIDE to nsStyleConst so other code can use it.
Attachment #492282 - Flags: review?(dbaron)
When a border is just painting a single filled opaque rectangle, let GetOpaqueRegion return that rectangle.

Currently, in the Firefox chrome, the row of pixels between the URL bar and the content area is a bottom-only border. This lets us know it's opaque, so we can know the whole URL bar + content area is opaque, which lets us use smaller Aero Glass margins for the chrome.
Attachment #492283 - Flags: review?(dbaron)
Very simple extension of GetLargestRectangle. We'll need this for the next patch.
Attachment #492284 - Flags: review?(tellrob)
This bug makes it important that the rectangle chosen as the basis for computing Aero Glass margins contains all plugins, so we minimize the area of plugins over the glass margins. This patch ensures that.
Attachment #492286 - Flags: review?(jmathies)
These patches fix problem (b) in comment #0. Problem (a) is partially fixed; with tabs-on-top and the URL bar showing, the top Aero Glass margin only reaches down to the top of the URL bar, so plugins in the content area are nowhere near it so "the effect" is not visible at the top edge of plugins. It's still visible on the other sides if the plugin is at the edge of the window. Hopefully it won't really show up.

Now that I've done this, I wonder if these problems are visible on any plugin except for our test plugin ... I find it hard to believe that all other plugins set alpha values in their windows, though.
Comment on attachment 492284 [details] [diff] [review]
Part 3: Add aContainingRect parameter to GetLargestRectangle

You should probably update the prose description of the algorithm as well. It's also not clear to me why Parts 1 & 2 don't fix this, based on comment 0.
Attachment #492284 - Flags: review?(tellrob) → review+
Attachment #492286 - Flags: review?(jmathies) → review+
Actually, patches 3 and 4 are a bit too hacky, I can do better.
Attached patch Part 3 v2 (obsolete) — Splinter Review
Sorry about the double review Rob. We can make SetLargestRectangle more general; there is no need to restrict the rectangle to being contained in the region (I'll use this in a revised part 4). Also, folding the in-rectangle bonus value into the area PRInt64 was a hack. Instead, use a proper pair of PRInt64s.
Attachment #492432 - Flags: review?(tellrob)
Attached patch Part 4 v2Splinter Review
We should try to find a "largest rectangle" that will include the plugin bounds *after* we've shrunk it by kGlassMarginAdjustment.
Attachment #492436 - Flags: review?(jmathies)
(In reply to comment #15)
> Created attachment 492436 [details] [diff] [review]
> Part 4 v2
> 
> We should try to find a "largest rectangle" that will include the plugin bounds
> *after* we've shrunk it by kGlassMarginAdjustment.

With this, wouldn't we end up with plugins that have borders where they overlap the side of the window? Or were those borders removed with 130078? In which case, do we still need the adjustment?
I'm not sure what you mean. We definitely still need to add kGlassMarginAdjustment to the glass margins to push the DWM's border under our opaque content.
(In reply to comment #17)
> I'm not sure what you mean. We definitely still need to add
> kGlassMarginAdjustment to the glass margins to push the DWM's border under our
> opaque content.

Hrm, Part 3 doesn't apply cleanly, so I can't test this.

I'm trying to understand if there's a negative side effect from this. The 2px adjustment hides a window border on the client area of the browser. (Which, AFAICT is actually only 1px, but that's a different discussion.) We inflate the client bounds by 2px and calculate the largest opaque region including the added margin. then we subtract the 2px from the result. Which would seem to leave us with the client area border on any side of the window where a plugin is clipped by the window frame.

If you apply these and load a plugin, then scrunch the lower frame border up so that the plugin is clipped by the frame, does the client area border suddenly show up?
..and if it does, what happens if we change kGlassMarginAdjustment to 1px?
(In reply to comment #18)
> (In reply to comment #17)
> > I'm not sure what you mean. We definitely still need to add
> > kGlassMarginAdjustment to the glass margins to push the DWM's border under our
> > opaque content.
> 
> Hrm, Part 3 doesn't apply cleanly, so I can't test this.
> 
> I'm trying to understand if there's a negative side effect from this. The 2px
> adjustment hides a window border on the client area of the browser. (Which,
> AFAICT is actually only 1px, but that's a different discussion.)

Yeah, but I tried changing it to 1px and you can see blurred border leaking out. Or something like that. It definitely looked bad. See comment #1.

> We inflate the
> client bounds by 2px and calculate the largest opaque region including the
> added margin. then we subtract the 2px from the result. Which would seem to
> leave us with the client area border on any side of the window where a plugin
> is clipped by the window frame.

No. GetLargestRectangle always returns a rectangle that's contained in the window opaque region. So if the plugin bounds + 2px margin extends outside the window opaque region, the part outside the window opaque region does not affect the result of GetLargestRectangle.

> If you apply these and load a plugin, then scrunch the lower frame border up so
> that the plugin is clipped by the frame, does the client area border suddenly
> show up?

No.
Comment on attachment 492436 [details] [diff] [review]
Part 4 v2

ok!
Attachment #492436 - Flags: review?(jmathies) → review+
Comment on attachment 492432 [details] [diff] [review]
Part 3 v2

The prose comment needs to be updated and this really ought to have tests. Otherwise looks good.
Attachment #492432 - Flags: review?(tellrob) → review-
Which prose comment needs to be updated? I did update the comment for nsRegion::GetLargestRectangle.
The big comment block above the code that describes the algorithm.
Attached patch Part 3 v3Splinter Review
Per comments
Attachment #493158 - Flags: review?(tellrob)
Attachment #493158 - Flags: review?(tellrob) → review+
Comment on attachment 492282 [details] [diff] [review]
Part 1: Move border-side opacity checking to nsStyleBorder

r=dbaron
Attachment #492282 - Flags: review?(dbaron) → review+
Attachment #492432 - Attachment is obsolete: true
Comment on attachment 492283 [details] [diff] [review]
Part 2: treat single-side borders as opaque areas when possible

>+      switch (i) {
>+      case eSideTop: aResult->height = m; break;
>+      case eSideBottom: aResult->y = aResult->YMost() - m; aResult->height = m; break;
>+      case eSideLeft: aResult->width += m; break;

This should be = rather than +=.

>+      case eSideRight: aResult->x = aResult->XMost() - m; aResult->width = m; break;
>+      default:
>+        NS_ERROR("Bad side value");
>+        break;

Given that the other cases are on one line, put default: ... break; all on one line too.

>+      }
>+      *aSide = i;
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;
>+}
>+
>+nsRegion
>+nsDisplayBorder::GetOpaqueRegion(nsDisplayListBuilder* aBuilder,
>+                                 PRBool* aOutTransparentBackground)

The parameter name here disagrees with aForceTransparentSurface in part 3 of bug 602757.  (Not sure which way you want to converge.)

r=dbaron with those things fixed
Attachment #492283 - Flags: review?(dbaron) → review+
Build 20101208. D3D9. Hardware acceleration back on. Flash plugin no longer overlays as such when scrolling up, however with multiple tabs open at the time, the tabs in the immediate area above the plugin are 'wiped / erased'. Hope that's clear - if not I'll clarify.
A screenshot would be great.
(In reply to comment #29)
> A screenshot would be great.

http://img535.imageshack.us/img535/4316/09122010142821.jpg

I also get this:

http://img716.imageshack.us/img71
Sorry that last link doesn't work. Try this one:

http://img716.imageshack.us/img716/6695/09122010143316.jpg
Still present as above in 20101210...
http://img709.imageshack.us/img709/3400/bugfn.png

is this the bug you are talking about?
I'm not sure.

There's no need to keep reminding us that the bug exists. The patches have not been checked in.
i don't want to remind you that the bug exists, i don't know if i need to file a new bug, in my shot no plugins were used, just xul
This should block, it's a regression.
blocking2.0: --- → betaN+
Keywords: regression
Whiteboard: [needs landing]
I checked in parts 3 and 4:
http://hg.mozilla.org/mozilla-central/rev/cf82d60b3f1c
http://hg.mozilla.org/mozilla-central/rev/9713b198fe3d

Currently, the Firefox chrome has a transparent row of pixels directly above the content area, so parts 1 and 2 don't help at all. So I'm not checking those in.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Duplicate of this bug: 637061
You need to log in before you can comment on or make changes to this bug.