Closed Bug 41333 Opened 25 years ago Closed 22 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: tor, Assigned: pavlov)

References

Details

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

Attachments

(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 appropriate. 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 enumeration.
Tim: 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 component. -P
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Blocks: mng
Here's an inventory of the places in mozilla that hardcode the image formats supported by mozilla: docshell/base/nsDSURIContentListener.cpp nsDSURIContentListener::CanHandleContent() has a hardcoded list of mimetypes that is willing to be the primary handler for. embedding/browser/gtk/src/GtkMozEmbedChrome.cpp GtkMozEmbedChrome::IsPreferred() ditto. embedding/browser/webBrowser/nsWBURIContentListener.cpp nsWBURIContentListener::IsPreferred() ditto. xpfe/browser/src/nsBrowserInstance.cpp nsBrowserInstance::IsPreferred() ditto, plus known image types (plus image/tiff?) are listed in a nsModuleComponentInfo list. Not sure what that list is for. modules/libimg/src/if.cpp sniffout_mimetype() identifies streams based on magic cookies. tests are hardcoded for the currently known formats for libimg. modules/libimg/src/scale.cpp il_emit_row() has a couple odd checks for image mimetypes. layout/build/nsLayoutDLF.cpp nsLayoutDLF::CreateInstance() has a hardcoded list of known image mimetypes (gImageTypes) that it checks before trying to create a document around a channel. netwerk/mime/src/nsXMLMIMEDataSource.cpp nsXMLMIMEDataSource::InitFromHack() has a big mapping list of mimetypes and filename extensions hardcoded. layout/html/base/src/nsObjectFrame.cpp 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. mailnews/mime/src/mimei.cpp 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. -p
tor: 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. -P
ok.ok. TWO files. netwerk/mime/src/nsXMLMIMEDataSource.cpp; nsXMLMIMEDataSource::InitFromHack() AND netwerk/mime/public/nsMimeTypes.h -P ;-)
Changing the OS platform to Linux as this was tested with M16 distribution to exist on Linux. -raghu@eng.sun.com
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. Gerv
All pnunn bugs reassigned to Pav, who is taking over the imglib.
Assignee: pnunn → pavlov
Status: ASSIGNED → NEW
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
stuart: 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). http://lxr.mozilla.org/mozilla/source/xpcom/components/nsICategoryManager.idl#25
Attached patch partial patch (obsolete) (deleted) — 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("@mozilla.org/image/decoder;2?type=") + aContentType to avoid a copy. do this and r=pavlov
Attachment #72463 - Flags: review+
Attached patch patch with stuart's comments (obsolete) (deleted) — 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) (deleted) — 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.
Status: NEW → RESOLVED
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.

Attachment

General

Created:
Updated:
Size: