1431466 - Add a setter for custom color transforms

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P3)

Description

[Eitan Isaacson [:eeejay]]

We need a way to apply color filters on an entire viewport. The idea is to allow extensions, and hopefully core UI, to adjust the colors to the users needs. This is an accessibility feature.

More background is available on this page: https://wiki.mozilla.org/Accessibility/ColorFilters

I already proposed a change to WebRender that would add a ColorMatrix filter operation. https://github.com/servo/webrender/issues/2297

The scope of this bug is adding a way to apply that color transformation to a viewport.

Why not do this in CSS? Some notes:

* Because of stacking context issues it is not enough to apply a CSS filter to a body or documentElement. It needs to be applied to the containing browser element in the parent doc for the filter to be applied to everything.
* An SVG <feColorMatrix> filter is also not an option at this time since it is not accelerated, and substantial work needs to be done to make it so.
* Originally I looked into doing this with layout with some kind of special filter function (-moz-color-transform). But it ended up being a pretty extensive patch with the undesirable effect of adding more prefixed CSS.

I found that the easiest, least disruptive, way of adding this feature would be through a setting in the dochshell. If there is a more agreeable way to do this, let me know!

Comment 1

[Eitan Isaacson [:eeejay]]

To be clear, this setter will be exposed in a followup WebExtensions API proposal I will make.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: expose colormatrix in docshell

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8943697 [details] [diff] [review]
expose colormatrix in docshell

>+nsDocShell::SetColorMatrix(float* aMatrix, uint32_t aMatrixLen) {
Nit, { goes to its own line.

Adding a method to docshell should fine, though I don't know how extensions get access to that.
Which docshell would they access? The top one in parent process? Or top one in each tab?

> 
>+  nsTArray<float>& GetColorMatrix() {
Nit, { goes to its own line

>+++ b/layout/painting/nsDisplayList.cpp
>@@ -2441,19 +2441,30 @@ already_AddRefed<LayerManager> nsDisplayList::PaintRoot(nsDisplayListBuilder* aB
>         if (!layerManager->BeginTransaction()) {
>           return nullptr;
>         }
>       }
>     }
> 
>     WebRenderLayerManager* wrManager = static_cast<WebRenderLayerManager*>(layerManager.get());
> 
>+    nsCOMPtr<nsIDocShell> docShell = presContext->GetDocShell();
>+    nsTArray<wr::WrFilterOp> wrFilters;
>+    nsTArray<float>& colorMatrix = nsDocShell::Cast(docShell)->GetColorMatrix();
>+    if (colorMatrix.Length() == 20) {
>+      wr::WrFilterOp gs = {
>+        wr::WrFilterOpType::ColorMatrix
>+      };
>+      memcpy(&(gs.matrix), colorMatrix.Elements(), 20*sizeof(float));
>+      wrFilters.AppendElement(gs);
>+    }
I'm not familiar with this code, but if this is rather hot, as I assume, I'd use just raw pointer for nsIDocShell in order to avoid
useless AddRef/Release. And could nsTArray<wr::WrFilterOp> be create only on demand? Not sure if nsTArray ctor/dtor would show up in profiles.


Someone who understand display list handling needs to take a look.

Comment 4

[Eitan Isaacson [:eeejay]]

Thanks for the feedback! Looping back into WebRender to finalize an API before proposing a followup patch.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #3)
> And could nsTArray<wr::WrFilterOp> be create only on
> demand? Not sure if nsTArray ctor/dtor would show up in profiles.
> 

I think it is created as a default empty array anyway here:
https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/gfx/layers/wr/StackingContextHelper.h#32

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #3)
> Adding a method to docshell should fine, though I don't know how extensions
> get access to that. Which docshell would they access? The top one in parent process? Or top one
> in each tab?
> 

The docshell in each tab, separately. I have a proposed webext patch in bug 1431473.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Expose colormatrix in docshell. r?kats

Assuming kats is a good reviewer here?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8950749 [details] [diff] [review]
Expose colormatrix in docshell. r?kats

Review of attachment 8950749 [details] [diff] [review]:
-----------------------------------------------------------------

I think somebody else should review the docshell bits, I'm not sure if that's an appropriate place to be storing this. But mostly the patch looks sane, see some comments below.

::: docshell/base/nsDocShell.cpp
@@ +14438,5 @@
> +
> +NS_IMETHODIMP
> +nsDocShell::SetColorMatrix(float* aMatrix, uint32_t aMatrixLen)
> +{
> +  NS_ENSURE_TRUE(aMatrixLen == 20 || aMatrixLen == 0, NS_ERROR_INVALID_ARG);

Today I discovered NS_ENSURE_TRUE was deprecated 4 years ago and the suggested replacement doesn't do the same thing at all. Ha!

@@ +14460,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  presShell->FrameNeedsReflow(frame, nsIPresShell::eStyleChange,
> +                              NS_FRAME_IS_DIRTY);

Does it actually need reflow? I feel like a frame->SchedulePaint() should be sufficient since changing the color matrix shouldn't cause layout positioning to change.

::: docshell/base/nsDocShell.h
@@ +995,5 @@
>    // the LOAD_NORMAL_ALLOW_MIXED_CONTENT flag is set.
>    // Checked in nsMixedContentBlocker, to see if the channels match.
>    nsCOMPtr<nsIChannel> mMixedContentChannel;
>  
> +  nsTArray<float> mColorMatrix;

I'd prefer to see this stored as a gfx::Matrix5x4 since that is supposed to also represent a color matrix. Likewise GetColorMatrix would return a Matrix5x4 and the code in nsDisplayList would read the elements from matrix.components field and put them into the filter struct.

::: layout/painting/nsDisplayList.cpp
@@ +2666,5 @@
> +    if (colorMatrix.Length() == 20) {
> +      wr::WrFilterOp gs = {
> +        wr::WrFilterOpType::ColorMatrix
> +      };
> +      memcpy(&(gs.matrix), colorMatrix.Elements(), 20*sizeof(float));

Would be good to add some assertions here to the effect that sizeof(gs.matrix) == 20*sizeof(float), so that if something changes we don't accidentally turn this into memory corruption.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> 
> I think somebody else should review the docshell bits, I'm not sure if
> that's an appropriate place to be storing this.

Oh never mind, I didn't see smaug already reviewed that part. So yeah I can give final r+ with the comments addressed.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> @@ +995,5 @@
> >    // the LOAD_NORMAL_ALLOW_MIXED_CONTENT flag is set.
> >    // Checked in nsMixedContentBlocker, to see if the channels match.
> >    nsCOMPtr<nsIChannel> mMixedContentChannel;
> >  
> > +  nsTArray<float> mColorMatrix;
> 
> I'd prefer to see this stored as a gfx::Matrix5x4 since that is supposed to
> also represent a color matrix. Likewise GetColorMatrix would return a
> Matrix5x4 and the code in nsDisplayList would read the elements from
> matrix.components field and put them into the filter struct.
> 

This would mean a fixed 20*float size increase to nsDocShell. I guess it can be a pointer?

The wr filter i added stores the matrix in SVG row order (r1_1, r1_2, r1_3, r1_4, r1_5, r2_1, ...), the gfx::Matrix5x4 seems to be glsl column order (c1_1, c1_2, c1_3, c1_4, c1_5, c2_1, ...). I can rotate the matrix when creating the wr filter, but that would be tedious, and we already do it once to translate the SVG order to glsl in webrender :P

Maybe the whole thing should have been done in column order to begin with?

(also, gfx::Matrix5x4 seems to be a misnomer, shouldn't it be Matrix4x5?)

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> This would mean a fixed 20*float size increase to nsDocShell. I guess it can
> be a pointer?

I don't think that's a huge burden, we don't create a lot of docshell objects.

> The wr filter i added stores the matrix in SVG row order (r1_1, r1_2, r1_3,
> r1_4, r1_5, r2_1, ...), the gfx::Matrix5x4 seems to be glsl column order
> (c1_1, c1_2, c1_3, c1_4, c1_5, c2_1, ...). I can rotate the matrix when
> creating the wr filter, but that would be tedious, and we already do it once
> to translate the SVG order to glsl in webrender :P
> 
> Maybe the whole thing should have been done in column order to begin with?
>
> (also, gfx::Matrix5x4 seems to be a misnomer, shouldn't it be Matrix4x5?)

I think so? Although using terminology like row order and column order here is confusing to me because my understanding is that just denotes how you're turning the matrix into a vector and vice-versa. Maybe there's a convention here that I don't know about, but I'm going to use 4x5 and 5x4 to avoid confusing myself. When I refer to row-order conversion I mean taking this 2x3 matrix:
| A B C |
| D E F |
and turning it into a vector/array like so:
| A B C D E F |
whereas column-order conversion would turn it into this:
| A D B E C F |

So, the SVG notation is 4x5, and with your current set of patches that gets converted (I assume) by row-order conversion into a vector of 20 elements, and that stays that way until it gets to picture.rs, where at [1] you do a column-order conversion back into a 5x4 matrix, effectively transposing the original SVG matrix.

What I'm suggesting is that instead of doing the transposition in picture.rs, you move it up to the docshell IDL boundary and store it in a Matrix5x4 in the docshell. Then you don't have to do the transposition in picture.rs at all which gives you two advantages: (1) Using a Matrix5x4 is much better than using a nsTArray, in terms of readability and strong-typing. It can also be dumped and debugged more easily. (2) You don't have to do the transposition in picture.rs which happens ~60 times per second; instead you just do it once when setting the filter.

Does that sound reasonable? I don't feel too strongly about this so I'm open to hearing arguments in favour of the current approach.

[1] https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/gfx/webrender/src/picture.rs#599-601

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Eitan Isaacson [:eeejay] from comment #10)
> > This would mean a fixed 20*float size increase to nsDocShell. I guess it can
> > be a pointer?
> 
> I don't think that's a huge burden, we don't create a lot of docshell
> objects.
> 
> > The wr filter i added stores the matrix in SVG row order (r1_1, r1_2, r1_3,
> > r1_4, r1_5, r2_1, ...), the gfx::Matrix5x4 seems to be glsl column order
> > (c1_1, c1_2, c1_3, c1_4, c1_5, c2_1, ...). I can rotate the matrix when
> > creating the wr filter, but that would be tedious, and we already do it once
> > to translate the SVG order to glsl in webrender :P
> > 
> > Maybe the whole thing should have been done in column order to begin with?
> >
> > (also, gfx::Matrix5x4 seems to be a misnomer, shouldn't it be Matrix4x5?)
> 
> I think so? Although using terminology like row order and column order here
> is confusing to me because my understanding is that just denotes how you're
> turning the matrix into a vector and vice-versa. Maybe there's a convention
> here that I don't know about, but I'm going to use 4x5 and 5x4 to avoid
> confusing myself. When I refer to row-order conversion I mean taking this
> 2x3 matrix:
> | A B C |
> | D E F |
> and turning it into a vector/array like so:
> | A B C D E F |
> whereas column-order conversion would turn it into this:
> | A D B E C F |
> 
> So, the SVG notation is 4x5, and with your current set of patches that gets
> converted (I assume) by row-order conversion into a vector of 20 elements,
> and that stays that way until it gets to picture.rs, where at [1] you do a
> column-order conversion back into a 5x4 matrix, effectively transposing the
> original SVG matrix.
> 
> What I'm suggesting is that instead of doing the transposition in
> picture.rs, you move it up to the docshell IDL boundary and store it in a
> Matrix5x4 in the docshell. Then you don't have to do the transposition in
> picture.rs at all which gives you two advantages: (1) Using a Matrix5x4 is
> much better than using a nsTArray, in terms of readability and
> strong-typing. It can also be dumped and debugged more easily. (2) You don't
> have to do the transposition in picture.rs which happens ~60 times per
> second; instead you just do it once when setting the filter.
> 
> Does that sound reasonable? I don't feel too strongly about this so I'm open
> to hearing arguments in favour of the current approach.

No, that sounds great. I'll put in a pull request tomorrow to WebRender to have the matrix be column-order.

Comment 13

[Eitan Isaacson [:eeejay]]

https://github.com/servo/webrender/pull/2430

Comment 14

[Eitan Isaacson [:eeejay]]

Attachment: expose colormatrix in docshell. r?kats

Sorry it took me a while to get back to this. Waited for the changes to come in from WR and got sidetracked.

I have to admit, I don't like using gfx::Matrix5x4 here. Since it's not a pointer we lose the non-null value we would use for applying a filter. We also need to pull in gfx data types to the dochsell which doesn't seem great. I don't see a real upside to this.

I do think it was the right choice to change the matrix value ordering, though.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8954185 [details] [diff] [review]
expose colormatrix in docshell. r?kats

Review of attachment 8954185 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with suggestions applied

::: docshell/base/nsDocShell.cpp
@@ +14502,5 @@
> +NS_IMETHODIMP
> +nsDocShell::SetColorMatrix(float* aMatrix, uint32_t aMatrixLen)
> +{
> +  if (aMatrixLen == 20) {
> +    memcpy(mColorMatrix.components, aMatrix, sizeof(mColorMatrix.components));

Add a MOZ_ASSERT(aMatrixLen * sizeof(*aMatrix) == sizeof(mColorMatrix.components)); here for better future-proofing (e.g. in case Matrix5x4 starts using doubles instead of floats)

::: layout/painting/nsDisplayList.cpp
@@ +2672,5 @@
> +    if (colorMatrix != Matrix5x4()) {
> +      wr::WrFilterOp gs = {
> +        wr::WrFilterOpType::ColorMatrix
> +      };
> +      memcpy(&(gs.matrix), colorMatrix.components, 20*sizeof(float));

Here also I'd like to see:
  MOZ_ASSERT(sizeof(gs.matrix) == sizeof(colorMatrix.components));
  memcpy(&(gs.matrix), colorMatrix.components, sizeof(gs.matrix));
to avoid unnecessarily assuming floats and to better guard against changes to these data structures. memcpy can be dangerous.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> I have to admit, I don't like using gfx::Matrix5x4 here. Since it's not a
> pointer we lose the non-null value we would use for applying a filter.

If you want you can use a Maybe<gfx::Matrix5x4> for more ergonomic code, but honestly I think that would be overkill. I much prefer a strongly typed data structure to random float* which can be easily abused or corrupted.

> We
> also need to pull in gfx data types to the dochsell which doesn't seem
> great. I don't see a real upside to this.

The gfx data types are already used widely across the codebase, I don't see using it here as anything wrong.

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: expose colormatrix in docshell. r?kats

I apologize for the additional r?, just want to make sure this is OK with UniquePtr.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8954507 [details] [diff] [review]
expose colormatrix in docshell. r?kats

Review of attachment 8954507 [details] [diff] [review]:
-----------------------------------------------------------------

I agree this is much nicer than the previous version. Thanks!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1af2f214487e
expose colormatrix in docshell. r=kats

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1af2f214487e