245684 - Add image encoding support

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Stuart Parmenter]

Need to add new interface and implementations for image encoding

Comment 1

[Stuart Parmenter]

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

Comment 2

[Stuart Parmenter]

Attachment: imgIEncoder.idl

The image encoder interface should look something like this.  I'll need to
probably create another interface for doing encoding.  That method probably
belongs close to the same place as loadImage, except that is currently in
imgILoader.. Maybe it would have been better for that to be called imgIManager
or somesuch.  I'll give that some more thought.

Comment 3

[Scott MacGregor]

Stuart, see the patch in:

Bug #223909

which contains the thunderbird work for encoding clipboard bitmaps as JPEGs.

Comment 4

[Benjamin Smedberg]

I would like to use code like this for the XP apprunner. "XUL apps" would ship
with icons in some format (probably PNG), and the OS-specific apprunner code
would convert these into OS-specific (ICO/XPM) icon files. Are there footprint
estimates  on how much this would cost? A custom-ordered icon writer might be a
simpler solution for the targetted use-case I'm thinking of.

Comment 5

[Stuart Parmenter]

No idea on size estimate yet.  Haven't gotten that far.  As for doing a one-off,
it would make more sense to me for everyone doing image encoding to go through
the same code path.

Comment 6

[Stuart Parmenter]

Attachment: partial patch.. moving along...

newer patch with some canvas accessors for use..

Comment 7

[Brett Wilson]

Attachment: Mostly complete patch for PNG encoding support

This patch is a mostly complete implementation of an image encoding interface
and the ability for a canvas to give you back a data URL encoding the image.

I added canvas.toDataURI as per WHATWG spec. This returns a PNG with
transparency.

I added a new canvas.toDataURIAs to extend this functionality to allow you to
pick an image type and pass an "option string" to the encoder containing
encoder-specific parameters. The idea is that in the future I want to be
able to get JPEGs of a given quality by doing something like:
getDataURIAs("image/jpeg", "quality=80")

Only a PNG encoder has been implemented in this patch. It allows one
option string "transparency=none" to have it give you a PNG with no alpha
channel (this is actually appropriate for most cases and saves a little
space).

I added a format parameter to the encoder::InitWithData so you can specify
different input formats instead of just premultipled ARGB. I added defines
for host-endian ARGB and host-independent RGB and RGBA.

Enabling write support in the PNG library expands it by ~77K in the debug
build.
Release bloat will be smaller, but I haven't checked that yet. JPEG should be
almost free when we add that.

Left to do

- There is a JPG encoder that was recently checked in for clipboard only.
  I'm unsure what to do about this, since it is not cross platform. I suggest
  that this specific functionality should be moved to a different interface.

- I didn't write imgPNGEncoder::Init which takes an imgIContainer as an
  argument. Shouldn't be hard to extract the data and feed it to the
  initFromData function.

- Add some configure flags to enable or disable encoder support.

- Fix makefiles to use said flags (modules/libimg/png/Makefile.in)

- Fix libimg/png to use said flags (modules/libimg/png/mozpngconf.h)

- Fix libpr0n to use said flags (modules/libpr0n/Makefile.in)
  (modules/libpr0n/build/Makefile.in)

Comment 8

[Brett Wilson]

Attachment: PNG Encoder source file

Whoops, that patch didn't get the new files. Here is the PNG encoder source
file that should go in modules/libpr0n/encoders/png/

Comment 9

[Brett Wilson]

Attachment: Header file for PNG encoder

Comment 10

[Brett Wilson]

Attachment: Makefile for png encoder (libpr0n/encoders/png/)

Comment 11

[Brett Wilson]

Attachment: Full encoder IDL. the clipboard function will probably be moved.

Comment 12

[Brett Wilson]

Attachment: Patch for configuration system

Comment 13

[Stuart Parmenter]

Attachment: updated full patch for trunk

vlad can you sr= this stuff for landing on the trunk.  I'd like to see this
also go in to the branch (that moves mscott's clipboard stuff around) so I'll
be posting another patch shortly for that.

Comment 14

[Stuart Parmenter]

ignore the bogus rule at the bottom of modules/libimg/png/Makefile.in, i've
removed that from my tree.

Comment 15

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

Comment on attachment 194410 [details] [diff] [review]
updated full patch for trunk


(vlad is a super-reviewer?)



+    nsCOMPtr<nsIInputStream> stream(do_QueryInterface(encoder));
+    *aStream = stream;
+    NS_ADDREF(*aStream);

return CallQueryInterface(encoder, aStream);

+//    Data URL bugs:
+//	 https://bugzilla.mozilla.org/show_bug.cgi?id=78876

that's resolved, why mention this specific bug? I'm sure there are many other
resolved data bugs...

+  nsCString aMimeType8 = NS_ConvertUCS2toUTF8(aMimeType);

NS_ConvertUTF16toUTF8 mimeType8(aMimeType);

(a prefix only for arguments)

+			  &numReadThisTime)) == NS_OK && numReadThisTime > 0) {

don't compare against NS_OK, use NS_SUCCEEDED

this ignores errors from Read, treating them like EOF...

+  if (! encodedImg) // not sure why this would fail
+    return NS_ERROR_FAILURE;


OOM comes to mind, and that's probably the best error code to use here...

+    NS_LITERAL_STRING(";base64,") + NS_ConvertUTF8toUCS2(encodedImg);

please use UTF16 rather than UCS2... the UCS2 methods should really be removed

+  // make into URI object
+  /*

why this commented out code?

+++ dom/public/idl/html/nsIDOMHTMLCanvasElement.idl	31 Aug 2005 04:09:18
-0000

needs a new IID


modules/libpr0n/encoders/Makefile.in
+# The Initial Developer of the Original Code is
+# Netscape Communications Corporation.

is it really?

modules/libpr0n/encoders/png/Makefile.in
+# The Initial Developer of the Original Code is
+# Netscape Communications Corporation.
+# Portions created by the Initial Developer are Copyright (C) 2005
+# the Initial Developer. All Rights Reserved.

this one is definitely wrong, since there's no netscape in 2005 anymore :)

+  if (aInputFormat != INPUT_FORMAT_RGB &&
+      aInputFormat != INPUT_FORMAT_RGBA &&
+      aInputFormat != INPUT_FORMAT_HOSTARGB)

so an imgIContainer can't be encoded? :/

+  if (aOutputOptions.Length() >= 17) {

is this hardcoded 17 really needed, why not use an NS_NAMED_LITERAL_STRING and
.Length()?

+    if (Substring(aOutputOptions, 0, 17) ==

StringBeginsWith

+ * imgIEncoder interface
+ *   Currently this is a very specific encoder designed to encode a native
clipboard image as a JPEG out to disk.
+ *   It is not intended to be a generic image encoder.

???

Also, the interface forward-declarations here are not needed...

Comment 16

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

Comment on attachment 194410 [details] [diff] [review]
updated full patch for trunk

also, the added lines in allmakefiles.sh are misindented:
	 modules/libpr0n/decoders/xbm/Makefile
+	  modules/libpr0n/encoders/Makefile
+	  modules/libpr0n/encoders/png/Makefile

Comment 17

[Stuart Parmenter]

Attachment: Updated trunk patch

Comment 18

[Stuart Parmenter]

biesi: Also, to answer your question, encoding of imgIContainers is not
supported yet due to the lack of any sane cross-platform format between them. 
Support for this will be added once we move a little closer towards thebes.

Comment 19

[Stuart Parmenter]

Attachment: Branch patch

This patch is the same as the trunk one except that it moves mscott's
imgIEncoder to imgIClipboardEncoder and updates callers.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 194495 [details] [diff] [review]
Branch patch

>Index: content/canvas/public/nsICanvasRenderingContextInternal.h
>===================================================================
>+  // Gives you a stream containing the image represented by this context.
>+  // The format is given in aMimeTime, for example "image/png".
>+  //
>+  // If the image format does not support transparency or aIncludeTransparency
>+  // is false, alpha will be discarded and the result will be the image
>+  // composited on black.
>+  NS_IMETHOD GetInputStream(const nsACString& aMimeType,
>+                            const nsAString& aEncoderOptions,
>+                            nsIInputStream **aStream) = 0;
>+

Fix comment to refer to "transparency=none" string, instead of
aIncludeTransparency parameter


>+NS_IMETHODIMP
>+nsHTMLCanvasElement::ToDataURL(nsAString& aDataURL)
>+{
>+  nsString type = NS_LITERAL_STRING("image/png");
>+  nsString options = EmptyString();
>+
>+  nsCOMPtr<nsIXPCNativeCallContext> ncc;
>+  nsresult rv = nsContentUtils::XPConnect()->GetCurrentNativeCallContext(getter_AddRefs(ncc));
>+  if (NS_SUCCEEDED(rv)) {
>+
>+    if (!ncc)
>+      return NS_ERROR_FAILURE;

So this won't actually work.  GetCurrentNCC will return the NCC of the original
xpconnect call into C++ native code, which isn't what you want here.  shaver
says you can find out what method JS was calling by calling something similar
to
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpccomponents.cpp#19
70  and checking for nsIDOMHTMLCanvasElement (or wherever ToDataURL is defined)
and the toDataURL method name.	However, if ncc is null, you don't want to
return an error, that just means that there's no JS caller involved. 
Basically, something like

type = "image/png";
GetCurrentNativeCallContext(&ncc);
if (ncc) {
  if (check_whether_js_called_this_method) {
    type = parse_arguments();
  }
}

Comment 21

[Stuart Parmenter]

Comment on attachment 194495 [details] [diff] [review]
Branch patch

I'll post a new branch patch in a bit.

Comment 22

[Benjamin Smedberg]

Why do we want this for the 1.8 branch?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Is the 18K codesize increase from this real, or just an artifact of the way we
compute codesize?  Specifically, this part:

  libimglib2.so
  	Total:	     +18512 (+18512/+0)
  	Code:	     +15523 (+15568/+0)
  	Data:	      +2989 (+2944/+0)
  	     +15568 (+15568/+0)	text (CODE)
  		     +15568 (+15568/+0)	UNDEF:libimglib2.so:text
  			      +3092	png_write_find_filter
  			       +893	png_write_IHDR
  			       +794	nsPNGEncoder::InitFromData(unsigned char const*, unsigned,
unsigned, unsigned, unsigned, unsigned, nsAString_internal const&)
  			       +747	png_do_write_interlace
  			       +696	MOZ_PNG_set_filter_heur
  			       +618	png_write_finish_row
  			       +605	png_write_start_row
  			       +545	MOZ_PNG_write_row
  			       +525	MOZ_PNG_cr_write_str
  			       +483	MOZ_PNG_set_filter
  			       +373	png_write_destroy
  			       +353	MOZ_PNG_write_init_3
  			       +352	png_write_IDAT
  			       +313	png_write_tRNS
  			       +271	MOZ_PNG_write_flush
  			       +269	png_write_filtered_row
  			       +262	png_write_PLTE
  			       +245	MOZ_PNG_write_init_2
  			       +231	nsPNGEncoder::QueryInterface(nsID const&, void**)
  			       +208	nsPNGEncoder::ConvertHostARGBRow(unsigned char const*, unsigned
char*, unsigned, int)
  			       +156	MOZ_PNG_write_info
  			       +149	MOZ_PNG_write_info_before_PLTE
  			       +144	MOZ_PNG_set_write_fn
  			       +143	nsPNGEncoder::WriteCallback(png_struct_def*, unsigned char*,
unsigned)
  			       +132	MOZ_PNG_dest_write_str
  			       +126	png_write_oFFs
  			       +123	nsPNGEncoderConstructor(nsISupports*, nsID const&, void**)
  			       +117	MOZ_PNG_set_comp_win_bits
  			       +116	MOZ_PNG_write_image
  			       +113	png_write_sig
  			       +112	png_write_gAMA
  			       +102	MOZ_PNG_set_comp_buf_siz
  			       +101	png_do_write_transformations
  			       +100	nsPNGEncoder::Read(char*, unsigned, unsigned*)
  			        +97	png_write_sRGB
  			        +94	MOZ_PNG_write_chunk_start
  			        +79	nsPNGEncoder::Release()
  			        +78	png_default_write_data
  			        +77	MOZ_PNG_get_oFFs
  			        +73	nsPNGEncoder::Close()
  			        +73	png_write_data
  			        +72	MOZ_PNG_set_comp_method
  			        +72	MOZ_PNG_write_chunk_data
  			        +71	MOZ_PNG_write_end
  			        +71	png_write_IEND
  			        +70	nsPNGEncoder::nsPNGEncoder[in-charge]()
  			        +70	nsPNGEncoder::nsPNGEncoder[not-in-charge]()
  			        +67	MOZ_PNG_write_rows
  			        +67	nsPNGEncoder::ReadSegments(unsigned (*)(nsIInputStream*, void*,
char const*, unsigned, unsigned, unsigned*), void*, unsigned, unsigned*)
  			        +66	MOZ_PNG_write_chunk
  			        +65	nsPNGEncoder::~nsPNGEncoder [in-charge]()
  			        +65	nsPNGEncoder::~nsPNGEncoder [not-in-charge]()
  			        +60	MOZ_PNG_write_chunk_end
  			        +54	nsPNGEncoder::StripAlpha(unsigned char const*, unsigned char*,
unsigned)
  			        +49	MOZ_PNG_def_flush
  			        +45	.nosyms.text
  			        +43	MOZ_PNG_set_oFFs
  			        +43	png_write_init
  			        +37	png_save_int_32
  			        +37	png_save_uint_32
  			        +33	png_flush
  			        +25	MOZ_PNG_set_flush
  			        +24	MOZ_PNG_set_comp_level
  			        +24	MOZ_PNG_set_comp_mem_lev
  			        +24	MOZ_PNG_set_comp_strategy
  			        +24	nsPNGEncoder::Available(unsigned*)
  			        +23	MOZ_PNG_get_comp_buf_siz
  			        +21	png_save_uint_16
  			        +19	nsPNGEncoder::IsNonBlocking(int*)
  			        +17	MOZ_PNG_set_write_status_fn
  			        +15	nsPNGEncoder::AddRef()
  			        +10	MOZ_PNG_set_read_fn
  			         +5	nsDependentString::AssertValid()
  	      +2816 (+2816/+0)	rodata (DATA)
  		      +2816 (+2816/+0)	UNDEF:libimglib2.so:rodata
  			      +2784	.nosyms.rodata
  			        +16	imgIEncoder::GetIID()::iid
  			        +16	nsIInputStream::GetIID()::iid
  	       +128 (+128/+0)	data (DATA)
  		       +128 (+128/+0)	UNDEF:libimglib2.so:data
  			        +56	components
  			        +44	vtable for nsPNGEncoder
  			        +28	.nosyms.data

Comment 24

[Stuart Parmenter]

bz: its real. we're now building some of the PNG write code that we didn't have
on before.

Comment 25

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

what about things like this:
  			        +21	png_save_uint_16
Don't these symbols need renaming too?

Comment 26

[Brett Wilson]

Attachment: Test case extension - generate thumbnails as you browse

This extension will wait for onload, render the page to a canvas, and dump out
the results of toDataURI to the console (I'm not sure if you need a debug build
for dump() to work).

This extension is not very robust. Small pages like google will be sized wrong
and you won't get good results. It works OK for full-window pages like slashdot
or google news. Also, it will try duping every page, whish isn't possible for
things like chrome, etc. so you'll get some errors.

Comment 27

[Glenn Randers-Pehrson]

In libpr0n/build/nsImageModule.cpp, shouldn't this

#ifdef XP_MAC
#define IMG_BUILD_gif 1 
#define IMG_BUILD_bmp 1 
#define IMG_BUILD_png 1 
#define IMG_BUILD_jpeg 1
#define IMG_BUILD_xbm 1
#endif

become
#ifdef XP_MAC
#define IMG_BUILD_DECODER_gif 1 
 etc.
#endif

Comment 28

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

XP_MAC ifdefs aren't ever hit, they should probably be removed.

Comment 29

[Glenn Randers-Pehrson]

Christian, re comment 25:  They are all (except png_write_init) "private"
functions in libpng.  That is, they are exported for use by other parts of
libpng but not meant to be part of the API.  Therefore they shouldn't need to be
renamed.  On the other hand, renaming them would be harmless.  png_write_init()
is a macro and is deprecated, having been replaced by png_write_init_3(), and
will eventually disappear from libpng.

Comment 30

[Glenn Randers-Pehrson]

Attachment: Pass MOZ_PNG_WRITE via configure instead of Makefile, add MOZ_PNG_READ.

The trunk patch and the proposed branch patch contain the following
modification
to libimg/png/Makefile.in

+#ifeq (png,$(filter png,$(MOZ_IMG_ENCODERS)))
+ifdef MOZ_IMG_ENCODERS
+DEFINES	 += -DMOZ_PNG_WRITE
+endif

I see a couple of problems with this.  First, it is not as robust as passing
the "-DMOZ_PNG_WRITE" through the configure/automake system, which makes sure
that every instance of png.h will see the definition. Currently,
libpr0n/encoders/nsPNGEncoder.cpp and libpr0n/decoders/nsPNGDecoder.cpp won't
see it.  The code currently works all right, though, because luckily, due to an
oversight, png.h provides most of the png-writing function prototypes even when
PNG_WRITE isn't defined.

Secondly, if you include "png" in the MOZ_IMAGE_ENCODERS string but not in
MOZ_IMAGE_DECODERS, you will still get the entire set of png-reading functions.
 There should be a similar treatment, with MOZ_PNG_READ passing PNG_READ into
the png headers.

This patch remedies both problems.  PNG_WRITE is passed in via
configure/automake instead of via special coding in Makefile.in, and
libimg/png/mozlibpngconf.h is modified to process a MOZ_PNG_READ macro which is
also passed in from configure.

Comment 31

[Glenn Randers-Pehrson]

Attachment: Revised MOZ_PNG_READ MOZ_PNG_WRITE configuration patch

This revision does a better job of reducing footprint of libimglib.so, by
several kbytes

Comment 32

[Brett Wilson]

Attachment: Patch for better error handling

This patch fixes some error handling. The old implementation forgot to set the
pointer to NULL, so we'd keep using the buffer after a Realloc failure and
return OK. It also forgot to check the return code from the initFromData
function, ignoring failures.

Comment 33

[Brett Wilson]

Attachment: Patch for better error handling (fixed)

This fixes the weird root directory offsets in the previous error handling
bugfix patch.

Comment 34

[Brett Wilson]

Attachment: JPEG encoder patch

This patch adds JPEG encoder support. Quality is passed in the option string,
so, on a canvas, you can say: toDataURLAs("image/jpg", "quality=30")

This patch is for the changed files. I don't have CVS access. You need the .cpp
.h and the Makefile for the new stuff, which I'll attach separately below.

Comment 35

[Brett Wilson]

Attachment: Makefile for new JPEG encoder

This is part of the JPEG patch above.
It should live in modules/libpr0n/encoders/jpeg

Comment 36

[Brett Wilson]

Attachment: .cpp file for new JPEG encoder

This is the main source file for the JPEG encoder patch above.
It should live in modules/libpr0n/encoders/jpeg

Comment 37

[Brett Wilson]

Attachment: .h file for new JPEG encoder

This is the header file for the JPEG encoder patch above.
It should live in modules/libpr0n/encoders/jpeg

Comment 38

[Glenn Randers-Pehrson]

Attachment: Jpeg encoder patch combined with libpr0n/encoder/jpeg files

Four preceding attachments combined into one patch file

Comment 39

[Brett Wilson]

Comment on attachment 196485 [details] [diff] [review]
Jpeg encoder patch combined with libpr0n/encoder/jpeg files

Glenn: Thanks for combining

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 196485 [details] [diff] [review]
Jpeg encoder patch combined with libpr0n/encoder/jpeg files

r=vladimir

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 196264 [details] [diff] [review]
Patch for better error handling (fixed)

r=me, sorry for the review delay; I misunderstood the other patch and that thought this was rolled in.

Comment 42

[Stuart Parmenter]

Comment on attachment 196264 [details] [diff] [review]
Patch for better error handling (fixed)

oops, sorry I missed this one.  Do you need me to check both of these in?  If so I'll do it this afternoon.

Comment 43

[Brett Wilson]

> oops, sorry I missed this one.  Do you need me to check both of these in?  If
> so I'll do it this afternoon.

I theoretically have priveleges now, so I'll use this as a test to see if I can check things in and not break everything.

Comment 44

[Brett Wilson]

(In reply to comment #31)
> This revision does a better job of reducing footprint of libimglib.so, by
> several kbytes

Glen, could you find somebody to review this that knows about PNG options and such? I don't really feel comfortable with this stuff myself, but it would be great if we can save a little space.

Comment 45

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the nsIInputStream implementations checked in for this bug were pretty severely broken (things like incorrectly implementing ReadSegments, returning the wrong boolean value from isNonBlocking(), not being threadsafe when they have to be, etc).  See patch in bug 318193, which corrects a whole bunch of issues.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This needed the security checks which have now been implemented in bug 293244.

Comment 47

[Robert Accettura [:raccettura]]

Is there a shot at getting this in Firefox 2.0?  Would be nice to use it for adding screenshot support to the reporter tool (bug 315756).

Comment 48

[jag (Peter Annema)]

Attachment: Make sure to rebuild nsImageModule when MOZ_IMG_ENCODERS changes

Benjamin: what do you think of making _img_list a prerequisite for nsImageModule.{o,obj} instead of "touch"ing Makefile during export?

Comment 49

[Benjamin Smedberg]

Comment on attachment 217840 [details] [diff] [review]
Make sure to rebuild nsImageModule when MOZ_IMG_ENCODERS changes

Why is _img_list: FORCE better than export::?

Comment 50

[jag (Peter Annema)]

Not necessarily better, it just delays doing that work till it's needed, which I thought was a bit cleaner, but I can go either way on that one. What about making _img_list a prerequisite instead of "touch"ing Makefile?

Comment 51

[Benjamin Smedberg]

> Not necessarily better, it just delays doing that work till it's needed, which
> I thought was a bit cleaner, but I can go either way on that one. What about
> making _img_list a prerequisite instead of "touch"ing Makefile?

I think that export:: is just as good in this case, and more idiomatic in our build system. The prerequisite idea sounds very good; touching makefiles is a hideous hack. You can probably just add it to EXTRA_DEPS or whatever that var is called.

Comment 52

[Benjamin Smedberg]

Comment on attachment 217840 [details] [diff] [review]
Make sure to rebuild nsImageModule when MOZ_IMG_ENCODERS changes

With the switch back to export::, r=me

Comment 53

[jag (Peter Annema)]

Attachment: Or we generate a .h file to be included.

That way we can get rid of the extra rule (yay for .deps)

Comment 54

[Brett Wilson]

It looks like parts of "Patch for better error handling (fixed)" didn't get in. Is there any reason I shouldn't check it in now?

Comment 55

[Glenn Randers-Pehrson]

Comment on attachment 195595 [details] [diff] [review]
Revised MOZ_PNG_READ MOZ_PNG_WRITE configuration patch

PNG_R/W patch is obsoleted by a new patch in bug #334110

Comment 56

[Martijn Wargers (dead)]

What is the status of this bug? Are there any patches that need to be made here or fixed? Or can this bug becomed resolved->fixed?

Comment 57

[Brett Wilson]

No idea, it can probably be closed.

Comment 58

[:Gijs (he/him)]

Per comment #56 and comment #57, --> FIXED.