370942 - Remove non-Cairo from jpeg decoder and optimize loop

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Alfred Kayser]

Attachment: V1: The first version for the review

* Remove the non-Cairo code
* Loop optimized from 20 CPU instructions to 13.
* Only query img interface when really needed
* Use GFX_PACKED_PIXEL for pixel packing

Comment 1

[Alfred Kayser]

Attachment: V2: updated to current trunk

Simple loop optimization and use GFX_PACKED_PIXEL macro

Comment 2

[tor]

Comment on attachment 256695 [details] [diff] [review]
V2: updated to current trunk

>-      for (PRUint32 i=0; i < mInfo.output_width; ++i) {
>-        PRUint8 r = *j1++;
>-        PRUint8 g = *j1++;
>-        PRUint8 b = *j1++;
>-        *ptrOutputBuf++ = (0xFF << 24) | (r << 16) | (g << 8) | b;
>+      for (PRUint32 i=mInfo.output_width; i>0; --i) {
>+        *ptrOutputBuf++ = GFX_PACKED_PIXEL(0xFF, j1[0], j1[1], j1[2]);
>+        j1+=3;
>       }

Why did you flip the loop control direction?

Comment 3

[Alfred Kayser]

This is the same also done for the PNG decoder:
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/png/nsPNGDecoder.cpp414         414         for (PRUint32 x=iwidth; x>0; --x) {
415           *cptr32++ = GFX_PACKED_PIXEL(0xFF, line[0], line[1], line[2]);
416           line += 3;
417         }

Because the walker value (x) is not needed in the loop itself, using a flipped loop allows for better (smaller and faster) looping code.
This reduces the looping code from 20 instructions to 13, and removes the need to check 'mInfo.output_width' in the loop.

Comment 4

[tor]

Comment on attachment 256695 [details] [diff] [review]
V2: updated to current trunk

One would hope that the compiler would already do that sort of optimization, but probably can't hurt.

Comment 5

[Alfred Kayser]

Attachment: V3: Also update the Makefile.in to include thebes

Comment 6

[Stuart Parmenter]

Comment on attachment 256838 [details] [diff] [review]
V3: Also update the Makefile.in to include thebes 

> REQUIRES	= xpcom \
> 		  string \
> 		  gfx \
>+          thebes \

please use tabs in makefiles.

sr= with that change

Comment 7

[Alfred Kayser]

Attachment: V4: Corrected the tab issue in Makefile

Who can do the checkin of this?

Comment 8

[Nickolay_Ponomarev]

Attachment: checked in

Comment 9

[Nickolay_Ponomarev]

Checking in nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.72; previous revision: 1.71
done

The makefile.in change was already in.

Comment 10

[Alfred Kayser]

Thanks!

Comment 11

[Nickolay_Ponomarev]

This may have caused orange on qm-xserve01 dep unit test, I'm waiting for it to cycle one more time to see if the box cures itself.

Comment 12

[Nickolay_Ponomarev]

Nope, looks like it was just a glitch with the box.