rewrite color management component

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: vlad, Assigned: jrmuizel)

Tracking

(Depends on 1 bug, {fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse meta])

Attachments

(2 attachments, 19 obsolete attachments)

265.30 KB, patch
Details | Diff | Splinter Review
380 bytes, patch
standard8
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [sg:nse meta]
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?
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''
It compiles and runs but I'm not sure how well it works yet.
Posted patch v2 (obsolete) — Splinter Review
A little build fix
Attachment #368648 - Attachment is obsolete: true
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?
Posted patch v3 (obsolete) — Splinter Review
Rough in support for precaching and sse2.
Attachment #368649 - Attachment is obsolete: true
(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.
(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.
(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.
Posted patch v4 (obsolete) — Splinter Review
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.
Attachment #368942 - Attachment is obsolete: true
+	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
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.
http://docs.sun.com/source/806-3568/ncg_goldberg.html
What Every Computer Scientist Should Know About Floating-Point Arithmetic
nit:

a.c:
//comment
gcc -pedantic a.c
a.c:28:1: warning: C++ style comments are not allowed in ISO C90
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)
I think the == 0 comparison is fine here, since its only intend seems to be preventing divide-by-zero....  perhaps I'm missing something.
>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.
(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.
Posted patch v5 (obsolete) — Splinter Review
A couple of fixes along with patches to the reftests.
Attachment #369420 - Attachment is obsolete: true
Attachment #369578 - Flags: review?(bobbyholley)
Attachment #369578 - Flags: review?(joe)
>> 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"
#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
printf("%e \n",det);

./a.out 
det != 0
-1.192093e-07
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)
Posted patch v6 (obsolete) — Splinter Review
Addresses some comments from Joe, fixes a unhandled malloc failure case and adds a license header. I used the same license as lcms.
Attachment #369578 - Attachment is obsolete: true
Attachment #369707 - Flags: review?(bobbyholley)
Attachment #369578 - Flags: review?(joe)
Attachment #369578 - Flags: review?(bobbyholley)
Posted patch v7 (obsolete) — Splinter Review
Update with more malloc failure handling and response to comments from Joe.
Attachment #369707 - Attachment is obsolete: true
Attachment #369759 - Flags: review?(bobbyholley)
Attachment #369707 - Flags: review?(bobbyholley)
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?
Posted patch v8 (obsolete) — Splinter Review
- 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.
Attachment #369759 - Attachment is obsolete: true
Attachment #370415 - Flags: review?(bobbyholley)
Attachment #369759 - Flags: review?(bobbyholley)
(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.
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....
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
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 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.
(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.
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.
Posted patch v9 (obsolete) — Splinter Review
- 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
Attachment #370415 - Attachment is obsolete: true
Attachment #370415 - Flags: review?(bobbyholley)
Posted patch v10 - redo build stuff (obsolete) — Splinter Review
This version redoes the build stuff so that qcms is built statically into thebes. I hope that it's mostly correct.
Attachment #370700 - Attachment is obsolete: true
fixes the --disable-libxul build, I'm still not sure that this correct.
Attachment #370794 - Attachment is obsolete: true
Posted patch v12 - some cleanup (obsolete) — Splinter Review
A little bit of cleanup on the previous patch
Attachment #370878 - Attachment is obsolete: true
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.
Attachment #370903 - Flags: review+
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.
Posted patch v13 - address build comments (obsolete) — Splinter Review
The build stuff should be in order and this should be landable now.
The bug about the test case is 486730.
Attachment #370903 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/6f3c2171bbb2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
I can't reproduce these failure in my local linux vm, so this might be tricky...
Maybe see if valgrind tells you anything interesting?
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 |
Posted patch v14 - fix test failures (obsolete) — Splinter Review
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.
Attachment #370924 - Attachment is obsolete: true
I accidentally had navigator.platform == "win32" instead of navigator.platform == "Win32"
Attachment #371063 - Attachment is obsolete: true
Keywords: checkin-needed
(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.
Duplicate of this bug: 468576
Wait.  Why is the rounding platform-dependent?
(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...
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.
Attachment #371080 - Attachment is obsolete: true
Attachment #371292 - Flags: review?(vladimir)
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.
Attachment #371292 - Flags: review?(vladimir) → review+
(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.
Fixes some allocation failure handling bugs.
Attachment #371292 - Attachment is obsolete: true
I had the order of the reftest condition reversed, this should fix it.
Attachment #371308 - Attachment is obsolete: true
I got sloppy and had some tags left over in one of the reftests. This fixes that.
Attachment #371377 - Attachment is obsolete: true
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 487252
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
What does thunderbird's build process do differently that would cause this?
bsmedberg mentioned something on irc about known issues with static builds?
It's a static build (--enable-static). Note that you can't actually build Firefox in that configuration, we unsupported it.
Here's an untested patch that should fix the build
Attachment #371734 - Flags: review?(ted.mielczarek)
Comment on attachment 371734 [details] [diff] [review]
Fix static build

I'll defer to Standard8.
Attachment #371734 - Flags: review?(ted.mielczarek) → review?(bugzilla)
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.
Attachment #371813 - Flags: review?(bugzilla)
(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.
(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.
Depends on: 487566
Depends on: 487754
Depends on: 487765
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.
Attachment #371813 - Attachment is obsolete: true
Attachment #371813 - Flags: review?(bugzilla)
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
Attachment #371734 - Flags: review?(bugzilla) → review+
Jeff, can you resolve the dependent bugs here as appropriate?
Flags: wanted1.9.0.x?
Flags: blocking1.9.1+
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)
The relevant revisions for pushing this to 1.9.1 are:
f64cafe80a27
25f74e0b80a6
eae0178c3e15
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.
(I'm working on landing it)
Depends on: 488468
Depends on: 488747
Depends on: 488800
Depends on: 488955
Depends on: 489133
There are a couple of reftests in the patch. Does it mean they are enough to check this rewrite?
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
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
No longer blocks: C191ConfSync
Blocks: 458719
You need to log in before you can comment on or make changes to this bug.