254659 - nsImageGTK::Draw flubs source rect; image splitting broken

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I'll attach a testcase that demonstrates the problem.  To reproduce:

1)  Load attached testcase.
2)  Print preview
3)  Scroll down to second page

EXPECTED RESULTS: second page shows bottom part of image that did not fit on
                  first page.

ACTUAL RESULTS: second page shows copy of top part of image from first page.

ANALYSIS:  After the initial clipping part, nsImageGTK::Draw completely ignores
the aSX and aSY arguments.  None of the functions called for actually drawing
things take those arguments.  The one I traced (XlibRectStretch) has   
xs1 = ys1 = xd1 = yd1 = 0 throughout the method (and one wishes the comment
before the method matched reality, but...).

In any case, this has two main effects:

1)  Splitting images in print preview is totally broken.  When we have columns
    working, it'll be broken there too.
2)  Images have to do extra (inefficient) work because attempting to only paint
    the dirty region paints a totally different rect instead.  See
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsImageFrame.cpp#1364
    for the XXX comment.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Image for testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

I should clarify this is the case when the image needs scaling.  When we're just
bit-blitting we manage to look at the source rect coordinates.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Anne, that URI you stuck in the URI field is wrong (since the file has changed
since comment 0).  Want to fix it to be the right URI or something?

Comment 5

[Anne (:annevk)]

The link given in comment 0 did not work anymore due to changes in layout I
believe. I just found the file and thought that was the comment you referred to.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It's not.  The right comment in
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsImageFrame.cpp starts
with "// XXXbz it looks like we should take"

Comment 7

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

Our GTK image scaling code is a disaster. It was never designed to handle source
rects properly, and as you noticed it's really hard to follow.

I can get it sort-of-working but the whole approach of adjusting the source and
destination rects to account for clipping-to-the-surface and the region that's
been decoded is just wrong and leads to problems where we're inconsistent about
which rows/columns we decide to duplicate. This creates weird artifacts which
we've seen in other bugs.

This code is all going to go away in 1.9, thankfully. In the meantime I would
really like to fix it for 1.1. I'll have to have another crack at it on Monday.

Comment 8

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

Right now I'm leaning towards not solving this for 1.8, on the grounds that any
solution will be risky and painful, we've lived with this bug in various forms
for a long time, and in 1.9 it will just go away.

Comment 9

[Mats Palmgren (inactive)]

In a recent Firefox trunk build on Linux there is no 2nd page at all,
the image is cut off at the end of the first page.

Comment 10

[Stuart Parmenter]

this should get a new title and description and summary if it is going to live in Gfx: Thebes given that none of the code mentioned in this bug is used anymore.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I've filed bug 384792 on the issue Mats sees in comment 9.

This bug is worksforme with thebes.  I'll attach a testcase that actually has the image splitting, and seems to work fine over here.

Would be nice to get this testcase into the regression test framework, but I'm not sure how.  Perhaps one test with two images (one per page) and one test with the two pasted together into a single image and splitting at the exact right place?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with workaround