Closed
Bug 1058756
Opened 10 years ago
Closed 10 years ago
[CSS/SVG Filters] Use consistent values for luminance in FilterSupport.cpp
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: mvujovic, Assigned: mvujovic)
References
Details
Attachments
(1 file, 1 obsolete file)
6.79 KB,
patch
|
mvujovic
:
checkin+
|
Details | Diff | Splinter Review |
Per the conversation here: https://bugzilla.mozilla.org/show_bug.cgi?id=948265#c234
Assignee | ||
Comment 1•10 years ago
|
||
What do you think about this refactoring? I kept the interpolation logic from a matrix to the identity matrix similar to before. For example: aOutMatrix[0] = aStartMatrix[0] + (1 - aStartMatrix[0]) * aAmount; I realized it's a little different than typical interpolation would be: aOutMatrix[0] = aStartMatrix[0] * (1 - aAmount) + 1 * aAmount; Assuming aStartMatrix[0] is in the range [0,1], the original logic creates a positive value when aAmount > 1. Typical interpolation creates a negative value when aAmount > 1. The visible effect of the original interpolation logic is when you set a very large saturation factor, you only start seeing colors with RGB channels = 1 or = 0. Typical interpolation makes those channels negative (and clamped to zero), so you see a lot of black instead.
Attachment #8483065 -
Flags: review?(milan)
Comment 2•10 years ago
|
||
Comment on attachment 8483065 [details] [diff] [review] Patch Review of attachment 8483065 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! I'm not official on review+ :), so I'll stick with the feedback+. A few minor comments mostly on the code documentation. ::: gfx/src/FilterSupport.cpp @@ +314,5 @@ > > +static void > +InterpolateToIdentityMatrix(const float aStartMatrix[20], float aAmount, > + float aOutMatrix[20]) > +{ This is the case, right? aAmount == 0 -> aOutMatrix == aStartMatrix aAmount == 1 -> aOutMatrix == Identity Probably worth a comment. @@ +326,5 @@ > + > + aOutMatrix[0] = aStartMatrix[0] + (1 - aStartMatrix[0]) * aAmount; > + aOutMatrix[1] = aStartMatrix[1] - aStartMatrix[1] * aAmount; > + aOutMatrix[2] = aStartMatrix[2] - aStartMatrix[2] * aAmount; > + Or... aOutMatrix[0] = (1-aAmount)*aStartMatrix[0] + aAmount; aOutMatrix[1] = (1-aAmount)*aStartMatrix[1]; aOutMatrix[2] = (1-aAmount)*aStartMatrix[2]; Given how the computation is setup, with a lot of the (1-aAmount) multiplying, you may want to consider having aAmount be the inverse, so that: aAmount == 0 -> aOutMatrix == Identity aAmount == 1 -> aOutMatrix == aStartMatrix but it doesn't really matter in the end... @@ +334,5 @@ > + > + aOutMatrix[10] = aStartMatrix[10] - aStartMatrix[10] * aAmount; > + aOutMatrix[11] = aStartMatrix[11] - aStartMatrix[11] * aAmount; > + aOutMatrix[12] = aStartMatrix[12] + (1 - aStartMatrix[12]) * aAmount; > +} The assumption here is that aStartMatrix elements 3,4,8,9,13,14,15,16,17,19 are all zero, and element 18 is 1, and the above is a shortcut of a full matrix addition and scalar multiply. Probably worth a comment in the function, just in case somebody comes along and wants to generalize it.
Attachment #8483065 -
Flags: review?(mstange)
Attachment #8483065 -
Flags: review?(milan)
Attachment #8483065 -
Flags: feedback+
Comment 3•10 years ago
|
||
Comment on attachment 8483065 [details] [diff] [review] Patch What Milan said.
Attachment #8483065 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #2) > Comment on attachment 8483065 [details] [diff] [review] > Patch > > Review of attachment 8483065 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very nice! I'm not official on review+ :), so I'll stick with the > feedback+. A few minor comments mostly on the code documentation. Great suggestions, thanks Milan :) > > ::: gfx/src/FilterSupport.cpp > @@ +314,5 @@ > > > > +static void > > +InterpolateToIdentityMatrix(const float aStartMatrix[20], float aAmount, > > + float aOutMatrix[20]) > > +{ > > This is the case, right? > > aAmount == 0 -> aOutMatrix == aStartMatrix > aAmount == 1 -> aOutMatrix == Identity > > Probably worth a comment. Yes, that's true in the first patch. Based on your next comment, I've now made the interpolation go the opposite way- from the identity matrix to the passed-in matrix. I've also renamed the function to "Interpolate*From*IdentityMatrix" and the passed-in matrix to "aToMatrix". I also put a comment above the function: // When aAmount == 0, the identity matrix is returned. // When aAmount == 1, aToMatrix is returned. // When aAmount > 1, an exaggerated version of aToMatrix is returned. This can // be useful in certain cases, such as producing a color matrix to oversaturate // an image. > > @@ +326,5 @@ > > + > > + aOutMatrix[0] = aStartMatrix[0] + (1 - aStartMatrix[0]) * aAmount; > > + aOutMatrix[1] = aStartMatrix[1] - aStartMatrix[1] * aAmount; > > + aOutMatrix[2] = aStartMatrix[2] - aStartMatrix[2] * aAmount; > > + > > Or... > aOutMatrix[0] = (1-aAmount)*aStartMatrix[0] + aAmount; > aOutMatrix[1] = (1-aAmount)*aStartMatrix[1]; > aOutMatrix[2] = (1-aAmount)*aStartMatrix[2]; Yes, I like this reduction. I also swapped (1 - aAmount) with aAmount like you suggest, so it looks like this: aOutMatrix[0] = aAmount * aToMatrix[0] + oneMinusAmount; aOutMatrix[1] = aAmount * aToMatrix[1]; aOutMatrix[2] = aAmount * aToMatrix[2]; > > Given how the computation is setup, with a lot of the (1-aAmount) > multiplying, you may want to consider having aAmount be the inverse, so that: > aAmount == 0 -> aOutMatrix == Identity > aAmount == 1 -> aOutMatrix == aStartMatrix > > but it doesn't really matter in the end... Done! > > @@ +334,5 @@ > > + > > + aOutMatrix[10] = aStartMatrix[10] - aStartMatrix[10] * aAmount; > > + aOutMatrix[11] = aStartMatrix[11] - aStartMatrix[11] * aAmount; > > + aOutMatrix[12] = aStartMatrix[12] + (1 - aStartMatrix[12]) * aAmount; > > +} > > The assumption here is that aStartMatrix elements 3,4,8,9,13,14,15,16,17,19 > are all zero, and element 18 is 1, and the above is a shortcut of a full > matrix addition and scalar multiply. > Probably worth a comment in the function, just in case somebody comes along > and wants to generalize it. Good idea. I like your wording, and I put a comment above the function: // This function is a shortcut of a full matrix addition and a scalar multiply, // and it assumes that the following elements in aToMatrix are 0 and 1: // x x x 0 0 // x x x 0 0 // x x x 0 0 // 0 0 0 1 0 I put the comment on top of the function instead of inside since it describes assumptions about the input. I'll land this tomorrow (thanks for the r+, Markus!) unless there are more ideas :)
Attachment #8483065 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
(In reply to Max Vujovic from comment #4) > ... > > I'll land this tomorrow (thanks for the r+, Markus!) unless there are more > ideas :) Awesome!
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b60964c73f7 https://tbpl.mozilla.org/?tree=Try&rev=b00d1e2c72db
Assignee | ||
Updated•10 years ago
|
Attachment #8483781 -
Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/4b60964c73f7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•