Closed Bug 108271 (bmprle) Opened 23 years ago Closed 22 years ago

Support RLE compression and bitfields for the BMP Decoder

Categories

(Core :: Graphics: ImageLib, enhancement, P1)

Product:
Core
Component:
Graphics: ImageLib
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: Biesinger, Assigned: neil)

References

Details

(Keywords: topembed+, Whiteboard: edt_x3)

Attachments

(8 files, 16 obsolete files)

Test case (non-RLE)
17.05 KB, image/bmp
Details
Test case (RLE)
5.55 KB, image/bmp
Details
RLE decoder
51.82 KB, text/plain
Details
C:\WIN30\CGALOGO.RLE
6.62 KB, image/bmp
Details
another bmp testcase
922 bytes, image/bmp
Details
crasher testcase
21.77 KB, image/bmp
Details
Transparent RLE test case
878 bytes, image/bmp
Details
Fixed tor's issues
27.84 KB, patch
Biesinger
: review+
tor
: superreview+
Details | Diff | Splinter Review 
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.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.7
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.
Attached patch Patch for Bitfields (obsolete) — — Splinter Review 
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.
Status: NEW → ASSIGNED
Keywords: patch, review
Priority: P2 → P1
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;
Attachment #58148 - Flags: review+
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
Attachment #58148 - Flags: superreview+
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.
Attachment #58148 - Attachment is obsolete: true
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.
Assignee: cbiesinger → netdemonz
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Status: NEW → ASSIGNED
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
Attached patch Patch to support all bitfields (obsolete) — — Splinter Review 
netdemon, if you're interested: This patch against CVS makes Mozilla support
all bitfields for 16-bit BMP images.
Rollback
Target Milestone: mozilla0.9.8 → mozilla1.0.1
Attached image Test case (non-RLE) — 
This is a non-RLE image, generated with GIMP for Windows.
Attached image Test case (RLE) — 
This bitmap is compressed with 8-bit RLE and should look identical to
attachment 89121 [details].  Generated with GIMP for Windows.
Keywords: topembed+
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=?
Attachment #63657 - Attachment is obsolete: true
Comment on attachment 105678 [details] [diff] [review]
bitfield patch with paper's comments addressed

looksPrettyNow :)

r=paper
Attachment #105678 - Flags: review+
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
Attachment #105678 - Flags: superreview+
Comment on attachment 105678 [details] [diff] [review]
bitfield patch with paper's comments addressed

checked in
Attachment #105678 - Attachment is obsolete: true
taking bug, I intend to implement RLE sometime soon
Assignee: netdemonz → cbiesinger
Status: ASSIGNED → NEW
Alias: bmprle
Attached file RLE decoder — 
I wrote this six years ago, so don't ask me to explain it :-)
Attached image C:\WIN30\CGALOGO.RLE — 
Test case (supported by above RLE decoder).
Attached patch patch for RLE8 support (obsolete) — — Splinter Review 
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.
Attached patch updated RLE8 patch (obsolete) — — Splinter Review 
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...
Attachment #105983 - Attachment is obsolete: true
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;
    }
}
Attached patch Updated RLE8/RLE4 patch (obsolete) — — Splinter Review 
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!)
Attachment #106114 - Attachment is obsolete: true
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!
Attached image another bmp testcase — 
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.
Attached patch updated rle8/4 path (obsolete) — — Splinter Review 
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.
Attachment #106382 - Attachment is obsolete: true
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)
Attachment #106531 - Attachment is obsolete: true
Attachment #106660 - Attachment is obsolete: true
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.
Attached image crasher testcase — 
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 :-)
Attachment #106804 - Attachment mime type: image/bitmap → image/bmp
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
Attached patch Polished patch (obsolete) — — Splinter Review 

        Attachment #106708 -
        Attachment is obsolete: true

        Attachment #107008 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Handles RLE transparency (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  
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?


    
    

    
  

      
      
      
      

        Attached patch
          
          
        You asked for it :-) (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Final patch, I hope! (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #107137 -
        Attachment is obsolete: true

        Attachment #107146 -
        Attachment is obsolete: true

    
  

        Attachment #107364 -
        Attachment is obsolete: true

    
    

    
  
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

        Attachment #107580 -
        Flags: review+

    
    

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

    
  

        Attachment #107580 -
        Flags: superreview?(tor)

    
    

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


    
    

    
  


  

        Attachment #107580 -
        Attachment is obsolete: true

    
  

        Attachment #108009 -
        Flags: superreview?(tor)

        Attachment #108009 -
        Flags: review?(cbiesinger)

    
    

    
  
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

        Attachment #108009 -
        Flags: review?(cbiesinger) → review+

    
    

    
  


  

        Attachment #108009 -
        Attachment is obsolete: true

    
  

        Attachment #108190 -
        Flags: superreview?(tor)

        Attachment #108190 -
        Flags: review?(cbiesinger)

        Attachment #108190 -
        Flags: review?(cbiesinger) → 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.

        Attachment #108190 -
        Attachment is obsolete: true

    
  

        Attachment #108356 -
        Flags: superreview?(tor)

        Attachment #108356 -
        Flags: review?(cbiesinger)

    
    

    
  
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.

        Attachment #108356 -
        Flags: review?(cbiesinger) → review+

    
    

    
  
> 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

        Attachment #108356 -
        Flags: superreview?(tor) → superreview+

    
  
Keywords: patch

    
  

        Attachment #107580 -
        Flags: superreview?(tor)

    
  

        Attachment #108009 -
        Flags: superreview?(tor)

    
  

        Attachment #108190 -
        Flags: superreview?(tor)

    
    

    
  
checked in
Assignee: cbiesinger → neil

    
    

    
  
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.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
  
Whiteboard: edt_x3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: