Adding support for BMPs was covered by bug 18502. What will get checked in from that bug will not include compression, though. It should be added.

From bug 18502: ------- Additional Comments From neil@parkwaycc.co.uk 2001-09-07 07:56 ------- When you get around to handling them, compressed bitmaps usually come in two flavours, BI_RLE8 for 8-bit bitmaps and BI_RLE4 for 4-bit bitmaps. However there also exists an undocumented bitmap format which is BI_RLE4 encoded but only uses a two colour table. To indicate this the bit count is 1 instead of 4. I would appreciate it if you could support this format as the extra work seems slight.

This patch adds bitfields support. Only 5-5-5 and 5-6-5 16-bit BMPs and 8-8-8 32-bit BMPs are supported; but Windows 9x only supports these anyway; so this is not much of a problem.

pavlov, could you review this patch? Note that this is fixes only bitfields. RLE compression will be done later with another patch. This patch also cleans up the decoder a bit.

Comment on attachment 58148 [details] [diff] [review] Patch for Bitfields please put all the shifts at the end of the struct so that they can be packed together (you'll save 8 bytes): i.e.: struct bitFields { PRUint32 red; PRUint32 green; PRUint32 blue; + PRInt8 redshift; + PRInt8 greenshift; + PRInt8 blueshift;

Comment on attachment 58148 [details] [diff] [review] Patch for Bitfields Still not a fan of the magic numbers, but we've already crossed that bridge. sr=tor

Chris: I'm implementing the RLE portion (as if you didn't already know)

I guess this is also where i should talk about the other stuff i'm doing for bitmap files. Not all kinds of bitmap files worked correctly, so I'm taking Jason Summer's test suite and adding RLE compression support for it and also going to do support for top to bottom bitmaps (negative height) and testcases. So right now I am doing a large changing of the bitmap code to make it more readable and work with more bitmaps, making the code simpler to read and removing a redundancy. Therefore.. I would probably hold off on doing anything with it for a little while. I doubt it will take too long. Chris: don't worry about the bitfields patches... we can add those manually. After we are done getting every case of bitmaps to work (which with the bitfields and RLE and top to bottom code should be done) then someone needs to add parameter checking so invalid bitmaps don't crash mozilla. As that is done I can make some invalid bitmap testcases. BTW Jason.. thanks for that wonderful testsuite. It was a good starting point for making testcases.

Comment on attachment 58148 [details] [diff] [review] Patch for Bitfields Has been checked in.

netdemon: > taking > Jason Summer's test suite and adding RLE compression support for it Jason's suite does contain RLE Compressed files >So right now I am doing a large changing of the bitmap code to make it more >readable Does that include fix for bug 110835? >Chris: don't worry about the bitfields patches... we can add those manually. They're already checked in >someone needs to >add parameter checking so invalid bitmaps don't crash mozilla. They shouldn't; do they?

Note about RLE compression: The palette size, calculated using the biClrUsed or biBitCount fields in the usual way, does not enforce the compression format. For example, biBitCount may be 1 when the biCompression is BI_RLE4.

neil: I'll have to check into that. If its "legal" then it shouldn't be a problem. I don't think we should be displaying invalid bitmaps if its not the case. christian: >> taking >> Jason Summer's test suite and adding RLE compression support for it >Jason's suite does contain RLE Compressed files Sorry... I meant abiltity to generate RLE Compressed files. He made them with paintshoppro or something. Therefore it won't be able to make a wide range of testcases. Sor far I have it able to make 2 but it breaks down on the third because of the short runs contained therein. >>So right now I am doing a large changing of the bitmap code to make it more >>readable >Does that include fix for bug 110835? Yes. Already fixed. >>Chris: don't worry about the bitfields patches... we can add those manually. >They're already checked in OK. Cool - I assume that was timeless's checkin. Does bitfields now work correctly on all cases possible (that you know of)? Also: is there full support for 32 bit bitmaps? (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/bitmaps_1rw2.asp) I haven't yet looked at all the code since i spent last night changing the member variables and a few other things. Maybe you could explain to me on irc what you guys mean by magic numbers. >>someone needs to >>add parameter checking so invalid bitmaps don't crash mozilla. >They shouldn't; do they? I don't know about crash... but I assume they would do something funky. For instance if the file is 500 bytes long and the width is set to 100000 or something. It also shouldn't be too trusting of certain parts of the file. For instance, the image size stored in the bitmapfileheader. Also... if it reaches the last row and column it should stop reading in the file (It might already do this.. i'm just throwing things out). Ping Pavlov: nsICODecoder.cpp nsICODecoder.h in the bmp directory - i don't think they are used anymore. Should they be removed?

>>So right now I am doing a large changing of the bitmap code to make it more >>readable >Does that include fix for bug 110835? Yes. Already fixed. >Does bitfields now work >correctly on all cases possible (that you know of)? It works for Jason's test files; that is, 16-bit 5-5-5 and 5-6-5 images and 32-bit 8-8-8 images. I haven't tested it on the mac. >Also: is there full support for 32 bit bitmaps? Sure, but see above. >I don't know about crash... but I assume they would do something funky. For >instance if the file is 500 bytes long and the width is set to 100000 or >something. I'm not sure, but if imglib doesn't crash, the BMP Decoder shouldn't, either. >It also shouldn't be too trusting of certain parts of the file. For >instance, the image size stored in the bitmapfileheader. Is ignored. > Also... if it reaches >the last row and column it should stop reading in the file It does. >Ping Pavlov: >nsICODecoder.cpp >nsICODecoder.h >in the bmp directory - i don't think they are used anymore. Should they be >removed? Huh? They are very much neded, they decode ICO files.

Christian: Sorry. I stopped building the icon decoders files so I wouldn't have to change the vars in yet and I thought they weren't used because it was displaying the icons still. I forgot to remove the .dll though - therefore I made the false assumption that /icon directory was now used.

I am still working on this. I talked to Neil on IRC and got some information about bitfields. I feel that can remove the magic numbers and make it support many different bitfield combinations without much more complicated code and thus would make it correct and our job on it would be done. I just don't feel right closing this bug with it done incorrectly. I am going to do the RLE code first. I especially feel bad about leaving it like this due to the fact that pretty soon XP and win2k are going to be the defacto windows OS. Chris: once I get it to a certain point in the rearrangement you could work on a less specific version of the bitfields code at the same time as I work on the RLE portion without it interfering - if you want. Otherwise we could leave the magic numbers in and file a new bug for making bitfields work for more general cases.

brian: Yes, that would be great. Let me know if you have a first patch.

One thing I have noticed is that when loading a large bitmap the unloaded portion of the bitmap is black, whereas for a GIF or JPEG image it is transparent.

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

reassigning to netdemon, because he is working on it.

Status: I have rewritten and or rearranged the code for the current bitmap loader to be simpler. I am now starting RLE support. BTW - RLE Bitmaps are sometimes saved as .rle

netdemon, if you're interested: This patch against CVS makes Mozilla support all bitfields for 16-bit BMP images.

Rollback

This is a non-RLE image, generated with GIMP for Windows.

This bitmap is compressed with 8-bit RLE and should look identical to attachment 89121 [details]. Generated with GIMP for Windows.

Comment on attachment 63657 [details] [diff] [review] Patch to support all bitfields Code looks okay.. just a couple of nitpicks: - put the { on a seperate line for functions - replace underscores with interCaps style. ie. change to calcBitmask, greenLeftShift, greenRightShift, etc. Do that and I'll r= unless I can't get it to compile or something ;)

ok, comments addressed, compiles & displays images from jason summer's bmp suite still correctly (bug 18502 has an url of it). (well, the rle one is of course still not displayed) paper, can I get an r=?

Comment on attachment 105678 [details] [diff] [review] bitfield patch with paper's comments addressed looksPrettyNow :) r=paper

oh and for clarification: The latest patch here does _not_ implement RLE support and does therefore not completely fix this bug. As a matter of fact I think hardly any BMP Image uses it, and I have no testcase for it.

Comment on attachment 105678 [details] [diff] [review] bitfield patch with paper's comments addressed sr=tor

Comment on attachment 105678 [details] [diff] [review] bitfield patch with paper's comments addressed checked in

taking bug, I intend to implement RLE sometime soon

I wrote this six years ago, so don't ask me to explain it :-)

Test case (supported by above RLE decoder).

Chris, I am not trying to step on your toes... but I was asked to look into RLE8 support last week... I worked this up to get RLE8 support. I am not sure how optimal it is. Unlike uncompressed scenarios my patch waits till the whole image is read in before decompressing. I did this to remove the state-management issues involved with a "run" spanning 2 or more data segments. I also add 2 members to the bmp class, however with some creativity we could re-use mRow and mRowBytes (but I would probably want to rename these).

>I worked this up to get RLE8 support. I am not sure how >optimal it is. Hm, I don't really like it so much... see below. >Unlike uncompressed scenarios my patch waits >till the whole image is read in before decompressing. Last time I tried to implement RLE (that was like a year ago, when I initially created the BMP Decoder), I stopped because I did not want to wait till the whole image is loaded nor did I think of a good way to do it incrementally... Also, I would really like it if RLE4 was implemented at the same time, but that's not so critical... some comments on the actual patch: + if (!mImageData) { + mImageData = new PRUint8[mBIH.image_size]; + } + if (mImageData) { you currently seem to return no error when this allocation fails. I would change the if to if (!mImageData) and return an NS_ERROR_FAILURE or something. + PRUint8* pDecompData = new PRUint8[mBIH.height * mBIH.width]; you could re-use mRow I suppose and continue doing it row-by-row... in addition, you don't null-check this pointer. + byte1 = *pSrc++; + byte2 = *pSrc++; + mByteCount -= 2; hm... what if the image is malformed, and has one byte less than expected? isn't this then a crash waiting to happen? + else if (byte2 == RLE_ESCAPE_DELTA) { should you maybe fill the intermediate data with zeroes? the documentation I found isn't really clear on this... currently, you're just leaving it as uninitialized memory... noticed this just now... in C++ there's no need to declare variables at the beginning of the block... can you declare them at the place where you need them? hmmm, I think you never send OnDataAvailable... you should...

+ if (mByteCount == mBIH.image_size) { I personally would change that to >=... if there's some additional data after the end of the image for some reason, I think it should be ignored.

Ok, I redid the patch so that it builds the image on the fly. I still have some more testing to do but I wanted to get your thoughts...

Comment on attachment 106114 [details] [diff] [review] updated RLE8 patch ah that looks so much better now. thanks. as for the code: could you make mState an enum? in the case statement for RLE_STATE_NEED_SECOND_ESCAPE_BYTE: instead of the if..elseif in there, you could use a switch +if (!decoded) return NS_ERROR_OUT_OF_MEMORY; please put the return on a new line, to allow setting a breakpoint there. p = &mRow[mRowBytes]; personally I prefer: p = mRow + mRowBytes; but I guess that's just a matter of taste. RLE_STATE_NEED_Y_DELTA this code duplicates the code from _EOL... I can't say I like that... if you would find a way to share the code, maybe by a new member function, that'd be good. // If we are moving down shouldn't this say "up" instead of "down"? also, there's no need to do the allocation of |decoded| in the while loop. if you do it outside, you don't need to do it for every iteration. if (mCurLine == mBIH.height) err... isn't mCurLine counting towards zero? RLE_STATE_FINISHED hm, another duplication of the code of _EOL... below this comment: // If we aren't at the end of our image then again, please do the allocation of decoded outside the loop. Now, let me say that I really like this patch in general. It is a great piece of work. Thanks for spending the time to make it work incrementally instead of waiting for the whole image to load. If you just address the issues I mentioned above, I'll put an r=biesi on the patch. ah yeah one more thing... what happens when you try to load BI_RLE4 images with the patch? I know they are not supposed to work, but does it make Mozilla hang? I have a suspicion that it does, please test this (I can't right now)

Comment on attachment 106114 [details] [diff] [review] updated RLE8 patch >- mCurLine = 0; >+ mCurLine = 0; >- mBIH.width, mBIH.height, mBIH.bpp, mBIH.compression)); >+ mBIH.width, mBIH.height, mBIH.bpp, mBIH.compression)); >- PRUint8* decoded = new PRUint8[bpr]; >- if (!decoded) >- return NS_ERROR_OUT_OF_MEMORY; >- >+ PRUint8* decoded = new PRUint8[bpr]; >+ if (!decoded) >+ return NS_ERROR_OUT_OF_MEMORY; >+ >- >+ What's the deal with all these "changes"?

I believe those are <cntr>M's differences. I noticed that when I edited this file with MSVC6, it was putting in ^M's at the end of the lines, so I went in with VI and removed them. However, I must have touched these lines, added a ^M and then didn't remove them. Still trying to figure out where in MSVC6 I turn off ending lines with ^M. Sorry! The version I check in (eventually) will be correct (promise). I have to fix several things in this patch and add RLE4 support, I posted this to get feedback on the approach.

Both WinCvs and cygwin diff will automatically convert line endings for me...

Comment on attachment 106114 [details] [diff] [review] updated RLE8 patch Can't wait to see your RLE4 decoder, but in the meantime... >+p = &mRow[mRowBytes]; >+for (int cnt = 0; cnt < mStateData; cnt++) { >+ *p++ = byte; >+} memset(&mRow[mRowBytes], byte, mStateData)? >+p = &mRow[mRowBytes]; >+while ((aCount > 0) && (mStateData > 0)) { >+ *p++ = *aBuffer++; >+ aCount--; >+ mRowBytes++; >+ mStateData--; >+} memcpy? >// If we are moving down then we need to dump >// out 'blank' lines Are these really blank, i.e. transparent? My reading of the RLE spec suggests that they should be. >+if (mStateData == 0) { >+ // In absolute mode, each run must >+ // be aligned on a word boundary >+ >+ if (((unsigned long)aBuffer & 1) && (aCount > 0)) { >+ aBuffer++; >+ aCount--; >+ } >+ mState = RLE_STATE_INITIAL; >+} >+// else state is still RLE_STATE_ABSOLUTE_MODE I think the logic is wrong here; you should stay in absolute mode unless both there is no more data to copy and the input buffer is word aligned, something like this (copying your word alignment algorithm, I can't tell whether or not that is correct, also fixed indentation to 4 spaces): if (mStateData == 0) { if ((((size_t)aBuffer) & 1) == 0) { mState = RLE_STATE_INITIAL; } else if (aCount > 0) { aBuffer++; aCount--; mState = RLE_STATE_INITIAL; } }

Ok, I looked through all the comments and THINK that I have addressed them ALL with the exception of whether we support "transparent" bmp's. Should we address that in a different bug so that we can move forward with this patch? I have made a couple of assumptions with this code. 1) That in RLE4 mode, the absolute,encoded & x delta all are based on an "even" number of indexes. To do otherwise is gonna put in a WORLD of hurt into the code 2) Delta XY write color(0,0,0) for the pixels it skips over and NOT color(mColor[0].red,mColor[0].green,mColor[0].blue) Also it WRITES a pixel over each one it skips... comments/suggestions/improvements (and if I missed addressing someone's comments, I really apologize, I thought I hit them all!)

Comment on attachment 106382 [details] [diff] [review] Updated RLE8/RLE4 patch + if (mDecoded) { + delete[] mDecoded; delete[] is null-safe. as in, calling delete with a null-pointer is guaranteed to work. + if (((mBIH.compression == BI_RLE8) && (mBIH.bpp != 8)) + || ((mBIH.compression == BI_RLE4) && (mBIH.bpp != 4))) { please refer to comment 1 in this bug + PRUint32 nMaxNumOfBytesPerRow; so is there any reason to use the n prefix on this variable? Seems to be the only place where you do it (fwiw, I dislike prefixes for the type, but you should at least be consistent) if (mBIH.compression == BI_RLE4) { please remove the { } from a one-line if. other places in your patch as well. + PRUint8 cnt; + cnt = (aCount < mStateData) ? aCount : mStateData; you could merge these two lines, you know also, you could use the PR_MIN macro. (hm, thinking about it, I should've used it in other parts of this file...) well, I leave it up to you if you o) just merge these lines or o) also use PR_MIN or o) fix other places to use PR_MIN as well + // If we aren't at the end of our image then + PR_LOG(gBMPLog, PR_LOG_DEBUG, ("BMP RLE8 compression unknown STATE\n")); well, this really should never get reached, I'd turn this into: NS_NOTREACHED("BMP RLE compression unknown state"); +inline nsresult nsBMPDecoder::RLE_WriteRow(PRUint32 numOfBytes) { I don't really think this should be inlined, looking at how long this function is. +inline nsresult nsBMPDecoder::RLE_WriteBlankRows(PRUint32 numOfRows) { you have a pretty long loop in there, so I don't think the inlining really buys much here either. this really looks good otherwise!

with your patch, this image here does not show quite correctly... the first line contains pixel dirt... I also have a similar 8-bit RLE image with the same issue. can attach it on request. (well, without your patch it doesn't show at all, of course :) )

I am not sure what you are seeing, but my build looks "ok" with this image (both debug & opt build). I am using win2k, I have tried one of my previous patches using linux but not the latest.

addressed all of the comments. NOTE: I don't have a rle4 1-bit image to test but I allowed rle4 1-bit images to be decoded.

Comment on attachment 106531 [details] [diff] [review] updated rle8/4 path two issues with this patch: 1) The cgalogo.rle attachment here crashes 2) the issue about the pixel dirt first line still happens. yes, this is on linux. haven't tried windows, and can't do that easily.

Comment on attachment 106531 [details] [diff] [review] updated rle8/4 path This just from looking at the code (will try compiling it later): >- mBIH.width, mBIH.height, mBIH.bpp, mBIH.compression)); >+ mBIH.width, mBIH.height, mBIH.bpp, mBIH.compression)); Missing a ^M? >+ if (((mBIH.compression == BI_RLE8) && (mBIH.bpp != 8)) >+ || ((mBIH.compression == BI_RLE4) && (mBIH.bpp != 4) && (mBIH.bpp != 1))) { Could be written if (mBIH.compression * bBIH.bpp > 8) { >+ sizeofmRow = (mBIH.width * mBIH.bpp)/8 + 4; Wrong, should be (mBIH.width / mBIH.compression) [+ whatever] Unfortunately you need to correct the allocation :-( >+ if (mBIH.compression == BI_RLE4) >+ mStateData = byte >> 1; >+ else >+ mStateData = byte; There are a number of cases where you effectively divide by mBIH.compression; as I see it you have 3 methods: 1. the if that you currently use 2. byte / mBIH.compression 3. byte >> (mBIH.compression - 1) which you would probably store in a local temporary. >+ if (byte > 0) { >+ if (byte) { Nit: inconsistent >+ rv = RLE_WriteBlankRows(byte); PR_MIN(byte, mCurLine) just in case...

Comment on attachment 106531 [details] [diff] [review] updated rle8/4 path >+ if (mBIH.compression == BI_RLE4) { >+ for (x = 0; x < numOfBytes; x++) { >+ SetPixel(d, ((*p & 0xf0) >> 4), mColors); >+ SetPixel(d, (*p & 0x0f), mColors); >+ p++; >+ } Should you be using Set4BitPixel here?

Comment on attachment 106531 [details] [diff] [review] updated rle8/4 path >+ if (mCurLine == 0) >+ return mObserver->OnStopFrame(nsnull, nsnull, mFrame); I think this is too late: mCurLine gets set to (height - 1) earlier.

This patch correctly displays attachment 105961 [details] but no other rle test case :-(

Ok, I updated once again. -I used neil's use of mRowSize -I fixed the incorrect setting of mState in EOL -I cleaned up the "if" nits (i.e. if (byte) .vs. if (byte > 0) -I now divide by mBIH.compression. I am assuming that "blah/mBIH.compression" is faster than "if (blah==RLE4) a >> 1 else a;" -I did the PR_MIN(byte,mCurLine) suggestion -I did NOT switch to using Set4BitPixel since my use of the 2 SetPixels should be faster than Set4BitPixel since I don't need to check for "x > width". But if you want me to use it, I can put it in. -I did NOT change to "if (mBIH.compress * mBIH.bpp >= 8) { since this would not allow "comment #1" and the support of 1-bit RLE4 data. (NOTE: I still haven't checked to see if we can process a 1-bit RLE4 BMP due to lack of testcase)

jdunn: Ah, I think I found out why this is not quite working (the latest patch does not show pixel dirt btw, just a black line which is also wrong). You don't call RLE_WriteRow or what that's called when the EOF marker is reached. However, as my image does not contain an EOL before the EOF, the final row is never set.

hmmm, when we get EOF, we set state to Finish. In Finish, we check to see if there is any data mRowBytes and if so WriteRow. So this shouldn't be what you are seeing. The one thing I fixed in yesterday's patch is that if in EOL, if mCurLine is 0, set state to Finish ELSE set state to Initial. Previously I was setting to Finish but then ALWAYS setting to Initial which is an "undetermined" process flow. I wonder if this fixes the problem you are seeing.

Comment on attachment 106660 [details] [diff] [review] Patch that only works with my rle :-( >+ if (mBIH.compression * mBIH.bpp >= 8) { D'oh, should have been > 8 :-/

Comment on attachment 106660 [details] [diff] [review] Patch that only works with my rle :-( Do you really want to do this? + if (mBIH.compression * mBIH.bpp > 8) It is "cryptic" and what about RLE8 with bpp==1? It is NOT supported and yet will be allowed by the code.

Comment on attachment 106660 [details] [diff] [review] Patch that only works with my rle :-( What I meant was, that I had accidentally excluded perfectly valid bitmaps. All my check does is ensure that the colour table isn't too big (too small is ok).

Comment on attachment 106708 [details] [diff] [review] Updated patch that seems to work with all tests > mCurLine = (mBIH.height - 1); I'm still worried that this will generate an off-by-1 error somewhere. I'm still seeing black lines at the top of RLEs. >- mRow = new PRUint8[(mBIH.width * mBIH.bpp)/8 + 4]; >+ if ((mBIH.compression == BI_RLE8) || (mBIH.compression == BI_RLE4)) >+ mRowSize = (mBIH.width / mBIH.compression) + 4; >+ else >+ mRowSize = (mBIH.width * mBIH.bpp)/8 + 4; >+ mRow = new PRUint8[mRowSize]; Actually I think I've found an improvement on the code: if ((mBIH.compression == BI_RLE8) || (mBIH.compression == BI_RLE4)) mRow = new PRUint8[(mBIH.width / mBIH.compression) + 4]; else mRow = new PRUint8[(mBIH.width * mBIH.bpp)/8 + 4]; Then, change memset(mRow, 0, mRowSize) to memset(mRow, 0, mRowBytes) Thus there is no need for an mRowSize variable.

I don't know why this RLE crashes (on Linux) - it opens fine in PSP.

Comment on attachment 106708 [details] [diff] [review] Updated patch that seems to work with all tests >rv = RLE_WriteRow(mRowBytes); Nit: Why pass a member variable to a member function?

Comment on attachment 106708 [details] [diff] [review] Updated patch that seems to work with all tests > PRInt32 mCurLine; >+ PRUint8 *mDecoded; // Holds one line of color image data >+ >+ ERLEState mState; // Maintains the current state of the RLE decoding >+ PRUint8 mStateData; // Decoding information that >+ // is needed depending on mState > > void ProcessFileHeader(); > void ProcessInfoHeader(); >+ nsresult RLE_WriteRow(PRUint32 numOfBytes); >+ nsresult RLE_WriteBlankRows(PRUint32 numOfRows); Assuming you're not sick of me yet I think this parameter should be a PRInt32 because mCurLine is.

Comment on attachment 106708 [details] [diff] [review] Updated patch that seems to work with all tests >+ if (mCurLine == 0) >+ mState = eRLEStateFinished; else mState = eRLEStateInitial ?

Comment on attachment 106804 [details] crasher testcase I see what the problem is - there are odd pixel lengths :-)

yeah, sorry I meant to say this. Yes there are odd-lengths and that is messing my code up. I am looking at it. As for the other problem (mCurLine ==0) checks, I found an issue in that if we read an EOF and set state to Finished, we don't actually go into it, because aCount==0. I am looking at both

Sorry to clash with you over this.

Comment on attachment 107008 [details] [diff] [review] Patch that works with all my testcases This looks great! The only change I would even bring up is in both Absolute & Encoding mode, doing the if (compression) test outside of the loop so that the "test" isn't needed every iteration. i.e. if (mBIH.compression == BI_RLE8) { for (cnt = 0; cnt < mStateData; cnt++) } else { for (cnt = 0; cnt < mStateData; cnt++) ... } However, this looks real good. My r= isn't worth much, but I will give it r=jdunn@netscape.com

Comment on attachment 107137 [details] [diff] [review] Polished patch Bah, I missed out a memset(mDecoded, 0, bpr); - see if you can figure out where :-)

Comment on attachment 107137 [details] [diff] [review] Polished patch Jim has just pointed out that a spurious NS_ENSURE_SUCCESS(rv, rv) crept into the RLE_ESCAPE_EOF case.

Sorry about the choice of image :-P

Comment on attachment 107146 [details] [diff] [review] Handles RLE transparency While creating the testcase I discovered that I need the following in two obvious places: if (mAlphaPtr + mStateData - mAlpha > mAbpr) return NS_ERROR_FAILURE;

Comment on attachment 107146 [details] [diff] [review] Handles RLE transparency + PRUint8 byte; can you move this down to where it is first used? +while (mStateData > 0) { the contents of this loop are basically identical to Set4BitPixel. please use the function instead (to do that, you need to count up, instead of down, I suppose, using mStateData as aMaxWidth and a new helper variable for aPos). +case RLE_ESCAPE_EOL : current style in the file is to not put a space before the colon, please fix that. +#if defined(XP_MAC) || defined(XP_MACOSX) + mDecoding += byte * 4; I wonder if it's worth making a #define/const for that... given that it's used already in one place. + if (byte == 0) + continue; // Nothing more to do given that this is a switch here, a break; would be clearer, imho. + while (aCount > 0 && mStateData > 0) { as above, please use Set4BitPixel in this loop. + } else if (aCount > 0) { // Not word Aligned don't you need to set mState as well here? +NS_NOTREACHED("BMP RLE compression unknown state"); nit: It would look nicer if there was a : after "compression", imho :) + // Because of the use of the continue statement + // we only get here for eol, eof or y delta + if (mCurLine < 0) { um... why < 0, instead of <= (or even ==) 0? +nsresult nsBMPDecoder::WriteRLERows(PRInt32 rows) that function is mostly identical to SetData; have you considered combining them? Also, it does not reset mDecoded for the other (ie. not first) rows that are to be written.

>>+ PRUint8 byte; >can you move this down to where it is first used? Just before the while loop, or just inside it? >>+while (mStateData > 0) { >the contents of this loop are basically identical to Set4BitPixel. please use >the function instead (to do that, you need to count up, instead of down, I >suppose, using mStateData as aMaxWidth and a new helper variable for aPos). Heh. What goes around comes around. I'll take another look. >>+case RLE_ESCAPE_EOL : >current style in the file is to not put a space before the colon, please fix >that. Ok. >>+#if defined(XP_MAC) || defined(XP_MACOSX) >>+ mDecoding += byte * 4; >I wonder if it's worth making a #define/const for that... given that it's used >already in one place. It is? I must have overlooked that, I'll check again. >>+ if (byte == 0) >>+ continue; // Nothing more to do >given that this is a switch here, a break; would be clearer, imho. I'm optimizing past the if (mCurLine < 0) test (as per that comment). >>+ while (aCount > 0 && mStateData > 0) { >as above, please use Set4BitPixel in this loop. Sure. I'll probably have to treat the pointers as arrays in that case. >>+ } else if (aCount > 0) { // Not word Aligned >don't you need to set mState as well here? Good catch! I lost it when I scrapped attachment 106660 [details] [diff] [review]. >>+NS_NOTREACHED("BMP RLE compression unknown state"); >nit: It would look nicer if there was a : after "compression", imho :) Well I'm actually doing decompression :-P >>+ // Because of the use of the continue statement >>+ // we only get here for eol, eof or y delta >>+ if (mCurLine < 0) { >um... why < 0, instead of <= (or even ==) 0? Because mCurLine starts at mHeight - 1 (previous patches showed this). >>+nsresult nsBMPDecoder::WriteRLERows(PRInt32 rows) >that function is mostly identical to SetData; have you considered combining >them? I didn't think it was worth it; SetData only handles one row without alpha. >Also, it does not reset mDecoded for the other (ie. not first) rows that are >to be written. That's because they're transparent by default (which is reset).

tor: cc'ing you because I have a question for you, below. >>I wonder if it's worth making a #define/const for that... given that it's used >>already in one place. >It is? I must have overlooked that, I'll check again. in nsBMPDecoder.h, one of the inline functions >Because mCurLine starts at mHeight - 1 (previous patches showed this). I know, and it has to, because the current line is 0-based. However... the last line has the number 0... also, if you get below zero, won't you set the image data for a negative row number at some time? >>Also, it does not reset mDecoded for the other (ie. not first) rows that are >>to be written. >That's because they're transparent by default (which is reset). iirc, if an image is supposed to be transparent, the color data must be 0 so that it works on windows... tor, is that correct? >I didn't think it was worth it; SetData only handles one row without alpha. well that is because currently there is no need to handle alpha :) just add an if (mAlpha) SetAlphaData(...); maybe?

mCurLine is now 1-based instead of 0-based. This means I decrement it before instead of after using it. Set4BitPixels now counts downwards rather than upwards. I defined and used twice a constant for bytes per pixel. I also applied the above changes to the ICO decoder. And I also addressed the other nits and fixes you spotted.

Comment on attachment 107364 [details] [diff] [review] You asked for it :-) + memset(mAlphaPtr, 0xFF, mStateData); won't this be pretty bad if mStateData is larger than the image width? while I'm at it, why are you using RGB_A8 rather than _A1, which would only need 1/8 of the memory? ah yeah. I would like it if RLE_WriteRows would be moved below the other helper functions like SetData, because it does a similar job, so grouping it with them would make sense, imho. ok... together with what I said on IRC, that's all I think.

Comment on attachment 107580 [details] [diff] [review] Final patch, I hope! hm, you're changing set4bitpixel quite a bit, but ok... the patch looks acceptable now. I'll test it tonight and put an r=biesi on it afterwards.

Comment on attachment 107580 [details] [diff] [review] Final patch, I hope! Bah, I've forgotton to credit myself again :-/

Comment on attachment 107580 [details] [diff] [review] Final patch, I hope! r=biesi

oh yeah. forgot to mention. my review is _only_ valid if you get sr=tor.

biesi: yes, the image data needs to be zeroed in transparent areas due to the way masked images are drawn with win32.

Comment on attachment 108009 [details] [diff] [review] Cleared out decoded pixels for win32 transparency + mAlpha = new PRUint8[mBIH.width]; + if (!mAlpha) + return NS_ERROR_OUT_OF_MEMORY; + memset(mAlpha, 0, mBIH.width); instead of new[] then memset to zero, please use calloc. same in the mDecoded allocation. r=biesi with that change, no need to attach a new patch unless tor wants one.

Comment on attachment 108009 [details] [diff] [review] Cleared out decoded pixels for win32 transparency actually marking my review

Comment on attachment 108190 [details] [diff] [review] Switched mDecoded and mAlpha from new/delete to malloc/calloc/free nsBMPDecoder::WriteRLERows() * alpha packer will walk past end of mAlpha if (alpha%8 != 0) * alpha clear can be "alpha" bytes instead of mBIH.Width nsBMPDecoder::ProcessData() * mCurLine decrement in loop missing? * RLEStateAbsoluteMode - walk off end of line? nsBMPDecoder.h * would like GFXBYTESPERPIXEL moved from SetPixel to near BMP_GFXFORMAT

Comment on attachment 108190 [details] [diff] [review] Switched mDecoded and mAlpha from new/delete to malloc/calloc/free Previous comment: s/alpha%8/mBIH.width%8/

nsBMPDecoder::WriteRLERows() * alpha packer will walk past end of mAlpha if (alpha%8 != 0) Fixed by allocating alpha * 8 bytes * alpha clear can be "alpha" bytes instead of mBIH.Width I need to clear unpacked pixels for the next pack loop nsBMPDecoder::ProcessData() * mCurLine decrement in loop missing? This is done in SetPixels/WriteRLERows * RLEStateAbsoluteMode - walk off end of line? Already checked before setting the state to absolute mode nsBMPDecoder.h * would like GFXBYTESPERPIXEL moved from SetPixel to near BMP_GFXFORMAT Done.

Comment on attachment 108356 [details] [diff] [review] Fixed tor's issues if you're only addressing comments that reviewer or super-reviewer requested, feel free to transfer the review yourself.

> if you're only addressing comments that reviewer or super-reviewer requested, > feel free to transfer the review yourself. Not sure if that works with the new system...

well, it will appear as "neil: review+", but usually people don't care

Comment on attachment 108356 [details] [diff] [review] Fixed tor's issues sr=tor

checked in

I believe this causes the crasher reported in bug 183980.

Or maybe that bug since this was just checked in. In any case I'm seeing a reproducable crash and if I back out this checkin it doesn't crash anymore. Here's now I reproduce: 1. Visit www.nat.org 2. Click on the first link. 3. Click on the link that says "moving to Boston" It crashes at this point. Crash looks something like: __builtin_vec_delete+0x0000001B [/usr/lib/libstdc++-libc6.2-2.so.3 +0x00033C6F] nsICODecoder::Close(void)+0x0000019D [/vol0/src/mozilla_gtk2/gtk2/dist/bin/components/libimglib2.so +0x00039D81] imgRequest::OnStopRequest(nsIRequest *, nsISupports *, unsigned int)+0x0000019A [/vol0/src/mozilla_gtk2/gtk2/dist/bin/components/libimglib2.so +0x000278C6] ProxyListener::OnStopRequest(nsIRequest *, nsISupports *, unsigned int)+0x0000005F [/vol0/src/mozilla_gtk2/gtk2/dist/bin/components/libimglib2.so +0x0002011B] nsStreamListenerTee::OnStopRequest(nsIRequest *, nsISupports *, unsigned int)+0x000000E5 [/vol0/src/mozilla_gtk2/gtk2/dist/bin/components/libnecko.so +0x000CB661]

Hmmm. So why does commenting out line 471 of nsICODecoder.cpp stop the crash?

Fixed.