Closed Bug 157303 Opened 22 years ago Closed 21 years ago

Mozilla for BeOS doesn't display images on many sites.

Categories

(Core Graveyard :: GFX: BeOS, defect)

Product:
Core Graveyard
Component:
GFX: BeOS
Platform:
x86
BeOS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Zaranthos_99, Assigned: beos)

References

Details

(Keywords: qawanted)

Attachments

(2 files, 5 obsolete files)

Screen shot
153.41 KB, image/png
Details
patch -up with new comment
13.33 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review 
Mozilla for BeOS doesn't display images on many web sites.  I tested several
builds in BeOS and they all exibit the same problem.  Mozilla 1.0 in Linux
doesn't have this problem.  I'll list a few sites that have this problem.

http://rus.delfi.ee
https://www.halifax-online.co.uk/_mem_bin/UMC001.asp
Zaranthos: ah, fill in the fields, and when you tab to the button it vanishes
http://www.pcworld.co.uk
http://www.nvidia.com
http://www.asus.com
Priority: -- → P5
Target Milestone: --- → mozilla1.1beta
tried to clean the cache or so? Reproducing a BeOS Bug is a bit complicate...
Tabbing to a button may make the image appear/disappear.  Scrolling the screen
up/down make make an image appear/disappear.  Moving the mouse over an image
make make it appear/disappear as will reloading a page sometimes.

Clearing the cache doesn't help.  The problem can be reproduced very easily on
many sites and happens the first time or everytime you visit the problem sites.
please don't set the target milestone or the priority unless you are the
assigned devloper - thanks

CC arougthopher@lizardland.net for BEOS
Keywords: qawanted
Priority: P5 → --
Target Milestone: mozilla1.1beta → ---
Over to ImageLib.
Assignee: Matti → pavlov
Component: Browser-General → ImageLib
QA Contact: asa → tpreston
problem is not that it don't show them at all, rather than those images
disappears. Or appears on some user actions, and then disappears again.
Installed BeOS here and I can reproduce the problem with a recent Mozilla
(2002071310).
Status: UNCONFIRMED → NEW
Ever confirmed: true
hm.. I have an "image ghost" here.. some images are displayed as being missing,
but loading them manually let them show up. The you go back to the page and they
are there. Shitf+reload -> image gone again... same bug?
Yes, Kai, that is this bug
It seems there are different reasons for those sites.
With last BeOS patches and maybe changes in above mozilla code, site *.delfi.ee
now WORKSFORME.
Other mentioned here still have problems
I have a feeling this is a network issues.  This is why Sergei does not see the problem, 
since he is running BONE, which has a better network configuration.  BeOS net_server is
crappy, to say the least.  
My hunch is that this is within NSPR as well (maybe a blocking issue), but have not
been able to verify this.  There is currently another bug, which I think has a patch that
I have not had a chance to test, for getting PRLog to a file working under BeOS again.
Once I have done that, I need to add some logging to the BeOS networking code, and try
to figure out what is going on in there with these images.

I can almost always see this on http://www.osnews.com, in the icons for the articles.
It seems as if mozilla gives up trying to load the image too quickly.
I see all those issues under BONE too.
Just one site from examples works bit better for me now
and i really doubt that it is (only) network-related. 
I think that this problem needs also ivestigation of code for those pages.
http://www.beosjournal.org is the latest victim of the image not displaying bug.
Reminder - should study code from http://www.3rd-evolution.de/BeOS/MDI/ 
Bug #1: code for ContainsRect in nsRegionBeOS is wrong.
Intersects != Contains !!!!
from the nsIRegion.h file:/**  * does the region intersect the rectangle?  *  * @param      rect to check for containment  * @return     true if the region intersects the rect  *  **/  virtual PRBool ContainsRect(PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight) = 0;So, do we believe the comment or the method name?  If the comment, then our implementationis correct.  If we take the method literally, then it must be fully contained within the rect.
Nice find Paul - I was just about to post the same thing.For reference:- qt calls mRegion.contains(rect);- windows calls RectInRegion(mRegion, &trect)- mac calls RectInRgn(&macRect, mRegion)
So to summarize where we're at, the problem can temporarily be fixed by removing the call to ConstrainClippingRegion in nsRenderingContextBeOS::UpdateView(). In this case, no drawing disappears, but looking at drop down boxes, list views, etc. there are visible problems where the drawing hasn't been clipped. So for some reason the clipping region is occasionally wrong. One thing I tried was calling ConstrainClippingRegion(NULL) in UpdateView() before applying the new region, but this didn't work. Technically though, it looks like it would be necessary - otherwise you would be further reducing the current region with the new one (basically intersecting them). Could someone please check whether the clipping is taking place in the correct units (i.e. pixels)?
I was dumping the BRegion last night, to see if anything was a little "odd".  I
did not notice anything, but, the values did seem to be in pixels.
I will try to implement missing IsEqual, and ContainsRect as "total inclusive"
check to see what happens.
Though, Be API for regions is quite poor. So it may need some work.
Btw, previous playing with ContainsRect solved here problems with:

http://rus.delfi.ee
and 
https://www.halifax-online.co.uk/_mem_bin/UMC001.asp
(missing "statical" buttons, so defects appearing on redraw only (like scrolling)
but not for renmaining sites which seems using (JS) layers or some other more
sofisticated technique
When I was dumping the information about each region. NONE of the regions were
complex, i.e. only single rectangles.
OK, my fellows!
It seems that bug is found!
Problem is with "nullregion". It is valid nsRegion with aX==aY==aWidth==aHeight==0.
But in BRegion it results in region with single BRect with
Frame().Width()==Frame().Height() = -1;

Thats not so big problem, but problem (and there is remedy) is
nsRegionBeOS::SetRegionType() implementation - it autoassigns mRegion with
RectsCount == 1 to type eRegionComplexity_rect which is definitely wrong.
Setting it for this case to eRegionComplexity_empty fixes our problem.

So i'm going to check if there any side-effects and submitting patch after that
(there are lot of other changes in my code, but only that one was definitive -
so trying with untouched nsRegionBeOS.cpp).
Sorry, don't care about previous post. Sending it to /dev/null.
My mistake - in reality i did same thing as Daniel - disable SetClippingRegion
at all.

But i have question - is possible that Mozilla adds rectangle to region
originating from another view?

P.S. Maybe we need better place to communicate:(
Addition to comment 
http://bugzilla.mozilla.org/show_bug.cgi?id=157303#c17

Replacing 
mView->ConstrainClippingRegion(region);
with
mView->ConstrainClippingRegion(0);

fixes both problems - disappearing images, and artifacts in menus and drop-downs.	
(Also seems cured issue with stalling scrolling down in big bookmarks menu)
(As far as i remember we are using 	ConstrainClippingRegion() in 3 more places -
CopyBits in nsRenderingContextBeOS.cpp

and in OnPaint() + Scroll() in nsWindowBeOS.cpp

maybe some of them is overhead?
	
Paul, Daniel, can you test? - solution from previous comment
(http://bugzilla.mozilla.org/show_bug.cgi?id=157303#c23)

fixed also famous long-time bug about buggy scrollbars here
(http://bugzilla.mozilla.org/show_bug.cgi?id=95348 )
If it is case for you, we should set dependency or duplication.
Sergei, this does the same thing as my change (commenting out the line completely). Either way, the drawing problems disappear, BUT new problems come up. For example, go to the Apple online store and try to configure a PowerBook, for example. You'll notice that if you play with the drop down boxes there to choose different options, sometimes the string will draw outside of its bounds. I've seen similar problems with text boxes on other sites too. In other words, turning off clipping completely is not a good solution either. We need to fix the clipping region instead.
Daniel, it may be not just the same, because in my case it cleans
ClippingRegion, which  maybe was set by 3 other instances of ConstrainClipping
Region mentioned above.
So i don't see yet any such hard issues.

But maybe we can do something alike:
in UpdateView:
if(!mClipRegion)

Daniel, it may be not just the same, because in my case it cleans
ClippingRegion, which  maybe was set by 3 other instances of ConstrainClipping
Region mentioned above.
So i don't see yet any such hard issues.

But maybe we can do something alike:
in UpdateView:
if(!mClipRegion)
  mView->ConstrainClippingRegion(0);
instead existing code,

and set clipping in nsRenderingContextBeOS::SetClipRegion in Windows port manner
- something alike:
NS_IMETHODIMP nsRenderingContextBeOS :: SetClipRegion(const nsIRegion& aRegion,
nsClipCombine aCombine, PRBool &aClipEmpty)
{
	BRegion *rgn, *currgn;
	rgn = currgn = nsnull;
	int  cmode, cliptype;

    aRegion.GetNativeRegion((void *&)rgn);
	
	if (rgn && currgn && mView)
	{
		mView->GetClippingRegion(currgn);
		switch (aCombine)
		{
			case nsClipCombine_kIntersect:
				currgn->IntersectWith(rgn);
				break;
		    case nsClipCombine_kUnion:
				currgn->Include(rgn);  
			    break;

		    case nsClipCombine_kSubtract:
    			currgn->Exclude(rgn);
				break;

		    default:
    		case nsClipCombine_kReplace:
        		currgn = rgn;
				 break;
		}

    PushState();
    mView->ConstrainClippingRegion(currgn);
	}
	else
    	return PR_FALSE; 

	if (currgn->Frame().IsValid())
    	aClipEmpty = PR_TRUE;
	else
    	aClipEmpty = PR_FALSE;

	return NS_OK;
}  

instead existing implementation.


P.S. Which browser you use? Your posts are non-wrapped, like ones posted from
NetPositive.
Daniel, little notice and question.
With code as is (ConstrainClippingRegion(region), obscured images appears when
you MOVE some other window over this disappeared image.
But don't if yoy hide Mozilla under some other window, and then activate Mozilla
again.

As i understand, Clipping region is thing for AppServer to deal with. But in one
case it affects redrawing, in other don't.

So maybe nsRegion code is ok, but we just don't call UpdateView when it supposed
to be called, so it don't update ClippingRegion via UpdateView when it supposed,
or just ignore some events?
addition to app_server notice (my previous comments):
This code in UpdateView - 
		if (mClipRegion ) 
		{
			mClipRegion->GetNativeRegion((void *&)region);
	        if(mView->Flags()&B_WILL_DRAW)
	           	mView->ConstrainClippingRegion(region);
		    else
		    	mView->ConstrainClippingRegion(0);
			
		}
does job very nice for me - no obscured images, bo artifacts, nice listboxes in
forms
I have suspects that reported in some other bugs disappearing caret in text
fiels (not the bug about too lefty 0-position) may be also related to this problem.
Attached patch Patch (obsolete) — — Splinter Review 
This is a patch based on Sergei's comments.  It really does seem to work.  I
have not seen any problems, and, works with the test Daniel had sent me.
And yes, it does seem to fix the missing caret problem.  (I've tried my
damndest to make it diappear, and it hasn't yet :-) )
Paul, a few problems here. First, there's no reason to get the native region if you're going to be setting it to NULL. Second, there's a typo in ConstrainClippingRegion(0) in your patch. But the big problem is that some drawing is no longer clipped. Try choosing an optional display from the Apple Store when configuring a PowerBook for example.
Attached image Screen shot — 
This shows that text was not clipped properly in both the drop down box for
choosing a monitor, as well as the top URL bar.
I also noticed problems with text in URL and some other places, Daniel, but that
is more weird problem than you imagine.

Text now don't go to wrap in some situation where it supposed to do so.

It looks like that some boundaries were reported according to native clip region
set (in some other place in Mozilla "higher-level" code), and maybe this is the
reason for missing wrapping. 
Little addition about this "non-hidden" text.

It behaves now JUST in oppoite way to disappeared images in comment
http://bugzilla.mozilla.org/show_bug.cgi?id=157303#c28

 - if you move SLOWLY some window over this "extra" text - you can erase it, so
all looks OK. But you cannot erase it by obscuring under another window - and
erased text goes visible again when you bring Mozilla window to front.
And also - you cannot erasi it totally, if you move "eraser" window too fast.
I wish Daniel provide me with other than URL-bar and AppleStore examples for
exceeding text arising in case of patch.

Both are same case - very annoying for BeOS hybrid object - drop-down box.

Problem is - those objects consist of BView belonging to top-level window and
separate window (created as eWindowType_popup) window. (So if you look at
BWindows list in AplleStore case, as for any window with drop-down boxes, e,g,
on BeZilla page, you can see bunch of w-> threads)

I have feeling that here appears again famous contradiction between BeOS and
remaining world - windows are in separate thread (and so at different loopers),
and popup window don't belong to window where "first item of list" is placed.
And i'm afraid that somewhere it wasn't took in account, so some Mozilla's
paint/update/drawing-setting message pass to such object only once, for object
as whole. Which is insufficient in BeOS case.
Found "ideal" solution for nsRenderingContextBeOS.
Just like each UpdateView() is balanced with UnlockLooper(), same way (almost)
each  UpdateView should be also balanced with ConstrainClippingRegion(0) in
drawing primitives (befor UnlockLooper).

No disappeared images, no strings with broken Z-order.

Patch http://bugzilla.mozilla.org/attachment.cgi?id=109359&action=view should be
removed/obsoleted
Comment on attachment 109359 [details] [diff] [review]
Patch

obsoleting
Attachment #109359 - Attachment is obsolete: true
Attached patch patch for testing (obsolete) — — Splinter Review 
Creating pair for UpdateView() - UnlockViewLooper() and replacing
mView->UnlockLooper() with it.

Though, i dislike current form and name of UpdateView - i prefer something like


bool LockAndSetView(BView *)
{
bool retvalue;
 ****
 if((retvalue =(mView && mView->LockLooper))) {
  ****
 }
 ****
 return retvalue;
}

and same for this new function.
New inestigation (as comment for previous patch) - it seems that
ContstrainClippingRegion(0) before UnlockLooper is needed only in 

NS_IMETHODIMP nsRenderingContextBeOS::DrawString(const char *aString, PRUint32
aLength,
	nscoord aX, nscoord aY, const nscoord *aSpacing).

It seems related to double draw mode change in this function.
Ohh, it appears that since spring of 2001 DrawImage and DrawTile is outside
nsRenderingContext in reality. in ns*Image.cpp.
Which does sets clipping and other things independently. And hasn't UpdateView
analog.
I suspect some collisions there.
Attached patch Patch for DrawString (obsolete) — — Splinter Review 
Anyway, submitting patch which fixes mentioned problems for me.
Inspite it seems more like hack.
Attachment #109825 - Attachment is obsolete: true
it seems there really are another clipping mistakes, supposedly related to
nsImage.cpp methods - for example on http://lingvo.yandex.ru background image
for submit buttons (on right) spreads itself above this button even with applied
patch.
Not just buttons either with not enough clipping. Like one of the bugs you've
linked to, images in table cells sometimes spread outside of their bounding
boxes (the fire animation in the tinderbox does this, as well as viewer demos 3
and 4). This is on a trunk nightly from 4 December, so no patches.

Also, I don't like the ConstrainClippingRegion(0) thing - it feels like a hack
to me.

BeOS gfx is still slow, and there are now quite a few bugs (scrolling,
disappearing images, also -moz-opacity seems to make things completely
invisible). What we really need is a full rewrite, after deciding on a much more
efficient looper-locking mechanism. We can implement it with just speed in mind
(using aFontID and GetWidth and things to speed rendering, for example). Plus,
the bugs with clipping would probably be fixed during this process.

What does anyone think?
A rewrite would be nice, but the total lack of documentation makes that very difficult. It's the main reason I'm not contributing to the project anymore. There is no explanation of how the graphics subsystem works, who owns what, how the double buffering and scrolling operate, what the coordinate system assumptions are, etc. All you get are one or two sentences above methods in the interface headers, things like "nsImage::Optimize() optimizes the image". Having said all that, I think that the BeOS widget code is even more broken than the gfx code. Text boxes, scrollbars, and drop downs all still have problems. Plus there's no drag-and-drop, no printing, and other higher-level features that people miss. In my opinion, the disappearing images is a priority, but apart from that the gfx implementation is acceptable enough to leave alone for a while, and fix bugs elsewhere.
I know there isn't a lot of documentation, I've been trying to find it for the
last couple of weeks. As you say the interface headers aren't particularly
helpful (a bit like Microsoft's .net help: "IsNavigable sets whether or not the
data grid is navigable"). However there are plenty of other platform gfx
implementations to look at - most of the purpose of the functions can be guessed
from this. Side note: is there a reference platform with everything implemented?
GTK, Windows?

I guess it depends on your system how important the gfx speed is. On my lowly
K6/2 450 it is usable enough to be my fulltime browser, but a lot slower than
moz on windows (or even IE). Slow enough that I have to quickly switch to a
different workspace and fire up soundplay or something to remind me I'm running
BeOS. Getting to the bottom of the images problem would be a good excuse to try
a new, fast gfx implementation. There's plenty of other stuff that needs doing
too, of course. But if we could get a 10x gfx speedup, I could live with
scrollbars that sometimes appear when they shouldn't and not having any printing ;)
Anyway, i'm doing gfx revision now. Some things are obsolete, far behind time.
E.g. in nsImageBeOS DrawToImage code was too old and caused big processor load
on animated images.
Also there were two version of DrawTile (background painting method) - and only
one of them exists now in other ports and even in public definition.

And more, seems here, in nsImageBeOS, is just reason for this bug: for quite
long time Mozilla uses DrawImage and DrawTile from nsImage instead
nsRenderingContext - and DrawImage in nsImageBeOS lacks
ConstrainClippingRegion(rgn) (all drawing methods in nsRendering contain it via
UpdateView). Once added, it fixed bug.
Does this fix all clipping problems - both disappearing images and images
spreading outside their bounding box? What about text box carets?

I guess this means ConstrainClippingRegion(0) is no longer needed. That would be
great, I was never happy with that as a permenant fix.
1)I didn't see bugs with viewer demos 3 and 4 for long time
2) I don't use ConstrainClippingRegion(0) in UpdateView(). Only place where it
was added in my code, as i noticed in one of comments - end of DrawString
functions. But now i removed it even from there.
3) Patch for polygons fixed part of caret problem in single-live fields for me,
but ther is another problem, related to focus/activation events, when caret
sometimes don't appear on field on mouse click - it appears there after some
keyboard input - and if you delete all input - cret is still visible in ultimate
left position
"mass" re-assigning of bugs from pav to myself
Assignee: pavlov → jdunn
*** Bug 125477 has been marked as a duplicate of this bug. ***
Patch for http://bugzilla.mozilla.org/show_bug.cgi?id=201624
obsoleted this problem.
Mozilla now uses XUL-scrollbars.
But problem is about status - should it be FIXED ? WONTFIX?
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
oops, closed wrong bug, reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 218134
*** Bug 201536 has been marked as a duplicate of this bug. ***
Attached patch very important patch (obsolete) — — Splinter Review 
Fixing this most long-standing, most awful and annoying BeOS GFX bug.
I think there are lot of several dups of it on bugzilla.

Problem was explained in http://bugzilla.mozilla.org/show_bug.cgi?id=157303#c41


To solve problem  in unified with nsRenderingContextBeOS way, i made former
UpdateView() public (like UpdateGC in gtk port), changed it to bool (in order
to return test results for mView existance and its locked state) and changed
this function name to LockAndUpdateView().

Then used this LockAndUpdateView() in nsImageBeOS Draw() and DrawTile()
methods.
Attachment #110178 - Attachment is obsolete: true
Comment on attachment 138338 [details] [diff] [review]
very important patch

review request
Attachment #138338 - Flags: review?(timeless)
Patch seems to solve many graphics problems. Works much better when scrolling.
No redraw-problems. Couldn't spot any rendering problems at all in fact.
Comment on attachment 138338 [details] [diff] [review]
very important patch

+	//LockAndUpdate() sets proper clipping region here and elsewhere in
nsImageBeOS.
+	if(((nsRenderingContextBeOS&)aContext).LockAndUpdateView()) {	

the comment should say LockAndUpdateView, not LockAndUpdate, apparently

	}
	beosdrawing->ReleaseView();
+	}

is this a -w patch? otherwise, the indentation here is wrong.
biesi,
yeah, i did diff -uwp.
Should i use different diff flags set?
Attachment #138338 - Attachment filename: nsImageBeOS.patch → nsImageBeOS.patch, diff -uwp
Assignee: jdunn → arougthopher
Status: REOPENED → NEW
Component: ImageLib → GFX: BeOS
QA Contact: tpreston → timeless
Attached patch Patch (diff -up) (obsolete) — — Splinter Review 
Comment changed.
type of LockAndUpdateView() changing to bool (to be compatible with
BView::LockLooper in return value and to look C/C++ "native" when used in
if()).
Attachment #138338 - Attachment is obsolete: true
Comment on attachment 138338 [details] [diff] [review]
very important patch

removing review request from obsoleted patch
Attachment #138338 - Flags: review?(timeless)
Comment on attachment 138385 [details] [diff] [review]
Patch (diff -up)

review request
Attachment #138385 - Flags: review?(timeless)
Added large comment (by biesi request) about LockAndUpdateView() method
Attachment #138385 - Attachment is obsolete: true
Comment on attachment 138389 [details] [diff] [review]
patch -up with new comment

marking r=timeless from irc
Attachment #138389 - Flags: review+
Status: NEW → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: