Need to add new interface and implementations for image encoding

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

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.

Stuart, see the patch in: Bug #223909 which contains the thunderbird work for encoding clipboard bitmaps as JPEGs.

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.

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.

newer patch with some canvas accessors for use..

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)

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

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.

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

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

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.

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

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 on attachment 194495 [details] [diff] [review] Branch patch I'll post a new branch patch in a bit.

Why do we want this for the 1.8 branch?

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

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

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

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.

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

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

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.

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.

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

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.

This fixes the weird root directory offsets in the previous error handling bugfix 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.

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

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

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

Four preceding attachments combined into one patch file

Comment on attachment 196485 [details] [diff] [review] Jpeg encoder patch combined with libpr0n/encoder/jpeg files Glenn: Thanks for combining

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

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 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.

> 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.

(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.

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.

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

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

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

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

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?

> 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 on attachment 217840 [details] [diff] [review] Make sure to rebuild nsImageModule when MOZ_IMG_ENCODERS changes With the switch back to export::, r=me

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

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

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?

No idea, it can probably be closed.

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