Bug 93939 – Add support for Skia Magnifier filter to Chromium layers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zachary Kuznia]

Add support for Skia Magnifier filter to Chromium layers

[Zachary Kuznia]

Created attachment 158248 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 158248 [details]
Patch

Attachment 158248 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13492458

[Peter Beverloo]

Alexandre: would this be something Chrome for Android can re-use for Link Preview?

[James Robinson]

What layer is this being applied to in CrOS?  I'm wondering why this isn't done by manipulating view parameters.

Peter - I doubt it, since this appears to magnify pre-rastered content and thus will be "blurry" when zooming in.  For the link preview content, you want to rasterize at the new scale.

[Peter Beverloo (cr-android ews)]

Comment on attachment 158248 [details]
Patch

Attachment 158248 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13506263

[Peter Beverloo (cr-android ews)]

Comment on attachment 158248 [details]
Patch

Attachment 158248 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13506273

[Dana Jansens]

@jamesr this will be applied to a layer over top of some subset of the screen (decided by the user) for accessibility magnification.

[Alexandre Elias]

This filter doesn't seem very useful.  If you're fine with blurriness, why not do a non-scaling renderpass copy and scale at draw time?

For the link disambiguation zoom, we want to do it at paint-to-tile time to avoid blurriness.  It seems you would want this for accessibility zoom as well in any case.  Is your problem that it's hard to extend this outside the web content layer?

[Dana Jansens]

(In reply to comment #9)
> This filter doesn't seem very useful.  If you're fine with blurriness, why not do a non-scaling renderpass copy and scale at draw time?

Maybe the purpose of it is not clear? This is something for aura to magnify a portion of the screen, UI included, whatever renderers fall within included. So, it does not magnify some subtree of layers, instead it magnifies some area within a number of subtrees.

[Alexandre Elias]

(In reply to comment #10)
> (In reply to comment #9)
> > This filter doesn't seem very useful.  If you're fine with blurriness, why not do a non-scaling renderpass copy and scale at draw time?
> 
> Maybe the purpose of it is not clear? This is something for aura to magnify a portion of the screen, UI included, whatever renderers fall within included. So, it does not magnify some subtree of layers, instead it magnifies some area within a number of subtrees.

Yes, I surmised that.  Even so, what is achieved by the Skia magnification?  A draw-time matrix can also magnify.  The Skia scaling will be slightly better quality but still mediocre.

It looks like what's really called for here is a method on LayerChromium to request a repaint of a subset at higher scale.  Layers that can do a high-quality magnify (like the web content layer) would do that, and those that have no option but a bitmap scale would fall back to that.

[Dana Jansens]

(In reply to comment #11)
> Yes, I surmised that.  Even so, what is achieved by the Skia magnification?  A draw-time matrix can also magnify.  The Skia scaling will be slightly better quality but still mediocre.
> 
> It looks like what's really called for here is a method on LayerChromium to request a repaint of a subset at higher scale.  Layers that can do a high-quality magnify (like the web content layer) would do that, and those that have no option but a bitmap scale would fall back to that.

Hm, well, it's intended to have some nice effects in the future - parabolic zoom on the edges. We'll need to make use of a Skia filter to do that. I guess we could still do that with enough information via foreground filters on all layers within some hitbox. Is that preferable?

[Alexandre Elias]

Well, I think any implementation that doesn't end up rasterizing fonts without blurriness is inadequate -- we aren't interested in reusing that on Android and I also think you guys would end up rewriting it someday to fix the problem.

I also think there is another completely opposite concern -- to allow pointer-controlled magnifying glass type designs, it would be nice to have very fast drawing to allow moving the magnified area at high framerate.  That argues for implementing your parabolic zoom effect via pixel shader instead of Skia.  So in summary, I think the ideal solution to this has two parts:

1) A mechanism to make high-res-subset-repaint requests to the underlying implementations of many layers, and then to glue the results together inside a new layer.  (No special filtering needed at this stage.)

2) Magnifying-glass effects at draw time via pixel shader and matrices.



Anyway, that is obviously more work than your current approach and I'm not saying you must do all that right now, but I just want to start a design discussion on the correct way to do this.

[Dana Jansens]

(In reply to comment #13)
> Well, I think any implementation that doesn't end up rasterizing fonts without blurriness is inadequate -- we aren't interested in reusing that on Android and I also think you guys would end up rewriting it someday to fix the problem.
> 
> I also think there is another completely opposite concern -- to allow pointer-controlled magnifying glass type designs, it would be nice to have very fast drawing to allow moving the magnified area at high framerate.  That argues for implementing your parabolic zoom effect via pixel shader instead of Skia.  So in summary, I think the ideal solution to this has two parts:
> 
> 1) A mechanism to make high-res-subset-repaint requests to the underlying implementations of many layers, and then to glue the results together inside a new layer.  (No special filtering needed at this stage.)

To be fair, you could indeed get nicer text by doing it this way, but only if the page was zoomed out. Otherwise you're going to have to increase the contentsScale() of any affected layers and wait for a repaint to get better text. Maybe that's what you'd want? It's certainly not fast though.

I think that blurryness was concidered accepted in the way that you can zoom in on a macbook and things become blurry there, but you're right in that we could potentially do better.

> 2) Magnifying-glass effects at draw time via pixel shader and matrices.

Zach can chime in here, but AIUI the filter is implemented in Skia both in software more and in ganesh, and in this case the ganesh (hardware) implementation would be used, similar to other compositor filters. The issue of tying filters into the compositor more tightly than that is perhaps covered in http://go/cssfilterdoc ?

[Alexandre Elias]

> > 1) A mechanism to make high-res-subset-repaint requests to the underlying implementations of many layers, and then to glue the results together inside a new layer.  (No special filtering needed at this stage.)
> 
> To be fair, you could indeed get nicer text by doing it this way, but only if the page was zoomed out. Otherwise you're going to have to increase the contentsScale() of any affected layers and wait for a repaint to get better text. Maybe that's what you'd want? It's certainly not fast though.

On Android, we already get the high-quality rendering in our link disambiguation zoom implementation.  We call the PageWidgetDelegate::paint()  method directly, providing a Skia canvas with a scale factor applied.  Given that it only happens at the beginning of the action, the performance is acceptable.

I wasn't thinking of mutating the contentsScale as such, rather I was thinking of a method on layers that explicitly invokes the underlying paint mechanism into a given canvas.

> 
> I think that blurryness was concidered accepted in the way that you can zoom in on a macbook and things become blurry there, but you're right in that we could potentially do better.
> 
> > 2) Magnifying-glass effects at draw time via pixel shader and matrices.
> 
> Zach can chime in here, but AIUI the filter is implemented in Skia both in software more and in ganesh, and in this case the ganesh (hardware) implementation would be used, similar to other compositor filters. The issue of tying filters into the compositor more tightly than that is perhaps covered in http://go/cssfilterdoc ?

Even with Ganesh though, is it fast enough to do at 60fps?  That said, I don't care as much about this part since it's easy to change later in any case.

[Dana Jansens]

(In reply to comment #15)
> > > 1) A mechanism to make high-res-subset-repaint requests to the underlying implementations of many layers, and then to glue the results together inside a new layer.  (No special filtering needed at this stage.)
> > 
> > To be fair, you could indeed get nicer text by doing it this way, but only if the page was zoomed out. Otherwise you're going to have to increase the contentsScale() of any affected layers and wait for a repaint to get better text. Maybe that's what you'd want? It's certainly not fast though.
> 
> On Android, we already get the high-quality rendering in our link disambiguation zoom implementation.  We call the PageWidgetDelegate::paint()  method directly, providing a Skia canvas with a scale factor applied.  Given that it only happens at the beginning of the action, the performance is acceptable.
> 
> I wasn't thinking of mutating the contentsScale as such, rather I was thinking of a method on layers that explicitly invokes the underlying paint mechanism into a given canvas.

Hm, well a readback/magnify is something we can do on impl. If the contentsScale is modified this is also something we could do directly on impl.

Calling over to the main thread and blocking the UI based on a slow renderer sounds like a nonstarter though.

> > 
> > I think that blurryness was concidered accepted in the way that you can zoom in on a macbook and things become blurry there, but you're right in that we could potentially do better.
> > 
> > > 2) Magnifying-glass effects at draw time via pixel shader and matrices.
> > 
> > Zach can chime in here, but AIUI the filter is implemented in Skia both in software more and in ganesh, and in this case the ganesh (hardware) implementation would be used, similar to other compositor filters. The issue of tying filters into the compositor more tightly than that is perhaps covered in http://go/cssfilterdoc ?
> 
> Even with Ganesh though, is it fast enough to do at 60fps?  That said, I don't care as much about this part since it's easy to change later in any case.

[Alexandre Elias]

> Hm, well a readback/magnify is something we can do on impl. If the contentsScale is modified this is also something we could do directly on impl.
> 
> Calling over to the main thread and blocking the UI based on a slow renderer sounds like a nonstarter though.

Right.  But impl-side painting will neatly solve this problem someday.

How about this: how about setting up the structure of this feature so that when  impl-side painting is done, it will be easy to flip a switch and get non-blurry painting.  So we would go ask the renderer impl thread, but not the main thread.  I'm fine with blurriness in the short term, I just would like to avoid having code that can't possibly get high-quality results.

[Dana Jansens]

Oh right, impl side painting. Keep in mind the maginify is in the host compositor, the layers to be redrawn are in a child compositor. This would also add some level of inter-ubercomp-complexity to the picture, to set things on the child compositor from the host compositor. And passing multiple quads from the child compositor for a layer at different scale factors? I'm not sure how that would entirely work..

I'll let zork comment with his thoughts of how this might positively/negatively affect the feature to apply it to each layer that intersects from region of the screen.

[Antoine Labour]

My understanding is that for accessibility magnification purpose, they explicitly don't want to rasterize at a higher resolution - they want to see the same pixels (scaled up).

At a high level, we want the feature that is the same as a background filter - make a small layer for the maximized area, and when it's drawn, take whatever is underneath and draw with a transformation.

The infrastructure to do it that way already exists (including support for occlusion, partial update etc., and CCDelegatingRenderer support will follow trivially), so it makes sense to hook it up that way.

Once the API is there, we can easily change the implementation (e.g. build the shader directly without going through skia) if it makes sense.

[Alexandre Elias]

(In reply to comment #19)
> My understanding is that for accessibility magnification purpose, they explicitly don't want to rasterize at a higher resolution - they want to see the same pixels (scaled up).

Maybe there's some concern I'm not aware of, but why would inferior quality be an explicit desire?

> 
> At a high level, we want the feature that is the same as a background filter - make a small layer for the maximized area, and when it's drawn, take whatever is underneath and draw with a transformation.
> 
> The infrastructure to do it that way already exists (including support for occlusion, partial update etc., and CCDelegatingRenderer support will follow trivially), so it makes sense to hook it up that way.

I agree it's much easier to do it that way.

OK, because the final implementation will depend on impl-side painting and that is still in a vague state, I'm okay with going with the blurry thing for now and not worrying too much about it until it's closer to realization.  Android's tap disambiguation can just use a separate codepath for now.  Thanks for bearing with me.

[Dana Jansens]

(In reply to comment #20)
> (In reply to comment #19)
> OK, because the final implementation will depend on impl-side painting and that is still in a vague state, I'm okay with going with the blurry thing for now and not worrying too much about it until it's closer to realization.  Android's tap disambiguation can just use a separate codepath for now.  Thanks for bearing with me.

Ok, thanks.

@jamesr Can you give this patch a look-over if this is enough information about its intent please? :)

[James Robinson]

Comment on attachment 158248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158248&action=review

> Source/Platform/chromium/public/WebFilterOperation.h:74
> +    WebRect zoomRect() const

newline between functions, please

> Source/Platform/chromium/public/WebFilterOperation.h:79
> +    int zoomInset() const

what does this field mean?  What's a zoom inset?

> Source/Platform/chromium/public/WebFilterOperation.h:108
> +    WebRect m_zoomRect;
> +    int m_zoomInset;

It sucks to grow this by even more - can you make some of the existing fields more generic and share data space?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:38
> +#include "SkMagnifierImageFilter.h"

I can't find this file in my skia checkout - has it not landed yet? Please link the skia rev or codereview so we can make sure the DEPS all happen in order

> Source/WebCore/platform/graphics/filters/FilterOperation.h:69
> +        ZOOM,

Why are you patching WebCore::FilterOperation?  Do we need to use this in cross-platform code (i.e. outside of chromium compositor stuff)?

[James Robinson]

This doesn't pass EWS, presumably because the skia code hasn't landed yet - or if it has, it hasn't rolled.

[Zachary Kuznia]

Created attachment 158755 [details]
Patch

[Zachary Kuznia]

Comment on attachment 158248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158248&action=review

This filter is used for an accessibility feature on Chrome OS for low vision users.  The intention of this magnifier is to enlarge the image without changing what is being rendered.  Additionally, more complex changes to the rendering tree create more edge cases, which we would ideally prefer to avoid.  The pixel shader for this is added in Skia, so it should be rendered by the gpu.

>> Source/Platform/chromium/public/WebFilterOperation.h:79
>> +    int zoomInset() const
> 
> what does this field mean?  What's a zoom inset?

Merged this field with amount.  Inset is the width of the transition effect around the border.

>> Source/Platform/chromium/public/WebFilterOperation.h:108
>> +    int m_zoomInset;
> 
> It sucks to grow this by even more - can you make some of the existing fields more generic and share data space?

Done

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:38
>> +#include "SkMagnifierImageFilter.h"
> 
> I can't find this file in my skia checkout - has it not landed yet? Please link the skia rev or codereview so we can make sure the DEPS all happen in order

It was landed in Skia at r5056.

>> Source/WebCore/platform/graphics/filters/FilterOperation.h:69
>> +        ZOOM,
> 
> Why are you patching WebCore::FilterOperation?  Do we need to use this in cross-platform code (i.e. outside of chromium compositor stuff)?

This was needed before I moved the actual graphics code to skia.  I've removed it.

[James Robinson]

Comment on attachment 158755 [details]
Patch

OK

[WebKit Review Bot]

Comment on attachment 158755 [details]
Patch

Rejecting attachment 158755 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Dana Jansens]

Comment on attachment 158755 [details]
Patch

You can set CQ? but not CQ+ if you're not a committer.

[WebKit Review Bot]

Comment on attachment 158755 [details]
Patch

Clearing flags on attachment: 158755

Committed r125903: <http://trac.webkit.org/changeset/125903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.