Last Comment Bug 129310 - BeOS Scrolling needs to be rewritten
: BeOS Scrolling needs to be rewritten
Status: RESOLVED FIXED
: fixed1.8
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: Other BeOS
: -- normal with 1 vote (vote)
: ---
Assigned To: Sergei Dolgov
: Nobody; OK to take it and work on it
:
Mentors:
http://my.opera.com/community/forums/...
: 275380 (view as bug list)
Depends on: 24240
Blocks: 266252 296856
  Show dependency treegraph
 
Reported: 2002-03-06 10:24 PST by Daniel Switkin
Modified: 2008-07-31 04:15 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Scroll and Paint rewrite (12.72 KB, patch)
2005-09-25 09:46 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch (12.65 KB, patch)
2005-09-25 13:31 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch (19.53 KB, patch)
2005-09-26 07:59 PDT, Sergei Dolgov
thesuckiestemail: review+
mtschrep: approval1.8b5+
Details | Diff | Splinter Review

Description Daniel Switkin 2002-03-06 10:24:11 PST
The biggest problem here is that children views do not scroll in sync with the parent, 
which causes streaks of garbage pixels while scrolling (which eventually get repainted). 
You can see this on any page which has iframes. The solution will needs to keep them in 
sync while retaining the hardware screen-to-screen blit for performance. Early experiments 
have not been that promising - it's a tough problem.
Comment 1 Sergei Dolgov 2002-03-06 10:31:49 PST
I tried to put existing NS_METHOD nsWindow::Scroll() code with all those blits
into children iteration loop, directly replaced in loop  mView with its children.
All seem stay in sync in this case, but scrolls very slowly.
Comment 2 Sergei Dolgov 2002-03-06 13:02:04 PST
I'm nor web-design guru, neither mozilla code specialist, but i wonder if BViews' nesting degree in Mozilla rendered page may be more than 1.
In such case even this "buggy" scrolling  may appear non-working.
Comment 3 Paul 2002-06-27 19:56:37 PDT
confirmed
Comment 4 Sergei Dolgov 2002-07-17 03:57:53 PDT
Code in mozilla/view/*
"moves" main view (and children) without repainting by calling SetPosition().
Then checks if widget can BitBlt but not for children.
If cannot BitBlt, it invalidates main view and "adjusts" children by itself.
BeOS case is a bit messy - coz it gets CanBitBlt property, it don't adjust
children, so we should move already "moved" child views. Last fact can be
checked if comment out children moving in BeOS widget Scroll () code, scroll
some page with inline frames and then resize window - all "unscrolled" children
jump on proper position.
Comment 5 Paul 2002-09-06 18:51:04 PDT
By separating the Scroll methods into ScrollWidgets and ScrollRect, we should be
able to fix the scrolling of child views.  We could, theorectially, lock the
looper on the parent, scroll the children, then self, and then, unlock the looper.

Currently the BaseWidget contains an empty implementation for the ScrollWidgets
and ScrollRect.  If am not sure if they are actually being called, yet.
Comment 6 Sergei Dolgov 2002-09-07 14:32:45 PDT
I tried to create both, but those seem never called.
Comment 7 Sergei Dolgov 2002-09-07 14:38:37 PDT
btw, i think that origin of problem is that child widgets are also BeOS BView
children in current implementation. Not sure if we really need that, maybe it is
possible to add those to parent's BWindow() as children. And Mozilla should care
about other aspects of parent/child/sibling relations itself.
Comment 8 Daniel Switkin 2002-09-07 16:33:57 PDT
Right Sergei. Since they are child BViews (which may or may not be necessary, I don't know) 
they scroll in the BeOS fashion, which is to change the topleft coordinate of the parent. 
So when scrolling down 10 pixels, the parent corner is 0, 10 and all the children stay 
where they are. The Mozilla model as I understand it is to keep the parent corner always at 
0, 0 and then move the chidren. Therefore we can't just use ScrollTo or ScrollBy in BView 
(too bad, because they're hardware accelerated). Right now, you see garbage because 
CopyBits for the parent is very fast, but moving and redrawing all the children is quite 
slow (it lags behind). Unfortunately there is no way to tell CopyBits to move the 
childrens' data also (that's what ScrollTo is for). So other than doing scrolling entirely 
in software (I tried, it's unusably slow) there are two paths I can think of:

1. Use ScrollTo and ScrollBy but then compensate for the offset (see SetOrigin() and 
Origin() in BView.h) whenever drawing or dealing with mouse coordinates.
2. Try to implement IFRAMES and whatever else uses child BViews right now in a different 
way where you have one big view, and then use CopyBits for the scrolling.

I'm no longer coding on Mozilla, so feel free to reassign the bug. But I'm happy to discuss 
ways of fixing this and maybe reviewing the code.
Comment 9 Sergei Dolgov 2002-11-17 12:02:48 PST
reminder for myself - should look at http://www.3rd-evolution.de/BeOS/MDI/
Comment 10 Sergei Dolgov 2002-12-21 08:16:53 PST
Daniel, i found wasy how to do bit copy for all page content, only (but seems
serious) problem are child ghosts which coexist with scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.Daniel, i found wasy how to do bit copy for all page
content, only (but seems serious) problem are child ghosts which coexist with
scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.Daniel, i found wasy how to do bit copy for all page
content, only (but seems serious) problem are child ghosts which coexist with
scrolled children images. 
Here is code (maybe not safe yet):
	if(mView && mView->LockLooper())
	{
		BRect src;
		BRect b = mView->Bounds();
		if(aClipRect)
		{
			src.left = aClipRect->x;
			src.top = aClipRect->y;
			src.right = aClipRect->XMost() - 1;
			src.bottom = aClipRect->YMost() - 1;
		}
		else
			src = b;

		BRegion	invalid;
		invalid.Include(src);

		// make sure we only reference visible bits
		// so we don't trigger a BView invalidate
		if(src.left + aDx < 0)
			src.left = -aDx;
		if(src.right + aDx > b.right)
			src.right = b.right - aDx;
		if(src.top + aDy < 0)
			src.top = -aDy;
		if(src.bottom + aDy > b.bottom)
			src.bottom = b.bottom - aDy;
		BRect parentsrc = mView->ConvertToParent(src);
		BRect dest = parentsrc.OffsetByCopy(aDx, aDy);
		mView->Parent()->ConstrainClippingRegion(0);
		if(parentsrc.IsValid() && dest.IsValid())
		mView->Parent()->CopyBits(src, dest);
		invalid.Exclude(mView->ConvertToParent(child->Frame()));
	
		invalid.Include(dest);
		int32 rects = invalid.CountRects();
		for(int32 i = 0; i < rects; i++)
		{
			BRect	curr = invalid.RectAt(i);
			nsRect	r;
			r.x = (nscoord)curr.left;
			r.y = (nscoord)curr.top;
			r.width = (nscoord)curr.Width() + 1;
			r.height = (nscoord)curr.Height() + 1;
			OnPaint(r);
		}

		mView->UnlockLooper();
	}
Main idea here is performing copybits operation for Parent() if scrolling-view
instead on scrolling-view itself.
Those iFrames children appear on corect place and work as links in correct place
too.

Hope you have time to test it, i applied it together with my last patch for
"disappearing images" bug.
Comment 11 Sergei Dolgov 2003-12-07 15:12:52 PST
After thousands of experiments i found only way to reduce i-frame lagging.
It is:
override BView::DrawAfterChildren;
in ::Scroll, while moving children, apply B_DRAW_ON_CHILDREN flag.
reset that flag after children were moved.

Unfortunately those children, while don't lagging anymore, blink a  bit.
Especially when child's frame reaches parent frame border.
Comment 12 Sergei Dolgov 2005-01-22 04:32:08 PST
*** Bug 275380 has been marked as a duplicate of this bug. ***
Comment 13 Doug Shelton 2005-09-12 23:35:18 PDT
(In reply to comment #11)
> After thousands of experiments i found only way to reduce i-frame lagging.
> It is:
> override BView::DrawAfterChildren;
> in ::Scroll, while moving children, apply B_DRAW_ON_CHILDREN flag.
> reset that flag after children were moved.
> 
> Unfortunately those children, while don't lagging anymore, blink a  bit.
> Especially when child's frame reaches parent frame border.

Sergei, seems like this may have been taken care of as part of another
bug/patch.  Is this still a valid bug to have open? 
Comment 14 tqh 2005-09-13 00:07:08 PDT
The blinking is still there.
Comment 15 Doug Shelton 2005-09-13 00:28:01 PDT
(In reply to comment #14)
> The blinking is still there.

True, it seems to be.  But doesn't that mean the code discussed here is already
implemented in some other patch?  Otherwise, we'd still have the streaks of
garbage pixels discussed in the original report.  So the original problem seems
fixed and not because of a patch for this bug but for some other.
Comment 16 Sergei Dolgov 2005-09-13 01:52:53 PDT
This bug is still valid.
Both description and origin of problem.
All measures we took were palliatives, so atm, we still have to choose between
two evils - ugly lagging and irritating blinking.
Comment 17 Sergei Dolgov 2005-09-25 09:46:55 PDT
Created attachment 197343 [details] [diff] [review]
Scroll and Paint rewrite

Patch. 

Scrolling changes:
Hiding children widgets while scrolling, unhiding at ::Update().
Calculating in-screen rect.
Painting dirty area at once, instead traversing rects - will be rewritten when
NS_PAINT really process regions, instead taking frame of those.

Painting changes:
1)nsViewBeOS::GetPaintingRegion() don't empty now whole pending drawing region,
there is new method for that nsViewBeOS::Validate(BRect), we are calling it in
::OnPaint(). So this substraction affect both synchronous and asynchronous
drawing.
2)::Ivalidate() group of methods now calls again BView::Draw() instead
Invalidate() in case of asynchronous painting. As we calculate update rects
correctly now in ::Scroll() - no more app_server involvement required.
3)Only eWindowType_child and _popup are drawing now. Includes changes in
::OnPaint() method, in ::StandardWindowCreate() and in ::CreateBeOSView()

I wish someone test this on plain cvs build, as we (me and tqh) are tested that

 with very non-standard sources only.
Comment 18 tqh 2005-09-25 11:43:27 PDT
I think we should reduce code in OnPaint by removing 
	static NS_DEFINE_IID(kRenderingContextCID, NS_RENDERING_CONTEXT_CID);
	static NS_DEFINE_IID(kRenderingContextIID, NS_IRENDERING_CONTEXT_IID);

	if (NS_SUCCEEDED(CallCreateInstance(kRenderingContextCID,
&event.renderingContext)))
	{
		event.renderingContext->Init(mContext, this);
		result = DispatchWindowEvent(&event);

		NS_RELEASE(event.renderingContext);
		result = PR_TRUE;
	}
	else
		result = PR_FALSE;

and replacing it with
	event.renderingContext = GetRenderingContext();
	if(event.renderingContext != nsnull)
	{
		result = DispatchWindowEvent(&event);
		NS_RELEASE(event.renderingContext);
	} else {
		result = PR_FALSE;
	}
Comment 19 Sergei Dolgov 2005-09-25 12:37:59 PDT
tqh, you didn't answer to my notice before about
event.renderingContext = GetRenderingContext();
maybe you missed it.

Ias far as i investigated it, GetRenderingContext in our case did absolutely same
as we do, aonly adding call overhead. Maybe it was changed, but at least you can
point me at current GetRenderingContext implementation
Comment 20 Doug Shelton 2005-09-25 13:00:21 PDT
patch applies cleanly to CVS but breaks build.  Also verified middle-button
scrolling icon leaves a trail.
Comment 21 Sergei Dolgov 2005-09-25 13:06:18 PDT
Doug, what is version you used to patch?
CVS may provide various versions.
Post full info from About page
Comment 22 Sergei Dolgov 2005-09-25 13:17:07 PDT
Comment on attachment 197343 [details] [diff] [review]
Scroll and Paint rewrite

found bug in Validate() declaration
Comment 23 Sergei Dolgov 2005-09-25 13:31:22 PDT
Created attachment 197354 [details] [diff] [review]
patch

Same as previous but :Validate() implementation now follows declaration.
Also unused variable in OnPaint removed.
Waiting also for tqh's comment about GetRenderingContext() benefits:
http://lxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsBaseWidget.cpp#605

http://lxr.mozilla.org/seamonkey/source/gfx/src/nsDeviceContext.cpp#220 (we
lack proprietary implemetation atm)
do_CreateInstance(kRenderingContextCID, &rv); - couldn't find code for that,
only
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsGenericFactory.cpp#75
Comment 24 Doug Shelton 2005-09-25 14:39:19 PDT
second patch version applies cleanly to CVS 
Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20050925 Firefox/1.6a1

fyysik, I hope this answers question about the version.  
Comment 25 Doug Shelton 2005-09-25 14:41:41 PDT
second patch version scrolls without flicker on pages with iFrames. 
Center-button scroll mouse trails are a separate problem - happen with CVS
version and also after patching.
Comment 26 tqh 2005-09-25 23:23:35 PDT
Fyysik, the only reason for getRenderingContext is so that we don't increase
code-size with duplicate functionality and if something is changed in
GetRenderingContext we don't have to do the same changes.

Btw, the overhead is unnoticable for me.
Comment 27 Sergei Dolgov 2005-09-26 07:59:11 PDT
Created attachment 197417 [details] [diff] [review]
patch

Same as above but:
1)sibling clipping check in ::Scroll() added
2)nsViewBeOS::DrawAferChildren() removed
3)Begin/EndResizingChildren emptied (to implement)
4)Added nsIRegion painting (mUpdateArea in *.h) to Scroll(), OnPaint(),
InvalidateRegion(), CallMethod()ONPAINT:
5)GetRenderingContext() used in OnPaint()

Hope tigerdog and tqh's will test it soon.
Comment 28 Doug Shelton 2005-09-26 08:33:58 PDT
downloaded nsWindow from CVS, applied 3rd version of patch.  anomalies with
chatzilla and 3rd button mouse-scroll icon are gone.  iFrame scrolling is much
cleaner than CVS version.  Scrolling is much improved over current CVS.
Comment 29 tqh 2005-09-26 10:26:44 PDT
Comment on attachment 197417 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se

Tested it and looked at the code, much better than before.
Comment 30 tqh 2005-09-26 10:31:07 PDT
It would be good if we could add this to the 1.8 branch as well. So that it is
included in Firefox 1.5 release.
Comment 31 Sergei Dolgov 2005-09-26 10:47:18 PDT
patch commited into trunk:
nsWindow.cpp
new revision: 1.95; previous revision: 1.94 
nsWindow.h
new revision: 1.36; previous revision: 1.35

Nielx, can you take care about branch?
Comment 32 Niels Reedijk 2005-09-26 11:03:17 PDT
Comment on attachment 197417 [details] [diff] [review]
patch

This patch touches BeOS-only files and therefore shouldn't be influencing the
quality of any other platform. Patch revieuwed, working verified and approval
will be highly appreciated.
Comment 33 tqh 2005-09-26 11:22:34 PDT
Adding an i-framed forum that is really good for testing this, even though it
might be the competitor :)
Comment 34 Mike Schroepfer 2005-09-26 14:38:18 PDT
Comment on attachment 197417 [details] [diff] [review]
patch

Approved per 9/26 bug triage meeting.

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