Last Comment Bug 182621 - (wbmp) WAP BMP (wbmp) format support
(wbmp)
: WAP BMP (wbmp) format support
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- enhancement with 10 votes (vote)
: ---
Assigned To: Arunan Balasubramaniam
:
Mentors:
http://www.wapforum.org/what/technica...
Depends on:
Blocks: wml
  Show dependency treegraph
 
Reported: 2002-11-29 03:39 PST by Manko
Modified: 2013-03-19 10:31 PDT (History)
24 users (show)
asa: blocking1.4a-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Sample WBMP image (460 bytes, image/vnd.wap.wbmp)
2002-11-29 03:43 PST, Manko
no flags Details
Make mozilla aware of image/vnd.wap.wbmp. (4.22 KB, patch)
2002-12-03 05:54 PST, Arunan Balasubramaniam
no flags Details | Diff | Splinter Review
WBMP decoder (10.38 KB, text/plain)
2002-12-03 05:56 PST, Arunan Balasubramaniam
no flags Details
WBMP decoder (5.49 KB, text/plain)
2002-12-03 05:57 PST, Arunan Balasubramaniam
no flags Details
modules\libpr0n\decoders\bmp\Makefile (1.27 KB, text/plain)
2002-12-03 09:12 PST, Arunan Balasubramaniam
no flags Details
nsWBMPDecoder.cpp (9.48 KB, text/plain)
2002-12-03 09:14 PST, Arunan Balasubramaniam
no flags Details
nsWBMPDecoder.h (5.26 KB, text/plain)
2002-12-03 09:16 PST, Arunan Balasubramaniam
no flags Details
modules\libpr0n\decoders\wbmp\Makefile.in (2.10 KB, text/plain)
2002-12-04 06:39 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
Make mozilla aware of image/vnd.wap.wbmp (6.68 KB, patch)
2002-12-04 06:44 PST, Arunan Balasubramaniam
cbiesinger: review+
Details | Diff | Splinter Review
Patch to mozilla\allmakefiles.sh (556 bytes, patch)
2002-12-04 14:32 PST, Arunan Balasubramaniam
no flags Details | Diff | Splinter Review
nsWBMPDecoder.cpp (9.49 KB, text/plain)
2002-12-16 11:59 PST, Arunan Balasubramaniam
cbiesinger: review-
Details
nsWBMPDecoder.h (5.36 KB, text/plain)
2002-12-16 12:01 PST, Arunan Balasubramaniam
cbiesinger: review-
Details
Zip of test files. (10.96 KB, application/octet-stream)
2002-12-19 14:02 PST, Arunan Balasubramaniam
no flags Details
Zip of more test files (2.67 KB, application/octet-stream)
2002-12-19 14:20 PST, Arunan Balasubramaniam
no flags Details
nsWBMPDecoder.cpp (11.35 KB, text/plain)
2002-12-19 14:22 PST, Arunan Balasubramaniam
cbiesinger: review-
Details
nsWBMPDecoder.h (5.09 KB, text/plain)
2002-12-19 14:23 PST, Arunan Balasubramaniam
cbiesinger: review-
Details
Screenshot for multi-byte width/height WBMP (55.01 KB, image/png)
2002-12-20 04:59 PST, Manko
no flags Details
modules\libpr0n\decoders\wbmp\Makefile.in (2.09 KB, text/plain)
2003-01-03 14:34 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp (12.27 KB, text/plain)
2003-01-04 10:58 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h (4.48 KB, text/plain)
2003-01-04 10:58 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
Size of DEBUG .obj files (2.33 KB, text/plain)
2003-01-07 14:37 PST, Arunan Balasubramaniam
no flags Details
modules\libpr0n\decoders\wbmp\Makefile.in (2.07 KB, text/plain)
2003-01-11 01:59 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h (4.84 KB, text/plain)
2003-01-16 11:02 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
Build changes (21.43 KB, patch)
2003-01-16 11:05 PST, Arunan Balasubramaniam
pavlov: review+
netscape: superreview+
Details | Diff | Splinter Review
Make mozilla aware of image/vnd.wap.wbmp (5.66 KB, patch)
2003-01-16 11:07 PST, Arunan Balasubramaniam
cbiesinger: review+
Details | Diff | Splinter Review
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp (12.24 KB, text/plain)
2003-01-22 04:29 PST, Arunan Balasubramaniam
cbiesinger: review+
Details
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp Fixes a bug in the multi-byte integer decoder (12.42 KB, text/plain)
2004-04-08 12:18 PDT, Arunan Balasubramaniam
cbiesinger: review+
Details
Build changes for WBMP (69.48 KB, patch)
2004-04-08 12:38 PDT, Arunan Balasubramaniam
benjamin: review+
Details | Diff | Splinter Review
Make mozilla aware of image/vnd.wap.wbmp and WBMP decoder (7.28 KB, patch)
2004-04-08 12:40 PDT, Arunan Balasubramaniam
cbiesinger: review+
Details | Diff | Splinter Review
XPFE changes (21.49 KB, patch)
2004-04-08 12:53 PDT, Arunan Balasubramaniam
benjamin: review+
Details | Diff | Splinter Review
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp after review (12.38 KB, text/plain)
2004-04-09 05:24 PDT, Arunan Balasubramaniam
cbiesinger: review+
Details

Description Manko 2002-11-29 03:39:40 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: 

According bug 35995 which near to be fixed (I hope), we need for support WAP BMP
images. See section 6 of PDF document at URL for wbmp specification

Reproducible: Always

Steps to Reproduce:
Comment 1 Manko 2002-11-29 03:43:33 PST
Created attachment 107757 [details]
Sample WBMP image
Comment 2 Michael Lefevre 2002-11-29 04:04:26 PST
yet another image type... if mozilla is going to do WML, then I guess this is
needed. confirming.
Comment 3 Arunan Balasubramaniam 2002-12-03 05:54:12 PST
Created attachment 108022 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp.
Comment 4 Arunan Balasubramaniam 2002-12-03 05:56:34 PST
Created attachment 108023 [details]
WBMP decoder
Comment 5 Arunan Balasubramaniam 2002-12-03 05:57:14 PST
Created attachment 108024 [details]
WBMP decoder
Comment 6 Arunan Balasubramaniam 2002-12-03 06:08:29 PST
I had a go at this not actually expecting to get anywhere, but here is something
that displays .wbmp files.  w00t!  It's the BMP decoder hacked and poked until
it produced an image from the test file.  This is just dumping my work in
progress, as I don't know if I'm going to have much free time for the next week
and a bit.

Issues:

1) The two enums defined have a very similar value - DECODING_FAILED and
DECODE_FAILED, just spotted now after uploading here.
2) This is in the BMP decoder's subdirectory as I just copied those files and
was hoping to re-use some common code.
3) Nothing added to imgLoader.cpp; don't know if I could/should or not.
4) Forgot how to create a new CID so I just incremented the BMP one.
5) I didn't know what to do about the copyright notice as it's mostly not my
code.  I think the thing to do is leave the copyright as it is in the BMP
decoder (cbiesi) and add myself as a contributor.
Comment 7 Arunan Balasubramaniam 2002-12-03 06:11:47 PST
Comments welcome :)
Comment 8 Arunan Balasubramaniam 2002-12-03 09:12:23 PST
Created attachment 108039 [details]
modules\libpr0n\decoders\bmp\Makefile

Updated makefile for BMP decoder directory.
Comment 9 Arunan Balasubramaniam 2002-12-03 09:14:48 PST
Created attachment 108040 [details]
nsWBMPDecoder.cpp
Comment 10 Arunan Balasubramaniam 2002-12-03 09:16:09 PST
Created attachment 108042 [details]
nsWBMPDecoder.h
Comment 11 Arunan Balasubramaniam 2002-12-03 09:21:19 PST
Just updated the files:

  - rename enums to avoid clash
  - change to 2 space identation as style guide says
  - found guidgen to create CID
  - filled in copyright notice

So if it is OK to be in the BMP directory, I think this can be reviewed.
Comment 12 Manko 2002-12-03 09:43:33 PST
Arunan, my regards :)

Please, request a review via Edit link at your attachment. See
http://bugzilla.mozilla.org/flag-help.html for more information.
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-03 09:56:13 PST
>modules\libpr0n\decoders\bmp\Makefile

um. no. firstly, modify Makefile.in, secondly, please make this a patch.
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-03 12:34:23 PST
ok now to the patch

it does not belong in the bmp directory. it has nothing to do with the BMP
Decoders, except the name. please make a new directory.

and yeah... the main issue will be that you need to get the imagelib module
owner to approve this. that is pavlov@netscape.com (he doesn't read bugmail)

Now. thing is, he is opposed to adding new image decoders to imagelib, last I
heard... but feel free to mail him and ask.

the actual code looks ok on first sight.
if you want a real review, just ask.
Comment 15 Arunan Balasubramaniam 2002-12-03 12:59:05 PST
Thanks for the comments.  I guess Makefile was left in my tree from the nmake ->
gmake switch.  Should have twigged it wasn't in CVS when diff -u would not work,
which was why I attached the whole thing.

I will move the files to a WBMP directory and redo the patches.  Then I'll ask
for review before approaching pavlov.
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-03 13:02:07 PST
severity -> enhancement, as it should be
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-03 13:27:59 PST
ah yeah more comments
o) yes you should change imgLoader.cpp, to look for the signature of WBMP files
o) create a CID by connecting to irc.mozilla.org and /msg botbot uuid?
please do not randomly invent CIDs.
o) yes, if you are modifying a file, it is correct to only add yourself as a 
contributor rather than changing the copyright
Comment 18 Arunan Balasubramaniam 2002-12-03 13:51:22 PST
The CID was created using guidgen from MSVC, which I thought was OK from
http://www.mozilla.org/docs/modunote.htm (section "About nsIIDs and nsCIDs").

Would it be better to re-attach the source and make files for a new "wbmp"
directory as a zip file or just to attach a new makefile.in?

From looking at the code, I think that the appropriate check to add to
imgLoader.cpp would be a check for the file starting with 0x00 and then check
the second byte by ANDing with 0x9F, expecting 0x00 result.  This check would
follow the .ICO check at line 752 as ICO checks for 0x00 0x00 which could match
but an ICO is more likely, and the following checks don't match for 0x00 in the
first byte.
Comment 19 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-03 14:22:30 PST
>The CID was created using guidgen from MSVC,

sure, that's fine. I was referring to your earlier "incremented bmp decoder's
cid by one" statement.

makefile.in patch is enough

yeah, that approach for the imgloader change sounds right
Comment 20 Arunan Balasubramaniam 2002-12-04 06:39:11 PST
Created attachment 108212 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Just twigged that Makefile is generated :)
The severity change was an accident - apologies.
Comment 21 Arunan Balasubramaniam 2002-12-04 06:44:42 PST
Created attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp
Comment 22 Arunan Balasubramaniam 2002-12-04 06:49:24 PST
Gah, sorry for spam.  Had my head in bug 97777 where I was looking for sr=
Comment 23 Manko 2002-12-04 06:54:33 PST
Nice work, Arunan :))

Can you any idea about bug 35995? If yes, welcome to discussion there.
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-04 13:03:58 PST
Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

you need to change allmakefiles.sh as well

"wbmp" this is the correct file extension for WBMP files? Ie. it is not a
3-letter one?
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-04 14:23:39 PST
/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp: In method
`nsresult nsWBMPDecoder::ProcessData(const char *, unsigned int)':
/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp:240: warning:
comparison between signed and unsigned
/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp:286: warning:
comparison between signed and unsigned


please fix that
Comment 26 Arunan Balasubramaniam 2002-12-04 14:32:49 PST
Created attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh
Comment 27 Arunan Balasubramaniam 2002-12-04 14:46:27 PST
Google says:

1) Opera apparently supports WBMP - http://www.opera.com/docs/specs/

2) Most sites think the extension is .wbmp but one suggests it as .wbm as well
as .wbmp

The specification does not mention a standard file extension, only the MIME type.

BTW, all this has only been compiled and run on W2K.
Comment 28 Manko 2002-12-05 04:15:15 PST
Sorry for spam.

Arunan, it looks as if I cannot send email to you. I would like if you read bug
35995 comment 30 and tell your considerations. Thanks beforehand! :)
Comment 29 Arunan Balasubramaniam 2002-12-16 11:59:43 PST
Created attachment 109434 [details]
nsWBMPDecoder.cpp

Adds a cast to remove a warning.
Comment 30 Arunan Balasubramaniam 2002-12-16 12:01:25 PST
Created attachment 109435 [details]
nsWBMPDecoder.h

Change mCurLine to PRUint32 to remove warnings.
Comment 31 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 13:44:16 PST
Comment on attachment 109435 [details]
nsWBMPDecoder.h

inline nsresult SetPixel

hm... you could make this void...
I think it is void in the BMP Decoder as well now.

inline WbmpIntDecodeStatus DecodeEncodedInt

what are you inlining this for - you have a pretty long loop in there, the wins
from inlining will be very small.

  char encodedByte;
why are you declaring this outside the loop?
Same for newFieldVal.

Actually, is there a reason for using newFieldVal rather than directly using
aField?
Comment 32 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 13:47:36 PST
Comment on attachment 109434 [details]
nsWBMPDecoder.cpp

you should init. mDecodingState in ::Init I think... not sure if objects are
supposed to be reusable...

  if (mDecodingState != DECODING_IMAGE_DATA) {

I don't think this optimization is worth the reduced understandability of the
code.

    // For now, ignore ext header field as it is not present in well defined
type 0 WBMP.

what happens if this field is present?

      WbmpIntDecodeStatus widthReadResult = DecodeEncodedInt (mWidth, aBuffer,
aCount);

What happens if this returns INT_PARSE_IN_PROGRESS - bad things, don't they?

    PRUint32 toCopy;
you're again declaring a variable outside the loop for no good reason.
(does the bmp decoder do the same things?)
Comment 33 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 13:48:41 PST
Comment on attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh

this attachment (allmakefiles) must have r=cls (but needs no super-review)
Comment 34 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 13:51:53 PST
Comment on attachment 108212 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

this one looks good.

Note that this review is _only_ valid if you get the module owner
(pavlov@netscape.com last I checked) to approve adding WAP BMP support to
imagelib..
Comment 35 Jim Dunn 2002-12-16 14:02:51 PST
It is me now... see bug 185140
I will get back to you shortly...
Comment 36 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 14:03:46 PST
Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

hm, yeah, this also looks good with the same limitation as above
Comment 37 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-16 14:03:48 PST
Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

hm, yeah, this also looks good with the same limitation as above
Comment 38 Arunan Balasubramaniam 2002-12-16 16:06:44 PST
Comment on attachment 109435 [details]
nsWBMPDecoder.h

> inline nsresult SetPixel
> 
> hm... you could make this void...
> I think it is void in the BMP Decoder as well now.

  Yes, it has changed in the BMP decoder (since I copied it :) and will here too.

> inline WbmpIntDecodeStatus DecodeEncodedInt
> 
> what are you inlining this for - you have a pretty long loop in there, the
> wins from inlining will be very small.

  inline removed.

>   char encodedByte;
> why are you declaring this outside the loop?
> Same for newFieldVal.

  Just personal style, changed now.

> Actually, is there a reason for using newFieldVal rather than directly using
> aField?

  Having the new and previous value lets you check for overflow without knowing
the size of the data type "if (newFieldVal < aField)".

> (From update of attachment 109434 [details])
> you should init. mDecodingState in ::Init I think... not sure if objects are
> supposed to be reusable...

  Moved.

>  if (mDecodingState != DECODING_IMAGE_DATA) {
>
> I don't think this optimization is worth the reduced understandability of the
> code.

  I will keep the if and make the code clearer by putting the if's contents
in a parse_wbmp_header function like in the BMP decoder.

>     // For now, ignore ext header field as it is not present in well defined
> type 0 WBMP.
>
> what happens if this field is present?

  If the ext header field is present, then bit 7 of the fix header field will be
set.  We check for this by ANDing the fix header field with 9F and checking that
Bits 7 and 4-0 are 0 as we expect.  If they are not (includes the case where
the header is present), we error out.
  This should be moot anyway as we also check for type 0 ealier, and at the
bottom of the specification it says a "well defined" type 0 will have all those
bits set to 0.
  I will add a comment to the code to say why the ext header should not be there.

>      WbmpIntDecodeStatus widthReadResult = DecodeEncodedInt (mWidth, aBuffer,
> aCount);
>
>What happens if this returns INT_PARSE_IN_PROGRESS - bad things, don't they?

  If INT_PARSE_IN_PROGRESS is returned, then the decoding state is not changed
and the flow of execution should drop to the bottom (as it will not match
further states) and return NS_OK.  So, I don't think bad things happen.
  Having just written that, I see I should have just made the return NS_OK
explicit in a final else clause, which is far clearer.  I'll do that.

>     PRUint32 toCopy;
> you're again declaring a variable outside the loop for no good reason.
> (does the bmp decoder do the same things?)

  That bit is as stolen from the BMP decoder (phew!).  Changed now.
Comment 39 Arunan Balasubramaniam 2002-12-16 16:10:17 PST
Sorry for spam, but to clarify:

> Having the new and previous value lets you check for overflow without knowing
> the size of the data type "if (newFieldVal < aField)".

  Although we know it's a PRUint32, by doing it this way we don't have to store
the number of bytes parsed if we only receive part of the encoded int field at
a time.
Comment 40 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-17 03:58:07 PST
>  inline removed.

I hope that includes moving it to the .cpp file.

>  I will keep the if and make the code clearer by putting the if's contents
>in a parse_wbmp_header function like in the BMP decoder.

I didn't mean that
This if just contains several other ifs who test the same variable. so no need
to test the if just there.

>it says a "well defined" type 0 will have all those
>bits set to 0.

Well, you should never trust input data... not showing the image is OK for
invalid images; but if you would crash, that would not be ok - this is why I asked.

>  If INT_PARSE_IN_PROGRESS is returned, then the decoding state is not changed
>and the flow of execution should drop to the bottom 

Yes, I knew that; sorry I should have been clearer. I meant - when the next part
of the integer will later arrive, will it work as expected? Ie. the variable
contains the full integer later?
Comment 41 Arunan Balasubramaniam 2002-12-17 05:54:16 PST
> I hope that includes moving it to the .cpp file.

  It helps it compile :)

> I didn't mean that
> This if just contains several other ifs who test the same variable. so no need
> to test the if just there.

  I know you didn't mean that, but it seems a better solution as the code is
overall clearer by factoring out the header parts (as the BMP decoder does).
  By doing that one test you eliminate lots of tests you know will fail in
the usual case, as the header is small compared to the image data.
  I have created ProcessHeaderData already but will put the code back into
ProcessData if you prefer.

> Well, you should never trust input data... not showing the image is OK for
> invalid images; but if you would crash, that would not be ok - this is why I
> asked.

  That's OK, we're not trusting the data unless it is exactly as specified.
So it's explicitly checking it's a good wbmp instead of assuming.

> Yes, I knew that; sorry I should have been clearer. I meant - when the next
> part of the integer will later arrive, will it work as expected? Ie. the
> variable contains the full integer later?

  It should work, as we store the running total.  I'll redo my test code for
that function to double check as I did modify it since I wrote it's test.

  Thanks for all the comments.  I'll create some more corrupted images and a
test page to display them so this code can be confirmed to not crash on bad
input data.  I don't know how to easily test image data being received in
fragments, but I will redo the harness for DecodeEncodedInt.
Comment 42 Manko 2002-12-17 06:10:48 PST
This bug is still an enhancement anyway.

Severity "trivial" means that bug is a slight cosmetic problem with an existing
Mozilla trunk code.

The rest of work at this bug is, I hope, indeed trivial (thanks!), but this
doesn't mean this bug to be trivial as a whole :)
Comment 43 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-17 06:49:07 PST
> I have created ProcessHeaderData already but will put the code back into
> ProcessData if you prefer.

ok, thanks for the explanation, ProcessHeaderData seems fine to me.


so... will you attach the current version of the .cpp/.h?
Comment 44 Arunan Balasubramaniam 2002-12-17 07:00:56 PST
I will try to create the test image files before attaching new versions of the
source as I don't really want to ask for review and then test and find a bug
which means I have to add them yet again and have them reviewed again.
Comment 45 Arunan Balasubramaniam 2002-12-19 14:02:44 PST
Created attachment 109781 [details]
Zip of test files.
Comment 46 Arunan Balasubramaniam 2002-12-19 14:20:05 PST
Created attachment 109785 [details]
Zip of more test files
Comment 47 Arunan Balasubramaniam 2002-12-19 14:22:28 PST
Created attachment 109786 [details]
nsWBMPDecoder.cpp
Comment 48 Arunan Balasubramaniam 2002-12-19 14:23:21 PST
Created attachment 109788 [details]
nsWBMPDecoder.h
Comment 49 Arunan Balasubramaniam 2002-12-19 14:33:05 PST
Oh, it was me who changed the severity again ... <sighs> sorry again.  I'll edit
my bugzilla prefs to not filter my own changes so I'll notice if/when it happens
again.

I know how this Internet thing works, honest.

<Double checks it still says enhancement>
Comment 50 Manko 2002-12-20 04:59:06 PST
Created attachment 109830 [details]
Screenshot for multi-byte width/height WBMP

This is a screenshot from Opera 6.05 and IrfanView for your multi-byte files.
Both viewers show same image, thus it seems to be correct.

What output produces your decoder?
Comment 51 Arunan Balasubramaniam 2002-12-20 08:11:41 PST
The images are the same in the local decoder.  The multi-byte tests are just to
ensure that multi-byte ints are decoded correctly.  The width one looks the way
it does as it is just the same image data four times so the first "wide" row is
the first four "normal" rows in a line, etc.  The other tests prove that the
image data would be decoded correctly, as do your screenshots (thanks).

<checks severity field>
Comment 52 Manko 2002-12-20 08:19:45 PST
Nice work :)

Waiting for review.

Comment 53 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-22 15:53:39 PST
Comment on attachment 109786 [details]
nsWBMPDecoder.cpp

#include "nsIImage.h"
#include "ImageLogging.h"
#include "imgIContainerObserver.h"

please verify that you really need these includes, and remove them if not. I
also now notice that the "string" line in Makefile.in can and should be
removed.

      if ((rowSize - mRowBytes) == 0) {
you could write this as
if (rowSize == mRowBytes)

	    SetPixel(d, (PRBool)(idx > 0));
uh is the PRBool cast really necessary? and if it is, the >0 part is not
necessary. SetPixel does not explicitly compare to PR_TRUE/FALSE, only does if
(aPixelWhite).

The rest looks good, but could you please add a comment to DecodeEncodedInt
that describes what exactly the function does, and how these encoded integers
look like?

Also, this function doesn't look like it needs to be a member function, just
make it a static function in the .cpp file.

with these changes you can get r=biesi, but please attach a new patch
Comment 54 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-22 15:57:40 PST
Comment on attachment 109788 [details]
nsWBMPDecoder.h

  DECODING_EXT_HEADER,
you never use this enum value... I would remove it...

#define WBMP_GFXFORMAT gfxIFormats::BGR
is this used in more than one place?
if not, there's no need to use this define and you could just #ifdef the code
in the .cpp file.

Ah... I see you did comment what DecodeEncodedInt does... but you do not make
it clear that if parse is in progress, another call of this function with the
same aField parameter does "the right thing".


again, r=biesi with these changes if you attach a new patch.
Comment 55 Arunan Balasubramaniam 2003-01-03 14:34:42 PST
Created attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Removed string from REQUIRES.
Comment 56 Arunan Balasubramaniam 2003-01-03 14:50:13 PST
Looking again at my test cases, I am wondering if it is OK that some of the
image cells are truly empty and some have no visible content (as if &nbsp). 
Surely Mozilla should produce the same result for any type of invalid image? 
What would the correct result be for an incorrect image?
Comment 57 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-03 15:02:08 PST
Comment on attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

better, thanks
Comment 58 Arunan Balasubramaniam 2003-01-04 10:58:01 PST
Created attachment 110662 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp
Comment 59 Arunan Balasubramaniam 2003-01-04 10:58:40 PST
Created attachment 110663 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h
Comment 60 Stuart Parmenter 2003-01-07 13:59:27 PST
I don't really know why we need this.. but has anyone looked at how big the 
libraries are?  If it isn't very big, I probably won't be too against putting 
it in to the tree.
Comment 61 Arunan Balasubramaniam 2003-01-07 14:37:31 PST
Created attachment 110891 [details]
Size of DEBUG .obj files

This is the size of the debug object files in my working tree.	It looks like
the WBMP decoder will probably be just a little bigger than the XBM decoder,
however big that is.
Comment 62 Manko 2003-01-08 00:06:14 PST
Stuart, this issue appeared as consequence of bug 35995. I know many people to
want see WAP/WML pages in plain HTML browser and they would use for it open
source browser instead of Opera. They want see not only text but WBMP images ;)

Additional code for WBMP support is IMHO not too big.
Comment 63 Arunan Balasubramaniam 2003-01-09 06:43:29 PST
Aside from the requested changes, I also changed the call to SetPixel:

        while (lpos < mWidth) {
          PRInt8 bit;
          PRUint8 idx;

          for (bit = 7; bit >= 0; bit--) {
            if (lpos >= mWidth)
              break;
            idx = (PRUint8)((*p >> bit) & 1);
            SetPixel(d, (PRBool)(idx > 0));
            ++lpos;
          }
          ++p;
        }

  became

        while (lpos < mWidth) {
          PRInt8 bit;

          for (bit = 7; bit >= 0; bit--) {
            if (lpos >= mWidth)
              break;
            PRBool pixelWhite = (*p >> bit) & 1;
            SetPixel(d, pixelWhite);
            ++lpos;
          }
          ++p;
        }

  so it is only for this change I re-requested r=.
Comment 64 Arunan Balasubramaniam 2003-01-09 06:46:40 PST
Comment on attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh

Requesting r= on the same "if module owner says OK" basis as the other patches.
Comment 65 Jim Dunn 2003-01-10 06:32:54 PST
sr=jdunn@netscape.com (NOTE: you still need r=
Comment 66 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-10 06:42:19 PST
Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

fyi - this patch has just bitrotted due to the checkin for bug 169304
Comment 67 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-10 08:59:48 PST
Comment on attachment 110663 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h

  nsresult ProcessHeaderData(const char*& aBuffer, PRUint32& aCount);
please describe in the comment for this function what happens to aBuffer and
aCount.
E.g. "After returning, aBuffer will point after the bytes used by this
function, and aCount will be decremented by the number of used bytes."

and a comment above SetPixel would be nice too.

r=biesi with that change
Comment 68 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-10 09:02:10 PST
um. jdunn: you are not listed at http://www.mozilla.org/hacking/reviewers.html
as a super-reviewer?
Comment 69 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-10 09:11:45 PST
Comment on attachment 110662 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

  NS_INIT_ISUPPORTS();
please remove this line, it's deprecated.

rest looks ok, r=biesi
no need for a new patch (just remove that line before checking in)
Comment 70 Arunan Balasubramaniam 2003-01-10 09:27:31 PST
> no need for a new patch (just remove that line before checking in)

Since I don't have check-in rights shall I attach a new version for whoever
checks this in for me?
Comment 71 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-10 09:43:45 PST
if you want :) but I'd say that whoever checks this in can also remove that line.
Comment 72 Arunan Balasubramaniam 2003-01-11 01:59:36 PST
Created attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Bug 169304 changed each decoder's Makefile.in by removing the EXPORT_LIBRARY
line.
Comment 73 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-11 07:21:45 PST
Comment on attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

looks good
Comment 74 Arunan Balasubramaniam 2003-01-16 11:02:10 PST
Created attachment 111722 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h
Comment 75 Arunan Balasubramaniam 2003-01-16 11:05:50 PST
Created attachment 111723 [details] [diff] [review]
Build changes

Changes for build (I believe to be reviewed by seawood - cls@seawood.org?)
Comment 76 Arunan Balasubramaniam 2003-01-16 11:07:05 PST
Created attachment 111724 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

Code changes to make moz show wbmp love.
Comment 77 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-16 14:19:21 PST
Comment on attachment 111724 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

this looks good
(wait, we have two nsContentDLF? someone ought to change that, really. nothing
you need to worry about, though)

yes, the build changes should be reviewed by cls - seawood@netscape.com iirc
Comment 78 hacker formerly known as seawood@netscape.com 2003-01-17 01:23:29 PST
Comment on attachment 111723 [details] [diff] [review]
Build changes

I'll r= the build changes after pavlov has given module owner approval.
Comment 79 Stuart Parmenter 2003-01-17 09:38:20 PST
Comment on attachment 111723 [details] [diff] [review]
Build changes

yeah sure
Comment 80 hacker formerly known as seawood@netscape.com 2003-01-17 12:21:26 PST
Comment on attachment 111723 [details] [diff] [review]
Build changes

r=cls
Comment 81 Arunan Balasubramaniam 2003-01-22 02:25:02 PST
Um, do we have sr= from comment 65 ?  Bug 185140 would appear to make it so.  I
read in a blog that Pavlov is working on other things anyway :)
Comment 82 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-22 03:37:18 PST
super-reviewers are listed at http://www.mozilla.org/hacking/reviewers.html
jdunn is not listed there

oh and btw, if you want to check this in before the 1.3 branch, you must get
approval from drivers.
Comment 83 Arunan Balasubramaniam 2003-01-22 04:29:08 PST
Created attachment 112275 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Removed the NS_INIT_ISUPPORTS() line for the super reviewer.
Comment 84 Christian :Biesinger (don't email me, ping me on IRC) 2003-01-22 05:05:58 PST
Comment on attachment 112275 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

the only change to the last patch is the NS_INIT_ISUPPORTS removal, right?
if so, r=biesi
Comment 85 Arunan Balasubramaniam 2003-01-22 05:21:26 PST
> the only change to the last patch is the NS_INIT_ISUPPORTS removal, right?

A newline crept in somehow at the end of ProcessData too, but the only code
change is the NS_INIT_ISUPPORTS removal.

This seems a low risk patch so I'll request drivers approval after getting sr=.
Comment 86 Manko 2003-01-22 05:28:43 PST
Critical means that this bug is causing Mozilla to be crashing, hanging,
security holing etc.

So, changing severity back ;)

Anyway, thank for your amazing work :))) I still hope for your contibution for
bug 35995.
Comment 87 tor 2003-01-22 08:52:32 PST
FYI: I'm not going to have a chance to sr this until next week.
Comment 88 Arunan Balasubramaniam 2003-01-23 04:10:25 PST
Sorry for inadvertently changing the severity again - I think I am a victim of
bug 33732 (mouse wheel scrolls list boxes even when outside them).  I have
disabled Moz in the wheel's list of applications to try to stop this happening.

tor: thanks for letting us know you will get to this later, rather than leaving
us wondering.
Comment 89 Manko 2003-03-06 00:39:00 PST
Hope to have this in 1.4 alpha.
Comment 90 Asa Dotzler [:asa] 2003-03-18 20:06:47 PST
blocking1.4a- because we wouldn't block the release for this change. 
Comment 91 Arunan Balasubramaniam 2003-03-19 03:12:20 PST
The patches attached to this have been broken since the trunk opened for 1.4
alpha anyway.  I have fixed them locally but didn't bother updating the bug in
case they broke again before they got super-reviewed.  I'll attach them if/when
requested.

I would think from the sentiments expressed in bug 197530 and bug 195280 that
this has little chance of landing on the trunk ever.  I think the chances would
be improved if image decoders were entirely pluggable (see
http://bugzilla.mozilla.org/show_bug.cgi?id=197530#c3) but not by much and from
a very small starting point in this case.

I am not moaning about this as I understand the need to keep minority features
out of the main build.  It would be nice if this could go in to the tree
disabled so people who wanted to play with WML could enable it for themselves in
their own builds.  And if it makes any difference, I would be happy to be
default owner for any and all bugs against this decoder.  My email is monitored
at least daily (except holidays :).
Comment 92 Christian :Biesinger (don't email me, ping me on IRC) 2003-03-19 10:10:41 PST
> if image decoders were entirely pluggable

see also bug 41333, esp bug 41333 comment 27,  and I think that for "normal" use
of wbmp images, the current level of pluggability suffices - all that (maybe)
wouldn't work is wbmp images for which no mimetype is available (e.g. FTP or
local files)
Comment 93 tor 2004-03-02 12:19:59 PST
brendan, chofmann: do you care about this for minimo?
Comment 94 chris hofmann 2004-03-02 12:32:09 PST
no specific requirement from any of the vendors we have talked to, but it sounds
like it might be good to get this in the tree disabled for the default build,
but we could get some folks to try it out and do some testing with a minimo build.
Comment 95 Arunan Balasubramaniam 2004-03-03 02:02:53 PST
I would be happy to get this working with current CVS if wanted.
Comment 96 Arunan Balasubramaniam 2004-04-08 12:18:21 PDT
Created attachment 145691 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Fixes a bug in the multi-byte integer decoder
Comment 97 Arunan Balasubramaniam 2004-04-08 12:38:20 PDT
Created attachment 145693 [details] [diff] [review]
Build changes for WBMP
Comment 98 Arunan Balasubramaniam 2004-04-08 12:40:24 PDT
Created attachment 145694 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp and WBMP decoder
Comment 99 Arunan Balasubramaniam 2004-04-08 12:53:20 PDT
Created attachment 145695 [details] [diff] [review]
XPFE changes

Seems to work, but I have just set and unset the WBMP checkbox.

I am not sure if the change to xpfe/components/build/nsModule.cpp should be in
this patch or patch 145694 "Make mozilla aware...".
Comment 100 Arunan Balasubramaniam 2004-04-08 12:56:21 PDT
Don't know who to request reviews for the build changes and the XPFE changes
from, or any super reviews. Could some kind soul set those requests for me please?
Comment 101 Christian :Biesinger (don't email me, ping me on IRC) 2004-04-08 13:39:49 PDT
Comment on attachment 145695 [details] [diff] [review]
XPFE changes

requesting review from neil... (not my patch)

+  deleteThisFile("Components", "wbmpdecoder2.shlb");

these lines are not necessary. such lines are here for upgrading from older
versions, because in current versions the image decoders are all part of
libimglib2.so
Comment 102 Christian :Biesinger (don't email me, ping me on IRC) 2004-04-08 13:55:54 PDT
Comment on attachment 145691 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Fixes a bug in the multi-byte integer decoder

/* ----- BEGIN LICENSE BLOCK -----

why are you using --- now instead of ***?

+  // ReadSegments does not return the error returned from ReadSegCb, so
+  // check for decoding errors here if we get a success return code.
+//  if (NS_SUCCEEDED(rv) && (mDecodingState == DECODING_FAILED))
+//    return NS_ERROR_FAILURE;

why is this commented out?
ah... looks like you're checking for this in ProcessData now. why not remove
this commented-out code, then?

+    if (aField & (0xFE << (8 * (sizeof (aField) - 1)))) {

hm... why not:
if (aField & 0xFE000000)
?
a PRUint32 will always be 32 bit large, hence the name...


Why are you no longer setting mDecodingState before returning an error? not
like it really matters, granted.
Comment 103 neil@parkwaycc.co.uk 2004-04-08 14:53:50 PDT
Comment on attachment 145695 [details] [diff] [review]
XPFE changes

FE bits look ok but I don't have a PC to test on... passing the buck on the
build stuff too.
Comment 104 Arunan Balasubramaniam 2004-04-08 17:07:38 PDT
Thanks for the quick replies.

> why are you using --- now instead of ***?

That came from diffing against the BMP decoder. I copied the block across to
make the diff clearer. Looking at
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c , I realise that
was a mistake as I *had* used the official boilerplate before.

> +  deleteThisFile("Components", "wbmpdecoder2.shlb");

OK.

> +//  if (NS_SUCCEEDED(rv) && (mDecodingState == DECODING_FAILED))
> +//    return NS_ERROR_FAILURE;

Ack, this was an attempt to have mozilla show the broken image icon, copied from
either the GIF or JPEG decoder. It didn't work, so I should have removed it.

> hm... why not:
> if (aField & 0xFE000000)
> ?
> a PRUint32 will always be 32 bit large, hence the name...

My paranoia from watching previous code I worked on evolve. You can change the
size aField to any type and the code would still work. But 0xFE000000 is
certainly clearer :)

> Why are you no longer setting mDecodingState before returning an error? not
> like it really matters, granted.

Failed return codes get caught in ReadSegCb and mDecodingState is set to
DECODING_FAILED in one place instead of many.

So, would you like me to attach new versions of the patches for any/all of the
noted points?

1) Could put the license block back to the original
2) Remove deleteThisFile("...
3) Remove the commented out code in the decoder
4) Use a more obvious/less paranoid bit mask

Or does review+ mean you are happy to leave them or modify them yourself?
Comment 105 Christian :Biesinger (don't email me, ping me on IRC) 2004-04-08 17:55:06 PDT
> So, would you like me to attach new versions of the patches for any/all of the
> noted points?

please do, for the patch that I marked review+ on... for the 2) part I'd wait
until bsmedberg comments on the patch.

> 1) Could put the license block back to the original
> 2) Remove deleteThisFile("...
> 3) Remove the commented out code in the decoder
> 4) Use a more obvious/less paranoid bit mask
Comment 106 Arunan Balasubramaniam 2004-04-09 05:24:56 PDT
Created attachment 145740 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp after review

License file back.

I put the commented out lines in WriteFrom() back in. Having that code recreate
the error return code stops an assertion at
netwerk/base/src/nsInputStreamPump.cpp line 451 from firing.

Bit mask now more obvious.
Comment 107 Benjamin Smedberg [:bsmedberg] 2004-04-22 11:13:30 PDT
Comment on attachment 145695 [details] [diff] [review]
XPFE changes

I'm pretty sure all of the "deleteThisFile" calls in the installer .jst files
are unnecessary. They are only necessary if there was previously a wbmp
component and now there isn't.
Comment 108 Arunan Balasubramaniam 2004-04-22 23:57:53 PDT
Ah, so those lines are only to clean up installs from before the decoders got
combined in Bug 168048?

Shall I reattach the patch with the .jst changes chopped off?
Comment 109 Aaron Kaluszka 2005-08-05 08:37:02 PDT
What's the status of this bug?  Patches were reviewed, but that was over a year
ago...
Comment 110 Alfred Kayser 2007-01-19 09:10:28 PST
This really probably needs to be updated after the Cairo changes
Comment 111 Matthew Wilson 2007-12-08 07:15:48 PST
Updated versions against current trunk are checked in at http://www.mozdev.org/source/browse/wmlbrowser/extensions/wbmp/
Comment 112 d 2009-06-08 09:44:35 PDT
What's going on here? Is it going to be included or not?
Comment 113 Joe Drew (not getting mail) 2011-10-07 09:29:37 PDT
The patches as-is aren't compatible with current Gecko, and there's not a pressing need to support wbmp any more. Feel free to reopen this if you really feel like I'm wrong here.
Comment 114 John Drinkwater (:beta) 2013-03-19 10:31:34 PDT
Bug 847310 kinda changed this to DUPLICATE FIXED.

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