Closed Bug 283225 Opened 16 years ago Closed 15 years ago

[BEOS] - semitransparent in-page "popups" rendering is broken

Categories

(Core Graveyard :: GFX: BeOS, defect)

x86
BeOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sergei_d, Assigned: sergei_d)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(9 files, 1 obsolete file)

http://rezo.net
with mouseover events on text at left side of the page, semitransparent
text-frames do appear.
But in BeOS gecko browser, something is worng with those.
First, without "touching" that area with mouse text at leadt side is visible
partially, like being partially covered with opaque upper layer.

Second, if onmouse text-frame is raised, this is again visible partially.

Two screenshot attachments will follow.

Problem is that i don't know which part is guilty for that - some bug or
unimplemented thing in our gfx code, or 
some issues with visibility/z-order control in our widget implementation, or
some bugs in invalidating/clipping region handling between those two.

In Windows version all is OK.
Attached image screenshot 1
bad page rendering without "popups"
Attached image screenshot 2
partially-rendered "popups"
If i only understand XML and CSS and what visibility style tag should result
into in our code
<style>
div.introduction {
                font-size: 10px;
                padding: 5px;
                border: 1px solid #324C48;
                background-color: #e0EBE9;
                visibility: hidden;
                position: absolute;
                left: 25px;
                top: 0px;
                width: 300px;
                -moz-opacity: 0.9; filter: alpha(opacity=90);
        }
</style>

div style="position: absolute; visibility: hidden; z-index: 2000; top: 0px;
left: -150px;" id="lesrubs"

onMouseOver="changestyle('lesrubs','visible');
changestyle('lesthemes','hidden'); changestyle('lactu','hidden');

function changestyle(couche, style) {
if (!(layer = MM_findObj(couche))) return;
layer.style.visibility = style;
}
adding roc to cc as view and layout guru

Maybe single look of such man at the problem is enough:)
Does simple use of 'opacity' work in BeOS? You might want to check nsBlender.cpp
to see if it's doing the right thing on BeOS.

And does the problem depend on which screen depth you use?
There are group of "bugs" in nsBlender.cpp code - wrong order of parameters in
several places, and wrong check for  if (srcSpan == destSpan)
(must be if (srcRowBytes == destRowBytes) )
But those bugs compensate each other as far as i understood,
though, it will be nice if someone will fix all that mess, to avoid confusing
newcomer programmers.

Though, our problem seems being somewhere else, maybe in nsDrawingSurfaceBeOS
handling in nsRenderingContextBeOS code.

Some changes in nsBlender.cpp do improve situation, but do not fix it totally,
so i will continue investigations until at least March 27, 2005
(Leaving in NYC March 28)
Addition to previous comment.
No total "bug-compensation", though.
Color-depth value is calculated wrongly if stride/span != rowBytes,
and in generic case it shouldn't be equal, as locked rect may be only part of
surface pixmap
Problem happens only with two-source blending,
i will try if option to reuse backbuffer has some effect here.
Windows and GTK don't reuse backbuffer, while our port does it (with noreuse
option applied browser work is buggy)
Well, i found the reason for improper rezo.net rendering.

Problem is our nsDrawingSurfaceBeOS::FillRect()
which is asynchronous, as all other drawing utils.

Two-source blanding requires white color for one surface, black for another.
So view manager calls FillRect() when aquires those buffers.
But for the moment of blending due nature of BeOS app server, BBitmap isn't yet
colored.

I do think that in order to avoid similar issues in future in other places, i
will add Sync() to UnlockDrawable(), applying it in case of offscreen surface.

Though, i'm still wondering if you should file nsBlender.cpp bug myself.
Attached patch patchSplinter Review
review request.
I hope this one allows to avoid some other painting glitches, though, we should
revise gfx code to remove unnecessary Sync() calls, as we do it now in
UnlockDrawable()
Attachment #175329 - Flags: review?(thesuckiestemail)
Comment on attachment 175329 [details] [diff] [review]
patch

why do you now need to nullcheck mView when you didn't before?
heh, nullcheck landed here from version-in-work where i tried to create 3rd kind
of surface, pure BBitmap without view, for blending exclusively.

But i don't think it makes here any bad. Actually it is good manner for place
where we call LockLooper() and UnlockLooper() to check that. 
removing extra Sync() from nsImageBeOS
as we'll do it at next line in UnlockView()
Attachment #175342 - Flags: review?(thesuckiestemail)
Comment on attachment 175329 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se
Attachment #175329 - Flags: review?(thesuckiestemail) → review+
Attachment #175342 - Flags: review?(thesuckiestemail) → review+
checked in:
nsDrawingSurfaceBeOS.cpp
new revision: 1.20; previous revision: 1.19
nsImageBeOS.cpp
new revision: 1.33; previous revision: 1.32
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I'm not sure we should call sync all that often - it more efficent to let the
app server get a full buffer before doing it's thing.

The only time I can think of when we need to have drawing commands carried out
*right now* are for caret flashing (there's a sync() there already) and just
before handing off the pointer to a BBitmap in nsDrawingSurface::Lock().

I think called sync on the BBitmap child BView in Lock() should be sufficient.
"out *right now* are for caret flashing (there's a sync() there already)"
Is it? So i missed it in my cleanup attempts. Should also to be removed, as
UnlockView follows that.
We also had sync before in nsImage, because i got artifacts without that at
pages with tiled backgrounds and moving over that animated gif. But now it is
done there also via UnlockDrawable.

Also i saw artifacts at rezo.net with text even when synced in ::Lock() before
providing pointer. 

Those artifact may depend on computer speed and some other coincidences -
remember how for some people your (and mine) Lock() worked sometimes, and
sometimes it didn't.

So i think too that putting it UnlockDrawable is reliable and will free us in
future from searching for blcak cat in dark room if again some glitches will happen.

Oops, sorry - about cursor/InvertRect() again.
Do you mean we need it even for on-screen drawing?
Can you test it? If it is still needed (probably it does) even with syncing
bitmap now?
I forgot testcases and sites for that problem.
I'll just make a new latest CVS build, then I will test this.

I accept your point on Sync in UnlockDrawable, it probably is safer. How often
is UnlockDrawable() called? Once per update or something?
UnlockDrawable is called once per each mozilla drawing method call.
(Which may have several calls for BeOS drawing inside).

Btw, i tested now cursor problem WITHOUT explicit Sync() call inside InvertRect,
and it looks ok. but i need your confirmation.
Seems like there may be an issue with this one. When you scroll a page it seems
images that are in a specific point of the page, 1 page and 1/4 down or so, may
get white horizontal lines across it. I'll provide a screenshot.
Attached image Line artifacts
That is http://www.aftonbladet.se btw.
That's interesting.
Though, this bug/patch partially depends on 
https://bugzilla.mozilla.org/show_bug.cgi?id=283343

But maybe there is other reason, which get highlighted now.
Syncing is absolutely legal operation which itself cannot create any artifacts,
and actually, it is used in places for whose we call Invalidate(*, Synchronous
== PR_TRUE) in nsWindow.cpp (those calls are made for just that backbuffer
rendering in our case, asynchronous are for UI), so it makes synchronous drawing
REALLY synchronous.

Looks like some Draw()/Invalidate() call gets lost or provide insufficient
update rect for that part of scrolling, which renders prviously hodden part of page.

Or maybe CopyBits uses bit wrong rect?
Are you sure that is due to this transparency patch?

I have also noticed some slightly odd scrolling behaviour - sometimes a pixel 
line gets missed out just scrolling a normal text page (no transparency I 
thought). I thought these might be yet more rounding issues.
Yes I suspect that too. I also have my heavily modified nsWindow which I should
try without. As it seems to appear on specific images at specific places on page
and it seems to be just one line rounding errors is highly suspected though.
Attached image More line artifacts - just on text (obsolete) —
Here's a screenshot of the apparent rounding errors just on a normal page. See
the link at the bottom and the one near the top. It seems to suggest there is
something wrong in the blit as I can't even think how you would draw a string
with a line missing from the middle. It doesn't belong in this bug really,
IMHO, but this was where the previous discussion was happening.
This is a better shot. It's just from bbc news - definately no iframes or
transparency. I caused the artifacts by scrolling down to the bottom of a
long(ish) page and then scrolling up and down a little bit.

I'm reluctant to put this build on BeBits while we have this issue.
Attachment #175707 - Attachment is obsolete: true
Done some bugzilla searching:

Check out bug 171282 and bug 152671

It seems these errors are caused by having a twips-to-pixels conversion factor
that is exactly divisible by 2. In other words, probably cause by the new
conversion factor stuff that Sergei included in his latest fonts patch.

We should move discussion to bug 189353
It seems our paranoid-safe solution with syncing at each drawable unlock produces very bad results at some too lame sites, like
http::/www.amdzone.com

To allow it work noticeably faster, we should roll back a bit, and add two(?) Sync()s - before providing bits in nsSurfaceBeOS::Lock() and after getting changed bits back in nsSurfaceBeOS::Unlock().

Will search a bit more for most effective but still reliable version
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like only Sync() in :: Lock() is really sufficient to solve http://rezo.net
 problem
Attached patch patchSplinter Review
returning to less generic but faster solution proposed by Simon.
review request
Attachment #202897 - Flags: review?(thesuckiestemail)
Comment on attachment 202897 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se
Attachment #202897 - Flags: review?(thesuckiestemail) → review+
Landed in trunk
-----------------
Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/gfx/src/beos/nsImageBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsImageBeOS.cpp,v  <--  nsImageBeOS.cpp
new revision: 1.37; previous revision: 1.36
done 
-----------
Closing again
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Maybe BBitmap::LockBits()/UnlockBits() to be removed.
Honestly we didn't understand well why we put bitlocks there and how those work in BeOS
tqh, can you reproduce problem described here:
http://www.livejournal.com/community/bezilla/147377.html?replyto=896177
?
I think it will be reproducable. I havn't compiled with these changes yet.
tqh, couldn't reproduce it at SeaMonkey at PIII 2*500 Mhz machine. Neither at Athlon XP 2400+ (both BeOS R5.3 + Bone7a). So your test is welcomed.
All that looks really strange, as we retrned to state which worked before with really small additions which should be safe according to BeBook.
Though, that previous state worked before tigerdog joined our team with his machine.

Also wondering if it may be Dano app_server specifics.
It seems that original version of UnlockDrawable had flaws in logic even before patch. Reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patchSplinter Review
maybe this one is more correct.
still waiting for tqh's feedback, as i'm afraid don't see myself some stupid inconsistency in code anymore:(
seems working for tigerdog, but will wait for his further tests
latest patch (11-14) works well - fixes problems seen here.
Comment on attachment 203020 [details] [diff] [review]
patch

fixing unreliable lock/unlock code.
review request.
Attachment #203020 - Flags: review?(thesuckiestemail)
Assignee: general → sergei_d
Status: REOPENED → NEW
Component: General → GFX: BeOS
Product: Mozilla Application Suite → Core
QA Contact: general → timeless
Comment on attachment 203020 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se
Attachment #203020 - Flags: review?(thesuckiestemail) → review+
checked in trunk:
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.22; previous revision: 1.21
done 
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Comment on attachment 203020 [details] [diff] [review]
patch

er, why don't you need to unlock mBitmap?
"er, why don't you need to unlock mBitmap?"
And why this code is here at all, you can ask if investigate thing more?:)
https://bugzilla.mozilla.org/show_bug.cgi?id=200657
well indeed :-) But I don't understand why the lock in LockDrawable doesn't need a paired Unlock. Is it safe to assume that that won't cause problems?
Bahhh!!!! Sorry biesi
I was absolutely sure that there must is mBitmap->IsLocked() in LockDrawable!!!
Not mBitmap->Lock().

People, don't submit/checkin patches at day when you are dropping smoking! Abstinence affects your brain - look at results here...

Though, we need tigerdog again to test next version...

/me hides ashamed
Standing by and ready to test, fyysik!  Patch me up!
tigerdog - change line
http://lxr.mozilla.org/seamonkey/source/gfx/src/beos/nsDrawingSurfaceBeOS.cpp#259
rv = mBitmap->Lock();

to rv = mBitmap->IsLocked();

and see if those crashes will be back.
rebuilt (non-optimized, debug version) and have no problems with this change.  Compiled and tested in standard clock mode, then retested in OC mode.  No crash on startup.  I'll rebuild my optimized static version, too, but I believe we're OK with this.
confirmed latest change is OK with optimized build both standard and OC modes.
Attached patch patchSplinter Review
hope this time all things are at proper place.
Attachment #203436 - Flags: review?(cbiesinger)
Attachment #203436 - Flags: review?(cbiesinger) → review+
Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.23; previous revision: 1.22
done

Checked in trunk
Going to land in the branch.
Blocks: 311032
Comment on attachment 202897 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: this is the first in the new patch queue
Attachment #202897 - Flags: approval1.8.1?
Comment on attachment 203020 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: second patch of this bug
Attachment #203020 - Flags: approval1.8.1?
Comment on attachment 203436 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: third and last patch on this bug.
Attachment #203436 - Flags: approval1.8.1?
Attachment #202897 - Flags: approval1.8.1? → approval1.8.1+
Attachment #203020 - Flags: approval1.8.1? → approval1.8.1+
Attachment #203436 - Flags: approval1.8.1? → approval1.8.1+
Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.20.8.1; previous revision: 1.20
done
Checking in mozilla/gfx/src/beos/nsImageBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsImageBeOS.cpp,v  <--  nsImageBeOS.cpp
new revision: 1.34.8.2; previous revision: 1.34.8.1
done 
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.