325906 - Remove one malloc from the JPEG decoder, and some optimizations

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

Right now in libjpeg, the structure decoder_source_mgr is dynamically allocated using PR_NEWZAP, however it is 32 byte structure allways used in the nsJPEGDecoder, so we can make it a fixed part of nsJPEGDecoder.
Also the members related to just the source_mgr can be moved to that structure to make it easier to access them (mBuffer, mBufferLen, etc).

Furthermore, the mBackBuffer allocates with increments of 16 bytes, but the general pattern then becomes 64 bytes, 128 bytes, 192 bytes, or something like that. Making it into chunks of 256 bytes, only one allocation of mBackBuffer is generally required, instead three. (This is based on some analysis of this allocation during web browsing).

This patch will remove some cruft, like replacing a two value enum and switch statement with a boolean. mPasses and mCompletedPasses are dead and can be removed.

Comment 1

[Alfred Kayser]

Attachment: V1: The first version for the review

Comment 2

[Alfred Kayser]

Attachment: V2: Slightly cleaner version

Cleaner version, by making the struct jpeg_source_mgr part of nsJPEGDecoder itself, removing the extra structure.

Comment 3

[Alfred Kayser]

The second patch makes jpeg_source_mgr a member of nsJPEGDecoder itself, and in the callback functions derives from the parameter jd->src the decoder pointer itself, so that it can directly reference to the members of the nsJPEGDecoder.

Note, the second patch is slightly bigger because:
* it also fixes the bug in the errorhandler with the errorcode translation (missing 'break' before the 'default:'
* the diff is completely confused by the whitespace change in fill_input_buffer in combination with the src->decoder-> replacement with decoder->

Comment 4

[Stuart Parmenter]

Comment on attachment 211763 [details] [diff] [review]
V2: Slightly cleaner version

i didn't actually check that this still builds but it looks good.  sorry for taking so long :(

Comment 5

[Ryan VanderMeulen [:RyanVM]]

The v2 patch is bitrotted badly. I'm un-bitrotting it and confirming it builds with MSVC2005 and will post it when I'm done.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted patch v2

As I said in my last message, this is the same patch but made off the current trunk. I've also verified that this compiles with MSVC 2005 and that JPEG rendering seems to be OK.

One thing I noticed when carrying over the patch is that there were some differences in nsJPEGDecoder::OutputScanlines(). Some of the changes in the v2 patch were already existent in there, so you might want to make sure that the changes this patch intends to make to that function are still desired.

Comment 7

[Alfred Kayser]

Comment on attachment 248594 [details] [diff] [review]
Un-bitrotted patch v2

Ok by me. Thanks for reviving this one!

Comment 8

[Stuart Parmenter]

Comment on attachment 248594 [details] [diff] [review]
Un-bitrotted patch v2

can one of you check it in or do you need me to do it?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

I don't have a CVS account

Comment 10

[Alfred Kayser]

Me neither, so if some one else can do the honour?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp 	1.68
mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h 	1.23

Comment 12

[Stuart Parmenter]

thanks gavin for landing this

Comment 13

[Alfred Kayser]

Note: This patch generated some warnings.

I will check if I can fix those

Comment 14

[Alfred Kayser]

gmake[6]: Entering directory `/builds/tinderbox/Fx-Trunk-Cairo/Linux_2.4.21-32.0.1.EL_Depend/mozilla/modules/libpr0n/decoders/jpeg'
nsJPEGDecoder.cpp
c++ -o nsJPEGDecoder.o -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.4.21-32.0.1\" -DOSARCH=\"Linux\" -DBUILD_ID=2007010412  -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/gfx -I../../../../dist/include/imglib2 -I../../../../dist/include/jpeg -I../../../../dist/include   -I../../../../dist/include/imgjpeg -I../../../../dist/include/nspr  -DMOZ_PNG_READ -DMOZ_PNG_WRITE  -I../../../../dist/sdk/include -I/usr/X11R6/include     -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O  -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsJPEGDecoder.pp nsJPEGDecoder.cpp
nsJPEGDecoder.cpp: In member function `virtual nsresult 
   nsJPEGDecoder::WriteFrom(nsIInputStream*, unsigned int, PRUint32*)':
nsJPEGDecoder.cpp:186: warning: unused variable `nsresult rv'
nsJPEGDecoder.cpp: In member function `PRBool 
   nsJPEGDecoder::OutputScanlines()':
nsJPEGDecoder.cpp:481: warning: unused variable `const int row_stride'
nsJPEGDecoder.cpp: In function `void skip_input_data(jpeg_decompress_struct*, 
   long int)':
nsJPEGDecoder.cpp:651: warning: invalid offsetof from non-POD type `class 
   nsJPEGDecoder'; use pointer to member instead
nsJPEGDecoder.cpp: In function `boolean 
   fill_input_buffer(jpeg_decompress_struct*)':
nsJPEGDecoder.cpp:688: warning: invalid offsetof from non-POD type `class 
   nsJPEGDecoder'; use pointer to member instead
nsJPEGDecoder.cpp: In function `void term_source(jpeg_decompress_struct*)':
nsJPEGDecoder.cpp:781: warning: invalid offsetof from non-POD type `class 
   nsJPEGDecoder'; use pointer to member instead

Comment 15

[Alfred Kayser]

See bug 366214 for the followup to remove the warnings and some more optimizations.