[chromium] Revise zoom animator backend to use full transform instead of just scale.
Created attachment 108160 [details] Patch
A couple of comments questions: 1) LINEAR interpolation is turned on for GESTURE_EVENTS to prevent text from looking overly pixelated while zooming. 2) The pixel test changes slightly with LINEAR vs NEAREST ... will this be a problem? Currently features.gypi has 'ENABLE_GESTURE_EVENTS=1' but does anyone test with it off? 3) I assume I can do mac/mac-cg baselines once the patch is approved? Perhaps take new baselines off the bots ...
Comment on attachment 108160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108160&action=review nits I think. > Source/WebCore/page/Settings.h:611 > + double m_zoomAnimatorPosX; shouldn't these get a default value somewhere? Just to be safe. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295 > + // TODO(wjmaclean): Render some interesting texture in unrendered regions. FIXME: in WebKit > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202 > + const GC3Dint filter = GraphicsContext3D::LINEAR; Do we want to be smarter about this? I am not entirely sure of the implications.
(In reply to comment #3) > (From update of attachment 108160 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108160&action=review > > nits I think. > > > Source/WebCore/page/Settings.h:611 > > + double m_zoomAnimatorPosX; Good point. I've added initialisers that will show up when I load the next version of the patch. > shouldn't these get a default value somewhere? Just to be safe. > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295 > > + // TODO(wjmaclean): Render some interesting texture in unrendered regions. > > FIXME: in WebKit Fixed. > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202 > > + const GC3Dint filter = GraphicsContext3D::LINEAR; > > Do we want to be smarter about this? I am not entirely sure of the implications. I was wondering this too ... I'm not sure if the interpolation settings are allowed to change from one display of the texture to the next. If it is, this perhaps could be added to the TiledLayerChromium interface?
(In reply to comment #4) > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202 > > > + const GC3Dint filter = GraphicsContext3D::LINEAR; > > > > Do we want to be smarter about this? I am not entirely sure of the implications. > > I was wondering this too ... I'm not sure if the interpolation settings are allowed to change from one display of the texture to the next. If it is, this perhaps could be added to the TiledLayerChromium interface? To expand on this a bit more: - it causes very subtle changes to about 14 existing gpu layout tests (rebaselining should suffice) - on most hardware I think the performance impact should be low or even negligible (although I may be wrong) ... and I don't know how big the fraction "1 - most" is. Certainly turning it on/off as needed would be nice, if possible. Not sure yet how hard this would be to plumb.
> Certainly turning it on/off as needed would be nice, if possible. Not sure yet how hard this would be to plumb. OK, TiledLayerChromium could ask it's LayerTreeHost if the current zoom transform isIdentity() ... will experiment with this ...
Created attachment 108168 [details] Patch
(In reply to comment #7) > Created an attachment (id=108168) [details] > Patch This patch plumbs from TiledLayerChromium to LayerTreeHost. The latter stores a boolean to indicate if the zoom transform is an identity, since presumably we test far more often than this value changes and the TransformationMatrix::isIdentity() function involves a lot of comparisons. This *seems* (prima facie) to work OK on my EGL stack, but not sure if all GL implementions will like it. It reduces number of broken layout tests, but some other *seemingly unrelated* stuff is present ... need to investigate some more ...
Created attachment 108173 [details] Patch
(In reply to comment #9) > Created an attachment (id=108173) [details] > Patch Added missing chromium-gpu-linux test image expected output.
Comment on attachment 108173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108173&action=review > Source/WebCore/page/Settings.h:612 > + double m_zoomAnimatorPosX; > + double m_zoomAnimatorPosY; do you really need double precision? could this be an IntPoint (or FloatPoint if you need floating point values) it's also unfortunate that this is growing into a bag of properties. Is there any reasonable way to consolidate these? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:201 > + const GC3Dint filter = (m_tiler->hasBorderTexels() || !layerTreeHost()->isZoomTransformIdentity()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST; This seems very wrong. If we are going to filter and apply a scale to tiled content then we have to have border texels or we'll generate rendering artifacts. It looks like this logic is attempting to apply filtering without uploading border texels which will not work. It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit. > Source/WebKit/chromium/src/WebViewImpl.cpp:2580 > + zoomMatrix.translate3d(m_page->settings()->zoomAnimatorPosX(), m_page->settings()->zoomAnimatorPosY(), 0); if you are doing a 2d translation then use translate(), don't use translate3d(..., ..., 0). the latter is less clear a the callsight and will do extra unnecessary math > Source/WebKit/chromium/src/WebViewImpl.cpp:2581 > + zoomMatrix.scale3d(m_page->settings()->zoomAnimatorScale(), m_page->settings()->zoomAnimatorScale(), 1); you want .scale() here.
(In reply to comment #11) > (From update of attachment 108173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108173&action=review > > > Source/WebCore/page/Settings.h:612 > > + double m_zoomAnimatorPosX; > > + double m_zoomAnimatorPosY; > > do you really need double precision? could this be an IntPoint (or FloatPoint if you need floating point values) We could use float here, although I figured since we were eventually targetting a TransformationMatrix double might make sense. I don't think IntPoint would make sense, and may cause the animation to be less smooth. > it's also unfortunate that this is growing into a bag of properties. Is there any reasonable way to consolidate these? Ultimately once the zoom front end code is in place I'd like to bypass Settings altogether if there's a better way to pass the values from FrameView down to WebViewImpl. I've avoided putting more complex types into Settings as I think this will be a temporary pathway, although we could just make it a TransformationMatrix if it stays. Let me know which route is preferred. > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:201 > > + const GC3Dint filter = (m_tiler->hasBorderTexels() || !layerTreeHost()->isZoomTransformIdentity()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST; > > This seems very wrong. If we are going to filter and apply a scale to tiled content then we have to have border texels or we'll generate rendering artifacts. It looks like this logic is attempting to apply filtering without uploading border texels which will not work. > > It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit. I guess we could always apply border texels to the root layer. Let me read through to see where to turn that on. > > Source/WebKit/chromium/src/WebViewImpl.cpp:2580 > > + zoomMatrix.translate3d(m_page->settings()->zoomAnimatorPosX(), m_page->settings()->zoomAnimatorPosY(), 0); > > if you are doing a 2d translation then use translate(), don't use translate3d(..., ..., 0). the latter is less clear a the callsight and will do extra unnecessary math Fixed. > > Source/WebKit/chromium/src/WebViewImpl.cpp:2581 > > + zoomMatrix.scale3d(m_page->settings()->zoomAnimatorScale(), m_page->settings()->zoomAnimatorScale(), 1); > > you want .scale() here. Fixed.
(In reply to comment #11) > > It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit. Two questions: Do all non-root layers at present get border texels? Who needs to know the difference between a root layer and a non-root layer? (I've looked at the code, but it's not very clear when we need to be able to distinguish if a layer is the root layer.)
Created attachment 108327 [details] Patch
(In reply to comment #14) > Created an attachment (id=108327) [details] > Patch I have removed the border texels & linear interpolation stuff and will submit it in another patch. I have changed the parameter types in Settings from double to float, but not used FloatPoint as (1) this is only temporary, and (2) I don't know if we want to add FloatPoint.h just for this case (I will change this if it is deemed desirable).
Created attachment 108584 [details] Patch
(In reply to comment #16) > Created an attachment (id=108584) [details] > Patch Remove unused function from CCLayerTreeHost.
Comment on attachment 108584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108584&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:296 > + m_context->clearColor(0.25, 0.25, 0.25, 1); It's okay to use braces here because the arms (including comments) aren't single-line.
Looks fine overall. r=me
Comment on attachment 108584 [details] Patch Clearing flags on attachment: 108584 Committed r95988: <http://trac.webkit.org/changeset/95988>
All reviewed patches have been landed. Closing bug.