Bug 108271 (bmprle)

Support RLE compression and bitfields for the BMP Decoder

RESOLVED FIXED in mozilla1.0.1

Status

()

Core
ImageLib
P1
enhancement
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: Biesinger, Assigned: neil@parkwaycc.co.uk)

Tracking

({topembed+})

Trunk
mozilla1.0.1
topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: edt_x3)

Attachments

(8 attachments, 16 obsolete attachments)

17.05 KB, image/bmp
Details
5.55 KB, image/bmp
Details
51.82 KB, text/plain
Details
6.62 KB, image/bmp
Details
922 bytes, image/bmp
Details
21.77 KB, image/bmp
Details
878 bytes, image/bmp
Details
27.84 KB, patch
Biesinger
: review+
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.

Created attachment 58148 [details] [diff] [review]
Patch for Bitfields

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 4

17 years ago
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 5

17 years ago
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?
Keywords: patch, review
(Assignee)

Comment 10

17 years ago
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.
(Assignee)

Comment 16

17 years ago
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

Updated

17 years ago
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
Created attachment 63657 [details] [diff] [review]
Patch to support all bitfields

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

Comment 22

16 years ago
Created attachment 89121 [details]
Test case (non-RLE)

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

Comment 23

16 years ago
Created attachment 89122 [details]
Test case (RLE)

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

Updated

16 years ago
Keywords: topembed+

Comment 24

16 years ago
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 ;)
Created attachment 105678 [details] [diff] [review]
bitfield patch with paper's comments addressed

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 26

16 years ago
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 28

16 years ago
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
(Assignee)

Comment 31

16 years ago
Created attachment 105960 [details]
RLE decoder

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

Comment 32

16 years ago
Created attachment 105961 [details]
C:\WIN30\CGALOGO.RLE

Test case (supported by above RLE decoder).

Comment 33

16 years ago
Created attachment 105983 [details] [diff] [review]
patch for RLE8 support

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.

Comment 36

16 years ago
Created attachment 106114 [details] [diff] [review]
updated RLE8 patch 

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)
(Assignee)

Comment 38

16 years ago
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"?

Comment 39

16 years ago
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.
(Assignee)

Comment 40

16 years ago
Both WinCvs and cygwin diff will automatically convert line endings for me...
(Assignee)

Comment 41

16 years ago
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;
    }
}

Comment 42

16 years ago
Created attachment 106382 [details] [diff] [review]
Updated RLE8/RLE4 patch

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!
Created attachment 106420 [details]
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 :) )

Comment 45

16 years ago
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.

Comment 46

16 years ago
Created attachment 106531 [details] [diff] [review]
updated rle8/4 path

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.
(Assignee)

Comment 48

16 years ago
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...
(Assignee)

Comment 49

16 years ago
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?
(Assignee)

Comment 50

16 years ago
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.
(Assignee)

Comment 51

16 years ago
Created attachment 106660 [details] [diff] [review]
Patch that only works with my rle :-(

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

Comment 52

16 years ago
Created attachment 106708 [details] [diff] [review]
Updated patch that seems to work with all tests

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

Updated

16 years ago
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.

Comment 54

16 years ago
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.
(Assignee)

Comment 55

16 years ago
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 56

16 years ago
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.
(Assignee)

Comment 57

16 years ago
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).
(Assignee)

Comment 58

16 years ago
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.
(Assignee)

Comment 59

16 years ago
Created attachment 106804 [details]
crasher testcase

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

Comment 60

16 years ago
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?
(Assignee)

Comment 61

16 years ago
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.
(Assignee)

Comment 62

16 years ago
Comment on attachment 106708 [details] [diff] [review]
Updated patch that seems to work with all tests

>+                        if (mCurLine == 0)
>+                            mState = eRLEStateFinished;
else
    mState = eRLEStateInitial
?
(Assignee)

Comment 63

16 years ago
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

Comment 64

16 years ago
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
(Assignee)

Comment 65

16 years ago
Created attachment 107008 [details] [diff] [review]
Patch that works with all my testcases

Sorry to clash with you over this.

Comment 66

16 years ago
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
(Assignee)

Comment 67

16 years ago
Created attachment 107137 [details] [diff] [review]
Polished patch
Attachment #106708 - Attachment is obsolete: true
Attachment #107008 - Attachment is obsolete: true
(Assignee)

Comment 68

16 years ago
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
:-)
(Assignee)

Comment 69

16 years ago
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.
(Assignee)

Comment 70

16 years ago
Created attachment 107146 [details] [diff] [review]
Handles RLE transparency
(Assignee)

Comment 71

16 years ago
Created attachment 107154 [details]
Transparent RLE test case

Sorry about the choice of image :-P
(Assignee)

Comment 72

16 years ago
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.
(Assignee)

Comment 74

16 years ago
>>+	     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?
(Assignee)

Comment 76

16 years ago
Created attachment 107364 [details] [diff] [review]
You asked for it :-)

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.
(Assignee)

Comment 78

16 years ago
Created attachment 107580 [details] [diff] [review]
Final patch, I hope!
Attachment #107137 - Attachment is obsolete: true
Attachment #107146 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
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.
(Assignee)

Comment 80

16 years ago
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.
(Assignee)

Updated

16 years ago
Attachment #107580 - Flags: superreview?(tor)

Comment 83

16 years ago
biesi: yes, the image data needs to be zeroed in transparent areas due to the
way masked images are drawn with win32.
(Assignee)

Comment 84

16 years ago
Created attachment 108009 [details] [diff] [review]
Cleared out decoded pixels for win32 transparency
Attachment #107580 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
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+
(Assignee)

Comment 87

16 years ago
Created attachment 108190 [details] [diff] [review]
Switched mDecoded and mAlpha from new/delete to malloc/calloc/free
Attachment #108009 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #108190 - Flags: superreview?(tor)
Attachment #108190 - Flags: review?(cbiesinger)
Attachment #108190 - Flags: review?(cbiesinger) → review+

Comment 88

16 years ago
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 89

16 years ago
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/
(Assignee)

Comment 90

16 years ago
Created attachment 108356 [details] [diff] [review]
Fixed tor's issues

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
(Assignee)

Updated

16 years ago
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+
(Assignee)

Comment 92

16 years ago
> 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 94

16 years ago
Comment on attachment 108356 [details] [diff] [review]
Fixed tor's issues

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

Updated

16 years ago
Keywords: patch
(Assignee)

Updated

16 years ago
Attachment #107580 - Flags: superreview?(tor)
(Assignee)

Updated

16 years ago
Attachment #108009 - Flags: superreview?(tor)
(Assignee)

Updated

16 years ago
Attachment #108190 - Flags: superreview?(tor)

Comment 95

16 years ago
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]
(Assignee)

Comment 98

16 years ago
Hmmm. So why does commenting out line 471 of nsICODecoder.cpp stop the crash?
(Assignee)

Comment 99

16 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Whiteboard: edt_x3
You need to log in before you can comment on or make changes to this bug.