Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175553002:
Raster PictureLayerTiling with fractional translation (Closed)
DescriptionRaster PictureLayerTiling with fractional translation
Normally layers are rastered in scaled local coordinate, this is not ideal
because the drawing will be blurred by resampling when the layer origin is not
aligned with pixel boundaries either due to layer position or transform.
This CL detect the case when a layer is static and has 1:1 pixel ratio to the
screen, shift the raster space to align with screen pixels.
BUG=521364
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 : Raster PictureLayerTiling with fractional translation #
Total comments: 15
Patch Set 2 : alternative: change raster translation if and only if raster scale mutates #
Total comments: 8
Patch Set 3 : use draw transform instead? #
Total comments: 18
Patch Set 4 : clean up #
Total comments: 8
Patch Set 5 : updated #Patch Set 6 : combined #Messages
Total messages: 60 (9 generated)
Description was changed from ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 ========== to ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
trchen@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
WIP
Patchset #1 (id:1) has been deleted
enne@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:866: static constexpr float kErrorThreshold = 0.001; Where does this number come from? Should you just remember the layer level translation and throw everything out if it changes at all? https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:868: tilings_->Remove(high_res); It'd be nice if RecalculateRasterTranslation did all of this work of cleaning up instead of doing it inside of AddTilingsForRasterScale. Shouldn't you throw out all of the tilings and not just the high res one? https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:984: if (draw_properties().screen_space_transform_is_animating) This is my biggest worry in the patch. I get that you want to not do this when something is animating, but that doesn't account for how often folks animate translations with Javascript. We had a small number of sites that had performance problem when we rerastered after a scale change, which we decided to handle with will-change: transform as the indicator for when to reraster. It's not clear to me how many sites would run into performance problem while translating, but intuitively I suspect there are probably a lot more translation than scales. I think it'd be good to have UMA stats for this in the same way that we did for scales so that we can make sure we're doing this carefully and will not regress some large number of sites. https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:988: if (!screen_space_transform.IsScaleOrTranslation()) Should this just be "is translation?" https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:988: if (!screen_space_transform.IsScaleOrTranslation()) Should this just be "is translation?" https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:470: current_geometry_rect_ = gfx::ToEnclosingRect(tile_bounds_in_dest_space); This is a little unexpected to me. You're generating geometry rects that are outside of the requested rect for the coverage iterator, right? Also, the layer / draw transform itself isn't shifted by the contents translation, so at best you're moving over the geometry rect by integer amounts? I don't understand how this gets tiling pixels to line up with destination pixels if the layer itself is still at an offset. Maybe I'm just missing something, but I expected somewhere to see adjustments to the layer size and the draw transform or something along those lines. https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:652: gfx::ToEnclosingRect(layer_rect), 1.f, &color); You should probably pass the translation through here.
https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:984: if (draw_properties().screen_space_transform_is_animating) On 2016/07/26 00:22:16, enne wrote: > This is my biggest worry in the patch. I get that you want to not do this when > something is animating, but that doesn't account for how often folks animate > translations with Javascript. We had a small number of sites that had > performance problem when we rerastered after a scale change, which we decided to > handle with will-change: transform as the indicator for when to reraster. > > It's not clear to me how many sites would run into performance problem while > translating, but intuitively I suspect there are probably a lot more translation > than scales. I think it'd be good to have UMA stats for this in the same way > that we did for scales so that we can make sure we're doing this carefully and > will not regress some large number of sites. Here's use counters that enne is referring to for scale (we since removed them): https://codereview.chromium.org/1946403003
Sorry I didn't update latest status with you. On Monday I chatted with chrishtr@ offline, we are thinking a somewhat more conservative plan. How about only apply translated rastering to layers that are composited for indirect reason, and use local transform instead of draw transform to determine the raster translation? The idea is that we don't aim to improve raster quality in general, but only to maintain the same quality and performance characteristics as the non-composited code path (whether it's good or not). Most part of this CL will stay the same (since they are just plumbing the raster translation to the raster source). Only the part that manages raster scale policy needs to be changed. WDYT?
Description was changed from ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
On 2016/07/27 23:05:08, trchen wrote: > Sorry I didn't update latest status with you. > > On Monday I chatted with chrishtr@ offline, we are thinking a somewhat more > conservative plan. > How about only apply translated rastering to layers that are composited for > indirect reason, and use local transform instead of draw transform to determine > the raster translation? The idea is that we don't aim to improve raster quality > in general, but only to maintain the same quality and performance > characteristics as the non-composited code path (whether it's good or not). Which transform are you planning to use as the local transform? LayerImpl::transform and LayerImpl::position are used only for tests now (or almost: there's still one non-test line that uses the root layer's position) and the plan was to remove them (and put them in LayerImplTestProperties), so using values from the transform tree or LayerImpl::offset_to_transform_parent would be better from that point of view.
Hello! I have tested this CL. It looks like text still blurry: https://drive.google.com/open?id=0Bw59Ysn8bedgVXNPTVpQel8tZFU
On 2016/08/02 at 12:46:19, lof84 wrote: > Hello! > > I have tested this CL. > It looks like text still blurry: https://drive.google.com/open?id=0Bw59Ysn8bedgVXNPTVpQel8tZFU In your case, is there a zoom factor being applied? If so then you need --enable-use-zoom-for-dsf also.
On 2016/08/02 15:28:40, chrishtr wrote: > On 2016/08/02 at 12:46:19, lof84 wrote: > > Hello! > > > > I have tested this CL. > > It looks like text still blurry: > https://drive.google.com/open?id=0Bw59Ysn8bedgVXNPTVpQel8tZFU > > In your case, is there a zoom factor being applied? If so then you need > --enable-use-zoom-for-dsf also. It's 125% + --enable-use-zoom-for-dsf
On 2016/08/02 at 21:13:55, lof84 wrote: > On 2016/08/02 15:28:40, chrishtr wrote: > > On 2016/08/02 at 12:46:19, lof84 wrote: > > > Hello! > > > > > > I have tested this CL. > > > It looks like text still blurry: > > https://drive.google.com/open?id=0Bw59Ysn8bedgVXNPTVpQel8tZFU > > > > In your case, is there a zoom factor being applied? If so then you need > > --enable-use-zoom-for-dsf also. > > It's 125% + --enable-use-zoom-for-dsf What does that mean exactly?
https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:866: static constexpr float kErrorThreshold = 0.001; On 2016/07/26 00:22:16, enne wrote: > Where does this number come from? Should you just remember the layer level > translation and throw everything out if it changes at all? I'm worrying about tiny errors due to FP precision. The layer can have draw transform mutated but having the same fractional offset. The threshold is set to 0.001px because with bi-linear filtering the resulting error in pixel value can't exceed 0.512, which sounds like a reasonable threshold. A better number would be 2^-10, but I don't think it matters... https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:868: tilings_->Remove(high_res); On 2016/07/26 00:22:16, enne wrote: > It'd be nice if RecalculateRasterTranslation did all of this work of cleaning up > instead of doing it inside of AddTilingsForRasterScale. Shouldn't you throw out > all of the tilings and not just the high res one? Not necessary. The low-res tiling is not pixel-exact and is subject to resampling & filtering anyway. Only high-res tiling with 1:1 pixel mapping matters. https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:984: if (draw_properties().screen_space_transform_is_animating) On 2016/07/27 22:52:17, danakj_OOO_back_8.8 wrote: > On 2016/07/26 00:22:16, enne wrote: > > This is my biggest worry in the patch. I get that you want to not do this > when > > something is animating, but that doesn't account for how often folks animate > > translations with Javascript. We had a small number of sites that had > > performance problem when we rerastered after a scale change, which we decided > to > > handle with will-change: transform as the indicator for when to reraster. > > > > It's not clear to me how many sites would run into performance problem while > > translating, but intuitively I suspect there are probably a lot more > translation > > than scales. I think it'd be good to have UMA stats for this in the same way > > that we did for scales so that we can make sure we're doing this carefully and > > will not regress some large number of sites. > > Here's use counters that enne is referring to for scale (we since removed them): > https://codereview.chromium.org/1946403003 I chatted with chris and also thought through a number of alternatives last week. I think the most practical solution is to recalculate raster translation if and only if raster scale is mutated. This way we can guarantee no performance loss. Most of time quality will improve, but can also degrade in some corner cases. https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:988: if (!screen_space_transform.IsScaleOrTranslation()) On 2016/07/26 00:22:16, enne wrote: > Should this just be "is translation?" Many bug reports are due to layers with odd pixel location on a 150% DPI display. I think we should cover that case. https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:470: current_geometry_rect_ = gfx::ToEnclosingRect(tile_bounds_in_dest_space); On 2016/07/26 00:22:16, enne wrote: > This is a little unexpected to me. You're generating geometry rects that are > outside of the requested rect for the coverage iterator, right? It won't, as the bound of the tiling would be layer_size*raster_scale+raster_translation. Doing the reverse won't exceed the layer bound for more than 1px (due to rounding up). > Also, the layer / draw transform itself isn't shifted by the contents > translation, so at best you're moving over the geometry rect by integer amounts? > I don't understand how this gets tiling pixels to line up with destination > pixels if the layer itself is still at an offset. The geometry rect is not a precise rect, even without this CL, because it is rounded up. It is okay to round it up because tiles overdraw 1px (i.e. border texels) to cover it Texture pixels line up with target pixels because the texture mapping is adjusted by the translation too. See line #516. Let's make a concrete example: Say we have a layer that has transform:translate(12.3px, 45.6px). The layer will be rastered into tiling with CTM of translate(0.3px, 0.6px). What happens when we draw the top-left (partial) pixel at (12, 45)? The GPU will generate sample point at the center of the pixel, that is (12.5, 45.5). Then it will be projected to (0.2, -0.1) in the layer space by inverse draw_transform. Then it will be projected to (0.5, 0.5) in the content space by the raster transform. Magic! that's the dead center of the top-left texel! > Maybe I'm just missing something, but I expected somewhere to see adjustments to > the layer size and the draw transform or something along those lines. No, the layer size should stay the same. The raster translation is applied at PictureLayerTiling, not layer level. Basically 3 spaces are involved here: draw_transform contents_scale target space <==============> layer space <==============> content(tiling) space What's done in this CL is to extend contents_scale with a translation. In theory any transform can be used, just that calculating coverage across multiple tilings become really hard. https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:652: gfx::ToEnclosingRect(layer_rect), 1.f, &color); On 2016/07/26 00:22:16, enne wrote: > You should probably pass the translation through here. IMO it is better to avoid RasterSource from knowing tiling details like contents space. Either way is doable. What's the advantage of letting RasterSource handle it?
https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:866: static constexpr float kErrorThreshold = 0.001; Note: This code path is removed in patch set 2, as the simpler policy just blindly reuse the old tiling if raster scale is not changed.
On 2016/08/02 21:29:12, chrishtr wrote: > On 2016/08/02 at 21:13:55, lof84 wrote: > > On 2016/08/02 15:28:40, chrishtr wrote: > > > On 2016/08/02 at 12:46:19, lof84 wrote: > > > > Hello! > > > > > > > > I have tested this CL. > > > > It looks like text still blurry: > > > https://drive.google.com/open?id=0Bw59Ysn8bedgVXNPTVpQel8tZFU > > > > > > In your case, is there a zoom factor being applied? If so then you need > > > --enable-use-zoom-for-dsf also. > > > > It's 125% + --enable-use-zoom-for-dsf > > What does that mean exactly? WTR: 1) Apply this patch to HEAD on master. 2) Build. 3) Run chrome.exe --user-data-dir=./22/ --force-device-scale-factor=1.25 --enable-use-zoom-for-dsf 4) Open page with text with translate.
https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:967: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { I think this function should use draw transform and not screen space transform, as the goal here is to align to the target pixels (which itself might not be aligned to its target, but that's a separate issue). As with what I said on https://bugs.chromium.org/p/chromium/issues/detail?id=629522#c13, I think we need something like "bool cc::Layer::ShouldBePixelSnapped()" and "cc::Layer::SetPixelSnapOffset(gfx::Vector2dF)". Then we can set particular layer types that cannot reraster (texture layers, video layers, image layers?) to have a pixel snapped position. This should probably all be wrapped up at the property tree level, I think. I'm not sure if we need a separate transform node for the pixel snap offset, or if the draw transform itself should just include the pixel snap offset? It might be worth doing that plumbing first and fixing WebGL, and then we tweak it for picture layers after that. https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:415: gfx::RectF dest_rect_in_contents_space(dest_rect_); This is the math that's most concerning to me. I think my understanding about the way contents translation works, is this. layer rect + original draw transform should be equivalent to content rect + adjusted draw transform + contents translation. That means that any content rect needs to have the contents translation applied. Do you also need to adjust the rect going into PictureLayerTilingSet::CoverageIterator as well, for consistency? Could these things be called "destination space" maybe, aka max content scaled layer rect without the contents translation, to differentiate from content space and layer space? https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:466: gfx::RectF tile_bounds_in_dest_space( I need some convincing about this math as well. My expectation is that the tiling data should be pixel aligned (if scale is the same as the render target), but that the invalidations and raster into that tiling data should be offset by the contents translation. If that's the case, then there should be no offset here, only scale. And, instead, any offset should be done at the draw transform / shared quad state level during append quads?
https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:967: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { On 2016/08/09 21:02:36, enne wrote: > I think this function should use draw transform and not screen space transform, > as the goal here is to align to the target pixels (which itself might not be > aligned to its target, but that's a separate issue). Agreed. However ajuma@ mentioned that target transform may go away in near future. Is okay to add more use of it at this time? > As with what I said on > https://bugs.chromium.org/p/chromium/issues/detail?id=629522#c13, I think we > need something like "bool cc::Layer::ShouldBePixelSnapped()" and > "cc::Layer::SetPixelSnapOffset(gfx::Vector2dF)". Then we can set particular > layer types that cannot reraster (texture layers, video layers, image layers?) > to have a pixel snapped position. This should probably all be wrapped up at the > property tree level, I think. > > I'm not sure if we need a separate transform node for the pixel snap offset, or > if the draw transform itself should just include the pixel snap offset? > > It might be worth doing that plumbing first and fixing WebGL, and then we tweak > it for picture layers after that. Ah! Now I see why we had the misunderstanding yesterday. We are talking about different approach to this problem. You are actually suggesting that we apply a small geometry tweak so that layers will be drawn at a position slightly moved from intended position. My approach doesn't change the geometry of a layer, but only change the sample position when we raster it. I think your approach is a better solution for contents not rastered by us, but I worry it could worsen edge bleeding issue. Right now the worst edge bleeding we could have is an edge right in the middle of a pixel, that results in 1-(1-0.5)*(1-0.5)=0.75 alpha value. With geometry snapping, a whole row of pixels can be skipped (unless we stretch the layer, that degrades quality too). It will become very visible even on high-DPI displays. https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:415: gfx::RectF dest_rect_in_contents_space(dest_rect_); On 2016/08/09 21:02:36, enne wrote: > This is the math that's most concerning to me. > > I think my understanding about the way contents translation works, is this. > layer rect + original draw transform should be equivalent to content rect + > adjusted draw transform + contents translation. That means that any content > rect needs to have the contents translation applied. Do you also need to adjust > the rect going into PictureLayerTilingSet::CoverageIterator as well, for > consistency? The draw transform is never adjusted. draw transform The relationship between: target space <==============> layer space, is not changed. This CL changes the relationship between layer space and contents space. The raster transform used to be only a uniform scale. This CL just loosen it so the raster transform can be a combo of uniform scale & transform. In theory we can do arbitrary transform, but coverage calculation becomes difficult. The CoverageIterator itself doesn't need to be changed as long as we return the correct geometry rect for each tile, which is indeed adjusted below. > Could these things be called "destination space" maybe, aka max content scaled > layer rect without the contents translation, to differentiate from content space > and layer space? Yes, I actually see the destination space as the layer space represented in fixed-point arithmetic. BTW I don't see a compelling reason why it needs to be tied to the finest tiling in the set. Why not just use the 26.6 format like LayoutUnit does? https://codereview.chromium.org/2175553002/diff/40001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:466: gfx::RectF tile_bounds_in_dest_space( On 2016/08/09 21:02:36, enne wrote: > I need some convincing about this math as well. My expectation is that the > tiling data should be pixel aligned (if scale is the same as the render target), > but that the invalidations and raster into that tiling data should be offset by > the contents translation. The tiling is not guaranteed to be pixel-aligned to anything. Although we do prefer it to align with the target space (or the layer space, if the layer is animated). Let's make a concrete example. Assume we have a layer of size (1000, 1000) with no scale involved. For illustrative purpose, we raster it with a ridiculously huge content translation (123.4, 234.5) at scale 1x. Now the layer bounds in the content space would be (l=123.4, t=234.5, w=1000, h=1000). As tiling always begin with (0, 0), the tiling size needs to be (1124, 1235). Consider the tile [0, 0]. It still covers the content space rect (l=0, t=0, w=256, h=256). Or minus the border texels, (l=0, t=0, w=255, h=255). Applying the inverse raster transform, the corresponding layer rect would be (l=-123.4, t=-234.5, w=255, h=255). Assume the dest space being 5.0x, then the tile bounds would be (l=-617, t=-1172.5, w=1275, h=1275) in the dest space. Taking the enclosing int rect it becomes (l=-617, t=-1173, w=1275, h=1276) which is the geometry rect. It is okay to take the enclosing rect because of the border texels. Now intersecting it with the layer bounds in dest space, we get (l=0, t=0, w=658, h=103) this is the quad bounds in dest space. > If that's the case, then there should be no offset here, only scale. And, > instead, any offset should be done at the draw transform / shared quad state > level during append quads? The offset needs to be applied when we map from the quad to actual texels. Continue the above example, the quad (l=0, t=0, w=658, h=103) will be emitted with our normal draw transform. Now what is its corresponding rect in texture space? The answer is to apply the raster transform again. (l=0, t=0, w=658, h=103) --> (l=0, t=0, w=131.6, h=20.6) in layer space --> (l=123.4, t=234.5, w=131.6, h=20.6) in contents space. Because it is the top-left tile, the texture space is the same as the contents space.
ajuma@chromium.org changed reviewers: + ajuma@chromium.org
https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:967: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { On 2016/08/10 01:44:14, trchen wrote: > On 2016/08/09 21:02:36, enne wrote: > > I think this function should use draw transform and not screen space > transform, > > as the goal here is to align to the target pixels (which itself might not be > > aligned to its target, but that's a separate issue). > > Agreed. However ajuma@ mentioned that target transform may go away in near > future. More specifically, once we're able to dynamically update render surfaces at draw time, it'd be nice to stop computing render surfaces (and draw transforms) on the pending tree, particularly since the render surfaces we compute there could change at draw time (if, say, opacities or transforms change).
https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:967: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { On 2016/08/10 at 01:44:14, trchen wrote: > You are actually suggesting that we apply a small geometry tweak so that layers will be drawn at a position slightly moved from intended position. My approach doesn't change the geometry of a layer, but only change the sample position when we raster it. > > I think your approach is a better solution for contents not rastered by us, but I worry it could worsen edge bleeding issue. Right now the worst edge bleeding we could have is an edge right in the middle of a pixel, that results in 1-(1-0.5)*(1-0.5)=0.75 alpha value. With geometry snapping, a whole row of pixels can be skipped (unless we stretch the layer, that degrades quality too). It will become very visible even on high-DPI displays. I think it is *required* that this approach change transforms in all cases. This is what scroll snapping does currently. If you do not snap the geometry to align with target pixels, the result will be blurry as it will smear across multiple pixels due to bilinear sampling. Additionally, a raster translation without a corresponding geometry counter-translation will end up rastering at the incorrect location on screen. If you are worried about edge pixels, then we can change the size of the layer as well. On 2016/08/10 at 13:32:04, ajuma wrote: > On 2016/08/10 01:44:14, trchen wrote: > > On 2016/08/09 21:02:36, enne wrote: > > > I think this function should use draw transform and not screen space > > transform, > > > as the goal here is to align to the target pixels (which itself might not be > > > aligned to its target, but that's a separate issue). > > > > Agreed. However ajuma@ mentioned that target transform may go away in near > > future. > > More specifically, once we're able to dynamically update render surfaces at draw time, it'd be nice to stop computing render surfaces (and draw transforms) on the pending tree, particularly since the render surfaces we compute there could change at draw time (if, say, opacities or transforms change). If we are going to commit to the complexity for this feature, I would like to implement it properly in both the present state of the code and in the future of property trees. I think it is required to snap to target pixels (and for those targets maybe to get a draw offset and pixel snap to their targets if needed, recursively). How are we going to accommodate this? Yes, scroll snapping currently goes to screen space, but it's a hack because it's not in a position to adjust geometry recursively up the tree. It's also much less likely for scrolling regions to be inside of a non-root render surface.
On 2016/08/10 18:35:05, enne wrote: > https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/2175553002/diff/40001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:967: gfx::Vector2dF > PictureLayerImpl::CalculateRasterTranslation() { > On 2016/08/10 at 01:44:14, trchen wrote: > > You are actually suggesting that we apply a small geometry tweak so that > layers will be drawn at a position slightly moved from intended position. My > approach doesn't change the geometry of a layer, but only change the sample > position when we raster it. > > > > I think your approach is a better solution for contents not rastered by us, > but I worry it could worsen edge bleeding issue. Right now the worst edge > bleeding we could have is an edge right in the middle of a pixel, that results > in 1-(1-0.5)*(1-0.5)=0.75 alpha value. With geometry snapping, a whole row of > pixels can be skipped (unless we stretch the layer, that degrades quality too). > It will become very visible even on high-DPI displays. > > I think it is *required* that this approach change transforms in all cases. > This is what scroll snapping does currently. If you do not snap the geometry to > align with target pixels, the result will be blurry as it will smear across > multiple pixels due to bilinear sampling. Additionally, a raster translation > without a corresponding geometry counter-translation will end up rastering at > the incorrect location on screen. If you are worried about edge pixels, then we > can change the size of the layer as well. The counter-translation happens at texture mapping level. The draw transform of layers doesn't change, and the drawing bound of the layers doesn't change either. The idea is that we try to align target space with texture (contents) space. Without this CL, the texture space aligns with local (layer) space, which doesn't necessarily align with the target space. This CL shifts the texture space by a translation so it aligns with the target. The only thing changed is the placement of the sample points in the texture. The layer geometry stays absolutely the same. > On 2016/08/10 at 13:32:04, ajuma wrote: > > On 2016/08/10 01:44:14, trchen wrote: > > > On 2016/08/09 21:02:36, enne wrote: > > > > I think this function should use draw transform and not screen space > > > transform, > > > > as the goal here is to align to the target pixels (which itself might not > be > > > > aligned to its target, but that's a separate issue). > > > > > > Agreed. However ajuma@ mentioned that target transform may go away in near > > > future. > > > > More specifically, once we're able to dynamically update render surfaces at > draw time, it'd be nice to stop computing render surfaces (and draw transforms) > on the pending tree, particularly since the render surfaces we compute there > could change at draw time (if, say, opacities or transforms change). > > If we are going to commit to the complexity for this feature, I would like to > implement it properly in both the present state of the code and in the future of > property trees. I think it is required to snap to target pixels (and for those > targets maybe to get a draw offset and pixel snap to their targets if needed, > recursively). How are we going to accommodate this? > > Yes, scroll snapping currently goes to screen space, but it's a hack because > it's not in a position to adjust geometry recursively up the tree. It's also > much less likely for scrolling regions to be inside of a non-root render > surface. Sure, I can change it to use target transform. In case that the target transform changed during tree commit, the drawing result will still be correct. Only with slightly degraded quality due to bilinear resampling (which happens anyway even if we rastered in local space).
https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:992: gfx::Transform draw_transform = DrawTransform(); We can certainly use draw transform to align instead. The above check at line 989 becomes weird though, because we can have static draw transform while having animating screen transform. Personally I think it is okay to use either. Using the draw transform makes more sense in today's world. In the future world I believe draw transform will be equal to screen transform in most of the cases (i.e. render surfaces align with device pixels).
It is still a lot of work to wrap my head around all of the subtle math changes in PictureLayerTiling. There's a lot of rects that need to be adjusted. Even if this is all correct as it stands, it seems quite fragile and easy to break in the future. I don't mean to force the approach that I had been thinking of first, but making the counter adjustment at the draw transform level keeps PictureLayerTiling and its iterator a lot simpler. Even if you just adjust the draw transform during AppendQuads, I think it's a whole lot less code and complication. What are your concerns with this approach? What are you going to do with webgl canvas layers? There's a lot of duplicated code to set up canvases. Probably RasterSource::PlaybackToCanvas should do this work? I'm not sure why you have commented that the longer form of PlaybackToCanvas is deprecated. Can you help me understand what's going on here? https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:992: gfx::Transform draw_transform = DrawTransform(); On 2016/08/12 at 22:11:30, trchen wrote: > We can certainly use draw transform to align instead. The above check at line 989 becomes weird though, because we can have static draw transform while having animating screen transform. > > Personally I think it is okay to use either. Using the draw transform makes more sense in today's world. In the future world I believe draw transform will be equal to screen transform in most of the cases (i.e. render surfaces align with device pixels). If you have a static draw transform, then you are already snapped to pixel coordinates and its your target that needs to adjust. Adjusting to screen coordinates would be incorrect. https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:417: dest_rect_in_contents_space.Offset(tiling->contents_translation()); It seems that contents translation is in layer space (given by the fact that you translate then scale when rastering). However here dest rect is scaled into content space and then you translate using this layer space translation? How is this correct?
On 2016/08/16 23:02:53, enne wrote: > It is still a lot of work to wrap my head around all of the subtle math changes > in PictureLayerTiling. There's a lot of rects that need to be adjusted. Even > if this is all correct as it stands, it seems quite fragile and easy to break in > the future. I think one way to improve maintainability of this code is to add a TranslateAndScale transform type to bundle raster scale and raster translate together. > I don't mean to force the approach that I had been thinking of first, but making > the counter adjustment at the draw transform level keeps PictureLayerTiling and > its iterator a lot simpler. Even if you just adjust the draw transform during > AppendQuads, I think it's a whole lot less code and complication. What are your > concerns with this approach? The biggest issue is edge bleeding, unless we also stretch the layer while snapping, edge bleeding could be worsen from 25% to 100%. However stretching layers can create geometric non-uniformity, e.g. layers 50.5px wide can be drawn as 50px or 51px. Also it modifies geometry of things. Basically it trades correctness for aesthetic. It is not necessarily a bad thing, just different trade off. I don't this CL adds much complexity. None of the math changed in terms of algebraic property. We just allow a broader set of raster transform. > What are you going to do with webgl canvas layers? I don't know yet. But unlike traditional HTML contents, it is a evolving standard and we don't have so much legacy contents to workaround. IMO we should reveal enough information for the web developer to place their canvas without aliasing, but nothing we can do if they insist to screw up. > There's a lot of duplicated code to set up canvases. Probably > RasterSource::PlaybackToCanvas should do this work? I'm not sure why you have > commented that the longer form of PlaybackToCanvas is deprecated. Can you help > me understand what's going on here? Because we leaked too much information to RasterSource through that interface. For example, what's the distinction between canvas_bitmap_rect and canvas_playback_rect, and why does raster source need to know it? What is content scale and why don't you just specify that in CTM? I agree it is useful to make a helper function to avoid duplicated code, but I think the helper function needs to be done at a higher level. I don't think these canvas setup should be done in either raster source or buffer provider level. > https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:992: gfx::Transform draw_transform = > DrawTransform(); > On 2016/08/12 at 22:11:30, trchen wrote: > > We can certainly use draw transform to align instead. The above check at line > 989 becomes weird though, because we can have static draw transform while having > animating screen transform. > > > > Personally I think it is okay to use either. Using the draw transform makes > more sense in today's world. In the future world I believe draw transform will > be equal to screen transform in most of the cases (i.e. render surfaces align > with device pixels). > > If you have a static draw transform, then you are already snapped to pixel > coordinates and its your target that needs to adjust. Adjusting to screen > coordinates would be incorrect. Agreed, just saying it may be too conservative. > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > File cc/tiles/picture_layer_tiling.cc (right): > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:417: > dest_rect_in_contents_space.Offset(tiling->contents_translation()); > It seems that contents translation is in layer space (given by the fact that you > translate then scale when rastering). However here dest rect is scaled into > content space and then you translate using this layer space translation? How is > this correct? max_scale contents_scale contents_translate dest <=========> layer <==============> <==================> contents Translate then scale on the canvas CTM means scale then translate from the layer's point of view. :) I agree the math is non-trivial here: dest_to_content = dest_to_layer * layer_to_content = dest_to_layer * (content_scale * content_translate) = (dest_to_layer * content_scale) * content_translate = dest_to_content_scale * content_translate It relies on the ordering of content_scale and content_translate, which really should be encapsulated by a TranslateAndScale class.
On 2016/08/17 at 00:05:13, trchen wrote: > On 2016/08/16 23:02:53, enne wrote: > > It is still a lot of work to wrap my head around all of the subtle math > > changes in PictureLayerTiling. There's a lot of rects that need to be > > adjusted. Even if this is all correct as it stands, it seems quite fragile > > and easy to break in the future. > > I think one way to improve maintainability of this code is to add a > TranslateAndScale transform type to bundle raster scale and raster translate > together. I think the way to increase maintainability is to simplify and to name the spaces that you're transforming between. Previously, "layer space" and "content space" had very specific meanings to them and straightforward ways to transform between the two. In this patch, content space becomes something else entirely and there's at least one other unnamed space. This is why it feels complex to me. This may all make sense to you, but this patch does not present a simple way to understand the code and the transformations between different geometrical spaces. Talking of spaces, one thing that doesn't make sense to me is that you use the same "contents" translation for every tiling, even though they have different scales. In some places like raster, you handle this by translating first in layer space and then scaling to content/canvas space after. In other places like EnclosingContentsRectFromLayerRect you scale first and then translate, even though that translation (I think) is in layer space. It seems like every tiling *should* need a different offset (at least in content space), because the same translation in layer space is not the same in every tiling. Similarly, I think a layer space translation must be between 0 and 1, but a content space translation does not respect that. I'm not sure your DCHECKs reflect that. You DCHECK that both layer space and content space translations are between 0 and 1, unless I continue to misunderstand how your patch works. > > I don't mean to force the approach that I had been thinking of first, but > > making the counter adjustment at the draw transform level keeps > > PictureLayerTiling and its iterator a lot simpler. Even if you just adjust > > the draw transform during AppendQuads, I think it's a whole lot less code > > and complication. What are your concerns with this approach? > > The biggest issue is edge bleeding, unless we also stretch the layer while snapping, edge bleeding could be worsen from 25% to 100%. However stretching layers can create geometric non-uniformity, e.g. layers 50.5px wide can be drawn as 50px or 51px. We cannot stretch layers, even very small amounts on very large layers. I have already learned that lesson too well. <_< Edges are already incorrect in this patch, as far as I understand it. In your patch, you raster at |t| offset and then adjust texture coordinates by |-t|. This means that when producing quads for |rect|, that the texture coordinate at |rect.width()| will be |rect.width() - t|. Because of the way edge antialiasing works in the GLRenderer, that means that it will clamp texture coordinates at |rect.width() - t - 0.5| and then fade from there. At large values of |t| it means that you will miss more and more of the last row and column of pixels in the tile. I think in any case, to do edges more correctly, we'd probably need to maybe specify some additional information and possibly also try to counteract the fact that the last row and column will always be only partially covered in these cases and so will already come with some non-opaque alpha. I'm not sure how important solving this correctly is, but I'm just saying that edge issues are already a problem with the patch as-is. > Also it modifies geometry of things. Basically it trades correctness for > aesthetic. It is not necessarily a bad thing, just different trade off. I don't think I agree that these are the trade offs. > I don't this CL adds much complexity. None of the math changed in terms of > algebraic property. We just allow a broader set of raster transform. I am not trying to be rude here, but I think this bit of code is complicated enough already. I was the original author of PictureLayer(Impl|Tiling|TilingSet) and so if it takes me a while to stare at your code and understand what it's trying to do, then it's going to take other people even longer (and future me longest of all when I have to fix a bug here). > > What are you going to do with webgl canvas layers? > > I don't know yet. But unlike traditional HTML contents, it is a evolving > standard and we don't have so much legacy contents to workaround. IMO we > should reveal enough information for the web developer to place their canvas > without aliasing, but nothing we can do if they insist to screw up. I don't think should be on the web developer to fix. I think we should provide a better experience by default. > > There's a lot of duplicated code to set up canvases. Probably > > RasterSource::PlaybackToCanvas should do this work? I'm not sure why you > > have commented that the longer form of PlaybackToCanvas is deprecated. Can > > you help me understand what's going on here? > > Because we leaked too much information to RasterSource through that > interface. For example, what's the distinction between canvas_bitmap_rect and > canvas_playback_rect, and why does raster source need to know it? What is > content scale and why don't you just specify that in CTM? > > I agree it is useful to make a helper function to avoid duplicated code, but > I think the helper function needs to be done at a higher level. I don't think > these canvas setup should be done in either raster source or buffer provider > level. I disagree. I think if every callsite needs to do the same thing, then it's not leaked information. Every raster source is recorded in layer space. If you played a raster source back at scale 1 with no transform, the origin of the canvas would be the origin of the layer and layer space and canvas space would be 1:1 with each other. A tile is a subrectangle of content at some scale. To get from layer space to content space, you need a scale. To raster a tile, you need the tile's content rect and a scale to go from layer space to content space. Additionally, the tile may only need to be partially rastered and so there's an additional "dirty content rect" which is contained by the tile's content rect. These two rects are the "raster_full_rect" and "raster_dirty_rect" given to OneCopyRasterBufferProvider::PlaybackToStagingBuffer, for example. The canvas_playback_rectangle corresponds to the raster_dirty_rect. It represents the portion of the raster source that needs to be played back in content space. The canvas_bitmap_rect is the full content rect of the tile. To play back a raster source into a tile, you need to do the following operations in all cases: (1) scale from layer space to content space (using the content scale) (2) clip for partial raster (using the playback rect) (3) translate from content space to tile space (using the bitmap rect) I think these operations are a fundamental part of using a raster source in cc. > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > > File cc/tiles/picture_layer_tiling.cc (right): > > > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > > cc/tiles/picture_layer_tiling.cc:417: > > dest_rect_in_contents_space.Offset(tiling->contents_translation()); It > > seems that contents translation is in layer space (given by the fact that > > you translate then scale when rastering). However here dest rect is scaled > > into content space and then you translate using this layer space > > translation? How is this correct? > [snip] > Translate then scale on the canvas CTM means scale then translate from the > layer's point of view. :) > [snip] Yes, but I am still not convinced me that this is correct. What space is contents translation supposed to be in? Is it in layer space? Is it in content space? Is it something else?
On 2016/08/17 22:17:56, enne wrote: > On 2016/08/17 at 00:05:13, trchen wrote: > > On 2016/08/16 23:02:53, enne wrote: > I think the way to increase maintainability is to simplify and to name the > spaces that you're transforming between. Previously, "layer space" and "content > space" had very specific meanings to them and straightforward ways to transform > between the two. In this patch, content space becomes something else entirely > and there's at least one other unnamed space. This is why it feels complex to > me. This may all make sense to you, but this patch does not present a simple > way to understand the code and the transformations between different geometrical > spaces. The other unnamed space you are referring to is probably the space that is scaled but not translated yet. I don't think anyone should use that space anywhere. That's why I propose doing the bundled TranslateAndScale so the intermediate space never leak out. > Talking of spaces, one thing that doesn't make sense to me is that you use the > same "contents" translation for every tiling, even though they have different > scales. In some places like raster, you handle this by translating first in > layer space and then scaling to content/canvas space after. In other places > like EnclosingContentsRectFromLayerRect you scale first and then translate, even > though that translation (I think) is in layer space. It seems like every tiling > *should* need a different offset (at least in content space), because the same > translation in layer space is not the same in every tiling. Similarly, I think > a layer space translation must be between 0 and 1, but a content space > translation does not respect that. I'm not sure your DCHECKs reflect that. You > DCHECK that both layer space and content space translations are between 0 and 1, > unless I continue to misunderstand how your patch works. It is a bit confusing to say doing translation first or later, as invoking canvas->translate() first actually means translating the object last. There is only one invariant here: point_in_content_space = point_in_layer_space.Scale(content_scale).Translate(content_translate) And the inverse: point_in_layer_space = point_in_content_space.Translate(-content_translate).Scale(1/content_scale) The DCHECK in picture_layer_tiling.cc:55 make sure we don't unnecessarily waste texture memory. Translation more than 1 means adding transparent padding pixels on the topmost rows / leftmost columns. > We cannot stretch layers, even very small amounts on very large layers. I have > already learned that lesson too well. <_< > > Edges are already incorrect in this patch, as far as I understand it. In your > patch, you raster at |t| offset and then adjust texture coordinates by |-t|. > This means that when producing quads for |rect|, that the texture coordinate at > |rect.width()| will be |rect.width() - t|. Because of the way edge antialiasing > works in the GLRenderer, that means that it will clamp texture coordinates at > |rect.width() - t - 0.5| and then fade from there. At large values of |t| it > means that you will miss more and more of the last row and column of pixels in > the tile. Yes, it will still have edge bleeding, but won't make edge bleeding worse. Antialiasing will happen in Skia, but won't happen again at GL level. Let's make an example: <div style="background:white; width:3px; height:3px; transform:translate(0.5px,0.5px);"> Without my CL, the texture would be rastered as: 1 1 1 1 1 1 1 1 1 And the ultimate drawing result would be 0.25 0.50 0.50 0.25 0.50 1.00 1.00 1.00 0.50 1.00 1.00 1.00 0.25 0.50 0.50 0.25 Due to clamping and bilinear filtering you explained above. With my CL, the texture will instead be rastered with a translation: 0.25 0.50 0.50 0.25 0.50 1.00 1.00 1.00 0.50 1.00 1.00 1.00 0.25 0.50 0.50 0.25 And the drawing result will stay the same, because each pixel on the screen will have texture sample point coincide with the center of each texels. > I think in any case, to do edges more correctly, we'd probably need to maybe > specify some additional information and possibly also try to counteract the fact > that the last row and column will always be only partially covered in these > cases and so will already come with some non-opaque alpha. It will be very difficult, essentially you'll be doing polygon coverage at pixel level. A more practical way is to give up bilinear filtering and do MSAA instead. Another idea I have in mind is to give more control to the web developers and hope them to understand the limitation of pixel-based graphics and to do the right thing. Specifically, extend CSS mix-blend-mode to allow SkXfermode::kPlus_Mode. > I'm not sure how important solving this correctly is, but I'm just saying that > edge issues are already a problem with the patch as-is. > > > I don't this CL adds much complexity. None of the math changed in terms of > > algebraic property. We just allow a broader set of raster transform. > > I am not trying to be rude here, but I think this bit of code is complicated > enough already. I was the original author of > PictureLayer(Impl|Tiling|TilingSet) and so if it takes me a while to stare at > your code and understand what it's trying to do, then it's going to take other > people even longer (and future me longest of all when I have to fix a bug here). This is what I don't understand. Most part of it is just mechanically replacing every instance of content_scale with a combo of scale+translate. I do believe that making an opaque class for the bundle would help abstracting away some complexity. > > > What are you going to do with webgl canvas layers? > > > > I don't know yet. But unlike traditional HTML contents, it is a evolving > > standard and we don't have so much legacy contents to workaround. IMO we > > should reveal enough information for the web developer to place their canvas > > without aliasing, but nothing we can do if they insist to screw up. > > I don't think should be on the web developer to fix. I think we should provide > a better experience by default. It became an ideology debate. I think a consensus should be reached by discussing with other vendors and web developers. > I disagree. I think if every callsite needs to do the same thing, then it's not > leaked information. > > Every raster source is recorded in layer space. If you played a raster source > back at scale 1 with no transform, the origin of the canvas would be the origin > of the layer and layer space and canvas space would be 1:1 with each other. > > A tile is a subrectangle of content at some scale. To get from layer space to > content space, you need a scale. To raster a tile, you need the tile's content > rect and a scale to go from layer space to content space. Additionally, the > tile may only need to be partially rastered and so there's an additional "dirty > content rect" which is contained by the tile's content rect. These two rects > are the "raster_full_rect" and "raster_dirty_rect" given to > OneCopyRasterBufferProvider::PlaybackToStagingBuffer, for example. > > The canvas_playback_rectangle corresponds to the raster_dirty_rect. It > represents the portion of the raster source that needs to be played back in > content space. The canvas_bitmap_rect is the full content rect of the tile. > > To play back a raster source into a tile, you need to do the following > operations in all cases: > > (1) scale from layer space to content space (using the content scale) > (2) clip for partial raster (using the playback rect) > (3) translate from content space to tile space (using the bitmap rect) > > I think these operations are a fundamental part of using a raster source in cc. Why does raster source need to know/handle all those tiling-specific logic? Raster source can be used without tiling. e.g. the resource-less mode. These common logic needs to be de-duped, but it is not raster source's responsibility. Even the raster buffer doesn't sound like the right place. Why does raster buffer need to be used in a tiled manner? IMO raster buffer implementations should only setup a SkCanvas, and it's up to the client to decide what to do with the canvas. > > > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > > > File cc/tiles/picture_layer_tiling.cc (right): > > > > > > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > > > cc/tiles/picture_layer_tiling.cc:417: > > > dest_rect_in_contents_space.Offset(tiling->contents_translation()); It > > > seems that contents translation is in layer space (given by the fact that > > > you translate then scale when rastering). However here dest rect is scaled > > > into content space and then you translate using this layer space > > > translation? How is this correct? > > [snip] > > Translate then scale on the canvas CTM means scale then translate from the > > layer's point of view. :) > > [snip] > > Yes, but I am still not convinced me that this is correct. What space is > contents translation supposed to be in? Is it in layer space? Is it in content > space? Is it something else? Neither, is is one of the two steps to convert between layer space and content space. It is unfortunate that applying dest_to_content_scale_ would yield an intermediate space. To make cleaner, I really should have eliminated dest_to_content_scale_ and make it dest<==>layer and layer<==>content two steps instead.
> > Yes, but I am still not convinced me that this is correct. What space is > > contents translation supposed to be in? Is it in layer space? Is it in content > > space? Is it something else? > > Neither, is is one of the two steps to convert between layer space and content space. This is where I continue to feel like we are just failing to communicate with each other. I'm sure this patch makes sense to you, but you are not doing a good enough job to help me understand how your patch works and to make it explainable. This translation is in either layer space or content space or some third space. Which space is it a valid translation in?
On 2016/08/18 17:31:24, enne wrote:
> > > Yes, but I am still not convinced me that this is correct. What space is
> > > contents translation supposed to be in? Is it in layer space? Is it in
> content
> > > space? Is it something else?
> >
> > Neither, is is one of the two steps to convert between layer space and
content
> space.
>
> This is where I continue to feel like we are just failing to communicate with
> each other. I'm sure this patch makes sense to you, but you are not doing a
> good enough job to help me understand how your patch works and to make it
> explainable.
>
> This translation is in either layer space or content space or some third
space.
> Which space is it a valid translation in?
To go from layer space to contents space, we have to
1. Multiply the coordinate by contents_scale
2. Add contents_translation into the coordinate
In between the two steps we are temporary in an unnamed space, which I referred
it as "scaled layer space". However this space has no practical use, and no
coordinates should be represented in this space outside of local scope of a code
block.
In previous comments I said that I should have made contents_scale and
contents_translation into a bundle. It would improve readability of this code
because we can always jump over the intermediate space.
---
Another thing I could have improved is to write in functional style over
in-place mutation. For example:
gfx::RectF tile_bounds_in_dest_space(
tiling_->tiling_data_.TileBounds(tile_i_, tile_j_));
tile_bounds_in_dest_space.Offset(-tiling_->contents_translation());
tile_bounds_in_dest_space.Scale(1 / dest_to_content_scale_);
current_geometry_rect_ = gfx::ToEnclosingRect(tile_bounds_in_dest_space);
This code is very hard to comprehend because tile_bounds_in_dest_space is
reassigned with 3 values, and only the last value really is the tile bound in
the dest space. A better way to write this is:
gfx::RectF tile_bounds_in_dest_space =
ScaleRect(tiling_->tiling_data_.TileBounds(tile_i_, tile_j_) -
tiling_->contents_translation(), 1 / dest_to_content_scale_);
current_geometry_rect_ = gfx::ToEnclosingRect(tile_bounds_in_dest_space);
I should have done that in the beginning but I wasn't familiar with ui::gfx
helper functions. Sorry for letting you read bad code. :(
On 2016/08/18 at 21:54:13, trchen wrote: > On 2016/08/18 17:31:24, enne wrote: > > > > Yes, but I am still not convinced me that this is correct. What space is > > > > contents translation supposed to be in? Is it in layer space? Is it in > > content > > > > space? Is it something else? > > > > > > Neither, is is one of the two steps to convert between layer space and content > > space. > > > > This translation is in either layer space or content space or some third space. > > Which space is it a valid translation in? > > To go from layer space to contents space, we have to > 1. Multiply the coordinate by contents_scale > 2. Add contents_translation into the coordinate So to be clear, the translation is in content space?
On 2016/08/18 21:58:00, enne wrote: > On 2016/08/18 at 21:54:13, trchen wrote: > > On 2016/08/18 17:31:24, enne wrote: > > > > > Yes, but I am still not convinced me that this is correct. What space > is > > > > > contents translation supposed to be in? Is it in layer space? Is it in > > > content > > > > > space? Is it something else? > > > > > > > > Neither, is is one of the two steps to convert between layer space and > content > > > space. > > > > > > This translation is in either layer space or content space or some third > space. > > > Which space is it a valid translation in? > > > > To go from layer space to contents space, we have to > > 1. Multiply the coordinate by contents_scale > > 2. Add contents_translation into the coordinate > > So to be clear, the translation is in content space? Converts from an unnamed space to content space.
> > > To go from layer space to contents space, we have to > > > 1. Multiply the coordinate by contents_scale > > > 2. Add contents_translation into the coordinate > > > > So to be clear, the translation is in content space? > > Converts from an unnamed space to content space. I think we're saying the same thing. contents_translation is a vector in content space, not in layer space. Since unnamed space and content space only differ by a translation, any vector is equivalent in either space. A vector in layer space will not be the same as a vector in layer space as those spaces differ by a scale as well. Would you agree that a translation in layer space to pixel snap a layer (or its texture coordinates) would necessarily be a vector whose components are between -1 and 1? (or 0 and 1 if you're only snapping in one direction). Maybe call this layer_translation? If so, then wouldn't contents_translation be a scaled version of layer_translation, scaled by contents scale?
"A vector in layer space will not be the same as a vector in layer space" => A vector in layer space will not be the same as a vector in *content* space
On 2016/08/18 22:16:51, enne wrote: > > > > To go from layer space to contents space, we have to > > > > 1. Multiply the coordinate by contents_scale > > > > 2. Add contents_translation into the coordinate > > > > > > So to be clear, the translation is in content space? > > > > Converts from an unnamed space to content space. > > I think we're saying the same thing. contents_translation is a vector in > content space, not in layer space. Since unnamed space and content space only > differ by a translation, any vector is equivalent in either space. A vector in > layer space will not be the same as a vector in layer space as those spaces > differ by a scale as well. The result would be equivalent but I wouldn't say contents_translation is a vector. It is really a matrix that converts points between spaces. It is stored as Vector2dF only because it is the closest class that offers all needed functionality. > Would you agree that a translation in layer space to pixel snap a layer (or its > texture coordinates) would necessarily be a vector whose components are between > -1 and 1? (or 0 and 1 if you're only snapping in one direction). Maybe call > this layer_translation? This is untrue. Let's say we have transform:scale(0.2)translate(3px,3px), in order to align texture space with target space a content translation of (0.6, 0.6) is required, but the same translation measures (3, 3) in layer space. > If so, then wouldn't contents_translation be a scaled version of > layer_translation, scaled by contents scale? You can say that, but why?
On 2016/08/18 at 22:26:55, trchen wrote: > On 2016/08/18 22:16:51, enne wrote: > > > > > To go from layer space to contents space, we have to > > > > > 1. Multiply the coordinate by contents_scale > > > > > 2. Add contents_translation into the coordinate > > > > > > > > So to be clear, the translation is in content space? > > > > > > Converts from an unnamed space to content space. > > > > I think we're saying the same thing. contents_translation is a vector in > > content space, not in layer space. Since unnamed space and content space only > > differ by a translation, any vector is equivalent in either space. A vector in > > layer space will not be the same as a vector in layer space as those spaces > > differ by a scale as well. > > The result would be equivalent but I wouldn't say contents_translation is a vector. It is really a matrix that converts points between spaces. It is stored as Vector2dF only because it is the closest class that offers all needed functionality. It *is* a vector, though. > > Would you agree that a translation in layer space to pixel snap a layer (or its > > texture coordinates) would necessarily be a vector whose components are between > > -1 and 1? (or 0 and 1 if you're only snapping in one direction). Maybe call > > this layer_translation? > > This is untrue. Let's say we have transform:scale(0.2)translate(3px,3px), in order to align texture space with target space a content translation of (0.6, 0.6) is required, but the same translation measures (3, 3) in layer space. Ok. So layer_translation really can be of any size, and thus contents_translation, being a scaled version of that, can also be of any size. What do you mean by "DCHECK(contents_translation.x() >= 0.f && contents_translation.x() < 1.f)" then? > > If so, then wouldn't contents_translation be a scaled version of > > layer_translation, scaled by contents scale? > > You can say that, but why? It's a different way of thinking about the problem. It means that every PictureLayerTiling shares the same layer_translation, but each PictureLayerTiling should have a unique contents_translation because they each have a different contents scale. Is that correct?
On 2016/08/18 22:31:34, enne wrote: > On 2016/08/18 at 22:26:55, trchen wrote: > > On 2016/08/18 22:16:51, enne wrote: > > > > > > To go from layer space to contents space, we have to > > > > > > 1. Multiply the coordinate by contents_scale > > > > > > 2. Add contents_translation into the coordinate > > > > > > > > > > So to be clear, the translation is in content space? > > > > > > > > Converts from an unnamed space to content space. > > > > > > I think we're saying the same thing. contents_translation is a vector in > > > content space, not in layer space. Since unnamed space and content space > only > > > differ by a translation, any vector is equivalent in either space. A vector > in > > > layer space will not be the same as a vector in layer space as those spaces > > > differ by a scale as well. > > > > The result would be equivalent but I wouldn't say contents_translation is a > vector. It is really a matrix that converts points between spaces. It is stored > as Vector2dF only because it is the closest class that offers all needed > functionality. > > It *is* a vector, though. It doesn't need to be. It could be arbitrary matrix, and the technical reason why it's limited to scale+translation is due to coverage iterator becomes very hard to implement if tile geometry are not rect. > > > Would you agree that a translation in layer space to pixel snap a layer (or > its > > > texture coordinates) would necessarily be a vector whose components are > between > > > -1 and 1? (or 0 and 1 if you're only snapping in one direction). Maybe call > > > this layer_translation? > > > > This is untrue. Let's say we have transform:scale(0.2)translate(3px,3px), in > order to align texture space with target space a content translation of (0.6, > 0.6) is required, but the same translation measures (3, 3) in layer space. > > Ok. So layer_translation really can be of any size, and thus > contents_translation, being a scaled version of that, can also be of any size. > > What do you mean by "DCHECK(contents_translation.x() >= 0.f && > contents_translation.x() < 1.f)" then? Because it would be a waste of space to translate by more than 1 texels. Translating the texture space by an integer amount results in the same set of sample points, just with different coordinates. Also to remove that condition we need to change cc/tiles/picture_layer_tiling.cc:60-62: - DCHECK(content_bounds_rect.origin() == gfx::Point()); - - gfx::Size tile_size = client_->CalculateTileSize(content_bounds_rect.size()); + DCHECK(content_bounds_rect.origin().x() >= 0 && content_bounds_rect.origin().y() >= 0); + + gfx::Size tile_size = client_->CalculateTileSize(content_bounds_rect.bottom_right()); > > > If so, then wouldn't contents_translation be a scaled version of > > > layer_translation, scaled by contents scale? > > > > You can say that, but why? > > It's a different way of thinking about the problem. It means that every > PictureLayerTiling shares the same layer_translation, but each > PictureLayerTiling should have a unique contents_translation because they each > have a different contents scale. Is that correct? Yes that could be done. Are you suggesting that we could snap the layer draw transform, while adding an offset to the whole RasterSource at the same time? I thought chrishtr@ tried that but more code complexity would be involved?
After staring at this for a long time, I think I understand what this patch is trying to do. This patch, when creating new tilings, tries to figure out if the tiling is at a scale where it matches the draw transform scale and if so calculates a contents translation in the range of 0-1 in both x and y to snap to the nearest pixel in content space. This offset used at raster time. The tiling size is grown by an extra pixel if needed. At draw time, the texture coordinates are also adjusted to try to account for this so that texture sampling lines up with target space. Is this correct? Here's the rest of my concerns, most of which are at a high level: Correctness. See: http://imgur.com/a/Pqv7E. I took The Verge (as an example of images and text) and added an 0.5 translation to it and then looked with and without your patch. Text does look a *lot* better with your patch. However, one major thing that is concerning is that the "Uprooted" background image has some bleeding on the left edge. Doing the same thing with --disable-gpu or without your patch does not have such bleeding. Also other minor differences stick out: Some images also appear to shift right one pixel (although text stays in place) vs without your patch. Some text appears to shift left one pixel with your patch vs --disable-gpu. Are these all expected? PictureLayerTiling management. I think PictureLayerTilingSet has not been touched as much as it should in this patch. In many cases, functions look for tilings by content scale and ignore translation, and I think this is not right in some cases. I've left some comments about this. RasterSource API. I still disagree about where this should live. I think it's fine for RasterSource to have a helper function for its common use case. If we ever end up with raster that doesn't need to go to tiles, we can add more interfaces. I don't think your changes make the code easier to read. Opaqueness. I'm not sure that opaqueness is supported properly here. I think you'd need to add extra border texels when rastering to do this. You'd need to pass the contents translation to RasterSource::PlaybackToCanvas in order for it to do that work, in my opinion. You could also say that such layers are not opaque or use EnclosedRect or something to limit the opaqueness as an alternate approach. Policy. I think PictureLayerImpl needs a "ShouldSnapRasterToTargetPixels" bool that defaults to false, and contents translation should always be empty in those cases. This would let you land this without breaking anything but still test it. Tests. I think this needs unit tests and a few layer tree pixel tests with different translations. Unit tests need to test opaqueness, activation, coverage iterator, and a sanity check on texture coordinates on quads. Pixel tests could just be a green square that's pixel aligned in target space but on a layer that's not pixel aligned. Comments. It took me a while to digest the math in this patch. Having more high and low level comments about what contents translation is doing and how it's implemented and what particular transformations are doing would be appreciated. https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:877: tilings_->FindTilingWithScale(raster_contents_scale_); What if the layer moves and the contents translation for the high res version becomes not ideal? Shouldn't this code not consider that high res and make another? Your patch also does not adjust PictureLayerImpl::GetPendingOrActiveTwinTiling and so two tilings that have different content translations will be considered twins, and so "required for activation" will be incorrect. I think you need to write a unit test for this activation case to make sure that if the main thread adjusts the layer offset that activation will not occur until a tiling with the correct translation is ready. https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:988: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { This function depending on raster_contents_scale_ is a little weird, and means that it can only be used for high res. Can you make it take raster_contents_scale_ as a parameter explicitly? https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:998: static constexpr float kErrorThreshold = 0.0000001f; I think this function needs some more comments. This block of code would be good to say something like "if the draw transform scale and raster content scale are not equal, then the tiling will not be aligned to the target and so a content scale would not help with making the tiling appear crisp". https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:55: DCHECK(contents_translation.x() >= 0.f && contents_translation.x() < 1.f); Please separate these conditions onto different lines and use DCHECK_GE/DCHECK_LT https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:60: DCHECK(content_bounds_rect.origin() == gfx::Point()); Can you make this DCHECK_EQ or maybe make a separate helper since you do this somewhere else as well? https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:468: tile_bounds_in_dest_space.Offset(-tiling_->contents_translation()); I'm not 100% convinced that this satisfies the assumptions being made by coverage iterator, in particular that it will always fill the destination space perfectly. Can you add some PictureLayerTilingSetTestWithResources unittests that include translation offsets as well at different scales? https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_set.cc:86: PictureLayerTiling* this_tiling = FindTilingWithScale(contents_scale); What if there are multiple tilings with the same scale but different translations?
Uploaded a new patchset to combine raster scale and translation to an opaque class. Fixed the twin tiling mismatch bug. Will look into the --disable-gpu differences today. Still need to fix compilation for all unit tests and also add new tests. On 2016/08/22 22:57:39, enne wrote: > After staring at this for a long time, I think I understand what this patch is > trying to do. This patch, when creating new tilings, tries to figure out if the > tiling is at a scale where it matches the draw transform scale and if so > calculates a contents translation in the range of 0-1 in both x and y to snap to > the nearest pixel in content space. This offset used at raster time. The > tiling size is grown by an extra pixel if needed. At draw time, the texture > coordinates are also adjusted to try to account for this so that texture > sampling lines up with target space. Is this correct? Yes, a more accurate way to rephrase it is that, without this CL, at draw time the texture coordinate won't align with target pixels, so bi-linear filtering will be in effect. This CL aligns the contents space with target pixel using a 0~1 translation, and because the texture space of each tile aligns with the contents space so the textures will be aligned too. > Here's the rest of my concerns, most of which are at a high level: > > Correctness. See: http://imgur.com/a/Pqv7E. I took The Verge (as an example of > images and text) and added an 0.5 translation to it and then looked with and > without your patch. Text does look a *lot* better with your patch. However, > one major thing that is concerning is that the "Uprooted" background image has > some bleeding on the left edge. Doing the same thing with --disable-gpu or > without your patch does not have such bleeding. Background color bleeding on left/top edge is expected if the composited element has a background color. The bottom/right edge already bleeds anyway... The bleeding edge may switch side, but the total bleed should not worsen. > Also other minor differences > stick out: Some images also appear to shift right one pixel (although text stays > in place) vs without your patch. Some text appears to shift left one pixel with > your patch vs --disable-gpu. Are these all expected? I expected some bleeding difference because background color bleeding happens on layer edges, where --disable-gpu simply don't create layers. However positioning of things should be identical. I suspect it is due to 0.5 being a critical number that can be rounded up or down depending on numerical error. But definitely need some investigation to make sure. > PictureLayerTiling management. I think PictureLayerTilingSet has not been > touched as much as it should in this patch. In many cases, functions look for > tilings by content scale and ignore translation, and I think this is not right > in some cases. I've left some comments about this. Yes it is incorrect in some cases. While I converted all content_scale to an opaque class, I vetted through all instances when we extracted scale from the content transform, I think the only dangerous case is the GetPendingOrActiveTwinTiling you mentioned. > RasterSource API. I still disagree about where this should live. I think it's > fine for RasterSource to have a helper function for its common use case. If we > ever end up with raster that doesn't need to go to tiles, we can add more > interfaces. I don't think your changes make the code easier to read. I agree with you that a helper is definitely useful. As for where the helper should live is a bikeshedding issue. I replaced the "deprecated" comment from RasterSource::PlaybackToCanvas with a comment explaining the input arguments. > Opaqueness. I'm not sure that opaqueness is supported properly here. I think > you'd need to add extra border texels when rastering to do this. You'd need to > pass the contents translation to RasterSource::PlaybackToCanvas in order for it > to do that work, in my opinion. You could also say that such layers are not > opaque or use EnclosedRect or something to limit the opaqueness as an alternate > approach. Filling extra border texels with background color was done in the prerequisite CL: https://codereview.chromium.org/2075873002/ See RasterSource::PrepareForPlaybackToCanvas, which got a nice clean up. :) > Policy. I think PictureLayerImpl needs a "ShouldSnapRasterToTargetPixels" bool > that defaults to false, and contents translation should always be empty in those > cases. This would let you land this without breaking anything but still test > it. Acknowledged. > Tests. I think this needs unit tests and a few layer tree pixel tests with > different translations. Unit tests need to test opaqueness, activation, > coverage iterator, and a sanity check on texture coordinates on quads. Pixel > tests could just be a green square that's pixel aligned in target space but on a > layer that's not pixel aligned. Acknowledged. > Comments. It took me a while to digest the math in this patch. Having more > high and low level comments about what contents translation is doing and how > it's implemented and what particular transformations are doing would be > appreciated. Some comments are added. As for a high level overview, I think the ideal place is a README.md but we don't have that in cc. What is the conventional place for such comments in cc? https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:877: tilings_->FindTilingWithScale(raster_contents_scale_); On 2016/08/22 22:57:39, enne wrote: > What if the layer moves and the contents translation for the high res version > becomes not ideal? Shouldn't this code not consider that high res and make > another? It was what was done in patchset 1, but later I changed to make content translation recalc lazier. The policy implemented in this patchset is to never change content translation of existing tile sets. > Your patch also does not adjust PictureLayerImpl::GetPendingOrActiveTwinTiling > and so two tilings that have different content translations will be considered > twins, and so "required for activation" will be incorrect. You are right. It'd be a best if we can allow an ideal tile set and a non-ideal tile set that has different translation. In the newly uploaded patchset I made GetPendingOrActiveTwinTiling to return nullptr if the transform doesn't match. During activation the old tile set will be purged. > I think you need to write a unit test for this activation case to make sure that > if the main thread adjusts the layer offset that activation will not occur until > a tiling with the correct translation is ready. https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:988: gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() { On 2016/08/22 22:57:39, enne wrote: > This function depending on raster_contents_scale_ is a little weird, and means > that it can only be used for high res. Can you make it take > raster_contents_scale_ as a parameter explicitly? Done. https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:998: static constexpr float kErrorThreshold = 0.0000001f; On 2016/08/22 22:57:39, enne wrote: > I think this function needs some more comments. This block of code would be > good to say something like "if the draw transform scale and raster content scale > are not equal, then the tiling will not be aligned to the target and so a > content scale would not help with making the tiling appear crisp". Done. https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:55: DCHECK(contents_translation.x() >= 0.f && contents_translation.x() < 1.f); On 2016/08/22 22:57:39, enne wrote: > Please separate these conditions onto different lines and use > DCHECK_GE/DCHECK_LT Done. https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:60: DCHECK(content_bounds_rect.origin() == gfx::Point()); On 2016/08/22 22:57:39, enne wrote: > Can you make this DCHECK_EQ or maybe make a separate helper since you do this > somewhere else as well? I think we need to fix this in base/logging.h. logging::MakeCheckOpString needs operator<< to work, while the chromium convention is to use ToString. :/ https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:417: dest_rect_in_contents_space.Offset(tiling->contents_translation()); On 2016/08/16 23:02:52, enne wrote: > It seems that contents translation is in layer space (given by the fact that you > translate then scale when rastering). However here dest rect is scaled into > content space and then you translate using this layer space translation? How is > this correct? It is a cheat to take advantage of transform associativity. In the newly uploaded CL I replaced dest_to_content_scale_ with the total matrix to go from dest space to content space to make this more obvious. https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:468: tile_bounds_in_dest_space.Offset(-tiling_->contents_translation()); On 2016/08/22 22:57:39, enne wrote: > I'm not 100% convinced that this satisfies the assumptions being made by > coverage iterator, in particular that it will always fill the destination space > perfectly. Can you add some PictureLayerTilingSetTestWithResources unittests > that include translation offsets as well at different scales? It will because the pre-rounding tile_bounds_in_dest_space already fill the dest space. With ToEnclosingRect, some pixels in the dest space will be covered by more than one tiles. In fact, some rounding math here is already questionable prior to my CL. I don't think we can simply take the enclosing rect for geometry rect. I will follow up with more numerical analysis. https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_set.cc:86: PictureLayerTiling* this_tiling = FindTilingWithScale(contents_scale); On 2016/08/22 22:57:39, enne wrote: > What if there are multiple tilings with the same scale but different > translations? Currently the easiest thing to do is to replace. It shouldn't be terribly difficult to extend the tiling set to accommodate both. I think we can do it in a separate CL.
> > Correctness. See: http://imgur.com/a/Pqv7E. I took The Verge (as an example of > > images and text) and added an 0.5 translation to it and then looked with and > > without your patch. Text does look a *lot* better with your patch. However, > > one major thing that is concerning is that the "Uprooted" background image has > > some bleeding on the left edge. Doing the same thing with --disable-gpu or > > without your patch does not have such bleeding. > > Background color bleeding on left/top edge is expected if the composited element has a background color. The bottom/right edge already bleeds anyway... The bleeding edge may switch side, but the total bleed should not worsen. I don't see any bleeding on the bottom right edge. Am I missing something? > > Also other minor differences > > stick out: Some images also appear to shift right one pixel (although text stays > > in place) vs without your patch. Some text appears to shift left one pixel with > > your patch vs --disable-gpu. Are these all expected? > > I expected some bleeding difference because background color bleeding happens on layer edges, where --disable-gpu simply don't create layers. However positioning of things should be identical. I suspect it is due to 0.5 being a critical number that can be rounded up or down depending on numerical error. But definitely need some investigation to make sure. I think it is unexpected and looks quite wrong, to be honest. > > PictureLayerTiling management. I think PictureLayerTilingSet has not been > > touched as much as it should in this patch. In many cases, functions look for > > tilings by content scale and ignore translation, and I think this is not right > > in some cases. I've left some comments about this. > > Yes it is incorrect in some cases. While I converted all content_scale to an opaque class, I vetted through all instances when we extracted scale from the content transform, I think the only dangerous case is the GetPendingOrActiveTwinTiling you mentioned. I disagree and have left a number of comments. > > Opaqueness. I'm not sure that opaqueness is supported properly here. I think > > you'd need to add extra border texels when rastering to do this. You'd need to > > pass the contents translation to RasterSource::PlaybackToCanvas in order for it > > to do that work, in my opinion. You could also say that such layers are not > > opaque or use EnclosedRect or something to limit the opaqueness as an alternate > > approach. > > Filling extra border texels with background color was done in the prerequisite CL: https://codereview.chromium.org/2075873002/ > See RasterSource::PrepareForPlaybackToCanvas, which got a nice clean up. :) Hmm, nobody cc'd me on that patch. :C Maybe it got cleaned up, but you didn't update the comments to reflect the fact that you could be clearing multiple rows and columns of pixels. Is it just as fast to do that if you have a full "outline box" to draw pixels vs clearing? > > Comments. It took me a while to digest the math in this patch. Having more > > high and low level comments about what contents translation is doing and how > > it's implemented and what particular transformations are doing would be > > appreciated. > > Some comments are added. As for a high level overview, I think the ideal place is a README.md but we don't have that in cc. What is the conventional place for such comments in cc? I don't think this is so complicated that it needs some larger doc about it. Comments in PictureLayerTiling seem like a good fit. > https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/2175553002/diff/60001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:877: tilings_->FindTilingWithScale(raster_contents_scale_); > On 2016/08/22 22:57:39, enne wrote: > > What if the layer moves and the contents translation for the high res version > > becomes not ideal? Shouldn't this code not consider that high res and make > > another? > > It was what was done in patchset 1, but later I changed to make content translation recalc lazier. The policy implemented in this patchset is to never change content translation of existing tile sets. I think the content transform on a tiling should be immutable, but that it's ok in some cases to use a non-ideal transform (translation or scale). > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:60: DCHECK(content_bounds_rect.origin() == gfx::Point()); > On 2016/08/22 22:57:39, enne wrote: > > Can you make this DCHECK_EQ or maybe make a separate helper since you do this > > somewhere else as well? > > I think we need to fix this in base/logging.h. logging::MakeCheckOpString needs operator<< to work, while the chromium convention is to use ToString. :/ ToString is fine? > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:468: tile_bounds_in_dest_space.Offset(-tiling_->contents_translation()); > On 2016/08/22 22:57:39, enne wrote: > > I'm not 100% convinced that this satisfies the assumptions being made by > > coverage iterator, in particular that it will always fill the destination space > > perfectly. Can you add some PictureLayerTilingSetTestWithResources unittests > > that include translation offsets as well at different scales? > > It will because the pre-rounding tile_bounds_in_dest_space already fill the dest space. With ToEnclosingRect, some pixels in the dest space will be covered by more than one tiles. > > In fact, some rounding math here is already questionable prior to my CL. I don't think we can simply take the enclosing rect for geometry rect. I will follow up with more numerical analysis. What is questionable about it? I'll be interested to hear your analysis. > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > File cc/tiles/picture_layer_tiling_set.cc (right): > > https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling_set.cc:86: PictureLayerTiling* this_tiling = FindTilingWithScale(contents_scale); > On 2016/08/22 22:57:39, enne wrote: > > What if there are multiple tilings with the same scale but different > > translations? > > Currently the easiest thing to do is to replace. It shouldn't be terribly difficult to extend the tiling set to accommodate both. I think we can do it in a separate CL. I don't think so. We can talk about how to stage these patches, but I think managing tilings with transformations instead of just scales inside of tiling set is a prerequisite rather than a follow up. https://codereview.chromium.org/2175553002/diff/80001/cc/base/scale_translate... File cc/base/scale_translate2d.h (right): https://codereview.chromium.org/2175553002/diff/80001/cc/base/scale_translate... cc/base/scale_translate2d.h:15: class ScaleTranslate2d { There are a lot of functions here. Which of these are used? https://codereview.chromium.org/2175553002/diff/80001/cc/base/scale_translate... cc/base/scale_translate2d.h:85: gfx::RectF Transform(const gfx::RectF& r) const { Would it make sense to make these functions match gfx::Transform and call them TransformRect, TransformPoint, TransformRectReverse? Then this class would just be an optimization of gfx::Transform in a sense. https://codereview.chromium.org/2175553002/diff/80001/cc/base/scale_translate... cc/base/scale_translate2d.h:98: float pre_scale_; bikeshed: I'd just call this "scale" with comments. I think in general in cc when there's a scale and a translation on something, the scale is always a pre scale. unless otherwise specified. https://codereview.chromium.org/2175553002/diff/80001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2175553002/diff/80001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:656: tilings_->FindTilingWithScale(tile->contents_transform().pre_scale()); This is also wrong and needs to take into account the full transform of the tile. https://codereview.chromium.org/2175553002/diff/80001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:730: twin_layer->tilings_->FindTilingWithScale( I think this you need to be more explicit here in most of the cases where FindTilingWithScale is used. Yes, you check the translation, but you should just pass that into FindTilingWithTransform() function. https://codereview.chromium.org/2175553002/diff/80001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:903: tilings_->FindTilingWithScale(raster_contents_scale_); Here is another case where I think FindTilingWithScale is wrong. I think this patch needs to be much more deliberate about when it is creating a new tiling that should snap and when it is reusing a tiling with any translation that can be blurry. https://codereview.chromium.org/2175553002/diff/80001/cc/playback/raster_sour... File cc/playback/raster_source.h (right): https://codereview.chromium.org/2175553002/diff/80001/cc/playback/raster_sour... cc/playback/raster_source.h:54: // canvas_playback_rect defines the damange rect in the content space. It's true that the canvas_playback_rect is the damage rect, but for the purposes of this function I think it's better to define it as the subset of the recording that should be rastered. https://codereview.chromium.org/2175553002/diff/80001/cc/tiles/tile.cc File cc/tiles/tile.cc (right): https://codereview.chromium.org/2175553002/diff/80001/cc/tiles/tile.cc#newcode48 cc/tiles/tile.cc:48: value->SetDouble("contents_transform_pre_scale", bikeshed: I'd leave this as contents_scale and contents_translation
On 2016/08/29 20:21:19, enne wrote: https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_... > > cc/tiles/picture_layer_tiling.cc:468: > tile_bounds_in_dest_space.Offset(-tiling_->contents_translation()); > > On 2016/08/22 22:57:39, enne wrote: > > > I'm not 100% convinced that this satisfies the assumptions being made by > > > coverage iterator, in particular that it will always fill the destination > space > > > perfectly. Can you add some PictureLayerTilingSetTestWithResources > unittests > > > that include translation offsets as well at different scales? > > > > It will because the pre-rounding tile_bounds_in_dest_space already fill the > dest space. With ToEnclosingRect, some pixels in the dest space will be covered > by more than one tiles. > > > > In fact, some rounding math here is already questionable prior to my CL. I > don't think we can simply take the enclosing rect for geometry rect. I will > follow up with more numerical analysis. > > What is questionable about it? I'll be interested to hear your analysis. Assuming no MSAA, the sample point of each pixel will be the center of that pixel. For a tile of 256x256, the top-left sample point has coordinate of (0.5, 0.5). Likewise, (255.5, 255.5) for the bottom-right. In other words, the texture sampling extent for a tile is only tile_size.Inset(0.5). Sample points outside of that range will be wrapped depending on the GL wrap mode. This is undesired for tiling except for the layer edges. Now look at our geometry rect: gfx::Rect content_rect = tiling_->tiling_data_.TileBounds(tile_i_, tile_j_); current_geometry_rect_ = gfx::ScaleToEnclosingRect(content_rect, 1 / dest_to_content_scale_); Is it possible for the geometry rect to cover a point that will result in an invalid sample point? The answer is yes, consider the configuration: layer_size = 1000x1000 layer_transform = = draw_transform = translate2d(0.6px, 0.6px) ideal_scale = 1 dest_scale = 1 + ε Consider an ideal tiling of contents_scale = 1 The content_rect of the top-left tile would be (l=0, t=0, w=255, h=255) The rect scaled to the dest space became (l=0, t=0, w=255+ε, h=255+ε) Taking the enclosing rect, current_geometry_rect = (l=0, t=0, w=256, h=256) Now as we draw the quad, the quad will cover (l=0.6, t=0.6, w=256-ε, h=256-ε), which include the screen pixel at (256.5, 256.5). Mapping it back to the texture space the sampling point would be (255.9, 255.9), that exceeded the valid texture range. We don't see this drawing artifacts often because the condition is very rare as most of the time the ideal tiling is the tiling with maximum scale. I think it can only happen during a pinch zoom out, and only on tiles that are unfortunately rounded outward by more than 0.5px. Anyway, bad math is bad math and we can certainly do it more correctly. The right way to calculate geometry rect is to take the inset of the tile size: gfx::RectF tile_sample_extent = tiling_data.TileBoundsWithBorder().Inset(0.5); gfx::RectF unrounded_geometry_rect = gfx::ScaleRect(tile_sample_extent, content_to_dest_scale); These are exact rect that contains neither false positive or false negative. And I would recommend to do coverage in float region instead of integer region. I don't think float region would be much slower than integer region as there is no arithmetic involved, only comparison. FP comparison should be as fast as integers as long as no special numbers are involved. Doing FP region would allow us to use a tighter overlap between tiles (1 instead of 2px), that saves about 0.7% of texture memory. If we have to stay with integer region, then we'll need to round inwards instead of outward: current_geometry_rect_ = gfx::ToEnclosedRect(unrounded_geometry_rect); Now the question becomes whether the rounded geometry rect will cover the dest space. The answer is yes with the following proof: 1. content_to_dest_scale >= 1 // because dest scale is max scale 2. right_edge_of_tile = left_edge_of_next_tile + 2 // 2px overlap between tiles 3. right_edge_of_sample_extent = right_edge_of_tile - 0.5 4. left_edge_of_next_sample_extent = left_edge_of_next_tile + 0.5 5. right_edge_of_geometry_rect = floor(right_edge_of_sample_extent * content_to_dest_scale) 6. left_edge_of_next_geometry_rect = ceil(left_edge_of_next_sample_extent * content_to_dest_scale) 7. right_edge_of_geometry_rect = floor(right_edge_of_sample_extent * content_to_dest_scale) = floor((left_edge_of_next_sample_extent + 1) * content_to_dest_scale) // plug 2,3,4 = floor(left_edge_of_next_sample_extent * content_to_dest_scale + content_to_dest_scale) >= floor(left_edge_of_next_sample_extent * content_to_dest_scale + 1) // because 1 >= ceil(left_edge_of_next_sample_extent * content_to_dest_scale) = left_edge_of_next_geometry_rect Q.E.D.
On 2016/08/31 at 00:24:05, trchen wrote: > In other words, the texture sampling extent for a tile is only tile_size.Inset(0.5). Sample points outside of that range will be wrapped depending on the GL wrap mode. This is undesired for tiling except for the layer edges. Ah, here's the disagreement. Tiles are clamped, so this is fine on external edges. Internal tile edges have an extra border texel that is not included in its content rect.
On 2016/08/31 00:27:45, enne wrote: > On 2016/08/31 at 00:24:05, trchen wrote: > > > In other words, the texture sampling extent for a tile is only > tile_size.Inset(0.5). Sample points outside of that range will be wrapped > depending on the GL wrap mode. This is undesired for tiling except for the layer > edges. > > Ah, here's the disagreement. > > Tiles are clamped, so this is fine on external edges. Internal tile edges have > an extra border texel that is not included in its content rect. Yes, I'm claiming that even with the extra border texel, the rounding still result in invalid sampling.
On 2016/08/31 00:44:39, trchen wrote: > On 2016/08/31 00:27:45, enne wrote: > > On 2016/08/31 at 00:24:05, trchen wrote: > > > > > In other words, the texture sampling extent for a tile is only > > tile_size.Inset(0.5). Sample points outside of that range will be wrapped > > depending on the GL wrap mode. This is undesired for tiling except for the > layer > > edges. > > > > Ah, here's the disagreement. > > > > Tiles are clamped, so this is fine on external edges. Internal tile edges > have > > an extra border texel that is not included in its content rect. > > Yes, I'm claiming that even with the extra border texel, the rounding still > result in invalid sampling. I think our current implementation isn't handling tile boundary quite right. I can reproduce the border artifact even without such convoluted case: http://jsbin.com/jidega/
On 2016/08/31 04:51:54, trchen wrote: > On 2016/08/31 00:44:39, trchen wrote: > > On 2016/08/31 00:27:45, enne wrote: > > > On 2016/08/31 at 00:24:05, trchen wrote: > > > > > > > In other words, the texture sampling extent for a tile is only > > > tile_size.Inset(0.5). Sample points outside of that range will be wrapped > > > depending on the GL wrap mode. This is undesired for tiling except for the > > layer > > > edges. > > > > > > Ah, here's the disagreement. > > > > > > Tiles are clamped, so this is fine on external edges. Internal tile edges > > have > > > an extra border texel that is not included in its content rect. > > > > Yes, I'm claiming that even with the extra border texel, the rounding still > > result in invalid sampling. > > I think our current implementation isn't handling tile boundary quite right. I > can reproduce the border artifact even without such convoluted case: > http://jsbin.com/jidega/ The above example is due to a separate issue about quad AA. I made another example to repro the bug I describe above: http://jsbin.com/dezalef/
On 2016/08/31 at 00:24:05, trchen wrote: > Now as we draw the quad, the quad will cover (l=0.6, t=0.6, w=256-ε, h=256-ε), which include the screen pixel at (256.5, 256.5). Mapping it back to the texture space the sampling point would be (255.9, 255.9), that exceeded the valid texture range. Oho! I get it now. Thanks for the explanation. :) > The right way to calculate geometry rect is to take the inset of the tile size: > > gfx::RectF tile_sample_extent = tiling_data.TileBoundsWithBorder().Inset(0.5); > gfx::RectF unrounded_geometry_rect = gfx::ScaleRect(tile_sample_extent, content_to_dest_scale); > > These are exact rect that contains neither false positive or false negative. > > And I would recommend to do coverage in float region instead of integer region. I don't think float region would be much slower than integer region as there is no arithmetic involved, only comparison. FP comparison should be as fast as integers as long as no special numbers are involved. Doing FP region would allow us to use a tighter overlap between tiles (1 instead of 2px), that saves about 0.7% of texture memory. I would really prefer to use integers, at least in terms of the geometry rectangle. We already have enough floating point issues in GLRenderer.
On 2016/09/02 00:40:59, enne wrote: > On 2016/08/31 at 00:24:05, trchen wrote: > > The right way to calculate geometry rect is to take the inset of the tile > size: > > > > gfx::RectF tile_sample_extent = tiling_data.TileBoundsWithBorder().Inset(0.5); > > gfx::RectF unrounded_geometry_rect = gfx::ScaleRect(tile_sample_extent, > content_to_dest_scale); > > > > These are exact rect that contains neither false positive or false negative. > > > > And I would recommend to do coverage in float region instead of integer > region. I don't think float region would be much slower than integer region as > there is no arithmetic involved, only comparison. FP comparison should be as > fast as integers as long as no special numbers are involved. Doing FP region > would allow us to use a tighter overlap between tiles (1 instead of 2px), that > saves about 0.7% of texture memory. > > I would really prefer to use integers, at least in terms of the geometry > rectangle. We already have enough floating point issues in GLRenderer. IMO floating points are not much more difficult to work with than fixed points. Many hard questions need to be considered either way, for example "should I round up or round down here?" "will it overflow?" "will it blew up the error range?". Thought I do agree that floating points are scary because the language and the hardware implicitly do rounding for you. Most of the time rounding is desired, but it only takes one incorrect rounding to ruin the result... Anyway I agree with you that floating point is not an ideal tool for the problem here, but for a different reason. The advantages of FP is the dynamic range and consistent "relative precision" for every range. But in texture/tiling problem space we are dealing with a space with uniform density. There is no reason why top-left tiles need higher arithmetic precision than bottom-right tiles. IMO fixed point number is the right tool here. I think somewhere between 24.8 ~ 22.10 would be the right format.
Had a discussion with chrishtr@ & danakj@ today. Taking note here in case I forget. The question is whether opaque fractionally translated layer should be treated as non-opaque. The pros & cons are: Treat as opaque: Pros: Simple. No complication with occlusion tracking. Cons: Will need to overfill edge pixels with opaque background color. Background color will bleed in a different way than non-composited code path. Treat as non-opaque: Pros: Will match non-composited rendering. Cons: Performance will hurt for various reason. 1. GL_BLEND needs to be enabled, which is costly. (But no more costly than before this CL because layer edge AA enables GL_BLEND too.) 2. Occlusion tracking can't be done. Also discussed whether we can apply an inset to occlusion rect just to exclude the non-opaque edge pixels. This may not be done because we don't yet know tiling decision at the time of occlusion calculation. Using enclosedIntRect in the target space is only correct if the draw transform is static (or only mutated by integral translation) so texture always aligns to target 1:1. Pinch zoom can break this. (Or only skip occlusion during pinch zoom? Can we inspect scale delta during occlusion calculation?)
On 2016/12/07 at 02:17:24, trchen wrote: > Had a discussion with chrishtr@ & danakj@ today. Taking note here in case I forget. > The question is whether opaque fractionally translated layer should be treated as non-opaque. The pros & cons are: > > Treat as opaque: > Pros: Simple. No complication with occlusion tracking. > Cons: Will need to overfill edge pixels with opaque background color. Background color will bleed in a different way than non-composited code path. > > Treat as non-opaque: > Pros: Will match non-composited rendering. > Cons: Performance will hurt for various reason. > 1. GL_BLEND needs to be enabled, which is costly. (But no more costly than before this CL because layer edge AA enables GL_BLEND too.) Good. Then no penalty for this either. > 2. Occlusion tracking can't be done. Also discussed whether we can apply an inset to occlusion rect just to exclude the non-opaque edge pixels. This may not be done because we don't yet know tiling decision at the time of occlusion calculation. Using enclosedIntRect in the target space is only correct if the draw transform is static (or only mutated by integral translation) so texture always aligns to target 1:1. Pinch zoom can break this. (Or only skip occlusion during pinch zoom? Can we inspect scale delta during occlusion calculation?) Since occlusion tracking is not turned on in the render process, there is no performance penalty to marking it as non-opaque.
Is this patch ready for further review? Unfortunately Enne is out the rest of the month. I think Dana will have to review in their place.
On 2016/12/07 02:20:33, chrishtr wrote: > On 2016/12/07 at 02:17:24, trchen wrote: > > 2. Occlusion tracking can't be done. Also discussed whether we can apply an > inset to occlusion rect just to exclude the non-opaque edge pixels. This may not > be done because we don't yet know tiling decision at the time of occlusion > calculation. Using enclosedIntRect in the target space is only correct if the > draw transform is static (or only mutated by integral translation) so texture > always aligns to target 1:1. Pinch zoom can break this. (Or only skip occlusion > during pinch zoom? Can we inspect scale delta during occlusion calculation?) > > Since occlusion tracking is not turned on in the render process, there is no > performance penalty to marking it as non-opaque. We inspected the code after. Occlusion tracking is not used for tile prioritization, but still used for quad culling. So no raster time penalty but still some draw time penalty.
On 2016/12/07 at 02:23:14, trchen wrote: > On 2016/12/07 02:20:33, chrishtr wrote: > > On 2016/12/07 at 02:17:24, trchen wrote: > > > 2. Occlusion tracking can't be done. Also discussed whether we can apply an > > inset to occlusion rect just to exclude the non-opaque edge pixels. This may not > > be done because we don't yet know tiling decision at the time of occlusion > > calculation. Using enclosedIntRect in the target space is only correct if the > > draw transform is static (or only mutated by integral translation) so texture > > always aligns to target 1:1. Pinch zoom can break this. (Or only skip occlusion > > during pinch zoom? Can we inspect scale delta during occlusion calculation?) > > > > Since occlusion tracking is not turned on in the render process, there is no > > performance penalty to marking it as non-opaque. > > We inspected the code after. Occlusion tracking is not used for tile prioritization, but still used for quad culling. So no raster time penalty but still some draw time penalty. I see. I still think it should be marked as non-opaque and therefore removed from the occlusion tracker, along with adding an UMA for the latter.
New CL uploaded. It will be a pain to review as a whole. Split out uploaded for your convenience: https://codereview.chromium.org/2559413002/ https://codereview.chromium.org/2560723006/ https://codereview.chromium.org/2563743004/ https://codereview.chromium.org/2566613002/ https://codereview.chromium.org/2565643002/ https://codereview.chromium.org/2555363004/
Are these ready for review? On Thu, Dec 8, 2016 at 7:02 PM, <trchen@chromium.org> wrote: > New CL uploaded. It will be a pain to review as a whole. Split out > uploaded for > your convenience: > https://codereview.chromium.org/2559413002/ > https://codereview.chromium.org/2560723006/ > https://codereview.chromium.org/2563743004/ > https://codereview.chromium.org/2566613002/ > https://codereview.chromium.org/2565643002/ > https://codereview.chromium.org/2555363004/ > > https://codereview.chromium.org/2175553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Dec 6, 2016 at 6:20 PM, <chrishtr@chromium.org> wrote: > Is this patch ready for further review? Unfortunately Enne is out the rest > of > the month. > I think Dana will have to review in their place. > You probably need to wait for enne, sorry. There's a lot of stuff going on in this CL and they've spent many days understanding it all already. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I reviewed all the smaller patches. Seems mostly fine, but needs a bunch more testing. If it's possible to land these in smaller patches, please feel free to do so. It wasn't clear whether you were thinking about landing those patches individually or if they were just split out for review purposes.
Description was changed from ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/01/03 22:55:01, enne wrote: > I reviewed all the smaller patches. Seems mostly fine, but needs a bunch more > testing. > > If it's possible to land these in smaller patches, please feel free to do so. > It wasn't clear whether you were thinking about landing those patches > individually or if they were just split out for review purposes. Sorry for taking so long! I uploaded another update just now. https://codereview.chromium.org/2559413002/ https://codereview.chromium.org/2560723006/ https://codereview.chromium.org/2563743004/ https://codereview.chromium.org/2566613002/ https://codereview.chromium.org/2565643002/ The 6th CL is gone because it was git cl format. I just meld it into the 5 CLs. I think it is possible to land in smaller patches. In each of the step all tests passed and should be no-op until the last step. I'll write CL description for each of them now.
Description was changed from ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Raster PictureLayerTiling with fractional translation Normally layers are rastered in scaled local coordinate, this is not ideal because the drawing will be blurred by resampling when the layer origin is not aligned with pixel boundaries either due to layer position or transform. This CL detect the case when a layer is static and has 1:1 pixel ratio to the screen, shift the raster space to align with screen pixels. BUG=521364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== |
