685471 - In PNG decoder, the allocating of mHeaderBuf can be prevented.

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

mHeaderBuf is dynamically allocated when PNG images are scanned for size only (isSizeDecode), but it is only 24 bytes, so replacing a 4 bytes pointer with a 24 bytes array in the PNGdecoder class is already better than mallocing 24 bytes (causing fragmentation), but actually only an 4 bytes buffer buffer is needed to get the width and height.

Comment 1

[Alfred Kayser]

Attachment: Proof of concept patch

Remove the need to allocate a dynamic buffer by only buffering the size bytes into an 8 bytes buffer. 

By also sorting the members of the class, the total class size remains the same:
before: PRUint8 *, PRUint32, PRUint8, PRPackedBool, PRPackedBool, PRUint32, PRPackedBool = 20 bytes
after: PRUint8[8], PRUint32, PRUint32, PRUint8, PRPackedBool, PRPackedBool, PRPackedBool = 20 bytes

Comment 2

[Alfred Kayser]

Attachment: Compilable and tested version

Comment 3

[Glenn Randers-Pehrson]

I don't see why this patch adds "PRUint8 mAlphaBits;"
to the header file.  That might be part of the patch for a different bug.

Comment 4

[Alfred Kayser]

Attachment: V2: Don't add back mAlphaBits and update to current build

Comment 5

[Alfred Kayser]

Attachment: V2: Don't add back mAlphaBits and update to current build

Comment 6

[Alfred Kayser]

Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Green tryserver run (except for the usual intermittent issues):
https://tbpl.mozilla.org/?tree=Try&rev=beec8c74c875

Comment 7

[Glenn Randers-Pehrson]

Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Based on my code examination and your try-server results, +r.  Not sure I'm a reviewer, so Joe, please review also.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Review of attachment 745039 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

::: image/decoders/nsPNGDecoder.cpp
@@ +110,5 @@
>   : Decoder(aImage),
>     mPNG(nullptr), mInfo(nullptr),
>     mCMSLine(nullptr), interlacebuf(nullptr),
>     mInProfile(nullptr), mTransform(nullptr),
> +   mHeaderBytesRead(0), mCMSMode(0), 

please remove the trailing whitespace

@@ +305,5 @@
> +        mSizeBytes[mHeaderBytesRead - WIDTH_OFFSET] = bptr[pos];
> +      }
> +      pos ++;
> +      mHeaderBytesRead ++;
> +    } 

trailing whitespace all over here

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please don't add PR integer types. We haven't used them since last August.

Comment 10

[Alfred Kayser]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf288039ea21

Followup patch to replace the PRUint's:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04abf5e608f1

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cf288039ea21
https://hg.mozilla.org/mozilla-central/rev/04abf5e608f1