8031 - [FIX] XBM images not [yet] displayed

Status: VERIFIED FIXED

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

Description

[Eli Goldberg]

* TITLE/SUMMARY
XBM images not [yet] displayed

(functionality not yet implemented; writing up for tracking purposes.)

* STEPS TO REPRODUCE
0) Launch Apprunner [checked 6.9.99 Win32, Linux & Mac OS builds]
1) View:
	a. http://slip/projects/marvin/imaging/img-xbm/img-xbm.html
	b. http://slip/projects/marvin/imaging/img-xbm/fancyclock.xbm

* RESULT
 - What happened
	a. Nothing displayed
	b. Nothing displayed

 - What was expected
	a. The web page displayed should contain a black & white fancy bitmap image
of a clock, followed below by a black and white bitmap image of a penguin. (This
is currently broken in 4.6, BTW, as checked on Mac OS.)

	b. A 1-bit clock bitmap.

	[XBM will be supported natively --- well, via an added library that hasn't
yet been added in --- this should work before 5.0 ships. Bug noted for tracking
purposes; the functionality is simply not yet implemented.]

* CONFIGURATIONS TESTED

- [Mac] Power Mac 8500/120 (233 Mhz 604e), 64 MB RAM (VM on; 1 MB of VM used),
1024x768 (Thousands of Colors), Mac OS 8.6

- [Win32] Vectra VL (233 Mhz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP3.

- [Linux] Vectra VL (266 Mhz P2), 96 MB RAM.

Comment 1

[pnunn]

XBM format will be used as an example for users adding
decoders in mozilla/extensions....later.

Comment 2

[pnunn]

later, later, later.

Comment 3

[Suresh Duddi (gone)]

M13 is beta. I dont think this is happening for beta.

Comment 4

[Matthew Tuck [:CodeMachine]]

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

Comment 5

[Eli Goldberg]

This was resolved as LATER without comment. Does that mean that XBM format
support will not in fact be needed by any 5.0 components?

[e-mailed pnunn to request confirmation.]

Comment 6

[Eli Goldberg]

pnunn confirmed by e-mail that XBM won't be supported in 5.0. Need to remember to
revise test plans and nuke test cases as appropriate...

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[timeless]

reopen. later went out of style.

Comment 9

[timeless]

QA: can you publish the test urls for mozilla.org? thanks.

Comment 10

[tor]

XBM is no longer used when creating web pages and is not a w3c recommended
format.  Let them die in peace...

Comment 11

[pnunn]

timeless, if you want xbm support, go for it.
Make a decoder component.
Seems you might already have xbm files yourself for
test....but if you don't, google search them.
-p

Comment 12

[Matthew Tuck [:CodeMachine]]

To my knowledge PNG is the only W3C format.  I believe we have MozClassic code
around.  And there are pages that use them (see bug #19120).  Do we want to
specifically not put in XBM?

Comment 13

[pnunn]

The imglib decoder component design was intented to make
putting in new image format decoders easy. There are some
glitches where other modules are hard coding image format
lists. With some work, these small barriers should be
gone fairly soon.

The point is, if someone wants XBM, or their own private 
secret 007 image format, they should be able to drop in their
component, add their mimetype/file extension mapping to the
list and **presto**! View their secret 007 images.

I haven't yet run into anyone who wants XBM image support enough
to actually do the work.

-P

Comment 14

[R.K.Aa.]

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

Comment 15

[Terri Preston]

Verified

Comment 16

[timeless]

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

Comment 17

[[e-mail]]

Reopening to assign to biesi

Comment 18

[[e-mail]]

Found a volunteer!

Comment 19

[Jay Farrell]

Just wanted to note that I ran into this problem on OpenVMS. The reason I was
trying to open an XBM file was that it's part of the Front-End tests in Cut And
Paste. The test name is "Copy-img". (running on a build from 20011205)

If this isn't going to be fixed, and as some of the comments suggest that not
many people will use this, then maybe the Mozilla Front-End test suite should
be modified. (Just a thought :-) Added Terri Preston to the Cc just for this
reason. (Hope that's OK!) 

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

[e-mail]: I am working on this bug.
I expect to have it done for 0.9.8 (as the Target Milestone field indicates).

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Current Status:
The XBM Decoder is working in my tree, I will clean it up a bit and attach it to
this bug.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix Part 1: Patch for existing files

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix Part 2: .tar.gz of new files

Create a directory modules/libpr0n/decoders/xbm and untar this file there

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Pavlov, could you review the two attachments? tia

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Correct Makefile.in

The .tar.gz contains a wrong Makefile.in file. This one is the correct one.
(the file didn't work on Windows)

Comment 26

[basic]

status?

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

The latest news I have is: pavlov doesn't think XBM support is
wanted/necessary/a good thing, but he'd think about it. That was a few weeks ago.

Comment 28

[basic]

 4 votes for this
 8 cc's besides pavlov
 most of the comments here seem to imply that people want this.
 [e-mail] mention that he would prefer that it not be implemented.
 there is a patch

If XBM is not wanted, why is there so much interest?

Comment 29

[Daniel Brooks [:db48x]]

why not fix it at this point? after all, someone's already done all the work, so
it's not like it'll take much of anyone's time.

Comment 30

[Roland Mainz]

Since Mozilla supports the Windows bitmap format, it sure seems logical that it
should support the X11 bitmap format (if we do not implement XBM - why do we
need Windows BMP support (it isn't a W3C recommendation either) ? =:-) ... :)

I would say: Implement it - it won't hurt and makes us a little bit more
compatible to NS4.x ... and at least the module can be used as example source
for further image decoder work...

Comment 31

[Eric A. Meyer (dead account)]

I'd like to voice my support for adding this patch, and have voted on the bug
mostly because a patch exists.  I've come across a site recently that displays
data in XBM format: http://www.2spi.com/ -- go to a product page and notice the
lack of prices in Mozilla.  So there's at least one real-world case of someone
using XBM, regardless of whether it makes sense to us.  Let's add this bad boy.

Comment 32

[Rupert Kolb]

There is a perl script "counter.pl". It displays the access numbers for a page
as a xbm image. The counter is far from being exact. But people get an idea of
the popularity of a page. Many people use it for their pages. Nearly all
browsers, except mozilla, can render the xbm images :-(

There are a lot of things which are not w3c recommended, but which are a *must*
for a real working browser. XBM rendering is one of them ;-)

Comment 33

[Rechtman]

I would like to add to the above comment that such use is not limited to perl.
(My personal preference - DCL on OpenVMS - but there are others) Page access
counts, displaying parameters without recourse to SSI, etc.

Comment 34

[basic]

from what I hear is that IE supports this too...

Comment 35

[Eric A. Meyer (dead account)]

Yes, it does.  As does Opera, according to my tests.  We really, really, really
ought to add this patch for Moz1.0.  I'm seriously considering a nomination for
topembed just to get it on enough radars to have the patch reviewed and
approved.  Hopefully someone will take those steps before I do.

Comment 36

[Eric A. Meyer (dead account)]

From this morning's devsupport e-mail:

>I am the site developer of the webpage I am trying to access. 
>I'm using a standard cgi hit counter provided by the domain host, 
>EarthLink, and it works fine with both IE and Opera and not at all 
>with Netscape.
Said hit counter is spitting out XBM images.  Earthlink has a lot of customers.
 We have a fix ready for review and inclusion, and seven votes for the bugs. 
I'm nominating topembed and nsbeta1.

Comment 37

[saari (gone)]

topembed- since it isn't blocking an embeddor, but marked embed since it would
be nice to have

Comment 38

[Henrik Gemal]

just replacing http://slip/projects/marvin/imaging/img-xbm/img-xbm.html with a
public available site

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix Part 1 version 2

un-bitrotted patch
includes patches for mac build files - I will take care of getting a project
file too before this is checked in

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix part 2 version 2

new version of the tarball with the new files; only changes are the inclusion
of the corrected Makefile.in (attachment 63315 [details] [diff] [review]) and the renaming of
Makefile.win->makefile.win

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

arg, I really should compile patches before attaching them...
In imgLoader.cpp:
    618     aContentType = nsCRT::strndup("image/x-xbitmap", 15);

A * is missing before aContentType, I'll of course add it.

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

/me hides

same file, one line below: return; -> return NS_OK;

Comment 43

[[e-mail]]

Comment on attachment 79791 [details] [diff] [review]
Fix Part 1 version 2

In makefile.win:

-DIRS = ppm gif png jpeg icon\win icon bmp mng
+DIRS = ppm gif jpeg icon\win icon bmp mng xbm

This looks as if you're losing png support!

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

er, oops, looks like I didn't change this back when I was testing using the MNG
decoder for PNGs instead of the PNG one. I will of course change this back.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

brade, please create a mac project file for the XBM decoder - the patch here
should already include the needed changes to the mac build files

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix Part 1 version 3

fixes the three issues mentioned above - compiles now and doesn't remove png
decoder when building on windows using nmake

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 79792 [details]
Fix part 2 version 2

I'll rewrite this to not use mozilla's strings & iterators, for speed and
efficiency of the code and for being able to read it :)

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Fix Part 2 version 3

New .tar.gz of the new files; not using nsCString + iterators anymore.

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 80587 [details]
Fix Part 2 version 3

should've tested this on large images earlier... because on such images only
the first few lines are displayed

Comment 50

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: hopefully final version

new tarball of the files for modules/libpr0n/decoders/xbm

Comment 51

[tor]

>        const int kDefineLen = sizeof(kDefineText) - 1;

strlen(kDefineText);

>        define1Pos += kDefineLen;
>        define1Pos = strchr(define1Pos, ' ');
>        if (!define1Pos)
>            return NS_ERROR_FAILURE;
>
>        mWidth = atoi(define1Pos);

if (sscanf(define1Pos, "#define %*s %d", &mWidth) != 1)
    return NS_ERROR_FAILURE;

>        const PRUint32 count = dataStart - mBuf;
>        memmove(mRow, dataStart, count);
>        mBufSize -= count;

Was this supposed to be memmove(mBuf, dataStart, count)?
While on the subject, you seem to be parsing by repeatedly
shifting the string with memmove.  For the size of the files
it's probably not worth doing all that work - just buffer the
whole thing and track your current position.

>            // Assuming values are always hexadecimal
>            PRUint32 pixel = strtoul(start, nsnull, 16);

strtoul(start, nsnull, 0) would eliminate that assumption.

>        mCurRow = 1;

zero-base, please.

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

>>        const int kDefineLen = sizeof(kDefineText) - 1;
>strlen(kDefineText);

Hm, you are sure the strlen will be evaluated at compile time?
Though, with your next comment that seems to be unnecessary anyway.

>>        const PRUint32 count = dataStart - mBuf;
>>        memmove(mRow, dataStart, count);
>>        mBufSize -= count;
>Was this supposed to be memmove(mBuf, dataStart, count)?

Eh, oops, indeed - I wonder why this worked?

>While on the subject, you seem to be parsing by repeatedly
>shifting the string with memmove.  For the size of the files
>it's probably not worth doing all that work - just buffer the
>whole thing and track your current position.

OK... might be indeed a better idea.

I'll attach a new patch with your suggestions.

Comment 53

[Michael Dunn]

Adjusting KWs 
Topembed nomination: INternal Reference: 
http://bugscape.netscape.com/show_bug.cgi?id=12494

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: another try

addressed tor's comments

Comment 55

[tor]

Attachment: suggested alterations to biesi's patch

Untested (uncompiled, even) patch which changes the parsing somewhat
and tries to keep the image even if we run into problems in the file.

Comment 56

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: yet another tarball

version with tor's patch applied & fixed

I'll attach a patch against my previous tarball too

Comment 57

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch against attachment 82823 ("another try")

the changes as patch; result is the same as attachment 83493 [details]

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

tm->1.1alpha since this will probably not get approval for 1.0, even if it gets
review and super-review in time

Comment 59

[Winnie Lam]

topembed+ since this impacts embedding customers, according to Mike's comment #53.

Comment 60

[tor]

Comment on attachment 83493 [details]
yet another tarball

return for sscanf failure should be NS_OK (partial header possible).  Change
that and sr=tor.

Comment 61

[tor]

Comment on attachment 80218 [details] [diff] [review]
Fix Part 1 version 3

sr=tor

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 83493 [details]
yet another tarball

changing the sr to r

Comment 63

[Kathleen :Brade]

Attachment: mac xml file (sorry I couldn't easily create a patch for this)

Comment 64

[Kathleen :Brade]

Attachment: patch to fix mac build list

This is needed after attachment 84800 [details] (or updated #) is checked in.

Comment 65

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84801 [details] [diff] [review]
patch to fix mac build list

marking obsolete since attachment 80218 [details] [diff] [review] already contains this

Comment 66

[Daniel Veditz [:dveditz]]

Comment on attachment 83493 [details]
yet another tarball

sr=dveditz based on tor's OK

Comment 67

[Daniel Veditz [:dveditz]]

Comment on attachment 80218 [details] [diff] [review]
Fix Part 1 version 3

sr=dveditz

Comment 68

[Brendan Eich [:brendan]]

Cool -- does
http://gaia.ecs.csus.edu/~dsmith/csc121/lecture_notes/wk14/doodlepad/doodlepad.html
work in Mozilla with this patch?  If not, file a sequel bug or fix in a revised
patch here.

/be

Comment 69

[Christian :Biesinger (don't email me, ping me on IRC)]

Bug 148967 filed.

Checked in this one to trunk.

Finally marking FIXED.

Comment 70

[Jaime Rodriguez, Jr.]

Teri - Can you pls verify this on the trunk? Thanks!

Comment 71

[Terri Preston]

Verified fixed win XP trunk build 2002060408, Mac OS X trunk build 2002060403
and linux trunk build 2002060404

Comment 72

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 80218 [details] [diff] [review]
Fix Part 1 version 3

>+    *aContentType = nsCRT::strndup("image/x-xbitmap", 15);
>+    return NS_OK;

Was this checked in like this? I think you'd want to check for 
out of memory condition and return error if so.

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

heikki, yes, it was - the rest of the function doesn't check for null either, so
I didn't do it here.

Comment 74

[Brendan Eich [:brendan]]

Checking for malloc failure is still a good thing in some subsystems (the JS
engine, which embeds in tiny systems that run out of memory often, must cope in
all known cases -- and thanks to [e-mail], it does), but I bet
Mozilla falls over left and right.

Still, worth the effort?  Be consistent, if you fix this.

/be

Comment 75

[Judson Valeski]

please land on the 1.0.1 branch. once there remove the "mozilla1.0.1+" keyword
and add the "fixed1.0.1" keyword.

Comment 76

[Judson Valeski]

the issue brendan raises is a good one. it can be treated independently of this
bug however (when in rome...). 

Comment 77

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, I filed bug 150142 for checking for malloc failure in that function

Comment 78

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on branch

Comment 79

[Terri Preston]

This is fixed in Win XP branch 2002061108 but not in Mac OS branch build
2002061105  or linux branch 2002061110

Comment 80

[Christian :Biesinger (don't email me, ping me on IRC)]

tpreston, that surprises me. can you try deleting component.reg and check again?

Comment 81

[Christian :Biesinger (don't email me, ping me on IRC)]

hmmm. you're right. libimgxbm.so is missing (on linux at least).
investigating.

Comment 82

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, I missed a file when I was checking in apparently... can you try again when
newer builds are available? thanks

Comment 83

[Terri Preston]

Okay, now verified on Mac OS branch build 2002061805 and linux branch build
2002061807

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

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