Bug 182621 (wbmp)

WAP BMP (wbmp) format support

RESOLVED WONTFIX

Status

()

Core
ImageLib
--
enhancement
RESOLVED WONTFIX
15 years ago
5 months ago

People

(Reporter: Manko, Assigned: Arunan Balasubramaniam)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.4a -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(11 attachments, 20 obsolete attachments)

460 bytes, image/vnd.wap.wbmp
Details
10.96 KB, application/octet-stream
Details
2.67 KB, application/octet-stream
Details
55.01 KB, image/png
Details
2.33 KB, text/plain
Details
2.07 KB, text/plain
Biesinger
: review+
Details
4.84 KB, text/plain
Biesinger
: review+
Details
69.48 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
7.28 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
21.49 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
12.38 KB, text/plain
Biesinger
: review+
Details
(Reporter)

Description

15 years ago
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:
(Reporter)

Updated

15 years ago
Blocks: 35995
(Reporter)

Comment 1

15 years ago
Created attachment 107757 [details]
Sample WBMP image

Comment 2

15 years ago
yet another image type... if mozilla is going to do WML, then I guess this is
needed. confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

15 years ago
Created attachment 108022 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp.
(Assignee)

Comment 4

15 years ago
Created attachment 108023 [details]
WBMP decoder
(Assignee)

Comment 5

15 years ago
Created attachment 108024 [details]
WBMP decoder
(Assignee)

Comment 6

15 years ago
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.
Assignee: pavlov → arunan_bala
(Assignee)

Comment 7

15 years ago
Comments welcome :)
(Assignee)

Comment 8

15 years ago
Created attachment 108039 [details]
modules\libpr0n\decoders\bmp\Makefile

Updated makefile for BMP decoder directory.
(Assignee)

Comment 9

15 years ago
Created attachment 108040 [details]
nsWBMPDecoder.cpp
Attachment #108023 - Attachment is obsolete: true
(Assignee)

Comment 10

15 years ago
Created attachment 108042 [details]
nsWBMPDecoder.h
Attachment #108024 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
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.
(Reporter)

Comment 12

15 years ago
Arunan, my regards :)

Please, request a review via Edit link at your attachment. See
http://bugzilla.mozilla.org/flag-help.html for more information.
>modules\libpr0n\decoders\bmp\Makefile

um. no. firstly, modify Makefile.in, secondly, please make this a patch.
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.
(Assignee)

Comment 15

15 years ago
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.
Severity: enhancement → blocker
severity -> enhancement, as it should be
Severity: blocker → enhancement
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
(Assignee)

Comment 18

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

Comment 20

15 years ago
Created attachment 108212 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Just twigged that Makefile is generated :)
The severity change was an accident - apologies.
Attachment #108039 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
Created attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp
Attachment #108022 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #108212 - Flags: superreview?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108213 - Flags: superreview?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108042 - Flags: superreview?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108040 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 22

15 years ago
Gah, sorry for spam.  Had my head in bug 97777 where I was looking for sr=
(Assignee)

Updated

15 years ago
Attachment #108040 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108042 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108212 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108213 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
(Reporter)

Comment 23

15 years ago
Nice work, Arunan :))

Can you any idea about bug 35995? If yes, welcome to discussion there.
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?
/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
(Assignee)

Comment 26

15 years ago
Created attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh
(Assignee)

Comment 27

15 years ago
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.
(Reporter)

Comment 28

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

Comment 29

15 years ago
Created attachment 109434 [details]
nsWBMPDecoder.cpp

Adds a cast to remove a warning.
Attachment #108040 - Attachment is obsolete: true
(Assignee)

Comment 30

15 years ago
Created attachment 109435 [details]
nsWBMPDecoder.h

Change mCurLine to PRUint32 to remove warnings.
Attachment #108042 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #109434 - Flags: review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #109435 - Flags: review?(cbiesinger)
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?
Attachment #109435 - Flags: review?(cbiesinger) → review-
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?)
Attachment #109434 - Flags: review?(cbiesinger) → review-
Comment on attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh

this attachment (allmakefiles) must have r=cls (but needs no super-review)
Attachment #108040 - Flags: review?(cbiesinger)
Attachment #108042 - Flags: review?(cbiesinger)
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..
Attachment #108212 - Flags: review?(cbiesinger) → review+

Comment 35

15 years ago
It is me now... see bug 185140
I will get back to you shortly...
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
Attachment #108213 - Flags: review?(cbiesinger) → review+
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
(Assignee)

Comment 38

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

Comment 39

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

Comment 41

15 years ago
> 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.
Severity: enhancement → trivial
(Reporter)

Comment 42

15 years ago
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 :)
Severity: trivial → enhancement
> 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?
(Assignee)

Comment 44

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

Comment 45

15 years ago
Created attachment 109781 [details]
Zip of test files.
(Assignee)

Comment 46

15 years ago
Created attachment 109785 [details]
Zip of more test files
(Assignee)

Comment 47

15 years ago
Created attachment 109786 [details]
nsWBMPDecoder.cpp
Attachment #109434 - Attachment is obsolete: true
(Assignee)

Comment 48

15 years ago
Created attachment 109788 [details]
nsWBMPDecoder.h
Attachment #109435 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #109786 - Flags: review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #109788 - Flags: review?(cbiesinger)
(Assignee)

Comment 49

15 years ago
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>
(Reporter)

Comment 50

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

Comment 51

15 years ago
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>
(Reporter)

Comment 52

15 years ago
Nice work :)

Waiting for review.

Alias: wbmp
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
Attachment #109786 - Flags: review?(cbiesinger) → review-
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.
Attachment #109788 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 55

15 years ago
Created attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Removed string from REQUIRES.
Attachment #108212 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110615 - Flags: review?(cbiesinger)
(Assignee)

Comment 56

15 years ago
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 on attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

better, thanks
Attachment #110615 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 58

15 years ago
Created attachment 110662 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp
Attachment #109786 - Attachment is obsolete: true
(Assignee)

Comment 59

15 years ago
Created attachment 110663 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h
Attachment #109788 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110662 - Flags: review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #110663 - Flags: review?(cbiesinger)

Comment 60

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

Comment 61

15 years ago
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.
(Reporter)

Comment 62

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

Comment 63

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

Comment 64

15 years ago
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.
Attachment #108262 - Flags: review?(cls)

Comment 65

15 years ago
sr=jdunn@netscape.com (NOTE: you still need r=
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 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
Attachment #110663 - Flags: review?(cbiesinger) → review+
um. jdunn: you are not listed at http://www.mozilla.org/hacking/reviewers.html
as a super-reviewer?
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)
Attachment #110662 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 70

15 years ago
> 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?
if you want :) but I'd say that whoever checks this in can also remove that line.
(Assignee)

Comment 72

15 years ago
Created attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

Bug 169304 changed each decoder's Makefile.in by removing the EXPORT_LIBRARY
line.
Attachment #110615 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #111283 - Flags: review?(cbiesinger)
Comment on attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

looks good
Attachment #111283 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 74

15 years ago
Created attachment 111722 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h
Attachment #110663 - Attachment is obsolete: true
(Assignee)

Comment 75

15 years ago
Created attachment 111723 [details] [diff] [review]
Build changes

Changes for build (I believe to be reviewed by seawood - cls@seawood.org?)
Attachment #108262 - Attachment is obsolete: true
(Assignee)

Comment 76

15 years ago
Created attachment 111724 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

Code changes to make moz show wbmp love.
Attachment #108213 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #111722 - Flags: review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #111724 - Flags: review?(cbiesinger)
(Assignee)

Updated

15 years ago
Attachment #108262 - Flags: review?(cls)
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
Attachment #111724 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

15 years ago
Attachment #111723 - Flags: review?(seawood)
Comment on attachment 111723 [details] [diff] [review]
Build changes

I'll r= the build changes after pavlov has given module owner approval.
Attachment #111723 - Flags: superreview?(seawood)
Attachment #111723 - Flags: review?(seawood)
Attachment #111723 - Flags: review?(pavlov)
Attachment #111722 - Flags: review?(cbiesinger) → review+

Comment 79

15 years ago
Comment on attachment 111723 [details] [diff] [review]
Build changes

yeah sure
Attachment #111723 - Flags: review?(pavlov) → review+
Comment on attachment 111723 [details] [diff] [review]
Build changes

r=cls
Attachment #111723 - Flags: superreview?(seawood) → superreview+
(Assignee)

Comment 81

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

Updated

15 years ago
Attachment #111283 - Flags: superreview?(tor)
(Assignee)

Updated

15 years ago
Attachment #111722 - Flags: superreview?(tor)
(Assignee)

Updated

15 years ago
Attachment #111724 - Flags: superreview?(tor)
(Assignee)

Comment 83

15 years ago
Created attachment 112275 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Removed the NS_INIT_ISUPPORTS() line for the super reviewer.
Attachment #110662 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #112275 - Flags: superreview?(tor)
Attachment #112275 - Flags: review?(cbiesinger)
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
Attachment #112275 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 85

15 years ago
> 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=.
Severity: enhancement → critical
(Reporter)

Comment 86

15 years ago
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.
Severity: critical → enhancement

Comment 87

15 years ago
FYI: I'm not going to have a chance to sr this until next week.
(Assignee)

Comment 88

15 years ago
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.
(Reporter)

Comment 89

14 years ago
Hope to have this in 1.4 alpha.
Flags: blocking1.4a?

Comment 90

14 years ago
blocking1.4a- because we wouldn't block the release for this change. 
Flags: blocking1.4a? → blocking1.4a-
(Assignee)

Comment 91

14 years ago
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 :).
> 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

13 years ago
brendan, chofmann: do you care about this for minimo?

Comment 94

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

Comment 95

13 years ago
I would be happy to get this working with current CVS if wanted.
(Assignee)

Comment 96

13 years ago
Created attachment 145691 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Fixes a bug in the multi-byte integer decoder
Attachment #112275 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #145691 - Attachment description: Fixes a bug in the multi-byte integer decoder → modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp Fixes a bug in the multi-byte integer decoder
Attachment #145691 - Flags: review?(cbiesinger)
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 97

13 years ago
Created attachment 145693 [details] [diff] [review]
Build changes for WBMP
(Assignee)

Updated

13 years ago
Attachment #111723 - Attachment is obsolete: true
(Assignee)

Comment 98

13 years ago
Created attachment 145694 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp and WBMP decoder
Attachment #111724 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #145694 - Flags: review?(cbiesinger)
(Assignee)

Comment 99

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

Comment 100

13 years ago
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 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
Attachment #145695 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #145693 - Flags: review?(bsmedberg)
Attachment #145694 - Flags: review?(cbiesinger) → review+
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.
Attachment #145691 - Flags: review?(cbiesinger) → review+
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.
Attachment #145695 - Flags: review?(neil.parkwaycc.co.uk) → review?(bsmedberg)
(Assignee)

Comment 104

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

Comment 106

13 years ago
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.
Attachment #145691 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #145740 - Flags: review?(cbiesinger)
Attachment #145740 - Flags: review?(cbiesinger) → review+
Attachment #145693 - Flags: review?(bsmedberg) → review+
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.
Attachment #145695 - Flags: review?(bsmedberg) → review+
(Assignee)

Comment 108

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

Updated

13 years ago
Attachment #145740 - Flags: superreview?(tor)
(Assignee)

Updated

13 years ago
Attachment #145694 - Flags: superreview?(tor)
(Assignee)

Updated

13 years ago
Attachment #112275 - Flags: superreview?(tor)
(Assignee)

Updated

13 years ago
Attachment #111724 - Flags: superreview?(tor)

Comment 109

12 years ago
What's the status of this bug?  Patches were reviewed, but that was over a year
ago...

Comment 110

10 years ago
This really probably needs to be updated after the Cairo changes

Updated

10 years ago
Attachment #111283 - Flags: superreview?(tor)

Updated

10 years ago
Attachment #111722 - Flags: superreview?(tor)

Updated

10 years ago
Attachment #145694 - Flags: superreview?(tor)

Updated

10 years ago
Attachment #145740 - Flags: superreview?(tor)

Comment 111

10 years ago
Updated versions against current trunk are checked in at http://www.mozdev.org/source/browse/wmlbrowser/extensions/wbmp/

Comment 112

8 years ago
What's going on here? Is it going to be included or not?
QA Contact: tpreston → imagelib
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Bug 847310 kinda changed this to DUPLICATE FIXED.
You need to log in before you can comment on or make changes to this bug.