Closed Bug 1016539 Opened 10 years ago Closed 10 years ago

Add support for accelerated inverted color and grayscale rendering

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla34
Project Flags:
feature-b2g 2.1

People

(Reporter: eeejay, Assigned: jrmuizel)

References

Details

(Keywords: access, Whiteboard: [ucid:graphics18,ft:graphic][2.1-feature-qa+])

Attachments

(1 file, 2 obsolete files)

a11y.patch
21.31 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review 
For accessibility purposes in B2G, we want to be able to toggle inverted colors on all rendered content on a global scale. In the past, I tried doing this with an SVG filter, but that slows things down considerably.

I bet it would be quick to implement this directly in our graphics stack, and then have the user toggle this feature if needed.
For reference, you could see what we did, and why the SVG version was pulled in bug 796230.
From David:
On a Mac->System Preferences->Display there are options:
Invert colors
Use grayscale
Enhance Contrast

I'm assuming we're looking at "exactly the same" thing for B2G?

In addition to the graphics work, we'll need some work in the Settings App, so we should probably open a Gaia bug for that, and decide what that's going to look like.
OS: Linux → Gonk (Firefox OS)
A properly accelerated color filter should be able to do this almost free.
Longer form explanation: I already added all the layer shaders for this. If we can wire up CSS filter effects to them we should be good.
Per discussion with Sandip and Milan, we will target this feature for Firefox OS 2.1. update the bug status accordingly.
Blocks: 1016796
feature-b2g: --- → 2.1
Whiteboard: [ucid:graphics18,ft:graphic]
(In reply to Andreas Gal :gal from comment #4)
> Longer form explanation: I already added all the layer shaders for this. If
> we can wire up CSS filter effects to them we should be good.

Very cool. Is it correct to say this bug depends on bug 948265? Also, if we are going back to the css filter route, this is probably a duplicate of bug 878883.
Stephany, this would need some UX to design where things go and what they look like, so tagging you to start.

Ivan, I just want to make sure you're chasing Gaia side (or chasing Sandip to chase Gaia) as there is an app (settings?) piece here, which should be chased in another bug.
Flags: needinfo?(swilkes)
Flags: needinfo?(itsay)
(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to Andreas Gal :gal from comment #4)
> > Longer form explanation: I already added all the layer shaders for this. If
> > we can wire up CSS filter effects to them we should be good.
> 
> Very cool. Is it correct to say this bug depends on bug 948265? Also, if we
> are going back to the css filter route, this is probably a duplicate of bug
> 878883.

These could be duplicates; for now, this bug captures the intent, without the implementation detail, so it's a good one to keep around.  It may very well be that it will depend on both of the ones you mention once we agree on the approach.

Andreas - I agree, I think this part is not a big deal on the platform side.
(In reply to Milan Sreckovic [:milan] from comment #7)
> Stephany, this would need some UX to design where things go and what they
> look like, so tagging you to start.
> 
> Ivan, I just want to make sure you're chasing Gaia side (or chasing Sandip
> to chase Gaia) as there is an app (settings?) piece here, which should be
> chased in another bug.

I could help implement this from the frontend side. If this is done right, it will simply be applying a filter style to the system <body>.
(In reply to Milan Sreckovic [:milan] from comment #2)
> From David:
> On a Mac->System Preferences->Display there are options:

Note to all, I should have said:
Mac->System Preferences-> *Accessibility* ->Display

(Sorry if that omission caused pain)
(In reply to Eitan Isaacson [:eeejay] from comment #9)
> (In reply to Milan Sreckovic [:milan] from comment #7)
> > Stephany, this would need some UX to design where things go and what they
> > look like, so tagging you to start.
> > 
> > Ivan, I just want to make sure you're chasing Gaia side (or chasing Sandip
> > to chase Gaia) as there is an app (settings?) piece here, which should be
> > chased in another bug.
> 
> I could help implement this from the frontend side. If this is done right,
> it will simply be applying a filter style to the system <body>.

That's great.  I just want to make sure it's tracked in all the spreadsheets and Google docs and stone tablets and all the other places where people look to understand what's coming up in the release. Some of the downstream teams count on it to plan (e.g., QA).
Removing the flag for myself. For the UX side, this would be an outcome of Casey's styles architecture work. Is this for Settings? Is this 2.0 work?
Flags: needinfo?(swilkes)
Not 2.0, thus feature-b2g=2.1 :)  As for whether it's for settings or not, I assume so, but UX should answer. This is an accessibility feature (I honestly don't know if it is "just" a visual impairment or goes beyond that), and we need guidance as to how it fits into the rest of the settings.  Perhaps it's an "accessibility" app.  Probably best for Casey to have some preliminary conversations with David and get us the big picture.
The feature in question is not a theming change, and is not related to Casey's work. We want to have a global filter that will alter the colors on *all* content, not just gaia apps. It will kind of have the effect of a live photoshop filter on the device's entire display.

As for UX, the main things we will need is an addition to to the settings Accessibility section that will expose whatever filter effects we have. Also, a global quick toggle (maybe via gesture) would be useful to allow users to quickly change modes. An example for when that is useful is when a user has a color invert filter and encounters white-on-black text. This should all be speced out in a spin-off bug.
Changing the description of this bug to avoid confusion.

The scope of this bug is platform implementation only.

We want to be able to toggle inverse colors for all the content in B2G, not styling. Meaning, images will be inverted too. This is primarily an accessibility feature.

Ideally, this will be implemented as accelerated CSS filters as Andreas suggests in comment #4. This will allow us to apply a filter to the system app's <body> to get the desired effect. In addition, this should open up possibilities for more accessibility color modes in the future, like high and low contrast.

I'll open a separate gaia bug for UX to work with.
Summary: Add preference for inverting colors → Add support for accelerated inverted color rendering
I'm going to let the assignee figure out the way forward, bug if this is to be done via CSS filters, I would suggest adding these as blockers:
* bug 948265
* bug 878883
* bug 869828
(In reply to Milan Sreckovic [:milan] from comment #7)

> Ivan, I just want to make sure you're chasing Gaia side (or chasing Sandip
> to chase Gaia) as there is an app (settings?) piece here, which should be
> chased in another bug.

It looks to me the gaia work is to allow the toggling on inverse color. Wait for Eitan to create the separate gaia bug and I will be tracking on it.
Flags: needinfo?(itsay)
The graphics team is going to investigate the viability of using CSS filters approach.  Stay tuned.
At this point, with 2.0 pressures, we're going with the "cheapest" approach which is most likely not going to the the CSS filters.
flagging Bruce for comment #2 and comment #12 (some minimal settings work involved)
Flags: needinfo?(bhuang)
Assignee: nobody → jmuizelaar
(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> Created attachment 8466388 [details] [diff] [review]
> Prototype of what this could look like

You can try this out by setting the layers.invert pref to true.
David & Eitan, a couple of questions:

1. I'm assuming no objections from you to the standard luminance formula for the B&W case:
Y = 0.2126 R + 0.7152 G + 0.0722 B

2. Are these effects meant to be cumulative?  In other words, can I apply both invert and B&W at the same time and get the negative film effect, so to speak?  The OS X version mentioned in comment 10 certainly allows it.
Flags: needinfo?(eitan)
Flags: needinfo?(dbolter)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> Created attachment 8466388 [details] [diff] [review]
> Prototype of what this could look like

The current approach does this in the generic LayerManager code an open question is whether we should only do it in CompositorOGL. I don't have a strong feeling about the answer to this yet.
For the question #1 in comment 23 above - I believe OS X actually uses:
Y = 0.299 R + 0.587 G + 0.114 B
(In reply to Milan Sreckovic [:milan] from comment #23)
> David & Eitan, a couple of questions:
> 
> 1. I'm assuming no objections from you to the standard luminance formula for
> the B&W case:
> Y = 0.2126 R + 0.7152 G + 0.0722 B
> 
> 2. Are these effects meant to be cumulative?  In other words, can I apply
> both invert and B&W at the same time and get the negative film effect, so to
> speak?  The OS X version mentioned in comment 10 certainly allows it.

If this is being implemented as accelerated CSS filters, then the answer to the two questions is "yes". The greyscale filter shorthand uses the formula above[1], and css filters allow cumulative effects via chaining (eg. filter: greyscale(100%) invert(100%);)

If this is being implemented as pref flags like Jeff's prototype above, then I still think the answer should be yes to stay consistent with the CSS filter spec for future proofiness.

As for the mac formula, they probably tweaked it for readability. I think we could achieve a similar effect in the future if we had more css filters accelerated like contrast.

1. http://www.w3.org/TR/filter-effects/#grayscaleEquivalent
Flags: needinfo?(eitan)
QA Whiteboard: [2.1-feature-qa+]
(In reply to Ivan Tsay (:ITsay) from comment #17)
> (In reply to Milan Sreckovic [:milan] from comment #7)
> 
> > Ivan, I just want to make sure you're chasing Gaia side (or chasing Sandip
> > to chase Gaia) as there is an app (settings?) piece here, which should be
> > chased in another bug.
> 
> It looks to me the gaia work is to allow the toggling on inverse color. Wait
> for Eitan to create the separate gaia bug and I will be tracking on it.

Just checking if this bug was created so I'm not duplicating it.  I guess this will go into the accessibility settings?
Flags: needinfo?(bhuang)
(In reply to Bruce Huang [:bhuang] <bhuang@mozilla.com> from comment #27)
> (In reply to Ivan Tsay (:ITsay) from comment #17)
> > (In reply to Milan Sreckovic [:milan] from comment #7)
> > 
> > It looks to me the gaia work is to allow the toggling on inverse color. Wait
> > for Eitan to create the separate gaia bug and I will be tracking on it.
> 
> Just checking if this bug was created so I'm not duplicating it.  I guess
> this will go into the accessibility settings?

I don't know of this bug, and there are no dependencies listed on this, so a new bug is likely necessary.
(In reply to Eitan Isaacson [:eeejay] from comment #26)
> (In reply to Milan Sreckovic [:milan] from comment #23)
> > David & Eitan, a couple of questions:
> > 
> > 1. I'm assuming no objections from you to the standard luminance formula for
> > the B&W case:
> > Y = 0.2126 R + 0.7152 G + 0.0722 B
> > 
> > 2. Are these effects meant to be cumulative?  In other words, can I apply
> > both invert and B&W at the same time and get the negative film effect, so to
> > speak?  The OS X version mentioned in comment 10 certainly allows it.
> 
> If this is being implemented as accelerated CSS filters, then the answer to
> the two questions is "yes". The greyscale filter shorthand uses the formula
> above[1], and css filters allow cumulative effects via chaining (eg. filter:
> greyscale(100%) invert(100%);)
> 
> If this is being implemented as pref flags like Jeff's prototype above, then
> I still think the answer should be yes to stay consistent with the CSS
> filter spec for future proofiness.

We'd want the results to be the same regardless of the implementation, we wouldn't want them to change as we switch from the "quick" to "CSS filters" in the future.  So it sounds like the answer is yes to both.  Thanks!
Flags: needinfo?(dbolter)
Blocks: 1049824
I created a dependent gaia bug for tracking the frontend design and implementation in the settings app: bug 1049824.
No longer blocks: 1049824
I'm reasonably happy with this. It supports inverse and grayscale with the "layers.invert", "layers.grayscale" prefs.

The basic idea is that we add mTwoPassTmpTarget to LayerManagerComposite that we use when ever we need to do two pass rendering.
Attachment #8466388 - Attachment is obsolete: true
Attachment #8468725 - Flags: feedback?(matt.woodrow)
Comment on attachment 8468725 [details] [diff] [review]
Add support for inverse and grayscale rendering

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

Looking ahead (can't help myself), is the idea with contrast to pass the argument to the shader, or to build up the matrix that has the argument baked in it?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +440,5 @@
> +    matrix._22 = 0.7152;
> +    matrix._32 = 0.0722;
> +    matrix._13 = 0.2126;
> +    matrix._23 = 0.7152;
> +    matrix._33 = 0.0722;

As much as I usually hate things written that way, this may be a good time to do this type of thing:
matrix._11 = matrix._12 = matrix._13 = 0.2126;
...
Comment on attachment 8468725 [details] [diff] [review]
Add support for inverse and grayscale rendering

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

Looks good so far!

::: gfx/layers/CompositorTypes.h
@@ +148,5 @@
>    YCBCR,
>    COMPONENT_ALPHA,
>    SOLID_COLOR,
>    RENDER_TARGET,
> +  RENDER_TARGET_COLOR_MATRIX,

COLOR_MATRIX should probably be a secondary effect. The primary effects describe the source, and the secondary effect describes additional effects to apply when drawing it.

I'd actually quite like to get rid of RENDER_TARGET entirely, I don't see how it adds any value. CompositingRenderTarget already inherits from TextureSource, so we could just use RGB. The d3d9/11 compositors already composite these effects identically.

::: gfx/layers/Effects.h
@@ +137,5 @@
> +    : EffectRenderTarget(EffectTypes::RENDER_TARGET_COLOR_MATRIX, aRenderTarget)
> +    , mColorMatrix(aMatrix)
> +  {}
> +
> +  virtual const char* Name() { return "EffectRenderTargetinvert"; }

Old name?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +410,5 @@
>      sFrameCount++;
>    }
>  }
>  
> +void LayerManagerComposite::Begin2ndPass()

Aren't we starting the first pass at this point?

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +46,5 @@
>          "uRenderColor",
>          "uTexCoordMultiplier",
>          "uTexturePass2",
> +        "uColorMatrix",
> +        "uColorMatrixVector",

I feel like we're missing some code here to actually use these new uniforms.
Attachment #8468725 - Flags: feedback?(matt.woodrow) → feedback+
Attached patch a11y.patch — — Splinter Review 
If you want this split up, I'm happy to do that.
Attachment #8468725 - Attachment is obsolete: true
Attachment #8469781 - Flags: review?(matt.woodrow)
Comment on attachment 8469781 [details] [diff] [review]
a11y.patch

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +267,5 @@
>  
>    void WorldTransformRect(nsIntRect& aRect);
>  
> +  void Begin2ndPass();
> +  void End2ndPass(RefPtr<CompositingRenderTarget> aPreviousTarget, nsIntRect aClipRect);

This may not compile.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +945,5 @@
>      }
>      break;
>    }
>    }
> +  config.SetColorMatrix(aColorMatrix);

We're still missing the definition/declaration for this, as well as the shader code generation for color matrix.
(In reply to Matt Woodrow (:mattwoodrow) from comment #35)
> Comment on attachment 8469781 [details] [diff] [review]
> a11y.patch
> 
> Review of attachment 8469781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.h
> @@ +267,5 @@
> >  
> >    void WorldTransformRect(nsIntRect& aRect);
> >  
> > +  void Begin2ndPass();
> > +  void End2ndPass(RefPtr<CompositingRenderTarget> aPreviousTarget, nsIntRect aClipRect);
> 
> This may not compile.

Ah yes. I forgot to change those names.

> 
> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +945,5 @@
> >      }
> >      break;
> >    }
> >    }
> > +  config.SetColorMatrix(aColorMatrix);
> 
> We're still missing the definition/declaration for this, as well as the
> shader code generation for color matrix.

That code is already there.
Flags: in-moztrap?(npark)
QA Contact: npark
QA Whiteboard: [2.1-feature-qa+]
Whiteboard: [ucid:graphics18,ft:graphic] → [ucid:graphics18,ft:graphic][2.1-feature-qa+]
(In reply to Jeff Muizelaar [:jrmuizel] from comment #36)

> That code is already there.

How convenient!
Attachment #8469781 - Flags: review?(matt.woodrow) → review+
Moztrap link for test case: (in progress) https://moztrap.mozilla.org/manage/case/14277/
https://hg.mozilla.org/mozilla-central/rev/9dad28d06cb5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Summary: Add support for accelerated inverted color rendering → Add support for accelerated inverted color and grayscale rendering
moztrap link to grayscale test case:
https://moztrap.mozilla.org/manage/case/14504/
Flags: in-moztrap?(npark) → in-moztrap+
QA Whiteboard: [failed-verification]
QA Whiteboard: [failed-verification] → [needs-verification]
Flagging down No-Jun to attempt verification here again with the dependency fixed.
Flags: needinfo?(npark)
Depends on: 1066664
Second attempt at verification failed due to bug 1066684.
QA Whiteboard: [needs-verification] → [failed-verification]
(In reply to Jason Smith [:jsmith] from comment #43)
> Second attempt at verification failed due to bug 1066684.

Correction, bug 1066664 not 84.
Flags: needinfo?(npark)
bug 1066664 is fixed, and verified in master branch.  The feature looks good, marking this feature as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: