Closed
Bug 481926
Opened 15 years ago
Closed 15 years ago
rewrite color management component
Categories
(Core :: Graphics: Color Management, defect)
Core
Graphics: Color Management
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: vlad, Assigned: jrmuizel)
References
Details
(Keywords: fixed1.9.1, Whiteboard: [sg:nse meta])
Attachments
(2 files, 19 obsolete files)
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.
Updated•15 years ago
|
Whiteboard: [sg:nse meta]
Comment 1•15 years ago
|
||
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•15 years ago
|
||
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''
Assignee | ||
Comment 3•15 years ago
|
||
It compiles and runs but I'm not sure how well it works yet.
Assignee | ||
Comment 4•15 years ago
|
||
A little build fix
Attachment #368648 -
Attachment is obsolete: true
Comment 5•15 years ago
|
||
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?
Assignee | ||
Comment 6•15 years ago
|
||
Rough in support for precaching and sse2.
Attachment #368649 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
(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•15 years ago
|
||
(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.
Reporter | ||
Comment 9•15 years ago
|
||
(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.
Assignee | ||
Comment 10•15 years ago
|
||
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
Comment 11•15 years ago
|
||
+ 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•15 years ago
|
||
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•15 years ago
|
||
http://docs.sun.com/source/806-3568/ncg_goldberg.html What Every Computer Scientist Should Know About Floating-Point Arithmetic
Comment 14•15 years ago
|
||
nit: a.c: //comment gcc -pedantic a.c a.c:28:1: warning: C++ style comments are not allowed in ISO C90
Comment 15•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
>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.
Assignee | ||
Comment 18•15 years ago
|
||
(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.
Assignee | ||
Comment 19•15 years ago
|
||
A couple of fixes along with patches to the reftests.
Attachment #369420 -
Attachment is obsolete: true
Attachment #369578 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•15 years ago
|
Attachment #369578 -
Flags: review?(joe)
Comment 20•15 years ago
|
||
>> 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•15 years ago
|
||
#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•15 years ago
|
||
printf("%e \n",det); ./a.out det != 0 -1.192093e-07
Comment 23•15 years ago
|
||
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)
Assignee | ||
Comment 24•15 years ago
|
||
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)
Assignee | ||
Comment 25•15 years ago
|
||
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)
Comment 26•15 years ago
|
||
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?
Assignee | ||
Comment 27•15 years ago
|
||
- 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)
Assignee | ||
Comment 28•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
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
Assignee | ||
Comment 31•15 years ago
|
||
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.
Reporter | ||
Updated•15 years ago
|
Attachment #370415 -
Flags: superreview+
Reporter | ||
Comment 32•15 years ago
|
||
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•15 years ago
|
||
(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.
Reporter | ||
Comment 34•15 years ago
|
||
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.
Assignee | ||
Comment 35•15 years ago
|
||
- 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)
Assignee | ||
Comment 36•15 years ago
|
||
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
Assignee | ||
Comment 37•15 years ago
|
||
fixes the --disable-libxul build, I'm still not sure that this correct.
Attachment #370794 -
Attachment is obsolete: true
Assignee | ||
Comment 38•15 years ago
|
||
A little bit of cleanup on the previous patch
Attachment #370878 -
Attachment is obsolete: true
Comment 39•15 years ago
|
||
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+
Comment 40•15 years ago
|
||
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.
Assignee | ||
Comment 41•15 years ago
|
||
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
Comment 42•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6f3c2171bbb2
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Group: core-security
Comment 43•15 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 44•15 years ago
|
||
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 |
Assignee | ||
Comment 47•15 years ago
|
||
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
Reporter | ||
Comment 48•15 years ago
|
||
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
Assignee | ||
Comment 49•15 years ago
|
||
I accidentally had navigator.platform == "win32" instead of navigator.platform == "Win32"
Attachment #371063 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 50•15 years ago
|
||
(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•15 years ago
|
||
Wait. Why is the rounding platform-dependent?
Assignee | ||
Comment 53•15 years ago
|
||
(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...
Assignee | ||
Comment 54•15 years ago
|
||
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
Updated•15 years ago
|
Attachment #371292 -
Flags: review?(vladimir)
Reporter | ||
Comment 55•15 years ago
|
||
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+
Assignee | ||
Comment 56•15 years ago
|
||
(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.
Assignee | ||
Comment 57•15 years ago
|
||
Fixes some allocation failure handling bugs.
Attachment #371292 -
Attachment is obsolete: true
Assignee | ||
Comment 58•15 years ago
|
||
I had the order of the reftest condition reversed, this should fix it.
Attachment #371308 -
Attachment is obsolete: true
Assignee | ||
Comment 59•15 years ago
|
||
I got sloppy and had some tags left over in one of the reftests. This fixes that.
Attachment #371377 -
Attachment is obsolete: true
Comment 60•15 years ago
|
||
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: 15 years ago → 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 61•15 years ago
|
||
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
Assignee | ||
Comment 62•15 years ago
|
||
What does thunderbird's build process do differently that would cause this?
Comment 63•15 years ago
|
||
bsmedberg mentioned something on irc about known issues with static builds?
Comment 64•15 years ago
|
||
It's a static build (--enable-static). Note that you can't actually build Firefox in that configuration, we unsupported it.
Comment 65•15 years ago
|
||
I think you probably just need to fix this: http://mxr.mozilla.org/mozilla-central/source/config/static-config.mk
Assignee | ||
Comment 66•15 years ago
|
||
Here's an untested patch that should fix the build
Attachment #371734 -
Flags: review?(ted.mielczarek)
Comment 67•15 years ago
|
||
Comment on attachment 371734 [details] [diff] [review] Fix static build I'll defer to Standard8.
Attachment #371734 -
Flags: review?(ted.mielczarek) → review?(bugzilla)
Comment 68•15 years ago
|
||
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)
Assignee | ||
Comment 69•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
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 72•15 years ago
|
||
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+
Reporter | ||
Comment 73•15 years ago
|
||
Jeff, can you resolve the dependent bugs here as appropriate?
Flags: wanted1.9.0.x?
Flags: blocking1.9.1+
Comment 74•15 years ago
|
||
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)
Assignee | ||
Comment 75•15 years ago
|
||
The relevant revisions for pushing this to 1.9.1 are: f64cafe80a27 25f74e0b80a6 eae0178c3e15
Comment 76•15 years ago
|
||
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.
Reporter | ||
Comment 77•15 years ago
|
||
(I'm working on landing it)
Comment 79•15 years ago
|
||
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
Comment 80•15 years ago
|
||
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
Updated•15 years ago
|
Blocks: C191ConfSync
Updated•15 years ago
|
No longer blocks: C191ConfSync
Reporter | ||
Updated•12 years ago
|
Flags: wanted1.9.0.x?
You need to log in
before you can comment on or make changes to this bug.
Description
•