Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2295343005:
Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely (Closed)
DescriptionImprove PictureLayerTiling::CoverageIterator to handle rounding more precisely
There are some rare corner cases that PictureLayerTiling::CoverageIterator
could generate quads that slightly exceed valid sample extent due to rounding
error.
Due to bilinear filtering, the valid sample extent of a texture without
introducing clamping error is the rect enclosed by texel sample points.
For example, a texture of 256 x 256 without MSAA has top-left sample point at
(0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the
sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255).
Previously the geometry rect of a tile is computed by:
geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest)
The tile_bounds is enclosed by the pixel border of the tile minus the extra
border texels, which a conservative estimate of the sample extent (reserves
a margin of 0.5 texels). However rounding out the rect in dest space can
introduce rounding error up to 1.0 texels, which blew up the error budget.
This CL changed the formula to:
geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest)
Where the sample extent is an exact rect with no error margin. Then we round
inward instead of outward for a conservative rect after scaling to dest space.
Numerical analysis for coverage proof can be found at https://crbug.com/643464
BUG=643464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6
Cr-Commit-Position: refs/heads/master@{#420170}
Patch Set 1 #
Total comments: 10
Patch Set 2 : simplify & add tests #
Total comments: 26
Patch Set 3 : address comments #Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : remove the 1.5 #
Messages
Total messages: 35 (16 generated)
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. This CL changes the way how tile geometry rects are rounded in dest space. The rect will be more conservative but should still perfect cover dest space. BUG=643464 ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. This CL changes the way how tile geometry rects are rounded in dest space. The rect will be more conservative but should still perfect cover dest space. BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. This CL changes the way how tile geometry rects are rounded in dest space. The rect will be more conservative but should still perfect cover dest space. BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. This CL changes the way how tile geometry rects are rounded in dest space. The rect will be more conservative but should still perfect cover dest space. BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
trchen@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
PTAL. I'm not very familiar with unit test techniques in cc. Which test would you recommend me to template from?
On 2016/09/02 at 01:26:00, trchen wrote: > PTAL. I'm not very familiar with unit test techniques in cc. Which test would you recommend me to template from? Probably PictureLayerTilingSetTest, although it's clear that it didn't catch this bug so needs to be modified as well. > This CL changes the way how tile geometry rects are rounded in dest space. Can you explain in better detail what the problem was and how your patch fixes it, more than just "this CL changes things"? https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:408: SkRect content_rect; What's with SkRect here? https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:411: // Shrink rect by 1/512 to avoid generating empty tiles due to FP error. Sorry, but I'm not sure I follow this. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:420: left_ = tiling_->tiling_data_.LastBorderTileXIndexFromSrcCoord( Can you leave some comments about where the 0.5 is coming from here? https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:464: // Important: To ensure no seams between tiles due to FP error, intermediate I would rather do the computations using gfx::Rect where you can (and explicit computation where you can't) rather than depending on SkRect implementation details. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:479: int r = (tile_i_ == data.num_tiles_x() - 1) ? data.tiling_size().width() Why do you need this conditional here?
No new CL uploaded yet, just replying comments. Will ping again as soon as I have a new CL. On 2016/09/02 19:04:54, enne wrote: > On 2016/09/02 at 01:26:00, trchen wrote: > > PTAL. I'm not very familiar with unit test techniques in cc. Which test would > you recommend me to template from? > > Probably PictureLayerTilingSetTest, although it's clear that it didn't catch > this bug so needs to be modified as well. Okay, I'll study it. > > This CL changes the way how tile geometry rects are rounded in dest space. > > Can you explain in better detail what the problem was and how your patch fixes > it, more than just "this CL changes things"? Acknowledged. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:408: SkRect content_rect; On 2016/09/02 19:04:54, enne wrote: > What's with SkRect here? LTRB rect is also preferred here. I'll covert it to explicit computation as well. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:411: // Shrink rect by 1/512 to avoid generating empty tiles due to FP error. On 2016/09/02 19:04:54, enne wrote: > Sorry, but I'm not sure I follow this. It is for avoid hitting DCHECK on line #487. Although we didn't take enclosing rect on line #410, FP error can still expand content_rect by 0.5 ulp. Also on line #470, the texel_extent can be shrunk by 0.5 ulp. This can cause a situation that at line #420~#429 we thought a tile barely intersected with dest rect then later at line #487 found otherwise. The constant 1/512px is picked by the fact that even if we missed a pixel for shrinking, the pixel value is guaranteed to be transparent after bilinear filtering. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:420: left_ = tiling_->tiling_data_.LastBorderTileXIndexFromSrcCoord( On 2016/09/02 19:04:54, enne wrote: > Can you leave some comments about where the 0.5 is coming from here? Acknowledged. It is difference between sample point location and its integer index. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:464: // Important: To ensure no seams between tiles due to FP error, intermediate On 2016/09/02 19:04:54, enne wrote: > I would rather do the computations using gfx::Rect where you can (and explicit > computation where you can't) rather than depending on SkRect implementation > details. Acknowledged. https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... cc/tiles/picture_layer_tiling.cc:479: int r = (tile_i_ == data.num_tiles_x() - 1) ? data.tiling_size().width() On 2016/09/02 19:04:54, enne wrote: > Why do you need this conditional here? Extend the right edge to layer bound like the left/top edge did. It is due to the fact that the texel extent only covers up to tiling_size().width() - 0.5, but we still want the quad to cover the whole layer rect, as that's what the PictureLayerTilingSet and quad system expects. BTW I'm thinking an alternative layer AA scheme using GL_CLAMP_TO_BORDER with transparent border. We can discuss about that next time we meet.
On 2016/09/02 at 23:06:54, trchen wrote: > No new CL uploaded yet, just replying comments. Will ping again as soon as I have a new CL. > > On 2016/09/02 19:04:54, enne wrote: > > On 2016/09/02 at 01:26:00, trchen wrote: > > > PTAL. I'm not very familiar with unit test techniques in cc. Which test would > > you recommend me to template from? > > > > Probably PictureLayerTilingSetTest, although it's clear that it didn't catch > > this bug so needs to be modified as well. > > Okay, I'll study it. > > > > This CL changes the way how tile geometry rects are rounded in dest space. > > > > Can you explain in better detail what the problem was and how your patch fixes > > it, more than just "this CL changes things"? > > Acknowledged. A code review style point: What does that mean exactly? "Acknowledged" should be used if you intend not to follow the suggestion but want to note that you heard it. In this case you will update the description to be more clear, right? Ditto with the "Acknowledged" further down. > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > File cc/tiles/picture_layer_tiling.cc (right): > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:408: SkRect content_rect; > On 2016/09/02 19:04:54, enne wrote: > > What's with SkRect here? > > LTRB rect is also preferred here. I'll covert it to explicit computation as well. > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:411: // Shrink rect by 1/512 to avoid generating empty tiles due to FP error. > On 2016/09/02 19:04:54, enne wrote: > > Sorry, but I'm not sure I follow this. > > It is for avoid hitting DCHECK on line #487. > > Although we didn't take enclosing rect on line #410, FP error can still expand content_rect by 0.5 ulp. Also on line #470, the texel_extent can be shrunk by 0.5 ulp. This can cause a situation that at line #420~#429 we thought a tile barely intersected with dest rect then later at line #487 found otherwise. > > The constant 1/512px is picked by the fact that even if we missed a pixel for shrinking, the pixel value is guaranteed to be transparent after bilinear filtering. > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:420: left_ = tiling_->tiling_data_.LastBorderTileXIndexFromSrcCoord( > On 2016/09/02 19:04:54, enne wrote: > > Can you leave some comments about where the 0.5 is coming from here? > > Acknowledged. > > It is difference between sample point location and its integer index. > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:464: // Important: To ensure no seams between tiles due to FP error, intermediate > On 2016/09/02 19:04:54, enne wrote: > > I would rather do the computations using gfx::Rect where you can (and explicit > > computation where you can't) rather than depending on SkRect implementation > > details. > > Acknowledged. > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:479: int r = (tile_i_ == data.num_tiles_x() - 1) ? data.tiling_size().width() > On 2016/09/02 19:04:54, enne wrote: > > Why do you need this conditional here? > > Extend the right edge to layer bound like the left/top edge did. > > It is due to the fact that the texel extent only covers up to tiling_size().width() - 0.5, but we still want the quad to cover the whole layer rect, as that's what the PictureLayerTilingSet and quad system expects. > > BTW I'm thinking an alternative layer AA scheme using GL_CLAMP_TO_BORDER with transparent border. We can discuss about that next time we meet.
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. This CL changes the way how tile geometry rects are rounded in dest space. The rect will be more conservative but should still perfect cover dest space. BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border pixels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border pixels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border texels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
On 2016/09/02 23:17:43, chrishtr wrote: > A code review style point: > > What does that mean exactly? "Acknowledged" should be used if you intend not to > follow the suggestion but want to note that you heard it. In this > case you will update the description to be more clear, right? > > Ditto with the "Acknowledged" further down. I thought it means a positive confirmation that "I'm going to do it, and will let you know when I'm done.". Did I misuse it all along? (panic)
On 2016/09/02 at 23:30:10, trchen wrote: > On 2016/09/02 23:17:43, chrishtr wrote: > > A code review style point: > > > > What does that mean exactly? "Acknowledged" should be used if you intend not to > > follow the suggestion but want to note that you heard it. In this > > case you will update the description to be more clear, right? > > > > Ditto with the "Acknowledged" further down. > > I thought it means a positive confirmation that "I'm going to do it, and will > let you know when I'm done.". Did I misuse it all along? (panic) The usual meaning in English would be the other one. Don't worry, no need to panic. :) Instead it's good to say "Done" if it's already done, and "Will do shortly" or similar if it's going to happen shortly. It's best to avoid the "Will do" version though because it requires the reviewer to look at your patch potentially more than once, which uses up time and task switching..better to just update the thing and then send your comments, if it won't take too long to do.
On 2016/09/02 at 23:06:54, trchen wrote: > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:411: // Shrink rect by 1/512 to avoid generating empty tiles due to FP error. > On 2016/09/02 19:04:54, enne wrote: > > Sorry, but I'm not sure I follow this. > > It is for avoid hitting DCHECK on line #487. > > Although we didn't take enclosing rect on line #410, FP error can still expand content_rect by 0.5 ulp. Also on line #470, the texel_extent can be shrunk by 0.5 ulp. This can cause a situation that at line #420~#429 we thought a tile barely intersected with dest rect then later at line #487 found otherwise. Why were there not floating point errors before? It seems like the same scaling is occurring there? I think I would prefer some more straightforward logic. The DCHECKs on 487-489 are for making sure that a tile is always directly adjacent to the tile to its left and has exactly the same top and bottom as that left neighbor. If going from floating point to integer space causes weirdness, what about just going from integer space to floating point / texture space instead? That seems a little safer. > The constant 1/512px is picked by the fact that even if we missed a pixel for shrinking, the pixel value is guaranteed to be transparent after bilinear filtering. I don't understand this guarantee. > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > cc/tiles/picture_layer_tiling.cc:479: int r = (tile_i_ == data.num_tiles_x() - 1) ? data.tiling_size().width() > On 2016/09/02 19:04:54, enne wrote: > > Why do you need this conditional here? > > Extend the right edge to layer bound like the left/top edge did. > > It is due to the fact that the texel extent only covers up to tiling_size().width() - 0.5, but we still want the quad to cover the whole layer rect, as that's what the PictureLayerTilingSet and quad system expects. Maybe I'm confused. The quad is not supposed to cover the whole layer rect, it just needs to cover the dest rect. I could understand extending by 0.5, but am confused about going all the way to the edge.
Patchset 2 uploaded! This version switched to a different numerical tolerance strategy, which simplified logic a lot. Also added unit tests. On 2016/09/06 18:28:21, enne wrote: > On 2016/09/02 at 23:06:54, trchen wrote: > > > > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > > cc/tiles/picture_layer_tiling.cc:411: // Shrink rect by 1/512 to avoid > generating empty tiles due to FP error. > > On 2016/09/02 19:04:54, enne wrote: > > > Sorry, but I'm not sure I follow this. > > > > It is for avoid hitting DCHECK on line #487. > > > > Although we didn't take enclosing rect on line #410, FP error can still expand > content_rect by 0.5 ulp. Also on line #470, the texel_extent can be shrunk by > 0.5 ulp. This can cause a situation that at line #420~#429 we thought a tile > barely intersected with dest rect then later at line #487 found otherwise. > > Why were there not floating point errors before? It seems like the same scaling > is occurring there? Prior to this CL, the geometry rect was rounded out. i.e. being very tolerant so it will always intersect with dest rect. The scaling is in opposite direction: content_rect at line #420~#429 was calculated by dest_rect * dest_to_content_scale. texel_extent at line #470 was calculated by texel_extent * content_to_dest_scale. Anyway I changed the tolerance strategy in patch set 2. Simply overhang each tile by 1/1024 px make things way easier (Also eliminate the need for LTRB rects.). > I think I would prefer some more straightforward logic. The DCHECKs on 487-489 > are for making sure that a tile is always directly adjacent to the tile to its > left and has exactly the same top and bottom as that left neighbor. If going > from floating point to integer space causes weirdness, what about just going > from integer space to floating point / texture space instead? That seems a > little safer. > > > The constant 1/512px is picked by the fact that even if we missed a pixel for > shrinking, the pixel value is guaranteed to be transparent after bilinear > filtering. > > I don't understand this guarantee. Yea, I don't know what was I saying. (insert shame cube meme here) Missing a near-miss pixel is bad. It has to be the other way around: Overhang a tile by 1/1024px (my previous calculation failed to consider both axis) won't introduce observable errors. > https://codereview.chromium.org/2295343005/diff/1/cc/tiles/picture_layer_tili... > > cc/tiles/picture_layer_tiling.cc:479: int r = (tile_i_ == data.num_tiles_x() - > 1) ? data.tiling_size().width() > > On 2016/09/02 19:04:54, enne wrote: > > > Why do you need this conditional here? > > > > Extend the right edge to layer bound like the left/top edge did. > > > > It is due to the fact that the texel extent only covers up to > tiling_size().width() - 0.5, but we still want the quad to cover the whole layer > rect, as that's what the PictureLayerTilingSet and quad system expects. > > Maybe I'm confused. The quad is not supposed to cover the whole layer rect, it > just needs to cover the dest rect. I could understand extending by 0.5, but am > confused about going all the way to the edge. For example, a layer of 256x256 can be fully covered by a tile of content scale 1.0. Consider a dest scale of 2.0, the tile will only cover (l=1, t=1, r=511, b=511) because the texel extent is (l=0.5, t=0.5, r=255.5, b=255.5). In patchset 2 I changed the strategy a little bit. External edges are considered infinite away (in practice we only need to expand it by 1.5px), and will be clamped to dest rect afterwards.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is looking much better. Thanks for all the comments and the tests. I have a bunch of small questions, but nothing really big. https://codereview.chromium.org/2295343005/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:838: float dest_scale = MaximumTilingContentsScale(); I assume both this and the picture_layer_impl_unittest.cc changes are due to this tripping DCHECKs when an incorrect destination scale is used? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:405: // Clamp dest_rect_ to the bounds of the layer. What code was passing in a larger dest rect than the bounds of the layer? This is kind of surprising that you couldn't just DCHECK this. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:417: // For example, texel (12, 34) has sample point at (12.5, 34.5). This isn't true when the layer is not aligned with its target pixels though, right? I think I don't quite understand how this bit works without knowing the target transform. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:461: // Calculate the current geometry rect. As we reserved 2px overlap between I thought we reserved an extra border texel for bilinear filtering reasons, not for margin of error? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:471: // avoid seams between tiles. The constant 1/1024 is picked by the fact This is really because scaling from texture coordinates back into geometry space might round incorrectly due to floating point, causing seams, right? I *really* appreciate your discussion of why you picked 1024, but I think I'd also like just a tiny bit more reasoning about why there might be seams in the first place. Is it possible that this would cause geometric overlap? I'm guessing the answer is no, because of the VerifyTilesExactlyCoverRect check? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:485: texel_extent.Inset(tile_i_ ? -epsilon : -1.5, Can you set the texel_extent to be 0 if it's the left/top edge and to be tile bounds if it's the right/bottom edge, rather than using this magical 1.5? Depending on the AA shader to fix up texture coordinates seems like something that will come back to bite later. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:141: // Skip check for external edges because they do overhang. As I said earlier, I'd prefer that external edges don't overhang, if that's possible? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:205: bool loose_texel_extent_check_; Just initialize this here instead of the ctor. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:993: TEST_F(PictureLayerTilingIteratorTest, QuadShouldNotUseLastHalfTexel) { This is the real test for issue 643464, right? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:1008: // This test verifies that when a dest pixel can be covered by more than Can you also DCHECK via TilingData that both tiles contain this? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:1017: // Similar to above test, but with an internal tile. Ditto here.
https://codereview.chromium.org/2295343005/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:838: float dest_scale = MaximumTilingContentsScale(); On 2016/09/09 21:46:33, enne wrote: > I assume both this and the picture_layer_impl_unittest.cc changes are due to > this tripping DCHECKs when an incorrect destination scale is used? Yes. And that DCHECK is important for correctness I think. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:405: // Clamp dest_rect_ to the bounds of the layer. On 2016/09/09 21:46:34, enne wrote: > What code was passing in a larger dest rect than the bounds of the layer? This > is kind of surprising that you couldn't just DCHECK this. I don't know if we ever do that in real code, but PictureLayerTilingIteratorTest::VerifyTilesCoverNonContainedRect does test that. Maybe we can make it a DCHECK and update those tests as a separate CL if you prefer? https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:417: // For example, texel (12, 34) has sample point at (12.5, 34.5). On 2016/09/09 21:46:34, enne wrote: > This isn't true when the layer is not aligned with its target pixels though, > right? I think I don't quite understand how this bit works without knowing the > target transform. It is referring to texels in the texture space. Each texel has a (or more with MSAA) sample point too. Because we don't know the target transform at this point, we have to be pessimistic, i.e. assume every point (a pair of real number, not necessary snapped to a pixel sample) inside of the content rect may be sampled. This code basically maps the boundary points into contents space, then find out the enclosing texture samples. For example, if we have dest_scale/content_scale = 1.23 dest rect = (l=123, t=234, r=345, b=345), Then the content_rect = (l=100.00, t=190.24, r=280.49, b=370.73) So we would need these sample points to guarantee coverage: wanted_texels(unshifted) = (l=99.5, t=189.5, r=280.5, b=371.5) Or wanted_texels(integer index) = (l=99, t=189, r=280, b=371), note that bottom&right edges are inclusive. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:461: // Calculate the current geometry rect. As we reserved 2px overlap between On 2016/09/09 21:46:34, enne wrote: > I thought we reserved an extra border texel for bilinear filtering reasons, not > for margin of error? Actually both... I don't know how to word this more concisely & clearly... Any suggestion? When I say reserved 2px overlap, I actually mean an overlap of 2 rows/cols of texel samples. Overlap of 2 sample points --> overlap of 1px sample extent Overlap of 1 sample points --> No overlap (tight) sample extent No overlap --> 1px of gap unsample-able between tiles And we need to have 1px overlap of sample extent for margin of error (due to rounding in dest space). https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:471: // avoid seams between tiles. The constant 1/1024 is picked by the fact On 2016/09/09 21:46:34, enne wrote: > This is really because scaling from texture coordinates back into geometry space > might round incorrectly due to floating point, causing seams, right? > > I *really* appreciate your discussion of why you picked 1024, but I think I'd > also like just a tiny bit more reasoning about why there might be seams in the > first place. It could happen due to FP error. right_edge_of_tile_ij = floor(right[i,j] * content_to_dest) left_edge_of_tile_i1j = ceil(left[i+1,j] * content_to_dest) In theory right[i,j] == left[i+1,j] + 1, thus provably there can be no gap between rounded edges. But because we use XYWH rects, the actual computation becomes: right_edge_of_tile_ij = floor(x[i,j] * content_to_dest + size[i,j] * content_to_dest) It could flip over a rounding boundary of both multiplication gets rounded down. Besides that, we could fail to cover the whole dest rect due to ulp errors between dest <---> content space conversion. e.g at line #428 we thought "tile 12 ought to be enough to cover x=345", then at here we realized "sorry, tile 12 only covered up to x=344.99999999". > Is it possible that this would cause geometric overlap? I'm guessing the answer > is no, because of the VerifyTilesExactlyCoverRect check? The overlap is eliminated below at line #504~510. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:485: texel_extent.Inset(tile_i_ ? -epsilon : -1.5, On 2016/09/09 21:46:34, enne wrote: > Can you set the texel_extent to be 0 if it's the left/top edge and to be tile > bounds if it's the right/bottom edge, rather than using this magical 1.5? > Depending on the AA shader to fix up texture coordinates seems like something > that will come back to bite later. Yes I've considered that. That's more correct. However the scaled layer bounds need to be computed. Plugging 1.5 here allows compiler vectorization. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:141: // Skip check for external edges because they do overhang. On 2016/09/09 21:46:34, enne wrote: > As I said earlier, I'd prefer that external edges don't overhang, if that's > possible? They still overhang because dest point (0, 0) maps to texture coordinate (0, 0), which results in clamping. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:205: bool loose_texel_extent_check_; On 2016/09/09 21:46:34, enne wrote: > Just initialize this here instead of the ctor. Done. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:993: TEST_F(PictureLayerTilingIteratorTest, QuadShouldNotUseLastHalfTexel) { On 2016/09/09 21:46:34, enne wrote: > This is the real test for issue 643464, right? Yep! https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:1008: // This test verifies that when a dest pixel can be covered by more than On 2016/09/09 21:46:34, enne wrote: > Can you also DCHECK via TilingData that both tiles contain this? Done. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling_unittest.cc:1017: // Similar to above test, but with an internal tile. On 2016/09/09 21:46:34, enne wrote: > Ditto here. Done.
Ping?
https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:405: // Clamp dest_rect_ to the bounds of the layer. On 2016/09/09 at 23:39:19, trchen wrote: > On 2016/09/09 21:46:34, enne wrote: > > What code was passing in a larger dest rect than the bounds of the layer? This > > is kind of surprising that you couldn't just DCHECK this. > > I don't know if we ever do that in real code, but PictureLayerTilingIteratorTest::VerifyTilesCoverNonContainedRect does test that. > > Maybe we can make it a DCHECK and update those tests as a separate CL if you prefer? This is fine. The code before was intersecting to the tiling size, so I guess there's nothing different here. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:417: // For example, texel (12, 34) has sample point at (12.5, 34.5). On 2016/09/09 at 23:39:19, trchen wrote: > On 2016/09/09 21:46:34, enne wrote: > > This isn't true when the layer is not aligned with its target pixels though, > > right? I think I don't quite understand how this bit works without knowing the > > target transform. > > It is referring to texels in the texture space. Each texel has a (or more with MSAA) sample point too. > > Because we don't know the target transform at this point, we have to be pessimistic, i.e. assume every point (a pair of real number, not necessary snapped to a pixel sample) inside of the content rect may be sampled. This code basically maps the boundary points into contents space, then find out the enclosing texture samples. Your description makes sense to me, but I don't feel like the comment reflects that extra bit of information and is ambiguous in its use of the phrase "sample point". I think you could just copy and paste your "Because we don't know..." paragraph with some additional example. I think the numeric example above is much more clear than the "// Instead comment" below. https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:461: // Calculate the current geometry rect. As we reserved 2px overlap between On 2016/09/09 at 23:39:19, trchen wrote: > On 2016/09/09 21:46:34, enne wrote: > > I thought we reserved an extra border texel for bilinear filtering reasons, not > > for margin of error? > > Actually both... I don't know how to word this more concisely & clearly... Any suggestion? > > When I say reserved 2px overlap, I actually mean an overlap of 2 rows/cols of texel samples. > > Overlap of 2 sample points --> overlap of 1px sample extent > Overlap of 1 sample points --> No overlap (tight) sample extent > No overlap --> 1px of gap unsample-able between tiles > > And we need to have 1px overlap of sample extent for margin of error (due to rounding in dest space). Maybe just say "due to border texels and floating point error, tiles might overlap" https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:485: texel_extent.Inset(tile_i_ ? -epsilon : -1.5, On 2016/09/09 at 23:39:19, trchen wrote: > On 2016/09/09 21:46:34, enne wrote: > > Can you set the texel_extent to be 0 if it's the left/top edge and to be tile > > bounds if it's the right/bottom edge, rather than using this magical 1.5? > > Depending on the AA shader to fix up texture coordinates seems like something > > that will come back to bite later. > > Yes I've considered that. That's more correct. However the scaled layer bounds need to be computed. Plugging 1.5 here allows compiler vectorization. I'd rather be simple first and optimize later, sorry. Can you set this to be zero if it's the left/top edge?
On 2016/09/15 18:06:23, enne wrote: > https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... > File cc/tiles/picture_layer_tiling.cc (right): > > https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:405: // Clamp dest_rect_ to the bounds of the > layer. > On 2016/09/09 at 23:39:19, trchen wrote: > > On 2016/09/09 21:46:34, enne wrote: > https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:417: // For example, texel (12, 34) has sample > point at (12.5, 34.5). > On 2016/09/09 at 23:39:19, trchen wrote: > > On 2016/09/09 21:46:34, enne wrote: > > > This isn't true when the layer is not aligned with its target pixels though, > > > right? I think I don't quite understand how this bit works without knowing > the > > > target transform. > > > > It is referring to texels in the texture space. Each texel has a (or more with > MSAA) sample point too. > > > > Because we don't know the target transform at this point, we have to be > pessimistic, i.e. assume every point (a pair of real number, not necessary > snapped to a pixel sample) inside of the content rect may be sampled. This code > basically maps the boundary points into contents space, then find out the > enclosing texture samples. > > Your description makes sense to me, but I don't feel like the comment reflects > that extra bit of information and is ambiguous in its use of the phrase "sample > point". > > I think you could just copy and paste your "Because we don't know..." paragraph > with some additional example. I think the numeric example above is much more > clear than the "// Instead comment" below. Done. > https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:461: // Calculate the current geometry rect. As > we reserved 2px overlap between > On 2016/09/09 at 23:39:19, trchen wrote: > > On 2016/09/09 21:46:34, enne wrote: > > > I thought we reserved an extra border texel for bilinear filtering reasons, > not > > > for margin of error? > > > > Actually both... I don't know how to word this more concisely & clearly... Any > suggestion? > > > > When I say reserved 2px overlap, I actually mean an overlap of 2 rows/cols of > texel samples. > > > > Overlap of 2 sample points --> overlap of 1px sample extent > > Overlap of 1 sample points --> No overlap (tight) sample extent > > No overlap --> 1px of gap unsample-able between tiles > > > > And we need to have 1px overlap of sample extent for margin of error (due to > rounding in dest space). > > Maybe just say "due to border texels and floating point error, tiles might > overlap" Done. With slightly different wording. > https://codereview.chromium.org/2295343005/diff/20001/cc/tiles/picture_layer_... > cc/tiles/picture_layer_tiling.cc:485: texel_extent.Inset(tile_i_ ? -epsilon : > -1.5, > On 2016/09/09 at 23:39:19, trchen wrote: > > On 2016/09/09 21:46:34, enne wrote: > > > Can you set the texel_extent to be 0 if it's the left/top edge and to be > tile > > > bounds if it's the right/bottom edge, rather than using this magical 1.5? > > > Depending on the AA shader to fix up texture coordinates seems like > something > > > that will come back to bite later. > > > > Yes I've considered that. That's more correct. However the scaled layer bounds > need to be computed. Plugging 1.5 here allows compiler vectorization. > > I'd rather be simple first and optimize later, sorry. Can you set this to be > zero if it's the left/top edge? Done.
https://codereview.chromium.org/2295343005/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:501: (tile_i_ != data.num_tiles_x() - 1) ? -epsilon : -1.5, I thought you were going to remove the 1.5?
CL uploaded. (Removed the 1.5 hack.) https://codereview.chromium.org/2295343005/diff/60001/cc/tiles/picture_layer_... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2295343005/diff/60001/cc/tiles/picture_layer_... cc/tiles/picture_layer_tiling.cc:501: (tile_i_ != data.num_tiles_x() - 1) ? -epsilon : -1.5, On 2016/09/16 17:50:48, enne wrote: > I thought you were going to remove the 1.5? I thought you only meant the top/left edges. :) Yes, it is possible to remove the 1.5 completely, we'll have to cache the layer bounds in dest space though.
lgtm
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border texels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border texels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border texels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely There are some rare corner cases that PictureLayerTiling::CoverageIterator could generate quads that slightly exceed valid sample extent due to rounding error. Due to bilinear filtering, the valid sample extent of a texture without introducing clamping error is the rect enclosed by texel sample points. For example, a texture of 256 x 256 without MSAA has top-left sample point at (0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255). Previously the geometry rect of a tile is computed by: geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest) The tile_bounds is enclosed by the pixel border of the tile minus the extra border texels, which a conservative estimate of the sample extent (reserves a margin of 0.5 texels). However rounding out the rect in dest space can introduce rounding error up to 1.0 texels, which blew up the error budget. This CL changed the formula to: geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest) Where the sample extent is an exact rect with no error margin. Then we round inward instead of outward for a conservative rect after scaling to dest space. Numerical analysis for coverage proof can be found at https://crbug.com/643464 BUG=643464 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6 Cr-Commit-Position: refs/heads/master@{#420170} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6 Cr-Commit-Position: refs/heads/master@{#420170} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
