Closed Bug 633387 Opened 14 years ago Closed 12 years ago

Rework painting code in PluginInstanceChild

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking2.0 -)

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: cjones, Unassigned)

References

Details

Before 626602, the code had some properties that made it a little hard to reason about - code external to ShowPluginFrame() was responsible for clearing the surface when conditions changed. This was because PluginInstanceChild only stored the "current" mWindow and mWsInfo, so whenever those changed, the code changing them had to decide whether to clear the surface. - the lowest-level PaintPlatformRectToSurface() had to sync window attributes with the plugin if something changed, say a temporary clip or a helper surface. This meant that the plugin could RPC into the browser and cause painting to be re-entered arbitrarily. This forced all code that changed state related to painting to beware of the re-entry flag. - it was unclear to me how mWindow.window related to mCurrentSurface on windows. Apparently at attempt was made to reuse the DC if the window size didn't change? I don't know under what conditions the DC can be reused and the code didn't document it. 626602 made this worse by adding more code that changed painting state. Additionally, background surfaces provide a competing idea of plugin "window size", vs. mWindow. When the background and mWindow disagree, we want to be able to add logic to prefer one or the other. Also, AsyncSetWindow() and background updates are sent completely independently on the browser side. This can lead to surface churn and unnecessary repaints. I think we should rework the logic as follows - have a new struct WindowState that wraps all paint-related state (possibly an abstraction thereof). Have s/mWindow/mNextWindow/. - add a Paint(rect, surface) function that cannot be re-entered and assumes that mCurrentSurface, SetWindow() et al. have all been taken care of. - add a SchedulePaint() function that creates a local WindowState, copies mNextWindow to the local, and prepares mCurrentSurface/SetWindow()/readback et al. as necessary. SchedulePaint() is aware of re-entrancy. If SchedulePaint() is re-entered, it immediately returns. If there was re-entrancy, the first invocation (bottom of the stack) can either go ahead with Paint() or defer another SchedulePaint() to the event loop, depending on the state that changed. - add an Invalidate(rect invalid, paintSoon) function that doesn't care about reentrancy. It simply adds the invalid rect to mNextWindow. If paintSoon, it calls SchedulePaint(). Otherwise, it defers a SchedulePaint() to the event loop. Then, the existing functions change as following - AsyncUpdateWindow() modifies mNextWindow as necessary, and calls Invalidate(window, !paintSoon). - UpdateBackground() calls Invalidate(updatedRect, paintSoon). - DestroyBackground() calls Invalidate(window, !paintSoon). How does this sound? (There are of course details to be worked out.)
I suggest blocking-final on this. After 626602, the code that's there will require acts of heroism to fix rendering glitches/crashes/security issues.
blocking2.0: --- → ?
Jim/Ben: are there any circumstances in which we'll use the old windows windowless-rendering code in PluginInstanceChild? Can it be removed?
(In reply to comment #2) > Jim/Ben: are there any circumstances in which we'll use the old windows > windowless-rendering code in PluginInstanceChild? Can it be removed? I've wondered that myself, I think it can be kicked in via an accelerated layers pref? bsmedberg would know the details better. It'd be great if we could remove all of that now.
(In reply to comment #2) > Jim/Ben: are there any circumstances in which we'll use the old windows > windowless-rendering code in PluginInstanceChild? Can it be removed? It's been so long since I looked at this painting code I honestly don't know :(
Oh sorry, that was directed at Ben S. Silly me, trying to use christian names.
(In reply to comment #5) Phew! I was slightly concerned that you could think that I might know!
I have an updated patch that notes that the generated assembly looks ok, and gcc just seems to be behaving oddly, but I'm having ISP problems and can't upload it.
Sorry, comment 7 was meant for another bug.
The old code is still active if you set the pref mozilla.plugins.use_layers to false (and on mac, of course). I didn't want to mess with it until we have full async painting on mac, and then I was going to rip it out all at once. The important thing to preserve which I don't see mentioned is the "invisible" case, where AsyncSetWindow informs the plugin not to paint. In this case we need to SetWindow(empty mWindow) and then not paint until we receive the visible-notification.
If SchedulePaint() is called and the clip rect is empty (and the first-paint has already been forced), then SchedulePaint() would return early.
I think you might have been better to just roll this up into bug 626602. Unless you have specific issues which are already a problem (which should just land with 626602), I don't think I want to block on this.
blocking2.0: ? → -
Does anyone happen to know if the forced SetWindow here http://hg.mozilla.org/mozilla-central/annotate/62ae87cc7f02/dom/plugins/PluginInstanceChild.cpp#l2330 is needed for compat issues? I'd like to only SetWindow when something relevant has changed. It would be easy to add a way to force it though.
Should get Steven Michaud's opinion on the necessity of various SetWindow calls.
Looking at the changeset that introduced that code http://hg.mozilla.org/mozilla-central/rev/7e449cbc19df it looks more like an ease of implementation reason than any compatibility issue. An assumption that, if the browser sent the SetWindow, it is worth passing it on to the plugin.
That was my assumption too. I don't think plugins would be able to tell that we didn't forward the new window delivered in AsyncSetWindow(), if nothing changed.
I believe it was just a shortcut to make sure that the visibility notifications were actually delivered.
(In reply to comment #12) > Does anyone happen to know if the forced SetWindow here > > http://hg.mozilla.org/mozilla-central/annotate/62ae87cc7f02/dom/plugins/PluginInstanceChild.cpp#l2330 > is needed for compat issues? I'd like to only SetWindow when > something relevant has changed. It would be easy to add a way to > force it though. (In reply to comment #13) > Should get Steven Michaud's opinion on the necessity of various > SetWindow calls. I don't know. And I also don't know how the visibility notifications work, so I can't really comment on that. One thing I *do* know (or think I know) is that we shouldn't change the timing of the very first call to a plugin's NPP_SetWindow() function. That's where we got in trouble with Josh's patch for bug 541018 (see bug 594482 comment #71).
Great to know, thanks!
(In reply to comment #11) > Unless > you have specific issues which are already a problem (which should just land > with 626602), I don't think I want to block on this. The one remaining issue from 626602 is that we can take a perf hit from losing a background and having to fall back on alpha recovery in some cases where with this work we could have kept the background. With Karl's excellent ideas, the code in 626602 ended up looking a bit more maintainable than it was shaping up to be, so I think I'll shelve this work for now. We might want to block the mac async plugin painting work on this, but we can cross that bridge when we come to it.
This code doesn't get touched anymore so this is unlikely to be worth anyone's time.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.