121015 - New server side image scaling code for gtk

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[tor]

I've been looking at image scaling on the gtk platform both in regards
to the horrible scrolling performance of scaled images (bug 82278) and
trying to get rid of the client side copy of images on the gtk
platform.

This new implementation of image scaling for the gtk platform uses
standard Xlib/gdk to perform image scaling on the server side image
copy.  Additionally this new code checks the size of the underlying
drawing surface to minimize the work it needs to do.  This helps
tremendously with scrolling, where now we only do the work needed for
the dirty region.  Summary:

Old code:

    * create client side 24bpp temporary size of entire scaled image
    * hand scale entire image
    * dither/putimage entire image (unless gdk is doing something
        smart behind our back)

New code:

    * create server side platform depth temporary size of 
        visible dirty region (if scaling in both directions)
    * visibleWidth+visibleHeight single pixel wide or high CopyAreas
        (less if scaling in only one direction)

If we get desperate for memory, the math can be tweaked slightly to
eliminate the need for a temporary scratchpad if scaling up in the
vertical direction.

There are, as you might expect, some drawbacks.  We're now scaling the
dithered, color reduced image.  Doesn't change the output for 24-bpp
displays, isn't noticeable on 16-bpp (at least, on the images I tested
on), but will suck visually on 1- and 8-bpp devices.  Are we willing
to sacrifice quality for performance on that segment of users?  The
first person to say "pref it" gets to write the code and convince mpt
that it's a good idea and deserves space in a pref panel.  :)

Comment 1

[tor]

Attachment: server side image scaling

Comment 2

[tor]

Attachment: nsImageGTK, but not as you know it :-)

Comment 3

[basic]

Does this patch also fix issues mentioned in bug 77334 ?

Comment 4

[tor]

Attachment: also move DrawToImage() work to the server

Comment 5

[tor]

Attachment: DrawToImage() must die...

Comment 6

[Roland Mainz]

If we "sacrify" 1bit and 8bit support then we will sacrify our support for
embedded devices as well - which are usually monocrome or 8bit color devices. I
doubt we want this (our <=8bit support is already very bad compared to what
NS4.x can do (NS4.x can dynamically change the palette, Mozilla's code doesn't
do this...)) ...

What about splitting the code into a optimised 16bit/24bit path and one
optimised <=8bit code path ?

Comment 7

[tor]

Attachment: fix clip boundary condition, fallback code for shallow framebuffers

Comment 8

[tor]

Attachment: merge to tip

Comment 9

[Adam D. Moss]

I've been running with this in my tree for a few days now
without seeing any problems (and at least one page with a
scaled animated GIF is significantly more responsive -- nice).

Comment 10

[Jan Varga [:janv]]

I've been running with this patch too, no problems so far
good work

Comment 11

[Adam D. Moss]

Okay, I should have kept my mouth shut because now I
found a problem.  :)  With the patch in my tree,
resource:///res/samples/gear1.gif yields one or two
frames and then completely stops animating (stops
using any CPU either).

That's with or without scaling.

I tried with-patch, without-patch, with-patch, without-patch
again and the problem is there consistantly with the patch
applied.  No problem without patch.  Hm.

Comment 12

[tor]

Attachment: handle DrawToImage() updates differently

Comment 13

[Stuart Parmenter]

Comment on attachment 70744 [details] [diff] [review]
handle DrawToImage() updates differently

looks good to me as well.  r=pavlov

Comment 14

[Adam D. Moss]

Yayyy, that fixes the problem.  Everything is good.

Comment 15

[Christopher Blizzard (:blizzard)]

Comment on attachment 70744 [details] [diff] [review]
handle DrawToImage() updates differently

sr=blizzard

Comment 16

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 17

[tor]

Checked in.

Comment 18

[Stuart Parmenter]

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