Bug 1016539 landed inverting colors and grayscale effect. This bug will cover some follow up changes and adding contrast. Contrast = 0 - unchanged color Contrast = -1 - everything is mid gray (0.5) Contrast > 0 - increasing contrast - values < 0.5 decrease, values > 0.5 increase, linearly, until we hit everything as black or white (because of clipping.)

Add more comments as to what we're actually doing...

Comment on attachment 8475619 [details] [diff] [review] Add contrast computation and rename some of the prefs. r=mwoodrow Review of attachment 8475619 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/LayerManagerComposite.cpp @@ +481,5 @@ > + matrix._33 = contrastPlus1*matrix._33; > + matrix._51 = contrastPlus1*matrix._51 - halfContrast; > + matrix._52 = contrastPlus1*matrix._52 - halfContrast; > + matrix._53 = contrastPlus1*matrix._53 - halfContrast; > + } Took me a while to convince myself this was correct. Markus is adding an operator* for Matrix5x4 in bug 1055661, I'd prefer if you used that. @@ +523,5 @@ > this->Dump(layersPacket); > LayerScope::SendLayerDump(Move(packet)); > } > > + if (gfxPrefs::LayersEffectInvert() || gfxPrefs::LayersEffectGrayscale() || gfxPrefs::LayersEffectContrast() != 0.0) { Add some newlines in here to keep us under 80-ish characters and stop it wrapping on my screen :) Same below.

(In reply to Matt Woodrow (:mattwoodrow) from comment #3) > Comment on attachment 8475619 [details] [diff] [review] > Add contrast computation and rename some of the prefs. r=mwoodrow > > Review of attachment 8475619 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/composite/LayerManagerComposite.cpp > @@ +481,5 @@ > > + matrix._33 = contrastPlus1*matrix._33; > > + matrix._51 = contrastPlus1*matrix._51 - halfContrast; > > + matrix._52 = contrastPlus1*matrix._52 - halfContrast; > > + matrix._53 = contrastPlus1*matrix._53 - halfContrast; > > + } > > Took me a while to convince myself this was correct. Markus is adding an > operator* for Matrix5x4 in bug 1055661, I'd prefer if you used that. Nice! I had it all written out on paper to convince myself of the previous ones as well. I'll make this depend on bug 1055661 and use that. > > @@ +523,5 @@ > > this->Dump(layersPacket); > > LayerScope::SendLayerDump(Move(packet)); > > } > > > > + if (gfxPrefs::LayersEffectInvert() || gfxPrefs::LayersEffectGrayscale() || gfxPrefs::LayersEffectContrast() != 0.0) { > > Add some newlines in here to keep us under 80-ish characters and stop it > wrapping on my screen :) Same below. Sounds good. Thanks for the quick turnaround.

I'll ignore the r+ on the previous patch and ask for another review, as I also made changes beyond the suggested ones (albeit fairly cosmetic.) I just don't want to go too far with asserts, explicit method names, etc., so a sanity check would help. https://tbpl.mozilla.org/?tree=Try&rev=4b140534a5a1

Note: the patch above does require bug 1055661 to land.

Comment on attachment 8476216 [details] [diff] [review] Add contrast computation and rename some of the prefs, rename some methods and add comments for clarity.. r=mwoodrow Review of attachment 8476216 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/LayerManagerComposite.cpp @@ +469,5 @@ > + // B' = 1 - B > + Matrix5x4 colorInvertMatrix(-1, 0, 0, 0, > + 0, -1, 0, 0, > + 0, 0, -1, 0, > + 0, 0, 0, 1, Alignment! @@ +471,5 @@ > + 0, -1, 0, 0, > + 0, 0, -1, 0, > + 0, 0, 0, 1, > + 1, 1, 1, 0); > + effectMatrix = effectMatrix * colorInvertMatrix; You could add operator*= to Matrix5x4 here, doesn't matter much though. @@ +539,5 @@ > > + /** Our more efficient but less powerful alter ego, if one is available. */ > + nsRefPtr<Composer2D> composer2D; > + > + // We don't need composer2D if we have layer effects 'can't use' rather than 'don't need'

(In reply to Matt Woodrow (:mattwoodrow) from comment #7) > Comment on attachment 8476216 [details] [diff] [review] > ... > > + effectMatrix = effectMatrix * colorInvertMatrix; > > You could add operator*= to Matrix5x4 here, doesn't matter much though. Already suggested it in bug 1055661, lets see if Markus bites :)

Address review comments, carry r=mattwoodrow

Link to moztrap test case: https://moztrap.mozilla.org/manage/case/14505/

This is also affected by Bug 1057461

This issue has been verified on Flame 2.2 and Flame 2.1: Flame 2.2 KitKat Base (319mb)(Full Flash) Environmental Variables: Device: Flame 2.2 Master BuildID: 20141006040204 Gaia: 470826d13ae130a5c3d572d1029e595105485fb0 Gecko: e0d714f43edc Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf Version: 35.0a1 (2.2 Master) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Flame 2.1 KitKat Base (319mb)(Full Flash) Environmental Variables: Device: Flame 2.1 BuildID: 20141006000205 Gaia: 778ebac47554e1c4b7e9a952d73e850f58123914 Gecko: c4a4b04c617c Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf Version: 34.0a2 (2.1) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Contrast on the Accessibility menu is implemented and fully functional.