194791 - assertion from nsImageWin::DrawTile() when I load a message

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[(not reading, please use [e-mail] instead)]

assertion from nsImageWin::DrawTile() when I load a message

this assertion is coming from the patch checked in for #194261

+  NS_ASSERTION(aSXOffset < mBHead->biWidth && aSYOffset < mBHead->biHeight,
+               "Offset(s) are bigger than image.  Caller to DrawTile needs to
do more thinking!");

aSXOffset = 0
mBHead->biWidth = 50
aSYOffset = 5
mBHead->biHeight = 5

so we assert because aSYOffset == mBHead->biHeight (instead of <)

NTDLL! 77f97704()
nsDebug::Assertion(const char * 0x027a80f4, const char * 0x027a80b8, const char
* 0x027a8080, int 724) line 280 + 13 bytes
nsImageWin::DrawTile(nsImageWin * const 0x046721d0, nsIRenderingContext & {...},
void * 0x03977e50, int 0, int 5, const nsRect & {...}) line 724 + 54 bytes
nsRenderingContextImpl::DrawTile(nsRenderingContextImpl * const 0x03157d08,
imgIContainer * 0x046c0378, int 0, int 68, const nsRect * 0x0012ef4c) line 905 +
43 bytes
nsCSSRendering::PaintBackgroundWithSC(nsIPresContext * 0x02f05eb8,
nsIRenderingContext & {...}, nsIFrame * 0x03957610, const nsRect & {...}, const
nsRect & {...}, const nsStyleBackground & {...}, const nsStyleBorder & {...},
const nsStylePadding & {...}, int 0) line 3314
nsCSSRendering::PaintBackground(nsIPresContext * 0x02f05eb8, nsIRenderingContext
& {...}, nsIFrame * 0x03957610, const nsRect & {...}, const nsRect & {...},
const nsStyleBorder & {...}, const nsStylePadding & {...}, int 0) line 2789 + 41
bytes
nsFrame::PaintSelf(nsIPresContext * 0x02f05eb8, nsIRenderingContext & {...},
const nsRect & {...}, int 0, int 0) line 972 + 37 bytes
nsBoxFrame::Paint(nsBoxFrame * const 0x03957610, nsIPresContext * 0x02f05eb8,
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer
eFramePaintLayer_Underlay, unsigned int 0) line 1461
nsBoxFrame::PaintChild(nsIPresContext * 0x02f05eb8, nsIRenderingContext & {...},
const nsRect & {...}, nsIFrame * 0x03957610, nsFramePaintLayer
eFramePaintLayer_Underlay, unsigned int 0) line 1545
nsBoxFrame::PaintChildren(nsIPresContext * 0x02f05eb8, nsIRenderingContext &
{...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay,
unsigned int 0) line 1677
nsBoxFrame::Paint(nsBoxFrame * const 0x038a4d40, nsIPresContext * 0x02f05eb8,
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer
eFramePaintLayer_Underlay, unsigned int 0) line 1488
PresShell::Paint(PresShell * const 0x02f0a49c, nsIView * 0x038a6560,
nsIRenderingContext & {...}, const nsRect & {...}) line 5746 + 36 bytes
nsView::Paint(nsView * const 0x038a6560, nsIRenderingContext & {...}, const
nsRect & {...}, unsigned int 0, int & 1242060) line 283
nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x04651e40,
nsIRenderingContext & {...}) line 1228
nsViewManager::RenderViews(nsView * 0x02f4f740, nsIRenderingContext & {...},
const nsRect & {...}, int & 0) line 1177
nsViewManager::Refresh(nsView * 0x02f4f740, nsIRenderingContext * 0x03157d08,
nsIRegion * 0x037dfca0, unsigned int 1) line 763
nsViewManager::DispatchEvent(nsViewManager * const 0x02f4f500, nsGUIEvent *
0x0012f72c, nsEventStatus * 0x0012f640) line 1780
HandleEvent(nsGUIEvent * 0x0012f72c) line 83
nsWindow::DispatchEvent(nsWindow * const 0x02f4f7ec, nsGUIEvent * 0x0012f72c,
nsEventStatus & nsEventStatus_eIgnore) line 1113 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f72c, nsEventStatus &
nsEventStatus_eIgnore) line 1139
nsWindow::OnPaint() line 5170 + 28 bytes
nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long *
0x0012fbe0) line 3868 + 17 bytes
nsWindow::WindowProc(HWND__ * 0x00140782, unsigned int 15, unsigned int 0, long
0) line 1400 + 27 bytes
USER32! 77e3a290()
USER32! 77e146fc()
USER32! 77e15556()
NTDLL! 77fa02ff()
USER32! 77e1a752()
nsAppShellService::Run(nsAppShellService * const 0x00fa97b0) line 480
main1(int 2, char * * 0x00271cd0, nsISupports * 0x00f0ca18) line 1273 + 32 bytes
main(int 2, char * * 0x00271cd0) line 1636 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e9ca90()

Comment 1

[ArronM (:paper)]

taking.  I can probably track down the problem.

Yeah, if aSYOffset is 5, we are being told to start at y-pos of 5 of the image,
but the image is only 5 in height, which is 0 to 4 y-pos.  Probably some logic
in PaintBackgroundWithSC that needs tweaking.  I will look into it.

Comment 2

[ArronM (:paper)]

sspitzer, a few questions to help me try to reproduce it:

What theme are you using?
Does it happen on any message you view?
Viewing as in seeing it in the main mail window, or double clicking to bring it
up in it's own window?

Comment 3

[ArronM (:paper)]

Attachment: Patch to fix rounding problem

nm about those questions, sspitzer, I was able to duplicate once I realized I
wan on the wrong build. :)  As I suspected, a rounding issue.


Example:
  - Image is 5 pixels wide, TwipsPerPixel is 15, so image is 75 twips.
  - nsCSSRendering calculates aSXOffset to 68 twips.  It would never be 75,
    because the code is good and would adjust the aDestRect instead.
  - nsRenderingContextImpl converts twips to pixels by rounding to nearest
    interval, which is 75.  75/15 = 5.
  - 5 x-pos is beyond our bounds

Solution:
  Fix nsRenderingContextImpl to convert offset with truncating/flooring instead
of rounding.

Comment 4

[ArronM (:paper)]

Comment on attachment 115468 [details] [diff] [review]
Patch to fix rounding problem

See Comment #3 for explanation if patch comments aren't enough :)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 63336.

We have to round all coordinates (NOT widths, heights, or offsets). So here we
should be rounding (x0,y0), and then using the pixel offset
transform2D(drawRect.x,drawRect.y) - transform2D(x0,y0). That would fix the bug,
would it not?

Probably we should change the interface of nsIRenderingContext::DrawTile to take
the offset as an origin coordinate, and do the drawRect.x-x0 etc inside
nsRenderingContextImpl, after rounding.

Comment 6

[ArronM (:paper)]

No, that won't fix it.  For the example in Comment #3, drawRect.x = 0
Tranform of drawRect.x will still be 0, and a tranform of x0 will be 5.
5 - 0  still equals 5, which is beyond the bounds of the image.

If you really want to rounding as a standard, then we should be doing a final
check after the transform to see if the new value is beyond our bounds, and then
force it to our bounds.  I can make a patch which does that, if that's the
approach we want to take.

IMO however, truncating works just fine for the calculating the start offset
into the image.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> If you really want to rounding as a standard, then we should be doing a final
> check after the transform to see if the new value is beyond our bounds, and
> then force it to our bounds.

That's the approach I want to take. We have to round pixel coordinates only,
anything else is going to burn us on some testcase.

So I think we should compute "(offsetX, offsetY) =
transform2D(drawRect.x,drawRect.y) - transform2D(x0,y0)", then take the modulus
"(offsetX%imageWidth, offsetY%imageHeight)" so that any out of bounds offsets
are put back in bounds.

The problematic testcase is like this (in one dimension):
scaled(x0) = 0.4
scaled(drawRect.x) = 4.6
Consider the pixel to be painted at device coordinate 5. It is obtained by
sampling the continuous plane at 5.5. That point is 5.1 pixels from the left
edge of the tiled image. The image is 5 pixels wide, so we wrap around and use
the leftmost image pixel. This is exactly what my formula computes.

Comment 8

[ArronM (:paper)]

Attachment: Wrap Offset

Roc, something like this then?

- Pass location where origin of image should start to nsRenderingContextImpl
- Calculate offset in nsRenderingContextImpl by Tranform(aTargetRect.x) -
Transform(aXImageStart)  (aXImageStart == x0), then mod by width height.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're not transforming dr. And the image dimensions aren't transformed either.

Comment 10

[ArronM (:paper)]

-
   mTranMatrix->TransformCoord(&dr.x, &dr.y, &dr.width, &dr.height);
-
-  // i want one of these...

Doesn't that tranform dr?


and the dimensions coming from gfxIImageFrame are in pixels already.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, you're correct. I misread the patch and got your gfx..Frame mixed up with
layout frames.

How would you like to also fix nsTransform2D.cpp to scale the xmost/ymost of the
rect instead of its width/height? That change is a bit scary but I'm sure it's
the right thing. And hey, we're in alpha :-).

Comment 12

[ArronM (:paper)]

Attachment: Wrap Offset using [XY]Most()

I know enough about Transform2D to make ruin it ;)  Plus I have no idea what
you just said. :P  However, it did get me thinking about xmost.

We should be modulusing XMost instead of width in this patch.  Which made me
realize that nsCSSRendering::PaintBackgroundWithSC was using the image's width
for tileWidth instead of xmost.  

So here's a patch replacing width & height with XMost and YMost.  If you are
serious about changing nsTranform2D, you'll have to explain it to me like I
understand next to nothing about nsTranform2D (which I do).  If not, a SR would
be nice :)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 115567 [details] [diff] [review]
Wrap Offset using [XY]Most()

This XMost()/YMost() is for animated images where some frames are smaller than
others, right?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll do the nsTransform2D fix in another bug.

Comment 15

[ArronM (:paper)]

Comment on attachment 115567 [details] [diff] [review]
Wrap Offset using [XY]Most()

yes, it's for some frames that are smaller than the container.

Arg, that means I could just used the container's width, doesn't it?  So
nsCSSRendering code was okay to begin with, and I have to change the
nsRenderdingContextImpl code to use the container's size too.

I apologize for all the patches.  I should have did more brainstorming.

Comment 16

[ArronM (:paper)]

Attachment: Wrap Offset

okay, nsRenderingContectImpl is now using the container's width & height for
the modulus thing.

The assertion is removed because it's bogus.  You can still have an offset
bigger than the frame if the container is bigger than the frame.  (ie, frame is
10x10 in size at position 0x0, and the container is 50x50.. offset can be
anywhere from 0 to 49)

On a side note, we don't properly handle tiling frames that aren't the same as
the container's size.  I have plans on fixing this, mainly because the same
logic will be needed for CSS3's tiling-with-spacing-between-images spec. 
That's another bug (already filed, but I'm too lazy to look up the #).

Comment 17

[ArronM (:paper)]

Comment on attachment 115571 [details] [diff] [review]
Wrap Offset

smontagu volunteered to review this ;)

Comment 18

[Simon Montagu :smontagu]

Comment on attachment 115571 [details] [diff] [review]
Wrap Offset

r=smontagu

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

jdunn, this bug is about the assertion you mentioned on irc

Comment 20

[Simon Montagu :smontagu]

Arron, do you have any objectiont to me checking this fix in?

Comment 21

[ArronM (:paper)]

Sorry, smontagu, go right ahead and check it in.  I've had very little time
lately to do anything with Mozilla.

Comment 22

[Simon Montagu :smontagu]

Checked in.