Closed Bug 36849 Opened 25 years ago Closed 24 years ago

asynchronous reflow and synchronous paint interact poorly

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.8

People

(Reporter: rouyer, Assigned: kmcclusk)

References

(
URL
)

Details

(Keywords: perf, polish, testcase, Whiteboard: [nsbeta2-][nsbeta3-])

Attachments

(13 files)

A small testcase, no images required
820 bytes, text/html
Details
Patch that makes the repaints asynch rather than (eek) synchronous
1.32 KB, patch
Details | Diff | Splinter Review
Better patch: sorry, the previous one was too wimpy
1.17 KB, patch
Details | Diff | Splinter Review
Modified test case: causes flashing even with patch 9793
1.08 KB, text/html
Details
Oops; this is the REAL modified test case, causing flashing even with patch 9793
671 bytes, text/html
Details
Patch to restore standard Windows event priorities
1.75 KB, patch
Details | Diff | Splinter Review
Animation example demonstrating that rapid-firing timers don't starve asynchronous painting on Windows
570 bytes, text/html
Details
Patch: causes paint after reflow is serviced: guarantees we won't starve display on any platform
891 bytes, patch
Details | Diff | Splinter Review
Patch to delay the invalidates until PlEvents are processed
9.55 KB, patch
Details | Diff | Splinter Review
Patch to delay invalidates that actually works
6.17 KB, patch
Details | Diff | Splinter Review
Updated patch which works with the mozilla cvs tip
7.24 KB, patch
Details | Diff | Splinter Review
Patch which removes all flicker - even when moving the mouse quickly - includes patches for both ViewManager and ViewManager2
22.93 KB, patch
Details | Diff | Splinter Review
Patch which removes all flicker - even when moving the mouse quickly - includes patches for both ViewManager and ViewManager2
22.93 KB, patch
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; m16) Gecko/20000421 BuildID: M14 M15 & M16 2000042108 Netscape4.x and Exploere 4/5x allows the dynamic clipping and positioning of an image at the same time to produce an animation. This occurs without the image flickering and it is an extremely useful technique. Current mozilla builds cause the image to refresh and flicker when clipping and repositioning occur at the same time. Just clipping or just positioning an image doest cause the flicker but together it does as it is shown in http://www.iex.net/rouyer/guru_test Reproducible: Always Steps to Reproduce: 1. flicker 2. flicker 3. flicker Actual Results: flicker Expected Results: The image after is has been dynamically repositioned and clipped at the same time should not be repainted causing a flicker. A very usefull animation technique is to take a single image comprised of multiple frames. You then apply dynamic clipping and repositioning of the image in a style sheet layer to perform the animation. This allows for realtime control of the animation other compared to no control of a gif image. This technique only requires one image not several as in standard image replacement animation. This technique is extremely bandwidth friendly and can be applied to multiple button states as well, but the flickering has to go. previous browsers had no problem with this method.
I see the flickering problem you mention, although when trying to compare it to IE 5.5 or NS 4.72 as you said was possible I encountered two different JavaScript errors. In IE 5.5: 'availableHeight is undefined' (also got a 'previous_position is undefined' error one time) In NS 4.72: document.getElementById is not a function. -and- guru.frame is not a function. I see the problem but have nothing to compare it against so I can see how it 'should' be. ->Javascript?
Assignee: asadotzler → rogerl
Severity: major → normal
Status: UNCONFIRMED → NEW
Component: Browser-General → Javascript Engine
Ever confirmed: true
QA Contact: jelwell → pschwartau
I updated the test page to show the same method working under IE 4/5x and NS 4x. You will see the animations not flicker (refresh) when performing a dynamic clip and position at the same time.
The test page has been updated to account for the new clipping method introduced in the April 24 M16 builds and greater. In fact, the image flashing problem has become worse... Early M16 and M15 builds do not support the new clipping methods, so please use te latest nightly.
Why getting this fixed is important to me and thousand others. Thus the problems I am facing should not be cast to less than "dogfood" bug status. To expand, I am writing a follow-up book to a previous successful book I have written on Dynamic HTML development (Dynamic HTML Web Magic). This one will focus on W3 standards and cross browser development to enable web developers to make pages for the future. I have a large library of methods planed for the new book and an enormous ready audience in both consumer and in higher education markets. If it is votes you want I will rally the charge!
The problem seems to be that setting "style.clip" causes an immediate reflow/repaint. Setting "style.left" does not cause immediate drawing.
For this method to work it requires images.
Adden: I cant see the problem (flicker) in attachment (id=9466). When the page load it simply appears, same with reloads. I created an example with clipping alone and I get no flicker. Another test with just positioning and no flicker. Together they cause problems.
OK, looking deeper, changing the CSS "left" property gives an NS_STYLE_HINT_REFLOW hint, meaning that the document needs to be reflowed. This is done asynchronously; the reflow command is posted and the script continues immediately, and everyone's happy. Changing the CSS "clip" property gives an NS_STYLE_HINT_VISUAL hint, meaning that the document does not need to be reflowed. Sounds like good news, doesn't it? But Mozilla takes the opportunity to update the display immediately and synchronously, before the script can continue. This causes the flashing and other interesting behaviours (such as the display being updated before previous style changes have been taken into account by reflows). I guess the question is, why isn't screen refreshing suppressed during script execution, e.g. using nsIViewManager::BeginUpdateViewBatch? Regarding the test case: just click on the box. After a little while it will start jumping around and flashing. Well, it does on my machine.
Well, after some experimentation it looks like just BeginUpdateViewBatch around the event handler won't do. The problem is that the screen still gets repainted before the reflow happens (and then again after the reflow happens, of course). What we need is a way to delay repainting until the asynchronous reflow command has been processed.
(I don't think this is a JS engine bug, so I'm going to reassign to attinasi and reset the component to layout.) A couple of things: - could we use the asynch reflow mechanisms to schedule asynch repaints, too? - if we've already got a reflow queued, there's no need for a separate paint: reflow requires that each frame repaint, I'm pretty sure.
Assignee: rogerl → attinasi
Component: Javascript Engine → Layout
I added Kevin to the CC list since he knows a lot about how the paint events would be handled if we did post them asynchronously. It seems like a pretty big change to make: DOM-related changes cause synch painting instead of asynch, and the message pump has to handle reflows before paints, and possibly discard paints? Maybe I'm just a scaredy-cat but this seems pretty risky so close to shipping. Contrary opinions? I'd be inclined to push this off until Gecko 2
Status: NEW → ASSIGNED
Assigned a milestone - not necessarily the correct milestone, just a milestone.
Target Milestone: --- → M20
Having double repaints when not needed is bad and should be fixed now.
Potentially causing many bugs to stem from the fix of something not used all that often is also bad, esp. when we're concentrating on stability now. I realize you depend on this to work, but we have to consider the repercussions of doing this, and have to weigh the result with the possible consequences (we want low risk/high reward bugs right now).
We will always consider successful patches from concerned Mozilla community members :-) Seriously, we are trying really hard to prioritize effectively so we can ship the most effective browser possible. We will forever be improving it... Mozilla 1.1 will be close behind 1.0
Believe me, many upon many developers will come across this situation and be bothered. It is not uncommon to apply two dynamic style change effect at the same time. When the full force of web developers get involved they will be bothered for sure. But to remove my assumption of this broad scope I will ask other developers what they think.
I decided to try a couple of easy changes to see if eliminating the synchronous repaints would help. Well, it does and it doesn't; if we do not paint synchronously then no animation is visible. This is because the testcase script uses a tight loop and the paint messages are never serviced until the script ends. If the script was changed to use timers to control the animation then asynch paints would work fine (probably). Intersetingly, IE5 seems to do the asynch paint policy. When I run the testcase in IE5 there is no visible animation, however the UI gets momentarily unresponsive so I'm guessing that it is executing the script. Probably we should make the paint asynch and force authors to use timers to animate. This way UI events could be serviced while the script is running as well, since the timers would allow us to get back and service the message pump. This is a big and scarey change, me thinks. However, we are at risk with this synchronous paint method we currently have; authors may take advantage of it and then if we 'fix' it (make paint asynch) their scripts will appear to be 'broken'... What to do?
IE5's precedent makes me happier -- waterson and I had talked about the case where someone does repeated updates of something like window.status, and don't see the interim values. Teaching EndViewUpdateBatch about ignoring paints that aren't required by a previous reflow is a lot of work: let's not do that. How about we just run event handlers and scripts inside a ViewUpdateBatch, and teach the NS_STYLE_HINT_VISUAL case to check the reflow queue before requesting that the frame paint? That seems relatively low-impact, in terms of interactions with other parts of the code. (Updating the summary to better reflect current thinking WRT the nature of this bug.)
Summary: Image Flicker when Clipping and Positioning → asynchronous reflow and synchronous paint interact poorly
RE: TImers. In my example I am using a setTimeout timer at 50 miliseconds. Are u refering to setInterval or something else?
Posting this pertinent comment I received via email: As a web developer I also rely heavily on clipping animations and like Jeff rouyer plan to release some NS6 based tutorials that rely heavily on this feature. Please it is imperative that this bug gets fixed sooner and not later as this will allow the development of tutorials and NS6 based content and thus expose NS6 to a huge web developer audience. Eddie Traversa ICQ # 8278642 DHTML Tutorials Fireworks & Dreamweaver Extensions and a bunch more! http://nirvana.media3.net/ The Future Now
Are these people using clipping changes in tight loops, or via timers? Jeff has indicated that he uses timers, which is good news, but if people are clamoring for this sort of thing to work, then our current thoughts on asynch repaint are going to need reconsideration: function shrinkWindow(window) { for (i = 0; i < 10; i ++) { window.clip.right -= 10; // no update happens between clips } } (What 4.x did, I think, was composite on a timer, based on the frame-rate settings. I don't think we can do that here, because we have reflow issues to worry about as well, and reflowing from the timer is likely to cause some performance pain. I guess we could try it out, though.)
Timers good! Loop-based animation bad! I don't want to see any animation in my test case. So far, no-one has been able to explain to me why you ever want to do synchronous repainting. Looping animation is just stupid in the event-driven DOM world, and people who use it deserve to be punished. attinasi, can you post your patch so others can try it out?
The general lore around here is that doing the synchronous paints was never intentional. Kevin McClusky, view manager owner, says we should NEVER be doing synchronous paints (possible exception is XUL menus, but even that is questionable). Please try the patch, see what happens. Hopefully nobody is relying on synch. painting so we can remove it, and the remainder of those annoying 'Recursive Paint' assertions will go away too.
If that's the case, why do we have all this infrastructure to support synchronous painting (NS_VMREFRESH_IMMEDIATE, Begin/EndUpdateViewBatch, etc)?
I'm no archaeologist, but from what I was told EndUpdateViewBatch used to not take any flags and always did synchronous updates. It was changed to support the better model of asynch updates, but support for immediate (synch) updates was thought to be required in some cases, hence the flag. That's what I heard anyway, but as to why paints were originally immediate as a result of EndUpdateViewBatch I do no know - maybe the mechanism for getting back to the message queue to get paint messages was borken or slow or maybe somebody just didn't think about it... Does anybody have any less vague explanations?
I tried your patch, but it still causes flashing on the html_guru2 page and a modified version of my test case, which I will attach.
I think we still have the problem where we are posting a paint, and that is being serviced before the reflow, then we reflow and paint again. As shaver suggested, we should try to eliminate the paint that comes before the reflow, but how to do that... Changing the paint to asynch is clearly not enough (or else I am missing a synch paint somewhere... I'll go back and check).
OK, here's what's happening: while handling the mouse click, an asynchronous repaint is requested, and an asynchronous reflow is also requested. Good. Problem is, the repaint message is dispatched by Windows before the PL event message which triggers the reflow. So we paint, reflow, paint again. The repaint event needs to be handled at lowest priority. Windows normally does this itself, but it appears that someone has deliberately overriden this behaviour in the Windows message loop code. Maybe other platforms are working...
The patch reverts to using the standard Windows event priorities. The guru page works properly, and so do my test cases. Everything seems functional, but menus feel slower.
Synchronous updating might make sense for some cases, e.g. tracking/responding to user input. For example, when the user is playing with menus, it's more important to repaint the menu than to repaint other windows. To support this, we should have a method in the PresShell, accessible through Javascript (window.repaintNow?) to force an immediate, synchronous update of a window. This would also let losers write their looping animation scripts. If this is thought to be necessary we should spin off a separate bug.
add hyatt: did you hack any of the painting stuff for menus?
Hi, This bug is absolutely a high priority for web developers as Jeff Rouyer states below. I am concerned about relying on the timer solution as I have found extensive use of timers in Windows creates memory leaks. Mozilla has to be an absolute star at handling dynamic images or it will be laughed off the web and all of your hard efforts will be wasted! It is totally worth it to delay the final release until this issue is resolved well - not half-heartedly and postponing to Gecko 2 as someone suggested below. Regards, Chimneytall
If you want to do animation, you need to use timers. Anything else is just a hack. If timers leak memory, then that's a separate bug and we'll fix it. If you want to register your support for this bug, please vote for it using the Bugzilla voting mechanism. Adding a "please fix" message to the discussion log just makes it harder for us to read what's going on while we work out a fix. Thanks.
Repaints can not be processed at the lowest priority because pages that have animation updates which are faster than the machine can keep up with result in starvation of the paint message servicing. (i.e the page freezes.. spending all or most of its time servicing timers which result in DOM manipulation, which result in invalidates, which result in additional paint messages on the message queue.) This is the reason the repaints and user-events are handled before the timer events.
The default PeekMessage behaviour (which is what my patch selects) services WM_TIMER last, below the priority of WM_PAINT. So I think that should be OK. Is there an existing test case or should I cook one up?
Just to be clear: the test case works fine with my patches.
I already voted prior to posting here. I wasn't sure that anyone was actually watching the vote count. I agree that timers are more intelligent than looping animations. My point is that I wanted the deeper sync/async/reflow issues to be cleaned up.
My problems with timers occurred when I had multiple, simultaneous, timers expiring at different intervals. Your example is terrific and very focused as examples should be. However, it is a very simple case of one timer expiring very quickly.
Thanks for voting. The test case is not meant to reveal memory leaks or anything like that, just to address Kevin McCluskey's concerns about event scheduling, which is what this bug is about. If you have a test case that reveals leaks with complex timers, please submit a new Bugzilla bug.
Michael Lowe did alot of work on the message pump, including the message priorities I believe - I'm cc'ing him to make sure he brings relevant information to the discussion before we change the logic in the pump...
Robert, you mentioned that your change services the timers after any pending paints on Windows - what about on other platforms? Kevin had an additional idea that would help guarantee starvation-prevention and would also solve the flicker problem. I'll relay it here: 1) Apply the two existing patches: mine for asynch paints, and Robert's for better (normal) message queue handling 2) After a reflow is performed, force a repaint (not from the message queue but right from the PresShell where the Reflow is kicked off, in ProcessReflowCommands). This should solve the flicker problem and ensure that the screen is updated frequently enough, even if there are zillions of reflows and the timers are somehow getting serviced before paints. The downside is very small: user interaction will be impossible for a little longer since the paint is happening on the tail-end of the reflow and no UI events will be handled until both are done. Painting generally takes very little time compared to Reflow so it should not be noticable... I'll try to prepare a patch for that part as well so we can see if we have a 'final solution' and something to nominate for nsbeta2.
> Robert, you mentioned that your change services the timers after any pending > paints on Windows - what about on other platforms? My patch only changes Win32-related code, so it only has to work on Win32. Doing the synchronous repaint after reflow sounds safe, but it's not necessary as far as I know.
OK - I tested this with the three patches (06/07/00 13:57,06/07/00 19:35,06/12/00 15:44) and the animation on the URL looks very nice. Can others look over the patches and try this out and report their findings? Also, since so many are interested in having this fixed I'd like to nominate it for nsbeta2 once we have a consensus that it solves the problem *and* that it won't cause any new, more severe, bugs.
I vote for all three patches to be checked in. In the longer term, I think we can and should remove patch #10001. I'm running fine without it. The XP issues are rather murky. Only patch #9790 should really impact non-Windows platforms. (#9840 only affects Windows and #10001 is just a backstop.) However, it can hardly create any new bugs since asynchronous repaint already had to work. I think the worst that can happen XP is that this bug isn't fixed by patch #9790 for platforms have similar event priority issues to the ones I fixed in my Windows patch. FYI, the PeekMessage docs are here: http://msdn.microsoft.com/library/psdk/winui/messques_8085.htm In particular, > Messages are processed in the following order: > Sent messages > Posted messages > Input (hardware) messages and system internal events > Sent messages (again) > WM_PAINT messages > WM_TIMER messages If there are any problems with the default handling, it'll most likely be that reflows (and PLEvents in general) have too high priority. Someone should define and write down exactly what we expect from the platform event queues in terms of event prioritization. I think the Windows priorities are nearly right, except we might want a class of "low priority" PLEvents that are serviced after input events.
I'm in agreement that we should get these patches in, however I cannot make the call on the risk/benefits. Nominating to nsbeta2 so PDT can make the decision. Benefit: eliminates flicker in common animation technique, and eliminates incorrect synchronous update of DOM-issued visual changes. Risks: changing the priority of messages coming off the Windows message queue: low level change, needs lots of testing... Additional note: the last patch I attached (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=10001) is not correct: tracing through it I see that it does not cause the correct view to be painted (I though that the views would be recursed but they are not).
Keywords: nsbeta2
Whiteboard: [fix in hand]
nsbeta2-, nominated for nsbeta3 to give more time that is needed for testing.
Keywords: nsbeta3
Whiteboard: [fix in hand] → [nsbeta2-][fix in hand]
Is there a Win32 build with the best working patch somewhere (nightly or experimental directories) for those who are not compiling the browser?
I can send you a copy of GKHTML.DLL which should work with recent builds. Email me and I'll email it back to you.
Changing to default QA contact for assigned component, Layout.
QA Contact: pschwartau → petersen
If the patches are apparently benign to beneficial so far, they should go into the trunk as soon as a mozilla milestone branch (from which nsbeta2 will branch) is created, so we can maximize testing. /be
I've been using patches #9790 and #9840 for weeks with no apparent problems. My confidence in them is high.
I've been running patch 9793 and 9809 and they are fine. Robert, 9840 is a testcase, could you recheck which patches you are using? I believe patch 10001 is unnecessary (it seems to have no effect).
Yeah, they're the ones I meant :-). 9793 and 9809.
Keywords: patch, perf, polish
I just queried for this bug again to say "now's the time (for checkin)". Anyway, Brendan was faster... :)
Approved for beta3: excellent benefits to performance, animation on par with Nav and IE. Well tested for many weeks by several people.
Whiteboard: [nsbeta2-][fix in hand] → [nsbeta2-][fix in hand][nsbeta3+]
Target Milestone: M20 → M18
Fixes checked in: nsCSSFrameConstructor.cpp - no sync updates now (all no-sync) nsAppShell.cpp and nsWinMaion.cpp (for Robert O'Callahan) - standard event priorities in message-loop Time to Rock & Roll, animators! Let's pull this and make sure we have not introduced any regressions (that would be bad).
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Excellent, thank you all!!!!
Marking verified in the Aug 24th build.
Status: RESOLVED → VERIFIED
Is this fixed just for Windows? What about event priorities on Mac and Linux?
Dunno, but if there are problems they should be filed as separate platform-specific bugs.
The changes to nsAppShell are going to be backed out. This checkin is the cause of no painting *at all* during layout. It negates any attempt at incremental layout. It's taken several days to figure this one out and the general feeling of unresponsiveness during layout has been around since this went in over a month ago. After backing out the changes, the brower is significantly more responsive during layout. I'm going to be a bit blunt here since this has had such a negative effect on performance. Please DO NOT make such radical changes to event handling in the future without much wider discussion of the issue. --Vidur (from jst's account)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
> Please DO NOT make such radical changes to event handling in the > future without much wider discussion of the issue. I'm sorry that this caused you such trouble. However, many luminaries have been CC'ed on this bug and its brethren (e.g. 44678) for months. I also tried to start a newsgroup discussion on how events should be prioritized (in .xpcom), but there was little interest. What else should have been done?
Anyway, now we're back to square one. We may have better responsiveness, but we're slower overall, because many content changes produce a paint->reflow->paint sequence, forcing us to paint the same area twice. And of course, there's the real reason this bug was filed in the first place: the first paint is frequently bogus because we haven't reflowed yet. Someone needs to lay down exactly what the relative event priorities should be XP, so that people can implement and use them correctly. (This is what I asked for months ago.) Once we have those rules, we can figure out how to fix this bug and other similar bugs.
Robert, it sounds like you did the right thing by trying to drive this discussion here and in newsgroups. I suspect that you might have got more of a response on porkjockeys than xpcom. I guess I'm still reeling from the statement that the change was tested with no perceived ill effects for several weeks before it was checked in. I recognize that layout of the complete document is slightly slower than it was with the change in, but the responsiveness and general usability of the browser is so improved now that it's a worthwhile and necessary tradeoff. Either way, as you say - we're back to square one. Should the discussion move to mozilla-porkjockeys?
I'm not sure it makes sense to classify all PLEvent uses in a single priority pool relative to other events. Seems to me that Robert's earlier suggestion of having more than one priority at which PLEvents are delivered might be a viable solution. For the cases in question, Necko-generated PLEvents might be considered low priority with respect to UI events, reflow-related PLEvents might be considered higher.
Maybe we just didn't look at the right sites, or under the right conditions... If a page loads very fast, then I probably don't notice whether it's displaying incrementally or not. If it loads very slowly, then depending on network conditions and other stuff, the Necko events may stop long enough for a reflow and paint. Let's have a discussion on porkjockeys...
Wow, I'm feeling pretty rotten about this. I really thought we tested this thoroughly, had all of the correct people looking at it, documented / publicized the change appropriatly, only to find out it caused people _days_ of grief trying to track it down. My most sincere apologies. I did post to m.p.l.checkins on Fri, 28 Jul 2000, and the posting did have all of the information about the event priorities. Since nsAppShell was backed out we need to back out /mozilla/webshell/tests/viewer/nsWinMain.cpp too since it has the same change. I'll take care of that (V: was there a bug number for your backout?) Also, clearing FIX IN HAND and nsbeta3+ from status whiteboard...
Whiteboard: [nsbeta2-][fix in hand][nsbeta3+] → [nsbeta2-]
Viewer is backed out now too. It looks like this bug is really dependent on 50104, or at least closely related, so I'm marking as such.
Status: REOPENED → ASSIGNED
Depends on: 50104
Target Milestone: M18 → Future
I tried the attachment from 6/8/00 and I can't tell what it's supposed to show me. Is it broken? Is it working? How can we make sure mac/linux aren't suffering from the same problems?
Synopsis: The current event processing does not ensure that paint events generated by visual changes will be processed at the same time as paint events generated by reflow. This causes flicker and performance problems for animations which manipulate both VISUAL and REFLOW properties within the same timer callback or event handler. Current processing sequence that causes problems: Within timer callback or event handler: 1.a script makes two changes; one results in a VISUAL_CHANGE (performs a invalidate), the other results in scheduling a REFLOW PLEvent. 2. return to the message pump. 3. dispatch PAINT messages for VISUAL_CHANGE invalidate. (See the results of visual change) 4. dispatch REFLOW PLEvent (The reflow invalidates portions of the window) 5. return to the message pump 6. dispatch PAINT messages generated by the REFLOW invalidate calls. (See the results of the reflow change) Proposed solution: Defer the dispatching of the PAINT events using an Invalidate PLEvent. The sequence would change to the following: 1. a script makes two changes; one results in a VISUAL_CHANGE (schedules an invalidate PLEvent), the other results in scheduling a REFLOW PLEvent 2. return to the message pump. 3. dispatch Invalidate PLEvent which performs an invalidate on the window. 4. dispatch REFLOW PLEvent (The reflow invalidates portions of the window) 5. return to the message pump 6. dispatch PAINT messages for the both the reflow and visual changes (See the results of the visual change and reflow change at the same time) The patch to delay invalidates does this.
Is the original testcase (the site in the URL) still available? That URL is not pointing to it any longer and I couldn't find it. rouyer, can you put the test page back up or point us to the new location? Thanks.
Ok, I put the test case back up at http://www.iex.net/rouyer/guru_test To view the same test with Explorer 5.5(Win)to see it displaying correctly.
Is this NS_VIEW_PUBLIC_FLAG_DONT_DOUBLEBUFFER stuff supposed to be checked in? It doesn't seem to be used anywhere. Also, the trunk nsViewManager2 does not support weak references, so the patch won't apply cleanly. After fixing that up, and using my view manager, I'm seeing all sorts of problems. Repaints in many places are happening slowly or not at all. This could be some interaction with my changes though, so other people should test...
Um, you never clear mPendingInvalidateEvent :-).
Oops.. I posted an old patchfile which has some problems as you found. Yes the ProcessPendingUpdates should reset the mPendingInvalidateEvent. The NS_VIEW_PUBLIC_FLAG_DONT_DOUBLEBUFFER is for bug 49743, but it should not be checked in until we confirm that it really helps to solve that bug. I ran all day yesterday with the correct version of the patch and it solved the problem described in this bug, and I didn't notice any major problems.
Can you post the real patch? I'm seeing MAJOR problems even after I put in code to clear mPendingInvalidateEvent.
I;'ve applied the patch and have been testing it. On the URL (http://www.iex.net/rouyer/guru_test/) it does seem to works correctly, at least I don't get flicker when changing the position. However, when the waves are started it sometimes shows the image in the old location before showing it in the new location (flickers). Also, those waves eat 100% of my CPU. I don't know if this is relevant to the bug, but I thought it was interesting. No other problems seen with this patch so far.
A further not on my last comment: the 100% cpu usage of the waves happens with or without the patch.
I paired down the guru test to just the figure animation.
I see two problems. One is that highlighting in menus is now really slow, it lags considerably behind mouse movement. The other problem is that if I quickly mouse over the "pos 0" to "pos 9" links in the test case, then I see the image jump around. As far as I can tell, there is nothing in the code to disable refresh while the moust event handler is running. Do you others see this?
Robert, my SeaMonkey build is not running right now so I have been using Viewer (clobbering and rebuilding now). I can believe that the menu response will have slowed down if it is doing asynch invalidates. For some reason I thought Menus were doing immediate (synchronous) invalidates, but I guess not.
In Viewer, I can still see the image jumping around when mousing over the "pos" links, but I have to move the mouse very fast. In Mozilla it's much more noticeable. Is there anything forcing the paint event to be processed after the reflow event? Not as far as I know.
I'm also seeing that, when typing, nothing gets painted until I stop typing for a bit. It's very bad. Do you see this?
Marking nsbeta3- due to high risk.
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3-]
I am seeing slow menu item updates with a pull from today without the patch. Robert: can you try it without the patch? If you don't want to back out the patch, you can modify nsViewManager2::EnableRefresh(PRUint32 aUpdateFlags) so it always calls ProcessPendingUpdates instead of Posting the invalidate event. I am seeing flicker when I move quickly between the positions with te patch installed.
I, too, am seeing very slow menu updates in a just pulled build. When I open a menu and mouse over an item, it takes a noticeably long time to select the item.
OK, the slow menus are definitely caused by something else. However, we can't really see what effect this has on the menus until the slow menus are fixed. I think the flicker we're still seeing is caused by the following sequence: -- mouse move event (triggers Javascript, posts invalidate and reflow PLEvents) -- PLEvents (posts paint event) -- mouse move event (triggers Javascript, posts invalidate and reflow PLEvents) -- paint event (paints bogus state) So I think that this fix is not quite right and should NOT be checked into the Mozilla trunk. As a 90% solution, it might still be suitable for the NS6 branch if Netscape PDT can be persuaded.
Over to kevein, per our conversation today (more of a rendering issue than a style issue).
Assignee: attinasi → kmcclusk
Status: ASSIGNED → NEW
Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that do not have the "testcase" keyword and yet have an attachement with the word "test" in the description field. Apologies for any mistakes.
Keywords: testcase
Status: NEW → ASSIGNED
*** Bug 60171 has been marked as a duplicate of this bug. ***
*** Bug 60171 has been marked as a duplicate of this bug. ***
*** Bug 62670 has been marked as a duplicate of this bug. ***
Set milestone to mozilla0.8
Target Milestone: Future → mozilla0.8
Analysis after instrumenting (widget, presShell, view) I confirmed the following: There are two separate problems with the flicker. (1) Flicker caused by changes (visual+reflow) changes from a timer event handler (2) Flicker caused by changes (visual+reflow) from a mouse-move event handler (1) is solved by posting a paint invalidate event and processing the invalidate at the same time as the reflow event. (2) is solved by flushing all pending reflows/invalidates at the end of the mouse-movement. It is the result of the inversion of event priorities which favors mouse events over paint events It flickers because of the following sequence: event PL-event(reflow1) Process pending reflows which generates invalidates event MouseMove fires JavaScript event handler reflow change - posts PL-event(reflow2) visual change - changes state of frame which will be visible if the frame is asked to paint event PAINT (generated from PL-event(reflow1) invalidate) >>>>>> This is bad because the PAINT event handler renders the state change of the frame modified in the MouseMove event. (This happens becauses the frame with the state change intersects the invalidated rect generated by processing PL-event(reflow1)). To solve this event sequence problem I added code in nsEventStateManager to flush the pending reflow and invalidate events after all of the mouse callbacks have happened. This changes the sequence to be: event PL-event (reflow1) Process pending reflows which generates invalidates event MouseMove reflow change - posts PL-event(reflow2) visual change - changes state of frame which will be visible if the frame is asked to paint FlushPendingReflow - causes PL-event(reflow2) in reflows to be processed generating invalidates FlushPendingInvalidates - causes the visual change to generate invalidates event PAINT reflow1, reflow2, and visual changes from the mouse move are rendered by the event handler of the paint message. This is needed so the invalidates can be combined with the reflow invalidates. The event state manager already flushes the pending reflows in GenerateDragGesture with every mouse move. I added another call to flush the reflows after the mouse move event handlers have been called. In most cases there will not be any pending reflows or invalidates because they are normally flushed by the GenerateDragGesture.
Added joki@netscape.com to CC list, to comment on the EventStateManager change.
What's the deal with the gViewManagers array being added in the patch? Is that needed here? This patch is, IMHO, acceptable, but it seems to me that it's a bit of a band-aid. The fix should really apply to all interactive events, not just mouse movement --- but applying this patch to all interactive events would be "wrong", because interactive events would need to take priority over these reflows and paints.
"What's the deal with the gViewManagers array being added in the patch? Is that needed here?" I added a sanity check to make sure the Invalidate event had a valid ViewManager instance in it. The mechanism I use to determine it is a valid view manager is to cycle through the existing gViewManagers array to determine if the invalidate event points to valid view manager. The invalidate event should be killed by the mEventQueue->RevokeEvents(this) in the destructor of the viewmanager, but I have seen the assertion indicating a bad viewmanager in the invalidate handle event triggered once after running with an earlier version of the patch. I'd like to see if the current patch ever invokes the bad view manager assertion. "The fix should really apply to all interactive events, not just mouse movement --- but applying this patch to all interactive events would be "wrong", because interactive events would need to take priority over these reflows and paints." I guess the main issue is "do the interactive events require the state of the display to be current?." (i.e all reflowing and invalidates) have been peformed before processing another interactive event. Currently, The key and mouse events have the highest priority, so they are done at the expense of keeping the display current. (The patch forces the reflow/invalidates for each mouse move to keep the display current.) Should this be done for mouse up/mouse and keyevents as well?. My guess is we only want to do it were worried about flicker caused by doing both a visual and reflow change as the result of an event handler. There is a performance tradeoff. No flicker vs. quicker processing of the events. I felt comfortable adding the forcing of reflow/invalidates for the mouse move would not impact current performance, since it already did a flush of all of the reflow commands with every mouse move in the GenerateDragGesture code.
You're right that there's a tradeoff between responsiveness and throughput which is determined by how often we update the screen in response to events. But that is not actually what we're dealing with here. My fault for introducing this red herring. The problem here is that updating has two phases --- reflowing and repainting --- and if we ever repaint while there are pending reflows, we display something that is not a valid rendering of any state of the content. I would argue that whenever this happens, it is a bug. Your patch fixes one instance of this by forcing a (reflow, repaint) cycle after mouse moves, which we have observed can trigger the bug. But other high-priority events can also trigger the bug. It would be easy to modify the testcase to exhibit the bug in response to other input events. But as you and I observe, it would be a mistake to apply your fix to all input events. I think that ultimately we need to tackle the problem head on, by processing any relevant pending reflows every time we repaint. I understand that this might not be easy to do, but any other complete solution will be much nastier.
"But as you and I observe, it would be a mistake to apply your fix to all input events.' I think it really depends on the target of the event. The target ultimately needs to decide if the display needs to synchronized before the target can perform it's processing. For example, if the target of a keyevent is a textfield then it wouldn't want the display to be synchronized. It needs to batch the keyevent's without updating the display. If the target of a keyevent is a javaScript event handler that manipulates the DOM to generate a single frame of animation driven by the keyboard then it would need the display to be synchronized to avoid flicker. "I think that ultimately we need to tackle the problem head on, by processing any relevant pending reflows every time we repaint." I think that would be worth investigating, but at this point I would prefer to go ahead with the current patch and file a new bug. Forcing the flushing of pending reflows with ever repaint would have a far larger impact on the current implementation than the change within this patch.
Tried this patch on Linux, I have not seen any problems so far. I will try mac next.
Hmmm, the latest patch has some failed hunks versus current CVS, even with a large 'fuzz'.
I created bug 65248 to cover the general issue which this bug exposed. Please add your self to the CC list of this bug if your still interested in this issue.
Adam: I just pulled the tip and applied the patch on WIN32 and it worked fine. No failed hunks.
The patch looks good to me as well. However, I do have one architectual concern. nsVoidArray* nsViewManager::GetViewManagerArray() returns a pointer to a private data member of nsViewManager. That means the caller can do all sorts of evil things to the view manager, like add or delete from the array, or even insert an element of an unexpected type. It would be *much* cleaner, and only a little extra work, to have GetViewManagerArray() return an iterator instead.
How about having it return a `const nsVoidArray*'.
that might work as well, though a little less safe since C++ makes "const" easy to cast away.
Making it a const should be adequate. The nsViewManager::GetViewManagerArray() method is not a nsIViewManager interface method and nsViewManager.h is not exported so the method can only be accessed within the view module.
I'm ok with that.
Checked in patch + modification to make nsViewManager::GetViewManagerArray() return a const.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
I just snagged a new build and aaaah, very nice, very smooth, like a clean shave - that animation is flicker free and happy :)
Marking verified in the March 5 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: