Add support for Skia Magnifier filter to Chromium layers
Created attachment 158248 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 158248 [details] Patch Attachment 158248 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13492458
Alexandre: would this be something Chrome for Android can re-use for Link Preview?
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.
Comment on attachment 158248 [details] Patch Attachment 158248 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13506263
Comment on attachment 158248 [details] Patch Attachment 158248 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13506273
@jamesr this will be applied to a layer over top of some subset of the screen (decided by the user) for accessibility magnification.
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?
(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.
(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.
(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?
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.
(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 ?
> > 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.
(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.
> 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.
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.
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.
(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.
(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? :)
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)?
This doesn't pass EWS, presumably because the skia code hasn't landed yet - or if it has, it hasn't rolled.
Created attachment 158755 [details] Patch
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.
Comment on attachment 158755 [details] Patch OK
Comment on attachment 158755 [details] Patch Rejecting attachment 158755 [details] from commit-queue. zork@chromium.org 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.
Comment on attachment 158755 [details] Patch You can set CQ? but not CQ+ if you're not a committer.
Comment on attachment 158755 [details] Patch Clearing flags on attachment: 158755 Committed r125903: <http://trac.webkit.org/changeset/125903>
All reviewed patches have been landed. Closing bug.