481926 - rewrite color management component

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

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

Due to a number of problems that have been identified in lcms, we're going to attempt a clean rewrite of the pieces that we actually use.  Jeff will be leading this up.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Just being curious, is the idea to merge that rewrite with lcms, or create a
separate smaller library which implements only the features we need?

Comment 2

[georgi - hopefully not receiving bugspam]

offtopic:

lcms has an interesting coding style. the author seem rightly paranoid and does checks where some coders wouldn't but some of the checks are ``suboptimal''

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: First pass at an lcms replacement

It compiles and runs but I'm not sure how well it works yet.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: v2

A little build fix

Comment 5

[Bobby Holley (:bholley)]

Looks like great progress Jeff - I'm excited.

Any idea when you'll be looking for review on this? The next 2 weeks will be pretty hectic for me (I'm moving to europe), but I want to make sure I can get to this as soon as it's ready to keep things on track for 3.1.

Also, what does 'q' stand for?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: v3

Rough in support for precaching and sse2.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> Looks like great progress Jeff - I'm excited.
> 
> Any idea when you'll be looking for review on this? The next 2 weeks will be
> pretty hectic for me (I'm moving to europe), but I want to make sure I can get
> to this as soon as it's ready to keep things on track for 3.1.

I was hoping to get something review ready by the end of this week. I don't know if that works or not...

> 
> Also, what does 'q' stand for?

q stands for quick. e.g quickly written, quickly performing.

I'm not particularly attached to the name, but it will do for now.

Comment 8

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> (In reply to comment #5)
> > Also, what does 'q' stand for?
> 
> q stands for quick. e.g quickly written, quickly performing.
> 
> I'm not particularly attached to the name, but it will do for now.

I assert, without any skin in the game or responsibility for its ongoing maintenance, that the correct name for a Mozilla-grown library that tries to get colors "just right" is, of course:

bikeshed.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Also, what does 'q' stand for?
> > 
> > q stands for quick. e.g quickly written, quickly performing.
> > 
> > I'm not particularly attached to the name, but it will do for now.
> 
> I assert, without any skin in the game or responsibility for its ongoing
> maintenance, that the correct name for a Mozilla-grown library that tries to
> get colors "just right" is, of course:
> 
> bikeshed.

This comment is so meta it hurts.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: v4

This version is pretty much feature complete, it survives a linux tp run. There are probably still bugs, but I don't know any at this point. When the try servers return I hope to get some better information on how this patch does.

Comment 11

[georgi - hopefully not receiving bugspam]

+	float det = matrix_det(mat);
+
+	if (det == 0) {

iirc floats shouldn't be checked for zero equality this way because of their internal representation.

iirc it should be something like:
if abs(det) < EPSILLON

Comment 12

[georgi - hopefully not receiving bugspam]

http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm

In other words, if you do a calculation and then do this comparison:

if (result == expectedResult)

then it is unlikely that the comparison will be true. If the comparison is true then it is probably unstable – tiny changes in the input values, compiler, or CPU may change the result and make the comparison be false.

Comment 13

[georgi - hopefully not receiving bugspam]

http://docs.sun.com/source/806-3568/ncg_goldberg.html
What Every Computer Scientist Should Know About Floating-Point Arithmetic

Comment 14

[georgi - hopefully not receiving bugspam]

nit:

a.c:
//comment
gcc -pedantic a.c
a.c:28:1: warning: C++ style comments are not allowed in ISO C90

Comment 15

[georgi - hopefully not receiving bugspam]

btw, some optimized color stuff is here:
http://www.jjj.de/bitwizardry/bitwizardrypage.html

colormix.h  (functions that add/average/multiply/mix color channels)
colormixp.h (versions of some of the above funcs that are correct to the last bit)
colormix-fl.h (color manipulation with floating point args)

Comment 16

[Brian Crowder]

I think the == 0 comparison is fine here, since its only intend seems to be preventing divide-by-zero....  perhaps I'm missing something.

Comment 17

[georgi - hopefully not receiving bugspam]

>I think the == 0 comparison is fine here, since its only intend

if this is the case probably yes, unless det == 0 should give other result for math reasons.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #17)
> >I think the == 0 comparison is fine here, since its only intend
> 
> if this is the case probably yes, unless det == 0 should give other result for
> math reasons.

yeah, the check is to see if the matrix is invertible. I could go both ways on this issue, however I'm not sure what a good epsilon would be.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: v5

A couple of fixes along with patches to the reftests.

Comment 20

[georgi - hopefully not receiving bugspam]

>> if this is the case probably yes, unless det == 0 should give other result for
>> math reasons.

>yeah, the check is to see if the matrix is invertible. I could go both ways on
>this issue, however I'm not sure what a good epsilon would be.

yeah, afaict there are at least two meanings of ``invertible matrix'' i.e. det = 0

1. the C/C++ case when you don't get division by zero - you are avoiding this

2. the pure math case. so in this case you analytically (with infitite precision)  get or prove det = 0 yet [1] says say det = 10^-6

i browsed the floating article and the main idea was close to this:
"there are infinitely many real numbers but floats are stored in a finite amount of memory so floats are approximations"

Comment 21

[georgi - hopefully not receiving bugspam]

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

int main(int ac, char **av) {
float a,b,c,d,sq2,det;
sq2=sqrtf(2.0);
a=sq2;d=sq2;b=1.0;c=2.0;
det=a*d-b*c;
if (det==0) {printf("det == 0\n");} else printf("det != 0\n");
printf("%f \n",det);
return(0);
}

./a.out 
det != 0
-0.000000

now checking with the math proggie "sage":
sage: sq2=sqrt(2)
sage: sq2
sqrt(2)
sage: a=sq2;d=sq2;b=1;c=2    
sage: ma=matrix(sq2.parent(),[ [a,b],[c,d]])
sage: ma.determinant()
0
sage: det=ma.determinant() 
sage: det.is_zero()
True

Comment 22

[georgi - hopefully not receiving bugspam]

printf("%e \n",det);

./a.out 
det != 0
-1.192093e-07

Comment 23

[georgi - hopefully not receiving bugspam]

so i don't claim det == 0.0 is a bug.

i learned this feature from some closed source static analyzer that considered it a stopper bug. don't remember the name (iirc it had something to do with airplane warez)

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Attachment: v6

Addresses some comments from Joe, fixes a unhandled malloc failure case and adds a license header. I used the same license as lcms.

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Attachment: v7

Update with more malloc failure handling and response to comments from Joe.

Comment 26

[Boris Zbarsky [:bzbarsky]]

So how well does qcmstypes.h work when compiling on Windows with icc?  Do we have a working stdbool on OS/2?

Was there a reason we couldn't use the NSPR types here?

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Attachment: v8

- Fixes LITTLE_ENDIAN not being defined on windows.
- Fixes a current crasher on trunk that occurs when a png has an invalid profile. (http://people.mozilla.com/~jmuizelaar/color/bar-invalid.png)
- Makes the type definitions in qcmstypes.h more portable.
- Fix some possible crashes on invalid profile data
- The code should now also be malloc failure safe and invalid data safe.

Comment 28

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #26)
> So how well does qcmstypes.h work when compiling on Windows with icc?  Do we
> have a working stdbool on OS/2?

I've changed the code so that it matches libpixman which should compile with icc on windows. I've also fixed changed the definition of bool to something that should work on OS/2. Thanks for catching these.

> Was there a reason we couldn't use the NSPR types here?

I want to avoid any external dependencies so that the code can easily used by other projects.

Comment 29

[Boris Zbarsky [:bzbarsky]]

It's not clear to me that changing this one header would be a great burden to other projects....  but ok.  I guess it's just that we have several parts of Gecko that were meant to be "reusable"; no on is reusing them, and we're paying the costs for them being reusable.  So reusability on its own is not a very strong argument if the costs are high enough.  In this case, they probably aren't....

Comment 30

[Brendan Eich [:brendan]]

I agree with bz. If stdint.h is still not available or reliably correct on all our targets (notably Windows under MSVC) then we should not waste time duplicating it just for reusability, especially potentially reusability.

If we take code from an upstream that already does its own portable-stdint hacks, ok. That seems not to be the case here, which looks like a mistake -- at least a waste of non-trivial time not only for the code writer but for readers. And are the local stdint-workalike hacks really equivalent to others (see bug 465640, bug 461841, and possibly others)?

This duplication has ongoing costs.

/be

Comment 31

[Jeff Muizelaar [:jrmuizel]]

I don't really care how the types are defined, but I would really prefer to use the stdint names throughout the code. What's the suggested way of achieving this?

I also plan on maintaining qcms as an independent library after it has landed, so the existing stdint-workalike hacks will still have some value there.

Comment 32

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

Comment on attachment 370415 [details] [diff] [review]
v8

This looks good for checkin, with a few changes:

- get rid of all references to lcms in the build pieces; no need to link with mozlcms or to have lcms in REQUIRES lines
- A few pieces of code are #if 0'd.. they should either be removed entirely, or a comment added explaining why they're #if 0'd.
- for qcmstypes, add a #ifdef MOZ_... something, not sure what's a good thing to use, but something that'll let you #include prtypes.h and then typedef your types in terms of nspr types.  That way things will work both with nspr and without.

Comment 33

[Jim Blandy :jimb]

(In reply to comment #31)
> I don't really care how the types are defined, but I would really prefer to use
> the stdint names throughout the code. What's the suggested way of achieving
> this?

If there's an agreement that this is the way to go, here's how to do it:

configure.in should arrange to define HAVE_STDINT_H as appropriate.  Some makefile early in the build process should install some $OBJ/dist/include/X/mozstdint.h.  When HAVE_STDINT_H is not defined, configure.in should also find sizes of the appropriate type.  js/src/configure.ac does something like this now, using the macros from build/autoconf/moznbytetype.m4.

SpiderMonkey has some namespace constraints, explained in js/src/jsinttypes.h; I don't think those will apply in mozstdint.h's case, so things might be simpler.  js/src/jsstdint.h actually defines the <stdint.h> types as needed.

I've suggested a mozstdint.h that all Mozilla could use, but if it would make it easier to support distributing qcms on its own, qcms could have its own qcmsstdint.h header, driven by values derived by the top-level configure script or qcms's own configure.

Comment 34

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

Not worth it, IMO -- if we're building inside mozilla, we'll have prtypes.h, and if we're building outside it's easiest to make some assumptions.  If we end up with a mozstdint at some point then maybe we can switch, but I don't think getting that working should be done here.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Attachment: v9

- changes the public api so no stdint types are used so that we don't leak our definitions
- defines stdint like types using prtypes.h when building in the mozilla tree
- addresses vlad's review comments
- some general cleanup
- I don't know any reason this shouldn't land on trunk

Comment 36

[Jeff Muizelaar [:jrmuizel]]

Attachment: v10 - redo build stuff

This version redoes the build stuff so that qcms is built statically into thebes. I hope that it's mostly correct.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Attachment: v11 - a better try at the build stuff

fixes the --disable-libxul build, I'm still not sure that this correct.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

Attachment: v12 - some cleanup

A little bit of cleanup on the previous patch

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 370903 [details] [diff] [review]
v12 - some cleanup

+CSRCS = iccread.c transform.c $(NULL)

No need for $(NULL) here, it's only useful for multi-line variable values with line continuations. (It literally expands to nothing.)

# This library is used by other shared libs in a static build

Drop the "in a static build".

+#	gfxColorManagementTest.cpp \

Are you intending to fix this test, or just drop it? If you're going to fix it, please file a followup bug and mention the bug # in a comment here, otherwise just remove this line instead of commenting it out.

+++ b/toolkit/toolkit-makefiles.sh

I assume you should remove the MAKEFILES_lcms bits in here. (Or are you leaving that code in the tree for some reason?)

 tier_external_dirs	+= modules/lcms
+tier_external_dirs	+= gfx/qcms

Similarly here, unless you want us to continue to build lcms.

r=me on the build bits with those nits fixed.

Comment 40

[(not currently active) Ted Mielczarek]

Also, I asked on IRC why this is a security-sensitive bug, given that it doesn't describe any specific vulnerabilities. All the bugs it blocks seem to already be sensitive, so why keep this bug closed? Just curious.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Attachment: v13 - address build comments

The build stuff should be in order and this should be landable now.
The bug about the test case is 486730.

Comment 42

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/6f3c2171bbb2

Comment 43

[Joe Drew (not getting mail)]

Had to back this out, because of failures on the Linux leak boxes.

http://hg.mozilla.org/mozilla-central/rev/9c5d2510b266
http://hg.mozilla.org/mozilla-central/rev/ee138ab24b7a
http://hg.mozilla.org/mozilla-central/rev/099fab4fd620
http://hg.mozilla.org/mozilla-central/rev/b025b0c6e380

Comment 44

[Jeff Muizelaar [:jrmuizel]]

I can't reproduce these failure in my local linux vm, so this might be tricky...

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe see if valgrind tells you anything interesting?

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This also caused 2 reftest failures on Windows unit test:

REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-unittest/build/modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn2c08.png |
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-unittest/build/modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn3p08.png |

Comment 47

[Jeff Muizelaar [:jrmuizel]]

Attachment: v14 - fix test failures

This should fix the reftest failures on windows and the leak test failures on linux. 

Leaktests were failing because the code was assuming that malloc returned 16 byte aligned blocks. I've fixed the code to ensure alignment when it's needed.
This patch also fixes some 64bit correctness problems.

The reftests errors look like rounding differences, so I've made the reference html dependent on the platform.

Comment 48

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

You should probably use posix_memalign if available; see:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#88

http://www.opengroup.org/onlinepubs/000095399/functions/posix_memalign.html

Comment 49

[Jeff Muizelaar [:jrmuizel]]

Attachment: v15 - fix the case of Win32 in the reftest

I accidentally had navigator.platform == "win32" instead of navigator.platform == "Win32"

Comment 50

[Bobby Holley (:bholley)]

(In reply to comment #39)

> 
> 
> +#    gfxColorManagementTest.cpp \
> 
> Are you intending to fix this test, or just drop it? If you're going to fix it,
> please file a followup bug and mention the bug # in a comment here, otherwise
> just remove this line instead of commenting it out.
> 

I started the process of fixing up the LCMS testing framework in a spare night I had over winter break. The bug is bug 466875. If you want testing for QCMS it's probably a good place to start since it's all blackbox testing. I can finish when I come back in june if you like.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Wait.  Why is the rounding platform-dependent?

Comment 53

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #52)
> Wait.  Why is the rounding platform-dependent?

I'm not really sure. It could be because the Windows output profile is different, but I'll investigate...

Comment 54

[Jeff Muizelaar [:jrmuizel]]

Attachment: v16 - fix the sse2 test code on _MSC_VER

The code was testing for _MSC_VER_ when it should've been testing _MSC_VER. As a side effect, I've changed things so the reftest should pass on non-x86.

Comment 55

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

Comment on attachment 371292 [details] [diff] [review]
v16 - fix the sse2 test code on _MSC_VER

Looked at interdiff from v13; looks fine, but still no posix_memalign which makes me a little sad since the alignment code is a little scary.

Comment 56

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #48)
> You should probably use posix_memalign if available; see:
> 
> http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#88
> 
> http://www.opengroup.org/onlinepubs/000095399/functions/posix_memalign.html

Yeah, I'll do that in a later patch.

Comment 57

[Jeff Muizelaar [:jrmuizel]]

Attachment: v17 - fix some allocation failure bugs

Fixes some allocation failure handling bugs.

Comment 58

[Jeff Muizelaar [:jrmuizel]]

Attachment: v18 - fix the direction of the reftest checks

I had the order of the reftest condition reversed, this should fix it.

Comment 59

[Jeff Muizelaar [:jrmuizel]]

Attachment: v19 - fix reftest mistake

I got sloppy and had some tags left over in one of the reftests. This fixes that.

Comment 60

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/mozilla-central/rev/eae0178c3e15

Landed per Jeff's request, and solemn promise that if anything breaks, he will personally apologize to all 250M firefox users.

Comment 61

[Jim Mathies [:jimm]]

Looks like this patch blew up on thunderbird builds, there are a bunch of unresolved externals in qcms code. 

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1239122840.1239129430.30181.gz

Comment 62

[Jeff Muizelaar [:jrmuizel]]

What does thunderbird's build process do differently that would cause this?

Comment 63

[Jim Mathies [:jimm]]

bsmedberg mentioned something on irc about known issues with static builds?

Comment 64

[(not currently active) Ted Mielczarek]

It's a static build (--enable-static). Note that you can't actually build Firefox in that configuration, we unsupported it.

Comment 65

[(not currently active) Ted Mielczarek]

I think you probably just need to fix this:
http://mxr.mozilla.org/mozilla-central/source/config/static-config.mk

Comment 66

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix static build

Here's an untested patch that should fix the build

Comment 67

[(not currently active) Ted Mielczarek]

Comment on attachment 371734 [details] [diff] [review]
Fix static build

I'll defer to Standard8.

Comment 68

[Phil Ringnalda (:philor)]

Attachment: Port build bits to commcentral

I didn't think too hard about whether all of this was needed or right, but this plus attachment 371734 [details] [diff] [review] let me make it through a static build, which is nice.

Comment 69

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #68)
> Created an attachment (id=371813) [details]
> Port build bits to commcentral
> 
> I didn't think too hard about whether all of this was needed or right, but this
> plus attachment 371734 [details] [diff] [review] let me make it through a static build, which is nice.

This looks fine to me.

Comment 70

[Philippe M. Chiasson (:gozer)]

(In reply to comment #68)
> Created an attachment (id=371813) [details]
> Port build bits to commcentral
> 
> I didn't think too hard about whether all of this was needed or right, but this
> plus attachment 371734 [details] [diff] [review] let me make it through a static build, which is nice.

Tested with both patches, built just fine on Linux.

Comment 71

[Mark Banner (:standard8)]

Comment on attachment 371813 [details] [diff] [review]
Port build bits to commcentral

I decided to take the patch in bug 487252 as we've already been red for a few days, and no-one has requested approval for branch for this bug yet.

Comment 72

[Mark Banner (:standard8)]

Comment on attachment 371734 [details] [diff] [review]
Fix static build

Based on what gozer did in attachment 371916 [details] [diff] [review] on bug 487252 this looks to be the right thing to do. I can't test it as FF doesn't allow static these days and comm-central apps use their own static-config.mk

Comment 73

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

Jeff, can you resolve the dependent bugs here as appropriate?

Comment 74

[Johnathan Nightingale [:johnath]]

Landed the static linkage fix:

central - http://hg.mozilla.org/mozilla-central/rev/f64cafe80a27

I thought Jeff wanted this pushed to 1.9.1 as well, but then promptly backed it out, since QCMS hasn't landed there yet (see http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1eb2802dbdc9 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/61997f96366e)

Comment 75

[Jeff Muizelaar [:jrmuizel]]

The relevant revisions for pushing this to 1.9.1 are:
f64cafe80a27
25f74e0b80a6
eae0178c3e15

Comment 76

[Phil Ringnalda (:philor)]

If you roll those up into a single patch (especially if that patch doesn't get toolkit/toolkit-makefiles.sh wrong, since your makefile is actually in gfx/qcms/Makefile, not modules/qcms/Makefile, and I'd rather not need to bother with having to land bug 488117 on 1.9.1), then you'll massively increase the odds of both finding someone to land it, and having it land right the first time.

Comment 77

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

(I'm working on landing it)

Comment 78

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

Checked in on 1.9.1

Comment 79

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There are a couple of reftests in the patch. Does it mean they are enough to check this rewrite?

Comment 80

[georgi - hopefully not receiving bugspam]

speaking of floats:

#include <stdio.h>
int main() {
volatile int i=16777217;
volatile float f = 16777217.0f,f2;
f2=(float) i;
printf("f=%f %f %d\n",f,f2,i);
return(0);
}

output (note the last digit before '.'):

f=16777216.000000 16777216.000000 16777217