Build ID: 19991110911 (M11) Platform: Windows NT To reproduce: - Launch the browser - Load the above URL - Scroll down to test 2.1.7 - Bitmap (BMP) Result: There is no image there. Expected result: You should see the image. This works correctly in IE but not in Nav 4.x.

To the best of my knowledge, this bug is invalid. We have never supported, and have no plans to natively support, the Microsoft Windows-specific .BMP format. Current supported file formats are PNG, JPEG, GIF and (sortof) XBM.

[changing to enhancement request]

Agreed that this is an enhancement request. The DTD does not specify *which* file formats to support ("widely recognized image formats include GIF, JPEG, and PNG"); this bug is only to point out that IE (on Windows, not Mac OS) supports .BMP files, but no version of Communicator does.

Chris: I'd entended to use making a bmp decoder component the example for a cookbook of how to add decoder components. Its about the easiest format around, and the image decoder problems would be trivial compared to creating the decoder as a component. So for sure, later. -p

Note also related bug 27729, in which attempting to open a .bmp image crashes Seamonkey.

pnunn, have you written anything that would add .bmp support? I don't see any reason why we shouldn't add it. adding myself to cc list.

jce2: oooooo, a volunteer! BMP is the simplest of the simple formats to implement. My schedule is filled right now with reload policy issues and imgcache changes. Enjoy. -P

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

Marking [IBENCH] to ease searching. (Shows up in i-bench test suite.)

I agree that .bmp file support would be pretty cool and pretty easy. Since 99.9% of Netscape users are windows users, this would also be pretty smart. Not having bmp support has bothered me many times, and if IE has it, it probably is not good that Netscape doesn't. You, of course, can't use windows specific APIs to do so, but writing a full decoders for BMPs are a piece of cake. I have done it a million times in my sleep. BTW: Don't forget bmp RLE support.

Reassigning to Brian since he clearly has the experience to do this, wants it implemented, and the current assignee has no intention of spending resources on this enhancement request. pnunn: I hope that's ok with you.

That's fine with me. Changing to M23 for now. I have never made an image decoder for Mozilla before. Perhaps someone can point me to something that explains how.

reassign away! I've got a few other bugs you could take off my hands....... ;> -p

Since the M19+ milestones will be removed, changed to mozilla1.1. Reporter: Since the .BMP format is something that isn't a web standard, and displaying such images is an enhancement request, I am changing the Summary to reflect that. The old summary was the following: "[IBENCH] .BMP images not displayed in Mozilla" I hope this is to your liking. Feel free to change it back. I am not planning to implement this immediately, but rather in a few months, probably. If someone wants to tackle this before that time, then let me know. I hope this takes some of the weight of pnunn's shoulders.

didnt mscott just recently implement the ability for mozilla to view windows system icons? Could this be related? something like a moz-icon: protocol....

I hope this feature makes it in sometime. Even though people aren't supposed to upload files like *.bmp and *.art to the web, many still do, and it would be a shame if I had to close Netscape and start Internet Explorer in order to see the picture one of these problematic web pages is describing.

Its good you added some testcases. Unfortunately, if Mozilla displays these bitmaps it will only be a start. If you look at the spec bitmaps, there are RLE encoded bitmaps, 24bit, 8bit, 4bit, monochrome, cursor files, icons, there are bitmaps that use the BitmapInfoHeader structure and ones that use the BitmapCoreHeader structure. Anyone who wants to develop a test suite of images for every bitmap possibility at multiple sizes for each case would be a great help. If anyone wants to implement this, feel free as I haven't started it yet. Changing os to all since Mozilla would display bitmaps on all oses if it was implemented.

Also notice: Although bitmaps are a bad format to use for the web, there are times when bitmaps come in handy. For instance, if someone wanted to put a bitmap on the internet so that none of the quality was lost in the image. There are also those bimbos who put bitmaps on web pages for whatever reason. Another case is if Mozilla was used to browse the hard drive as a file explorer (http://filezilla.mozdev.org/). For windows systems and linux systems, it would be nice if people could view bitmaps on their systems. Bitmap is not a severely complicated and I don't see why it would hurt for Mozilla to support bitmaps. The only thing I am worried about is putting bitmap support in might encourage people to use bitmaps in their web pages. But if they do, then what the heck - that is their problem. Usually, when I see a bitmap in a webpage I send a letter to the webmaster telling him he should know better. As long as everyone does this, I don't see bitmaps ever taking over the web until the day when compression is not necessary - i.e. never.

"For instance, if someone wanted to put a bitmap on the internet so that none of the quality was lost in the image." 24-bit PNG can accomplish this with much better compression. But sometimes people like to share their Windows wallpaper... BTW, just a thought... in IE, AOL ART image support is downloadable as a separate component, so people who don't want it that badly don't have to download it. Since the common accusation is that Netscape 6 is bloated, maybe something similar to this should be considered for BMP image support.

On Windows, one could probably use the Win32 API to do this pretty easily. I'd take a look, but alas too many bugs to fix. :( But...one can probably take the sample plugin at /mozilla/modules/plugin/test and fix it up so it works for BMP mime types pretty easily. If Mozilla ever gets a plugin download area like Netscape has, it could also put the plugin there for almost seemless download when a user a page that needs it.

Hmmmm.... I wonder if there are already Bitmap plugins for Netscape browsers. There is one thing, though. If we made it optional, then people would probably not know they can add it.

Peter: there is a function in the Win32 API called LoadBitmap or something. Windows does all the work for you. But, back in the days when I did a lot of Windows-specific programming (back in win3.11), there wansn't an API to load bitmaps. You had to do all the work yourself to convert the image from a DIB to a DDB (device dependant bitmap). I am currently working on Autoscroll/Panning bug 22775, and after that, I will write OS-independant non-plugin code for the image library. If someone has experience with image code but doesn't know how to write bitmap code and wants to help, just email me and I will explain how bitmaps work.

"If we made it optional, then people would probably not know they can add it." In IE, I think you can do a full, recommended, minimal, or custom install, and everything except minimal automatically has the ART image support (of course, with IE the bitmap support is built in). This way users will get the ART support unless they specifically don't want it. This is the same category of users who think Netscape 6 is bloated, and the people who don't care will do a full or recommended install and get the bitmap plugin. After all, Netscape 6 already includes a lot of other stuff most people don't want, such as RealPlayer and AOL Messenger...

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

I would like to make a case against adding internal BMP support. Here are some of my reasons: 1. Adds no functionality. Can not do anything that PNG cannot do. 2. Is not necessary for compatibility. Is not a W3C Recommendataion. No serious web authors intentionally use it, or wish to use it. 3. Large file size makes it bad for the web. No compression at all except for 4bpp and 8bpp varieties, and then only a rather poor RLE compression. 4. MSIE already supports it, so if Mozilla does too, it will become another de facto standard image format, and soon every web browser may need to support it in perpetuity. That isn't a decision that should be made lightly, even if it may seem to benefit Mozilla in the short run. 5. BMP is not as simple as it may seem, if you want to support all the flavors of it that are in use. 6. AFAIK, there is no official specification for the BMP format. 7. BMP may not be stable. Microsoft has defined new-fangled V4 and V5 bitmaps, which can be quite complex. It's not clear if they are supposed to be allowed in BMP files (see previous), but it seems plausible that some day they will be used. ---- I have written a BMP plugin for Windows web browsers: http://entropymine.com/jason/webbmp/ Like all plugins, it canNOT be used to add full support for that image type. The reason is very simple: no browser that I know of will ever invoke a plugin for an <img> element -- the web page author must use <embed>. (Well, theoretically one could use <object> for all images, but that's not practical at this time due to generally dismal browser support.) I DO think it ought to be possible for the user to add full support for new image formats using plugins. (Ideally, *all* image formats should be implemented with plugins.) Efforts should be made in this direction, rather than adding internal BMP support. I probably shouldn't mention that I also have a set of BMP test images: http://entropymine.com/jason/bmpsuite/

Of course BMP files aren't good for the web. If only I could get every idiot who posts these blasted things all over the internet to understand that, we wouldn't need support for this. However, as long as there are BMP files out there, and as long as IE supports them, there's a strong argument for including at least basic support for BMP images in Mozilla.

having .bmp support has other effects too right? like the favicon stuff.

If you're talking about what I think you're talking about, favicon.ico files are a special kind of bitmap formatted for use as an icon. And I sincerely hope there are no plans to support favicon.ico in Mozilla.

There is one other problem that you guys haven't thought of (which I mentioned earlier). It is that in order for Mozilla to become a viable File Explorer for Windows, it needs bitmap support. Secondly, .bmp support might not be good for external use (i.e. pages that others will be viewing), but for internal use - i.e. your own page that only you go to on your LAN - a bitmap is a viable option. I have had many times that I have needed this. Bitmaps are the image format of choice for Windows Users for internal use if they don't care about file size and/or if they don't know how to make any other image format. See http://meow.mozdev.org/. Thirdly, there are standards for .bmp and they are in the Windows API Documentation. Support is only needed for bitmaps that are loaded from files (Known as file bitmaps), not memory bitmaps. V4 and V5 are structures used to represent bitmaps with added support for gifs, jpegs, and pngs in memory. I doubt that Windows will ever change the bitmap format to cater to these forms which don't work on every system. Nor do we have to worry about it for now. Yes, there is a bug for favicon implemtation. It is bug 32087 and is assigned to me. Although .ico files aren't true bitmaps, they are very similiar to .bmp files, and can be done in the same implementation. Now, we should try to push people away from using icon files for their favorite icons and to use gifs, pngs, or jpegs instead. Therefore, we will be the ones with full favicon support and IE won't. Also, favicons won't have to be named favicon.ico and won't be searched for on the server. They will have to be linked in. Please see the bug. As for the web getting covered with bitmaps, I don't believe that will ever be a problem. People know better than to use bitmaps, and bitmaps aren't defined in the standard, so people are ill-advised to use bitmaps in their web pages. Whenever I come across a commercial site containing a bitmap, I email the author telling them that they don't have the proper image format on their site. I assume that many other people do the same. Therefore, I don't think you have anything to worry about. As for people with home pages, they have the right to use any image format they please, and they shouldn't have to use Internet Explorer to view the pages. Where do you think all the newbie web designers are going to go. Implementing this will be more of a convenience to Windows user than anything else. I don't see any reason not to support bitmaps as not supporting bitmaps is something Netscape gets bashed about all the time from people who don't know better.

The Windows SDK is rather ambiguous about V4/V5 support. But the link from Jason's site (http://support.microsoft.com/support/kb/articles/q216/2/05.asp) shows that you aren't supposed to save in V4 and V5 format and instead are supposed to load pngs, jpegs, and gifs, (as stated in the SDK) into the V4/V5 structures. Therefore, I do not feel there is any need for V4/V5 support. Jason, although you don't agree about .bmp support in Mozilla, thank you for the test-suite. Can we count on your site being up indefinately? I might extend your bitmap creation code when I start to implement this. I would also like to say that I believe if Internet Explorer for the Mac doesn't support bitmaps and Mozilla does eventually, that would provied for quite a bit of irony. Can anyone verify whether Mac IE supports bitmap?

Mac IE relies on QuickTime for much of its image support, IIRC. I believe - again, if I remember correctly - that QuickTime supports Windows bitmap files as of version 3.0, so Mac IE should in theory be able to deal with the format.

[Concerning my web pages about BMP things] I expect to keep them available indefinitely, assuming I don't get hit by a bus or something. [IE requires Quicktime] is mostly a myth. The problem is that IE is overly willing to allow its own internal support for various formats to be overridden by other program such as Quicktime. (Except when using the <img> element, of course.) IE for Windows is quite capable of displaying BMP images without using Quicktime. ["People know better than to use bitmaps" on the web.] Nonsense. Most people do not even understand the concept of file formats at all, let alone the difference between various *image* file formats. It's not uncommon to find BMP files on web sites renamed to end in .gif -- and then the owner asks why his "GIF files" don't work in Netscape. Fortunately, *because* they don't work in Netscape, he discovers his error and fixes it.

Win32 and UNIX IE of course have nothing to do with QuickTime. The Mac OS version of IE does, however, rely on it at least in part for image rendering. I've since confirmed with the QA lead on Mac IE that they do support BMP files, at least in 5.0 and later.

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

Reading through all the comments was surprisingly useful. if i get the Quicktime plugin will get some bmp support for Mozilla in windows. That's roughly half my problem solved. The reason people put Bitmaps on web pages is because the dont know any better and neither Frontpage nor Internet Explorer give them any warning that they shouldn't. It does not convert them to another format (even an uncompressed gif would be better) like the way Netscape Composer does/did.

What is going on with this bug? There has been no activity. I think that not supporting a simple format like bmp is a big problem. Why don't we change the "Target Milestone" to be Mozilla 1.0? I don't think this would be a huge burden. Right now it is set to Mozilla 1.1 and I think that this is a big problem especially with the penetration of favicons.

burton: Per comments, I will add a mozilla1.0 request. Note that favicons are probably a completely separate discussion from this.

Removing URL since it is a dead link. There is a simplified testcase for this bug.

If I read these comments correctly, Brian would like to implement it, but doesn't have time for it at the moment. In this case, I'd like to implement this feature. I will probably not be able to do RLE compression, though.

OK, I've started work on this now, and some BMPs already display. This patch contains the changes to the existing files. It will probably not change. I will attach a tarball of the added files, unpack it in the mozilla/ directory. Please note that these files are work-in-progress, they do not work for all uncompressed images, let alone compressed ones. But some are displayed fine :) Oh, and NeTDeMoN, feel free to assign this bug on me, as I'm currently working on it.

> This patch contains the changes to the existing files. > It will probably not change. You are *THE MAN* ! > I will attach a tarball of the added files, unpack it in the > mozilla/ directory. Please note that these files are work-in-progress, > they do not work for all uncompressed images, let alone compressed ones. > But some are displayed fine :) Was this patch against HEAD from CVS? How should I apply the patch. I will try testing it out with my application. It pulls in bitmaps from a ton of places. The least I can do is give feedback.

You can apply the patch by entering "patch -p0 < filename_of_patchfile" in the Mozilla Source Directory. I haven't tested this under Linux yet, so some feedback how it works there is nice.

And about the tarball: Create a directory moduled/libpr0n/decoders/bmp and untar the tarball there (tar xzf filename.tar.gz)

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.

<biesi> Hm... W/o RLE, I'm probably finished by 0.9.5. With, hm, 0.9.6 is more likely cool

OK, this version should display almost all uncompressed BMPs. Known problems: 1) The first line is not decoded, a grey line appears instead (I don't know yet why) 2) Doesn't display OS/2 Bitmaps 3) It was only tested under Windows To use it, apply "Patch: Part 1", then untar this attachment in the mozilla/ directory.

I don't think this is ready for review at the moment.

If you want this to work for larger files, change line 243 in modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp from if (mPos == mBFH.dataoffset) { to if (mPos >= mBFH.dataoffset) {

I must say that i'm opposed teaching moz to handle BMP. BMP is a really horrible format so IMHO we should not encourage people to use it. I don't think the argument "not many people uses moz anyway" really holds because 1) if nobody will use moz then why are we developing it? and 2) lots of people are using moz if you count all browsers derived from the moz codebase. The fact that only IE support BMP has kept it off the web, however if both NS and IE supports it there is a really big risk that developers will start using it more often. If we contact authors and say "you shouldn't use BMP becuase it dosn't work in mozilla browsers such as NS" they are far more likely to listen then if we say "you shouldn't use BMP becuase it's a BadThing, but it will work in most browsers"...

I disagree with you. 1. It's the webmaster's decision what he/she puts on his homepage. If this is a BMP, well, then so be it. You could argue that GIF is a bad format, because of this Unisys patent they're now enforcing. However, you don't suggest removing GIF support. Why is it a bad format? Because it's lossless? Because there's no patent whatsoever on it? Or because there's not really a compression, besides RLE? You seem to think that many people would put BMPs on the web, if just Netscape would support them. I seriously doubt that. There is a wish that BMPs are supported. As a Netscape employee reported this bug, apparently this company wants it. Your reason for not wanting to include this is just: "This makes people put more BMPs on the web", correct? BTW, this tarball is the latest and greatest version of the patch, fixes some problems. The first line is not just a grey line anymore, but the correct content. It works for every image in the BMP Testsuite by Jason Summers, except the compressed ones, the 16bit and 32 bit ones, and the OS/2 one

> The fact that only IE support BMP has kept it off the web, however if both NS > and IE supports it there is a really big risk that developers will start using > it more often. No where in the HTML specification does it say "don't support BMP" "these are the supported image formats". BMP is a very popular format and people are using it. Therefore we *have* to support it. Of course if we want to take your non-pragmatic approach, we should also drop GIF support because of the UNISYS patent. I don't see that happening anytime soon. When a user double clicks on a BMP file and Mozilla can't handle it - it makes Moz look bad. Especially when IE can handle it just fine.

> Why is it a bad format? Because it's lossless? Because there's no patent > whatsoever on it? Or because there's not really a compression, besides RLE? Because it has lousy compression. > > The fact that only IE support BMP has kept it off the web, however if both > > NS and IE supports it there is a really big risk that developers will start > > using it more often. > No where in the HTML specification does it say "don't support BMP" "these are > the supported image formats". BMP is a very popular format and people are > using it. Therefore we *have* to support it. BMP is *not* a "popular format" other then on windows, and even there I wouldn't call it popular among people dealing with graphics. We don't *have* to support BMP, we've done just fine without it for several years. > Of course if we want to take your non-pragmatic approach, we should also drop > GIF support because of the UNISYS patent. I don't see that happening anytime > soon. However GIF is a format we *have* to support since there is so many sites depending on it, if there wasn't I don't think we should have added support for it no. IMHO there are the exact same arguments for adding support for the IE extension document.all, desition was taken some time ago that we should *not* support that but rather go for other better alternatives. And in that case there were much stronger arguments for support then there is for supporting BMP, more sites depending on it and no other cross-browser alternatives. So, should we add support for that too? Remember that once support for something is added in mozilla we are pretty much stuck with it for all eternity

End users will be disappointed if an image displays in MSIE and doesn't display in Mozilla. I personally think it's more important to prevent situations where pages don't work than to worry about being the superhero that squashed BMP images on the web. I don't expect any commercial or otherwise highly public website to start using BMP just because two browsers support it (in fact, if they know what they're doing, they're still desigining their pages to work in Netscape 4). The newbies are the ones who do this, and they won't listen to complaints about their images not working in Mozilla any sooner than they'll listen to complaints about BMP being a bad format for the web.

So shoule we add support for document.all and document.layers becuase "users will be dissapointed if a page that works in IE/NS4 doesn't work in mozilla" ? And again: when adding something to the mozilla tree it's not just added to the mozilla browser, it's added to all browsers derived from mozilla, so yes, it makes a difference

> BMP is *not* a "popular format" other then on windows, and even there I > wouldn't call it popular among people dealing with graphics. We don't *have* > > to support BMP, we've done just fine without it for several years. I am sorry but Windows has 95% of the market. This makes BMP a popular format! I could also make the analogy that "Air is *not* 'popular format' other then with people that are living". :) > However GIF is a format we *have* to support since there is so many sites > depending on it, if there wasn't I don't think we should have added support > for it no. Sorry. I think that the UNISYS patent issue *clearly* outweighs your arguments against BMP. If we are going to drop image formats I believe it should be in this order... gif then bmp. If you want to drop bmp then fine but you should first acknowledge that we should drop gif. (BTW I am not advocating doing either) > Remember that once support for something is added in mozilla we are pretty > much stuck with it for all eternity Let's not be dramatic. No we don't have to support anything "for all eternity". If there is a reason to remove BMP in the future, we will do so. BTW. I am not a Windows supporter. In fact I haven't used Windows for years. I only use Free Software. I just don't want to give the impression that I am a windows user/developer who think that BMP should be supported. The reason that I need BMP support is that I am writing a P2P application using Mozilla and some people are syndicating BMP files. These usually come from Windows platforms and I need to view them under Linux within Mozilla. I think this is a clear example of why we need BMP. We need to be format agnostic. Ideally Mozilla should be able to support everything including png, gif, bmp and also multimedia formats. My $.02.

Jonas: document.all and document.layers are DHTML. DHTML is not common enough or important enough to the functionality of websites to worry about it. Authors who understand DHTML also typically understand the cross-browser issues involved. BMP images can be placed on the web by anyone, particularly newbies. If it doesn't work, images are lost and websites are difficult or impossible to use. And there is already a patch in progress. There's no good reason to start a crusade against fixing this bug now that it's already almost fixed.

This latest patch is a diff against the latest CVS. It adds BMP to some lists that I've previously forgotten. Pavlov, please review it when you have time.

missing from the 9/15 patch is change to "xpinstall/packager/packages-mac" add something like: viewer:Components:bmpdecoder.shlb

This works neither on Linux nor on Mac. Linux: No Image is displayed, Browser window is completely white Mac: Titlebar displays "Image 1x1" or similar and consequently only displays one pixel. I'll try to fix this on Linux, but I've got no idea why it doesn't work.

Comment on attachment 49443 [details] [diff] [review] Patch: Part 1, second revision Pavlov gave me r=pavlov on IRC

In the part1 of the patch in packages-unix, libbmp.so must be changed to libimgbmp.so

The REQUIRES Line is wrong, should be: REQUIRES = xpcom \ necko \ gfx \ gfx2 \ imglib2 \ string \ $(NULL) And: I finally found the problem on Linux! It was: SetImageData should be called with bpr as second argument, not width*3; so the next version of the patch will work on Linux as well as on Windows.

Removing helpwanted and review keyword.

is this patch ready for review? :)

basic: No, at least not the version that is attached to this bug. What's not working in the latest version (not attached to this bug) is this: x) Support for compressed BMPs, including those using Bitfields x) Support for OS/2 BMPs And last time it was tested on Mac, it wasn't working even though it should have. Linux support is not optimal. I would like to fix at least the Linux Issue that I currently have before checking in the first version; maybe also Bitfields.

OK, this fixes the following problems from the last tarball: o) Doesn't work on Mac. It should work fine now. o) Doesn't work on Linux. Both Linux and Mac have problems opening a local file. Loading one from network is fine, as is loading via <img src="">, at least on Linux (untested on mac). o) REQUIRES Line is wrong. I corrected it. o) Maybe some other small fixes that I forgot To use it, a rather recent tree is needed because of a change to the image decoder interface which happened yesterday.

Comment on attachment 49443 [details] [diff] [review] Patch: Part 1, second revision needs work; mac packages file needs updating as well as mac build list

OK, the patch I just attached corrects the filename in xpinstall/packager/packages-unix and the mac files.

Comment on attachment 53573 [details] [diff] [review] Patch: Part 1, third revision Will attach a new version, which makes it possible to directly open local files. Also, will change the image/bmp part of the Accept: Header to image/bmp;q=0.1 (to indicate that we'd rather want another format if possible). Pavlov: Since your last review, Mac build was also fixed; is your r= still valid? If no, could you please re-review?

yeah, r=pavlov

Comment on attachment 53742 [details] [diff] [review] Patch: Part 1, fourth (final?) revision Pavlov told me r=pavlov on IRC

Excellent work here, Christian. If you don't mind, I'm going to take this bug, get it compiling with the current trunk, and help shepherd this patch into the tree. I'm going to be leveraging this code to produce an ICO decoder.

Comment on attachment 53742 [details] [diff] [review] Patch: Part 1, fourth (final?) revision Obsoleting the last patch. A new one is coming that compiles on the trunk.

Comment on attachment 53327 [details] Latest tarball of new files. untar into modules/libpr0n/decoders/bmp Hyatt's patch includes the new files

Note, I will fix the ENDIAN problems on the sniffing code. I just need to ensure that the 2-byte PRUint16 is converted using the LITTLE_ENDIAN macros. Ready for r/sr.

a few comments: you are added the bmp stuff to the mac build list.. do you have a mac project file? if not, you will break the tree... just comment it back out :) in the http accept header string, you should move icons to after bmp and with a lower q level than them. 0.1 for both bmp and icon should be fine. we should check, at some point, to make sure that only checking for 01 for icons is good enough sniffing.. fix the first 2 and r=pavlov

Comment on attachment 55905 [details] [diff] [review] I'll see your BMP and raise you an ICO. sr=ben@netscape.com

[jeez, I wish we could segregate the advocacy debate] + Close(); /* XXX Is this necessary? No. Delete it. */ Fix this or fix the comment please. +NS_IMETHODIMP nsBMPDecoder::Flush() +{ + PR_LOG(gBMPLog, PR_LOG_DEBUG, ("nsBMPDecoder::Flush()\n")); + // Pavlov said this: This function is called at the end, and here I + // can process whatever was left over from previous calls to Write* + return NS_ERROR_NOT_IMPLEMENTED; +} Should return NS_OK, no? Why make the caller think there was an error? (and how the *current* caller actually deal with the error is no excuse). +#ifdef XP_MAC + *aDecoded++ = 0; // Mac needs this padding byte +#endif Is this a Mac issue or an Endian issue? Ah, I see you are exlicitly making triplets for non-Mac and quads for Mac. I trust this follows imglib rules. Making SetPixel function calls in the main loop is butt-ugly. *please* use a macro (or inline function if you trust the compiler that far) to get these expanded inline. Also, you are *far* better off having separate inner loops rather than switching on (the unchanging) mBIH.bpp for each and every pixel. Doing that switch once per row is probably acceptable. Does someone want to talk about the testing that has been done on this code? On which platforms? On what variety of image sizes and pixel depth? This is just not going to get the level of real world testing from daily builds that the other formats get. We need to catch the problems early.

Comment on attachment 55905 [details] [diff] [review] I'll see your BMP and raise you an ICO. --sr kick me if this is rude, but I think you need to fix some stuff.

Comment on attachment 55905 [details] [diff] [review] I'll see your BMP and raise you an ICO. --r=pavlov.. lets see a new patch with the things that jband suggested... some linux testing would be good too.

Just tested ICO support on Linux, seems to work fine.

http://www.ordsvy.gov.uk/wallpapers/archive2000.htm I loaded a bunch of huge remote bitmaps from this site. They worked fine and rendered incrementally. In addition I've loaded about 50-60 bitmaps from my local machine without trouble. I've loaded 10-15 ico files remotely and they load fine. Ready for r/sr, pav and jband, on my revised patch.

I take it back. I do see problems with some of the BMPs on the above URL. The colors appear to be wrong. I will debug further.

(arg, hyatt could have added me to cc when he took this bug) This code was tested on Windows, Linux and the Mac. It works for all the images in the bmpsuite, except the RLE and Bitfield ones (and in this version, OS/2 ones). The bmpsuite is available from http://entropymine.com/jason/bmpsuite/.

Sorry about that, Christian. I assumed assignees became cc'ed automatically. Anyway, I'm pleased to report that the BMP I thought was malfunctioning is fine. biesi and I spoke on IRC, and he wants to clean up the BMP code a bit more before we land, so I'll wait for his revised patch before we do another round of r/sr.

Comment on attachment 56066 [details] [diff] [review] Diff of the revised BMP/ICO decoders with jband's changes made. I don't see any changes to build/mac/build_scripts/MozillaBuildList.pm so the Mac bmp project won't build. The line in that file for building the Mac project should be uncommented (it is already in the tree). Do we need a separate project for ICOs? Please correct these comments before checking in: + * Additionally, this patch was only tested on Windows and Linux. + * It won't work on Mac because Mac Project files are missing at the moment */

brade: I think we don't need a new project for bmp; however, the file nsICODecoder.cpp must be added to the BMP project file. (it's in the same directory as nsBMPDecoder.cpp)

OK, brade kindly added nsICODecoder.cpp to the Mac Project file (pavlov: yes, it was already there); so this should build on the Mac.

Brade, attachment 56066 [details] [diff] [review] is just a partial diff, since none of the other files changed. If you look at the previous diff, it includes changes to add the BMP Mac project file to the build. Christian, do you want us to go ahead and get r/sr again and land this thing, or do you want to make more changes to the BMP code first?

hyatt: I'm working on an updated patch right now I hope to have it done in <= 2 hours

As Guardian of the Accept: header, your accept changes are bogus. You've put image/x-icon in there with no q value, so it has the default q of 1.0, and image/bmp with the same q as */*, meaning you might as well not have it at all. Given that there's approximately zero cases in which we'd prefer BMP to some other format, there is no gain to be had here, as they can be incorporated into */*. Making this change would bloat every GET request we make by 31 bytes. We are already getting complaints that our GETs are too big. Please leave the Accept: header as it is now. Also, just a reminder: if any of the files with the MPL on are completely new and not derivatives, could they be tri-licensed, please? Gerv

<rereads> Please? :-) Gerv

So just remove the changes to the pref completely?

OK, this is a complete diff (ie. includes changed files). I think it addresses all the comments which hyatt's earlier patches didn't (also yours, Gerv; I removed the change to that header). Note that I haven't changed nsICODecoder.cpp; I know too little about the ICO format. This patch also adds support for OS/2 BMPs.

r/sr, folks?

Some initial thoughts on looking over the patch: * ICO decoder - the gtk alpha optimizations need SetAlpha() to be called before SetData() on the corresponding row. Behavior is unspecified otherwise. * eyeballing the patch there appear to be large chunks of copied code in the two decoders. Maybe they could be pulled into some utility functions for sanity's sake when fixing bugs? Or if the formats are really almost identical maybe just one decoder with a little more smarts might be cleaner. * magic constants floating in the code - please make up appropriately named #defines to help others in understanding the code. A few examples: + if (mPos >= 6 + (mCurrIcon*sizeof(mDirEntryArray)) && + mPos < 6 + ((mCurrIcon+1)*sizeof(mDirEntryArray))) { + if (mPos == 22+mCurrIcon*sizeof(mDirEntryArray)) { + if (mPos == mImageOffset + 40) {

* ICO decoder - the gtk alpha optimizations need SetAlpha() to be called before SetData() on the corresponding row. Behavior is unspecified otherwise. The icons seem to be properly working on Linux though.

Comment on attachment 56173 [details] [diff] [review] Hopefully final patch sr=jband I'm not happy with all the magic numbers either. But, I'm not so sure a bunch of #defines would make it more readable and I imagine it would increase the chance for coding error.

Comment on attachment 56173 [details] [diff] [review] Hopefully final patch r=pavlov I'm with jband on the magic numbers.. not sure defines would help.. tor: why would SetAlpha have to be called before SetData? That is pretty bogus. I don't believe that we can or should impose that restriction on current or future decoders.

pav: we need the alpha data to be valid before nsImageGTK::ImageUpdated() called.

--sr: I would like at least the latter two concerns (duplicate code, magic constants) addressed before checkin, as we don't want unmaintainable code going into the tree. hyatt: if ImageUpdated is called before the alpha data is set things will often work out fine because there will be uninitialized garbage in the alpha channel. If you get unlucky you'll get a zeroed chunk of memory and the image will be optimized away. This was happening occasionally on the UI gifs until the ordering in that decoder was changed.

Last time I tested this patch on Mac, I was unable to open local *.bmp files. Has this issue been addressed yet? I'd prefer that this patch not be checked in until that issue is also addressed.

brade: This issue should be fixed now; however, it wasn't tested on the mac yet.

OK this smells of a fillibuster here... please folks let this get checked in and post bugs on the remaining issues...

I don't think it's a filibuster if someone brings new information before others and proposes a fix that's easily made (why wait? sequel bugs sometimes don't get fixed). Is that not the case? /be

Yep. My bad. Better to have it work right the first time.

I filed bug 108271 for supporting compressed BMPs and those using bitfields.

I have added the magic numbers. I'm going to hold off on code sharing and land this now and file a separate bug for the alpha problem. The reason for this is that I'm going to rewrite the ICO decoder to be non-incremental, and then I'll ensure that alpha info is set up before pixel info. By the time I do that, the ICO code and BMP code will overlap only minimally, so I'm not going to waste time worrying about code overlap now. This bug is 108318. I am also not going to enable this code on the Macintosh until it has been tested per brade's request. I will not be checking in the modifications to the build list, nor will I be checking in a Mac project file.

Fixed.

Bug 108320 covers enabling the BMP and ICO decoders on the Mac.

I was able to view .ico files in todays mac build. Seems to work fine!

I'm seeing black garbage on the top and left edges of both inline BMP images in attachment 36229 [details]. The effect seems amplified on the stretched one. Feel free to split off another bug for that if you think it's necessary. However, both tests in that testcase pass for me otherwise, 2001110703 W98. The format of that testcase is 256 color bitmap, saved in MS Paint. As soon as I find a BMP image that doesn't load into Adobe Photoshop I will check it in the browser, since that seems like a potential common weakness. Always nice to see RFEs get fixed.

After looking more closely, I think the black garbage may be connected with the image placeholder not being properly cleared. The black garbage goes away if the pictures are loaded from cache.

Okay, this is fixed on Win XP build 2001120303 and Linux build 2001120308 and I am able to view these files on Mac OS 92001-12-03-11-trunk and X 2001-12-03-08-trunk but only if I drag and drop the files, trying to open directly (File->Open file) the files are greyed out, I'll mark nanny this verified and open a new bug

For new people here: This bug is fixed. Mozilla can now display .bmp and .ico files. Although most cases of bitmaps are displayed correctly, there are a few that won't display correctly. As of now, all active bitmap and ico bugs are: bug 110835, bug 108271, bug 110848, bug 108318 and bug 113345. Please correct me if I forgot any. Any bitmap problems not covered in these bugs should have a new bug added for them.

remove self