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

RESOLVED FIXED in Future

Status

()

Core
ImageLib
P3
normal
RESOLVED FIXED
17 years ago
11 years ago

People

(Reporter: tor, Assigned: Stuart Parmenter)

Tracking

({arch, helpwanted})

Trunk
Future
arch, helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [imglib])

Attachments

(3 obsolete attachments)

(Reporter)

Description

17 years ago
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.

Comment 1

17 years ago
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

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M17
(Reporter)

Updated

17 years ago
Blocks: 18574
(Reporter)

Comment 2

17 years ago
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.

Comment 3

17 years ago
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

Comment 4

17 years ago
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
 

Comment 5

17 years ago
ok.ok. TWO files.
netwerk/mime/src/nsXMLMIMEDataSource.cpp; nsXMLMIMEDataSource::InitFromHack()
AND
netwerk/mime/public/nsMimeTypes.h

-P ;-)

Comment 6

17 years ago
Changing the OS platform to Linux as this was tested with M16 distribution
to exist on Linux.

-raghu@eng.sun.com
OS: Solaris → Linux
Blocks: 44868
(Reporter)

Comment 7

17 years ago
*** Bug 44868 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Keywords: nsbeta3
Target Milestone: M17 → M18
(Reporter)

Comment 8

17 years ago
*** Bug 49455 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Whiteboard: [nsbeta3-]

Comment 9

17 years ago
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

Updated

17 years ago
Target Milestone: M18 → Future

Updated

17 years ago
Blocks: 61532

Updated

17 years ago
QA Contact: elig → tpreston

Comment 10

17 years ago
Added Arch keyword, and changed Platform/OS to All/All, since this isn't
platform specific.
Keywords: arch
OS: Linux → All
Hardware: Sun → All

Updated

17 years ago
Keywords: nsbeta3 → helpwanted
Whiteboard: [nsbeta3-]
(Reporter)

Comment 11

17 years ago
*** Bug 64745 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Blocks: 66964
(Assignee)

Updated

17 years ago
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

Comment 13

17 years ago
All pnunn bugs reassigned to Pav, who is taking over
the imglib.
Assignee: pnunn → pavlov
Status: ASSIGNED → NEW

Comment 14

17 years ago
pavlov, was this fixed with the new imglib rewrite?
Whiteboard: [imglib]
(Assignee)

Updated

17 years ago
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.

Updated

16 years ago
No longer blocks: 18574
Adding bug 104906 and bug 97324 as dependency
Depends on: 97324, 104906
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
Created attachment 72463 [details] [diff] [review]
partial patch

this patch eliminates some lists of image mime types. pavlov, could you review?
(Assignee)

Comment 19

16 years ago
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+
Created attachment 72657 [details] [diff] [review]
patch with stuart's comments

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+
(Reporter)

Updated

16 years ago
Attachment #72657 - Flags: superreview+
(Reporter)

Comment 22

16 years ago
Comment on attachment 72657 [details] [diff] [review]
patch with stuart's comments

sr=tor with the string change suggested by dbaron
Created attachment 73031 [details] [diff] [review]
final patch

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 25

16 years ago
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
No longer blocks: 44868
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 :)
Depends on: 188586
This bug looks like it was fixed a while ago. Can it be resolved?
I agree.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Blocks: 18574
Depends on: 231886
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.