For some image formats, now that SetImageData (and SetAlphaData) can handle multiple rows, we can set multiple rows with one call to SetImageData. nsBMPDecoder.cpp: * Replace SetImageData(array); if (cnt==0) memset(array, 0) with SetImageData(cnt?nsnull:array) * Optionally for all rows except first (cnt!=0) do the SetImageData only once, but for now I find the line calculation a bit tricky to touch... nsICODecoder.cpp: * When decodedLineLen == bpr, do one SetImage for whole image * Optional: Instead of doing malloc(alpharow), convert alpha bits to bytes, SetAlphaData, free(alphaRow) do a 'GetAlphaData(ptr)', and expand all alpha bits directly into alpha array from frame (or add a function to GfxImageFrame to this effect: gfxImageFrame::SetAlphaBits (doing expansion from 1bit to 8bit if needed?)). nsIconDecoder.cpp: * Replace multiple SetImageData with one SetImageData (for whole image). imgContainerGIF.cpp: * Combine: BlackenFrame(mCompositingFrame); SetMaskVisibility(mCompositingFrame, PR_FALSE); into: ClearFrame(mCompositingFrame); nsGIFDecoder2.cpp: * Replace multiple SetImageData(nsnull) with one. nsIconDecoder.cpp: * Replace multiple SetImageData(data) with one. * Replace multiple SetAlphaData(data) with one.

See also bug 315966 and 301594

Comment on attachment 204768 [details] [diff] [review] Optimize calls to SetImageData from the imageDecoders >--- icon/nsIconDecoder.cpp 15 Nov 2004 04:16:06 -0000 1.13 >+++ icon/nsIconDecoder.cpp 2 Dec 2005 10:42:48 -0000 >@@ -137,38 +132,22 @@ NS_IMETHODIMP nsIconDecoder::WriteFrom(n > > PRUint32 bpr, abpr; > PRInt32 width, height; > mFrame->GetImageBytesPerRow(&bpr); > mFrame->GetAlphaBytesPerRow(&abpr); > mFrame->GetWidth(&width); > mFrame->GetHeight(&height); > >- i = 0; >- PRInt32 rownum = 0; // XXX this better not have a decimal >- >- PRInt32 wroteLen = 0; >+ mFrame->SetImageData(data, bpr*height, 0); My compiler complains about an invalid conversion from (char*)data to (PRUint8*) here. I've gone through most of this patch. Looks good so far.

Comment on attachment 204768 [details] [diff] [review] Optimize calls to SetImageData from the imageDecoders In addition to the error from comment 2: >@@ -137,38 +132,22 @@ NS_IMETHODIMP nsIconDecoder::WriteFrom(n >- // now we want to send in the alpha data... >- for (rownum = 0; rownum < height; rownum ++) >- { >- PRUint8 * line = (PRUint8*) data + abpr * rownum + height*bpr; >- mFrame->SetAlphaData(line, abpr, (rownum)*abpr); >- } >+ // now we want to send in the alpha data... >+ PRUint8 * line = (PRUint8*) data + height*bpr; >+ mFrame->SetAlphaData(line, abpr*height, 0); Don't call this |line| any more. When compiling nsGIFDecoder2.cpp, I get a warning that initialization will occur in an order other than what's shown in the ctor implementation. nsGIFDecoder2.cpp: 83 , mAlphaLineMaxSize(0) 84 , mRGBLineMaxSize(0) nsGIFDecoder2.h: 119 PRUint32 mRGBLineMaxSize; 120 PRUint32 mAlphaLineMaxSize; Could you reverse the order in nsGIFDecoder2.cpp as long as you're modifying that file? r=me for a new patch with these three changes.

One other fix: * in nsIconDecoder.cpp, make sure that the allocated buffer is freed in error situations, to prevent MLK... Assuming r=mark, going for superreview!

Comment on attachment 205630 [details] [diff] [review] V2: Incorporated comments from Mark >Index: icon/nsIconDecoder.cpp >=================================================================== >RCS file: /cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp,v >retrieving revision 1.13 >diff -u -8 -p -r1.13 nsIconDecoder.cpp >--- icon/nsIconDecoder.cpp 15 Nov 2004 04:16:06 -0000 1.13 >+++ icon/nsIconDecoder.cpp 12 Dec 2005 14:59:08 -0000 >@@ -92,83 +92,56 @@ NS_IMETHODIMP nsIconDecoder::Close() > > NS_IMETHODIMP nsIconDecoder::Flush() > { > return NS_ERROR_NOT_IMPLEMENTED; > } > > NS_IMETHODIMP nsIconDecoder::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval) > { >+ PRUint8 *buf = (PRUint8 *)PR_Malloc(count); > if (!buf) return NS_ERROR_OUT_OF_MEMORY; /* we couldn't allocate the object */ > > // read the data from the input stram... > PRUint32 readLen; >+ nsresult rv = inStr->Read(buf, count, &readLen); Can't convert (PRUInt8*)buf to (char*) without a cast. >+ if (NS_FAILED(rv)) goto exit; "Oops, what am I destroying?" Can't goto crossing initialization of nsIntRect r. [...] > rv = mFrame->Init(0, 0, w, h, gfxIFormats::RGB_A1, 24); >+ if (NS_FAILED(rv)) goto exit; Ditto. Just put >+ nsIntRect r(0, 0, width, height); >+ mObserver->OnDataAvailable(nsnull, mFrame, &r); into its own {scope}. I see that we're sometimes checking whether mObserver is null and sometimes not in this method (and others). Is there some reason for this that I'm missing?

Fixed all references to mObserver to first check it. Put nsIntRect into its own scope (from the 'if (mObserver)'). Made buf a 'char *' and data a 'PRUint8 *' for better casting.

Comment on attachment 205707 [details] [diff] [review] V3: The whole patch (except for the 'icondecoder' part, unchanged from V2) You included a diff for nsIconURI.cpp in this patch, but that looks like it was intended for bug 319977. r=me, not covering nsIconURI.cpp. NOTE FOR CHECKIN: DO NOT CHECK IN nsIconURI.cpp.

Updated to current tree (after bug 326714 did some changes to the nsIconDecoder) This patch optimizes calls to SetImageData and SetAlphaData to set whole image data in one call instead of per row. In the GIF decoder, the pair BlackenFrame and SetMaskVisibility is replaced by one ClearFrame function. Furthermore, it fixes the potential leak in the nsIconDecoder.

Comment on attachment 211760 [details] [diff] [review] V4: updated to current tree (see bug 326714) >Index: icon/nsIconDecoder.cpp >+ nesult rv = inStr->Read((char*)buf, count, &readLen); This patch clearly hasn't been tested. >+ if (NS_FAILED(rv)) goto exit; Why don't you use nsAutoArrayPtr (and new[]) instead of gotos?

Oops, now fixed and using nsAutoArrayPtr, which is indeed much cleaner!

Comment on attachment 211829 [details] [diff] [review] v5: Using nsAutoArrayPtr to prevent the leak Just because I commented doesn't mean I'm an appropriate reviewer for the entire patch.

sigh, another patch i should have reviewed a long time ago. i'm not sure that we still need most of this patch now that we in thebes land copy directly in to the buffer and avoid SetImageData calls all together. the GIF stuff might be useful though. anyone have time to look at this?

Indeed, this is now totally bitrotted. But no problem, parts of it are still interesting: IconDecoder: in case of Cairo: change to directly write the image. BMPDecoder: Can still get rid for the silly for loop, and even using some direct writing to the image, instead of via the mDecoded buffer... And indeed the GIFDecoder can also be re-used. So, let me take this one up again. Note in terms of priorities for Gecko 1.9/2.0 this is all aimed at P1: memory footprint reduce, and P2: startup performance, and generally about code cleanup. Note, will non-cairo code need to be kept?

For the ICO/BMP decoder check bug 360000.

we can probably start removing non-cairo code now.

Made redundant by all the Cairo changes to the image decoders, marking WONTFIX.