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

Status: RESOLVED FIXED

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

Description

[tor]

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

[pnunn]

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

Comment 2

[tor]

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

[pnunn]

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

[pnunn]

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

[pnunn]

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

-P ;-)

Comment 6

[Raghu Sakleshpur]

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

[e-mail]

Comment 7

[tor]

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

Comment 8

[tor]

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

Comment 9

[Stuart Ballard]

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.

Comment 10

[Jason Eager]

Added Arch keyword, and changed Platform/OS to All/All, since this isn't
platform specific.

Comment 11

[tor]

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

Comment 12

[Gervase Markham [:gerv]]

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

[pnunn]

All pnunn bugs reassigned to Pav, who is taking over
the imglib.

Comment 14

[Terri Preston]

pavlov, was this fixed with the new imglib rewrite?

Comment 15

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

I'm currently trying to add a new Image decoder.
In this progress, I found that this bug does not seem to be fixed.

Comment 16

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

Adding bug 104906 and bug 97324 as dependency

Comment 17

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

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

Comment 18

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

Attachment: partial patch

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

Comment 19

[Stuart Parmenter]

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

Comment 20

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

Attachment: patch with stuart's comments

it wasn't quite as easy as it sounded

Comment 21

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

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

marking stuart's r=

Comment 22

[tor]

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

sr=tor with the string change suggested by dbaron

Comment 23

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

Attachment: final patch

attaching the final patch here for reference

Comment 24

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

Comment on attachment 73031 [details] [diff] [review]
final patch

marking stuart's r= and tor's sr=

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 73031 [details] [diff] [review]
final patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 26

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

Comment on attachment 73031 [details] [diff] [review]
final patch

checked in

Comment 27

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

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 :)

Comment 28

[Benjamin Smedberg]

This bug looks like it was fixed a while ago. Can it be resolved?

Comment 29

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

I agree.

Comment 30

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

bug 231886 filed for first part of comment 27.