Closed Bug 41333 Opened 25 years ago Closed 22 years ago

Adding a new image type is way too complicated (architecture problem)


(Core :: Graphics: ImageLib, defect, P3)






(Reporter: tor, Assigned: pavlov)



(Keywords: arch, helpwanted, Whiteboard: [imglib])


(3 obsolete files)

This is more of a general mozilla architecture problem, but mostly
related to libimg.  Feel free to add more people to the CC list as

I'm working on adding a new image type to mozilla, and it turns out
that this is much more involved than just creating a new image decoder
component.  So far the chunks I need to modify are core libimg, layout,
necko, and mailnews.  These areas should just call into libimg to figure
out if a particular mimetype is supported instead of duplicating the
I didn't go into it in the last email, but
the changes in mime sniffing in imglib(and the
other changes in necko, layout, & mailnews) should not
be needed.

The problem is in the mapping from the server sent
content type and necko's interpreting the content type
as belonging to an imagelib component. As long as the
image decoder component is placed in the bin/components
directory (or whatever is the correct place for the
specific platform) it should be able to find the correct
Target Milestone: --- → M17
Blocks: mng
Here's an inventory of the places in mozilla that hardcode the image formats
supported by mozilla:


   nsDSURIContentListener::CanHandleContent() has a hardcoded list
   of mimetypes that is willing to be the primary handler for.


   GtkMozEmbedChrome::IsPreferred() ditto.


   nsWBURIContentListener::IsPreferred() ditto.


   nsBrowserInstance::IsPreferred() ditto, plus known image types
   (plus image/tiff?) are listed in a nsModuleComponentInfo list.
   Not sure what that list is for.


   sniffout_mimetype() identifies streams based on magic cookies.
   tests are hardcoded for the currently known formats for libimg.


   il_emit_row() has a couple odd checks for image mimetypes.


   nsLayoutDLF::CreateInstance() has a hardcoded list of known image
   mimetypes (gImageTypes) that it checks before trying to create a
   document around a channel.


   nsXMLMIMEDataSource::InitFromHack() has a big mapping list of
   mimetypes and filename extensions hardcoded.


   nsObjectFrame::IsSupportedImage() has a hardcoded list of supported
   image mimetypes and filename extensions that it uses to determine if
   it can handle a particular object.


   mime_find_class() has a hardcoded list of mimetypes that mailnews
   is capable of displaying internally inline.
Thanks, tor. This will make the near future much easier.
Some of the references, like mime sniffer in imglib, don't
exclude other formats from passing in. But do verify some
of the very basic formats.

Others may be more problematic. From what I learned at last
meeting, we may _temporarily_ need to add the mime type to
one include file. And there is some code, unfinished, that 
handles a dialog for adding new file extension mapping to mime
content types.

Would you be interested in looking at that code? mscott@netscape
mentioned to me yesterday. You could ping him about it. 


My current theory is that the only file we *should* have
to change is
netwerk/mime/src/nsXMLMIMEDataSource.cpp; nsXMLMIMEDataSource::InitFromHack()

This is the hack that replaces the dialog that should let us
input new file extension to mime content-type mapping.

I say should, because, well ...sometimes theres a distance between
theory and implementation.

Lets put the MNG stuff here for now.

ok.ok. TWO files.
netwerk/mime/src/nsXMLMIMEDataSource.cpp; nsXMLMIMEDataSource::InitFromHack()

-P ;-)
Changing the OS platform to Linux as this was tested with M16 distribution
to exist on Linux.
OS: Solaris → Linux
*** Bug 44868 has been marked as a duplicate of this bug. ***
Keywords: nsbeta3
Target Milestone: M17 → M18
*** Bug 49455 has been marked as a duplicate of this bug. ***
Whiteboard: [nsbeta3-]
Nominating for Moz1.0. A comment in bug 18574 suggests that this is the cause of
MNGs not working in chrome; it's listed as a blocker for 18574 (and thus is a
major part of the reason we are still using gifs in the chrome) and besides,
getting a sensible architecture in place for developers to build on sounds like
something most people agree should be a goal for 1.0.
Keywords: mozilla1.0
Target Milestone: M18 → Future
Blocks: 61532
QA Contact: elig → tpreston
Added Arch keyword, and changed Platform/OS to All/All, since this isn't
platform specific.
Keywords: arch
OS: Linux → All
Hardware: Sun → All
Keywords: nsbeta3helpwanted
Whiteboard: [nsbeta3-]
*** Bug 64745 has been marked as a duplicate of this bug. ***
Blocks: 66964
Depends on: 70938
Is this going to be fixed by the libimg rewrite? If not, we should sort it, as 
MNG in chrome would be good for 1.0.

All pnunn bugs reassigned to Pav, who is taking over
the imglib.
Assignee: pnunn → pavlov
pavlov, was this fixed with the new imglib rewrite?
Whiteboard: [imglib]
No longer blocks: 61532
I'm currently trying to add a new Image decoder.
In this progress, I found that this bug does not seem to be fixed.
No longer blocks: mng
What about using nsICategoryManager for the various decoders? Using a category
like "ImageDecoders" and mime-types as entries, this would easily allow
enumeration of image decoders and an easy way for getting one (the value could
be the Contract ID).
Attached patch partial patch (obsolete) — Splinter Review
this patch eliminates some lists of image mime types. pavlov, could you review?
Comment on attachment 72463 [details] [diff] [review]
partial patch

You should be able to use:
NS_LITERAL_CSTRING(";2?type=") + aContentType
to avoid a copy.  do this and r=pavlov
Attachment #72463 - Flags: review+
Attached patch patch with stuart's comments (obsolete) — Splinter Review
it wasn't quite as easy as it sounded
Attachment #72463 - Attachment is obsolete: true
Comment on attachment 72657 [details] [diff] [review]
patch with stuart's comments

marking stuart's r=
Attachment #72657 - Flags: review+
Attachment #72657 - Flags: superreview+
Comment on attachment 72657 [details] [diff] [review]
patch with stuart's comments

sr=tor with the string change suggested by dbaron
Attached patch final patch (obsolete) — Splinter Review
attaching the final patch here for reference
Attachment #72657 - Attachment is obsolete: true
Comment on attachment 73031 [details] [diff] [review]
final patch

marking stuart's r= and tor's sr=
Attachment #73031 - Flags: superreview+
Attachment #73031 - Flags: review+
Comment on attachment 73031 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73031 - Flags: approval+
Comment on attachment 73031 [details] [diff] [review]
final patch

checked in
Attachment #73031 - Attachment is obsolete: true
OK, if someone is interested in this:
If you want to add an image decoder, these are the places you have to change:
o) uriloader/exthandler/nsExternalHelperAppService.cpp around line 124
This not strictly necessary; it is for mapping file extensions to mime types.
(Note that the OS will be asked if no entry for a given extension is in this
list). If this fails, the file content is sniffed, see below.

o) content/build/nsContentDLF.cpp around line 132
This is necessary so that "full-page" images work, ie. those loaded directly,
not as part of the page. bug 97324 is filed to remove this need.

o) modules/libpr0n/src/imgLoader.cpp - in the function starting at line 691
This is necessary so that content sniffing works, when no mimetype is available.
If it is a local file, Mozilla first tries to get the type from the extension
(basically, at least) (see my first point), then it checks the file contents.
For remote files, the content check is done first, if that fails extension is
used. bug 104906 is filed to change this, by moving the content sniffing into
each decoder, or something like that.

Thanks to bz for explaining some of the above to me, I hope I got it right here :)
This bug looks like it was fixed a while ago. Can it be resolved?
I agree.
Closed: 22 years ago
Resolution: --- → FIXED
Blocks: mng
bug 231886 filed for first part of comment 27.
No longer depends on: 231886
You need to log in before you can comment on or make changes to this bug.