33589 - opacity loses precision in nsBlender

Status: VERIFIED INVALID

(Core Graveyard :: GFX, defect, P3)

Description

[Chris Hill]

opacity is passed to blender as a floating point number [0.0,1.0] (as it is 
specified in CSS).  Inside nsBlender::Blend it gets converted to an integer 
[0,100] but before it is actually used it gets converted to [0,255].  It seems 
like it shouldn't ever get converted below the precision of the final blend 
computation [0,255].

Instead of [0.0,1.0]->[0,100]->[0,255]
it should be [0.0,1.0]->[0,255]

If there is some reason that the internal precision of the blend shouldn't be 
revealed, please use a number bigger than 255 such as 100000.
[0.0,1.0]->[0,100000]->[0,255]

Comment 1

[Kevin McCluskey (gone)]

reassigning to dcone

Comment 2

[Gervase Markham [:gerv]]

[e-mail] - is this bug still present in recent builds of Mozilla?

Gerv

Comment 3

[Chris Hill]

Attachment: testcase

Comment 4

[Chris Hill]

Yes, this bug is still present in nsBlender.cpp.  See the test case I just 
attached.  There is a description in a comment in the attached HTML test.

I discovered this bug by looking at the source.  It isn't very serious but it is 
also very easy to fix.

Comment 5

[Gervase Markham [:gerv]]

Confirming. Chris Hill - do you want to make a patch and attach it?

Gerv

Comment 6

[Chris Hill]

Attachment: patch to nsBlender.cpp to fix precision problem

Comment 7

[dcone (gone)]

Actually the patch will loose precision.. mostly because of the number you have 
choosen.  For example 255 * .17 = 43.35.  This value is rounded before it gets 
to the blend. Having non zero values in the ones and 10's place will give 
numbers that get rounded.
100 * .17 is 17.00, rounded to 17 which gets passed to the blend.  Then 
multiplying by 255 and dividing by 100 
gives you 43.35, no loss of precision, because the 100 will give you precision 
to the 100th's place, which should be enough precision.  I don't think we will 
blend at 17.5 percent.  If the percentages are not decimal.. then there will be 
no roundoff. 100 is a good number for this because of the two 0's which gives 
you two places of precision, 1000 would be better if this value was going to be 
17.5 percent for example, or three places of precision.

Comment 8

[Chris Hill]

The blending happens with numbers in the range [0..255]  Is there some reason to 
only use 101 of the 256 possible values?  Using 255 for values that have more 
than 2 decimal places the result isn't exact, but it is more ACCURATE.  I think 
I made the sin of using the word precision when I meant accuracy.

Taking your example:

255*.17 = 43.35 -> Truncation -> 43

100*.17 = 17.00 -> Truncation -> 17
17*255 = 4335
4335/100 = 43 (Remember this is integer division)

So both give the same result.

For (0.00,0.01,....,1.00) both computations give the same result.  It is the 
other numbers that I am concerned about.  You said "I don't think we will blend 
at 17.5 percent"  Well, I want to get as close as 0..255 allows instead of using 
the closest integer percent.  The result will not be worse. I think the accuracy 
makes up for the loss in precision.

Take .175:

255*.175 = 44.625 -> Truncation -> 44

100*.175 = 17.50 -> Truncation -> 17
17*255 = 4335
4335/100 = 43

44/255 = 0.1725, (0.1750-0.1725)=0.0025
43/255 = 0.1686, (0.1750-0.1686)=0.0064

1000 is better than 100, but remember that ultimately the numbers are converted 
back to 0..255 for blending.  Using 1000 instead of 100 will work, but it gives 
the same results as 255.  Using 1000 would also require changes to the types of 
the parameters to the Blend functions (it is currently an unsigned 8-bit 
integer)

Again Take .175:

255*.175 = 44.625 -> Truncation -> 44

1000*.175 = 175.0 -> Truncation -> 175
175*255 = 44625
44625/1000 = 44

Comment 9

[dcone (gone)]

My point was this, if you multiply by 255 before you pass in the percentage..it 
gets rounded off.  If you multiply by 100 before you pass in the percentage.. it 
does not get rounded off because of the 0's in the ones and tens position.  So 
then the actual blend routines can use or does not have to use this precision.

The next point is that you are correct to increase the precision if we are going 
to blend at 17.5 percent.  In that case I would prefer to pass in floats instead 
of integers so the precision and correctness is left to the actual blend routine 
and not to the routine that calls it.  The blend was designed to take 
percentages as input (2 places of presision).  

So if the bug becomes (or is) that we do not support fractional percentages 
(17.5 percent), that is correct.  But we do support non fractional percentages 
without loss of precision as they are passed into the blender.

Comment 10

[Chris Hill]

I agree that the exact precision should be left to the actual routine (as I 
mentioned in the initial comment when I filed the bug).  The data is passed 
through the public interface as a float, what happens later is for the 
implementation to decide.  Floating point seems like overkill, but it would 
address the problem.  I am not comparing my patch to other possible changes, I 
am comparing it to the current implementation.  The current system converts 
fractional percentages to nonfractional percentages.

I think my proposal is more reasonable than the current system.  
If:
1) The current system "doesn't support" fractional percentages 
2) My patch doesn't change the behavior for the supported percentages
3) My patch is more accurate for unsupported percentages
then there is no reason not to apply it right?  Or do you think that being 
precise is more important than being accurate here?  If you do, I guess we'll 
have to agree to disagree.

I don't want to get caught up in semantics here or deciding exactly what this 
bug is really about.  I think we both understand what the issue is, and if you 
think the current behavior is better than my proposal I should stop wasting my 
time.

Comment 11

[Chris Petersen]

Marking verified invalid.

Comment 12

[[e-mail]]

Why is this INVALID? Either opacity should be passed to the blender [0.0,1.0] as
specified in CSS, or the precision and dynamic range should be preserved into
the blender where we know full well it will be [0,255]
The use of [0,100] costs us two pointless arithmetic operations and makes
Mozilla mysteriously imprecise in its CSS implementation.
This is a low priority but isn't "INVALID", I have no power to fix that in
Bugzilla so I leave it to the powers that be.

Comment 13

[dcone (gone)]

There is another bug that addresses changing the API's for the values passed 
into the blender.  This one is invalid.. the other takes care of the API's for 
passing in the blender values.