283343 - GFX::nsBlender - wrong order of parameters may cause wrong color depth calculation

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Sergei Dolgov]

In <a
href="http://lxr.mozilla.org/seamonkey/source/gfx/src/nsBlender.cpp#137">first
::Blend() method</a> of gfx/src/nsBlender.cpp

RowBytes and Stride paramters are swapped  in surf->Lock() calls and in following
Blend() calls inside the method
(compare to nsIDrawinSurface.h, if you wish).

Also if () at <a
href="http://lxr.mozilla.org/seamonkey/source/gfx/src/nsBlender.cpp#187">line
187</a> should use RowBytes instead spans.

Not so nice, despite double bug results in (almost) proper work.
But there is one issue with that - in <a
href="http://lxr.mozilla.org/seamonkey/source/gfx/src/nsBlender.cpp#182">PRUint32
depth = (srcRowBytes / aWidth) * 8;</a> - as srcRowBytes here is actually filled
with source span value, which isn't equal to to byte-width of rect-to-lock in
common case. Rect may be smaller than whole backbuffer, and even if those are
formally equal, actual span and byte-width may differ due to extra alignment
bytes used in some case in pixmaps.

Comment 1

[Sergei Dolgov]

adding roc to cc

Comment 2

[Sergei Dolgov]

Attachment: patch ( diff -uw4 )

here is fix

Comment 3

[jhp (no longer active)]

This patch fixes bug 282080.  However, on the Mac, I see the following assertion:
###!!! ASSERTION: Mismatched bitmap formats (src/dest) in Blender: 'srcSpan ==
destSpan', file /Users/pedemonte/builds/trunk/mozilla/gfx/src/nsBlender.cpp,
line 186

Comment 4

[Sergei Dolgov]

2 Javier
You are right. I put atention on real code, but forgot about assertions.
spans in assertion must be replaced by row-bytes, as it was done for if() check.

Will prepare updated putch.

Anything else?

Comment 5

[Sergei Dolgov]

Attachment: patch - same as previous but assertion fixed

Changed one assertion, also it's text to differentiate it from next assertion
and to reflect situation more exactly.

Javier, please test this one, if you do it before February 28, and all is ok, I
will set review request myself, but if you'll do it later, set r=? yourself, as
i'm departing and
 afraid that first week in NYC i'll lack Internet access.

Comment 6

[jhp (no longer active)]

Sergei, this latest patch works just as well, and no assertion.  Thanks.

Comment 7

[Sergei Dolgov]

*** Bug 282080 has been marked as a duplicate of this bug. ***

Comment 8

[Sergei Dolgov]

Comment on attachment 175557 [details] [diff] [review]
patch - same as previous but assertion fixed

i hope no more swapped parameters remained

Comment 9

[Sergei Dolgov]

roc, hope someone checks that in,
as i lack permissions outside BeOS-only code.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 175557 [details] [diff] [review]
patch - same as previous but assertion fixed

checked in