The default bug view has changed. See this FAQ.

Add image encoding support

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Stuart Parmenter, Assigned: Stuart Parmenter)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 17 obsolete attachments)

43.89 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
275.88 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
4.53 KB, application/octet-stream
Details
2.43 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
24.67 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
2.01 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
Need to add new interface and implementations for image encoding
(Assignee)

Comment 1

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

Comment 2

13 years ago
Created attachment 150103 [details]
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

13 years ago
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.
(Assignee)

Comment 5

13 years ago
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.
(Assignee)

Comment 6

12 years ago
Created attachment 183550 [details] [diff] [review]
partial patch.. moving along...

newer patch with some canvas accessors for use..
Attachment #150103 - Attachment is obsolete: true

Comment 7

12 years ago
Created attachment 194388 [details] [diff] [review]
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)
Attachment #183550 - Attachment is obsolete: true

Comment 8

12 years ago
Created attachment 194391 [details]
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

12 years ago
Created attachment 194393 [details]
Header file for PNG encoder

Updated

12 years ago
Attachment #194391 - Attachment description: > 2. For the tag drop down UI, I liked the third one with the PNG Encoder file → PNG Encoder source file

Comment 10

12 years ago
Created attachment 194395 [details]
Makefile for png encoder (libpr0n/encoders/png/)

Comment 11

12 years ago
Created attachment 194396 [details]
Full encoder IDL. the clipboard function will probably be moved.

Comment 12

12 years ago
Created attachment 194398 [details]
Patch for configuration system
(Assignee)

Comment 13

12 years ago
Created attachment 194410 [details] [diff] [review]
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.
Attachment #194388 - Attachment is obsolete: true
Attachment #194391 - Attachment is obsolete: true
Attachment #194393 - Attachment is obsolete: true
Attachment #194395 - Attachment is obsolete: true
Attachment #194396 - Attachment is obsolete: true
Attachment #194398 - Attachment is obsolete: true
Attachment #194410 - Flags: superreview?(vladimir)
Attachment #194410 - Flags: review+
(Assignee)

Comment 14

12 years ago
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
(Assignee)

Comment 17

12 years ago
Created attachment 194453 [details] [diff] [review]
Updated trunk patch
Attachment #194410 - Attachment is obsolete: true
Attachment #194453 - Flags: superreview?(vladimir)
Attachment #194453 - Flags: review+
(Assignee)

Comment 18

12 years ago
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.
Attachment #194453 - Flags: superreview?(vladimir) → superreview+
Attachment #194410 - Flags: superreview?(vladimir)
(Assignee)

Comment 19

12 years ago
Created attachment 194495 [details] [diff] [review]
Branch patch

This patch is the same as the trunk one except that it moves mscott's
imgIEncoder to imgIClipboardEncoder and updates callers.
Attachment #194495 - Flags: superreview?(vladimir)
Attachment #194495 - Flags: review+
Attachment #194495 - Flags: approval1.8b4?
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();
  }
}
Attachment #194495 - Flags: superreview?(vladimir) → superreview-
(Assignee)

Comment 21

12 years ago
Comment on attachment 194495 [details] [diff] [review]
Branch patch

I'll post a new branch patch in a bit.
Attachment #194495 - Flags: approval1.8b4?
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
(Assignee)

Comment 24

12 years ago
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?

Comment 26

12 years ago
Created attachment 194542 [details]
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.
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.
Created attachment 195499 [details] [diff] [review]
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.
Created attachment 195595 [details] [diff] [review]
Revised MOZ_PNG_READ MOZ_PNG_WRITE configuration patch

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

Updated

12 years ago
Attachment #195499 - Attachment is obsolete: true

Comment 32

12 years ago
Created attachment 196243 [details] [diff] [review]
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

12 years ago
Created attachment 196264 [details] [diff] [review]
Patch for better error handling (fixed)

This fixes the weird root directory offsets in the previous error handling
bugfix patch.
Attachment #196243 - Attachment is obsolete: true

Comment 34

12 years ago
Created attachment 196265 [details] [diff] [review]
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

12 years ago
Created attachment 196266 [details]
Makefile for new JPEG encoder

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

Comment 36

12 years ago
Created attachment 196267 [details]
.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

12 years ago
Created attachment 196268 [details]
.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
Created attachment 196485 [details] [diff] [review]
Jpeg encoder patch combined with libpr0n/encoder/jpeg files

Four preceding attachments combined into one patch file
Attachment #196265 - Attachment is obsolete: true
Attachment #196266 - Attachment is obsolete: true
Attachment #196267 - Attachment is obsolete: true
Attachment #196268 - Attachment is obsolete: true

Comment 39

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

Glenn: Thanks for combining
Attachment #196485 - Flags: superreview?(vladimir)
Attachment #196485 - Flags: review?(pavlov)

Updated

12 years ago
Attachment #196264 - Flags: superreview?(vladimir)
Attachment #196264 - Flags: review?(pavlov)
(Assignee)

Updated

12 years ago
Attachment #196485 - Flags: review?(pavlov) → review+
Comment on attachment 196485 [details] [diff] [review]
Jpeg encoder patch combined with libpr0n/encoder/jpeg files

r=vladimir
Attachment #196485 - Flags: superreview?(vladimir) → superreview+
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.
Attachment #196264 - Flags: superreview?(vladimir) → superreview+

Updated

12 years ago
Attachment #196264 - Flags: review?(pavlov) → review?(annie.sullivan)
(Assignee)

Comment 42

12 years ago
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.
Attachment #196264 - Flags: review?(annie.sullivan) → review+

Comment 43

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

12 years ago
(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.

Updated

12 years ago
OS: Windows XP → All
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.
Depends on: 293244
Blocks: 315756
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

11 years ago
Created attachment 217840 [details] [diff] [review]
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?
Attachment #217840 - Flags: review?(benjamin)
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

11 years ago
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
Attachment #217840 - Flags: review?(benjamin) → review+

Comment 53

11 years ago
Created attachment 217891 [details] [diff] [review]
Or we generate a .h file to be included.

That way we can get rid of the extra rule (yay for .deps)
Attachment #217840 - Attachment is obsolete: true
Attachment #217891 - Flags: review?(benjamin)
Attachment #217891 - Flags: review?(benjamin) → review+
Blocks: 333613

Comment 54

11 years ago
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
Attachment #195595 - Attachment is obsolete: true
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

11 years ago
No idea, it can probably be closed.

Updated

11 years ago
Blocks: 291218, 346849

Comment 58

10 years ago
Per comment #56 and comment #57, --> FIXED.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 373338

Updated

8 years ago
QA Contact: imagelib
You need to log in before you can comment on or make changes to this bug.