Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12730010:
Fix rounding rules for skia operations to work with non-integer scaling factors. (Closed)
|
Created:
7 years, 9 months ago by kevers Modified:
7 years, 9 months ago CC:
chromium-reviews, rsesek+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix rounding rules for skia operations to work with non-integer scaling factors. Previously, sizes were truncated on scale up. This works for integer scale factors, but fails when the scaled value is non-integer. Instead, sizes should round up to align with image assets provided for scaled sizes. For tiled operations where we wish to extract a subimage of a given size, the origin of the subregion should be rounded down and the size rounded up. This aligns Rect based size calculations with the expected image size for a scaled resource.
BUG=179884
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188434
Patch Set 1 #
Total comments: 6
Patch Set 2 : Apply consistent rounding rules for skia ops. #Patch Set 3 : Revert changes to skbitmap. #Patch Set 4 : Cosmetic tweaks. #Patch Set 5 : Fix cropping. #Patch Set 6 : Update sizes. #
Total comments: 8
Patch Set 7 : Fix nits. #
Total comments: 1
Patch Set 8 : Expand on comment. #Patch Set 9 : Enforce consistent rounding. #Patch Set 10 : Fix CRLF line ending. #Patch Set 11 : Remvove extra blank line. #
Total comments: 8
Patch Set 12 : Revert change to ButtonImageSource since the image and mask can be different sizes. #Patch Set 13 : Move CropImageSkiaRep functionality back into ExtractSubsetImageSource. #Messages
Total messages: 24 (0 generated)
Hi Peter, Can you please take a look at this CL.
https://codereview.chromium.org/12730010/diff/1/ui/gfx/image/image_skia_opera... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/1/ui/gfx/image/image_skia_opera... ui/gfx/image/image_skia_operations.cc:226: gfx::Point point = gfx::ToFlooredPoint(gfx::ScalePoint( You should create a generic method which does this logic at at the top of this file. ResizeSource and ExtractSubsetImageSource should get the same treatment. https://codereview.chromium.org/12730010/diff/1/ui/gfx/image/image_skia_opera... ui/gfx/image/image_skia_operations.cc:274: class ButtonImageSource: public gfx::ImageSkiaSource { Nit: Switch this class to inherit from BinaryImageSource as well. https://codereview.chromium.org/12730010/diff/1/ui/gfx/skbitmap_operations.cc File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/12730010/diff/1/ui/gfx/skbitmap_operations.cc... ui/gfx/skbitmap_operations.cc:84: // Allow for a 1 pixel mismatch in height or width, which may result from I would rather that you did the clipping before calling CreateImageSkiaRep(first_rep, second_rep) in BinaryImageSource. You can use SkBitmap::extractSubset() to do the clipping.
https://chromiumcodereview.appspot.com/12730010/diff/1/ui/gfx/image/image_ski... File ui/gfx/image/image_skia_operations.cc (right): https://chromiumcodereview.appspot.com/12730010/diff/1/ui/gfx/image/image_ski... ui/gfx/image/image_skia_operations.cc:226: gfx::Point point = gfx::ToFlooredPoint(gfx::ScalePoint( On 2013/03/12 15:54:37, pkotwicz wrote: > You should create a generic method which does this logic at at the top of this > file. > ResizeSource and ExtractSubsetImageSource should get the same treatment. Done. https://chromiumcodereview.appspot.com/12730010/diff/1/ui/gfx/image/image_ski... ui/gfx/image/image_skia_operations.cc:274: class ButtonImageSource: public gfx::ImageSkiaSource { On 2013/03/12 15:54:37, pkotwicz wrote: > Nit: Switch this class to inherit from BinaryImageSource as well. Done. https://chromiumcodereview.appspot.com/12730010/diff/1/ui/gfx/skbitmap_operat... File ui/gfx/skbitmap_operations.cc (right): https://chromiumcodereview.appspot.com/12730010/diff/1/ui/gfx/skbitmap_operat... ui/gfx/skbitmap_operations.cc:84: // Allow for a 1 pixel mismatch in height or width, which may result from On 2013/03/12 15:54:37, pkotwicz wrote: > I would rather that you did the clipping before calling > CreateImageSkiaRep(first_rep, second_rep) in BinaryImageSource. > You can use SkBitmap::extractSubset() to do the clipping. Done.
LGTM with Nits However, please explain as to why you are using ToEnclosingRect(). https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:37: ImageSkiaRep CropBitmap(ImageSkiaRep& source, gfx::Rect& bounds) { Nit: Rename to CropImageSkiaRep Rename |bounds| to |bounds_in_pixel|. https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:77: // Allow for maximum of 1 pixel mismatch due to rounding errors. Nit: "due to rounding errors because of a fractional scale factor". You can also simplify this code by using ImageSkiaRep::pixel_width() and ImageSkiaRep::pixel_height(). https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:92: dh = first_size.height() - second_size.height(); Use ImageSkiaRep::pixel_width() and ImageSkiaRep::pixel_height() here too :) https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:252: dst_h_), scale); ToEnclosingRect() does something different than using Point::ToFlooredPoint() and Size::ToCeiledSize() that you were doing in patch set #1. Is this change intentional?
+sky for OWNERS https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:37: ImageSkiaRep CropBitmap(ImageSkiaRep& source, gfx::Rect& bounds) { On 2013/03/13 04:03:48, pkotwicz wrote: > Nit: Rename to CropImageSkiaRep > Rename |bounds| to |bounds_in_pixel|. Done. https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:77: // Allow for maximum of 1 pixel mismatch due to rounding errors. On 2013/03/13 04:03:48, pkotwicz wrote: > Nit: "due to rounding errors because of a fractional scale factor". > You can also simplify this code by using ImageSkiaRep::pixel_width() and > ImageSkiaRep::pixel_height(). Done. https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:92: dh = first_size.height() - second_size.height(); On 2013/03/13 04:03:48, pkotwicz wrote: > Use ImageSkiaRep::pixel_width() and ImageSkiaRep::pixel_height() here too :) Done. https://codereview.chromium.org/12730010/diff/7003/ui/gfx/image/image_skia_op... ui/gfx/image/image_skia_operations.cc:252: dst_h_), scale); On 2013/03/13 04:03:48, pkotwicz wrote: > ToEnclosingRect() does something different than using > Point::ToFlooredPoint() and Size::ToCeiledSize() that you were doing in patch > set #1. Is this change intentional? The size resulting from using ToEnclosingRect is a pixel larger in width and height than using ToFlooredPoint + ToCeiledSize if both the top-left and bottom-right corners are not integer aligned after floating point scaling. This change is intentional.
https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:79: int dh = first_rep.pixel_height() - second_rep.pixel_height(); drive-by question: where / in which process the rounding error is happening now? Just curious and having a pointer to such case in the comment would be helpful to understand for those who need to touch this code later.
On 2013/03/13 14:47:52, oshima wrote: > https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_o... > File ui/gfx/image/image_skia_operations.cc (right): > > https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_o... > ui/gfx/image/image_skia_operations.cc:79: int dh = first_rep.pixel_height() - > second_rep.pixel_height(); > drive-by question: where / in which process the rounding error is happening now? > Just curious and having a pointer > to such case in the comment would be helpful to understand > for those who need to touch this code later. This patch addresses rounding errors on the tab strip. We see the issue when doing masking operations such as hover highlight, semi-transparency, etc. Consider the process of rendering the left edge of a tab. We have an image asset with a pixel size that is not an integer scale of the DIP size. For the background of the tab, we use an image tile operation which sets the 140P representation based on scaling up the dip size. The UX team can use different rounding rules when scaling up an asset, thus we can get off by one errors. Any discrepancy larger than 1 pixel will get flags and show in red. I can add a more terse explanation as part of the comment (e.g. resource asset size versus auto-scaled image size).
On Wed, Mar 13, 2013 at 8:02 AM, <kevers@chromium.org> wrote: > On 2013/03/13 14:47:52, oshima wrote: > > https://codereview.chromium.**org/12730010/diff/16001/ui/** > gfx/image/image_skia_**operations.cc<https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_operations.cc> > >> File ui/gfx/image/image_skia_**operations.cc (right): >> > > > https://codereview.chromium.**org/12730010/diff/16001/ui/** > gfx/image/image_skia_**operations.cc#newcode79<https://codereview.chromium.org/12730010/diff/16001/ui/gfx/image/image_skia_operations.cc#newcode79> > >> ui/gfx/image/image_skia_**operations.cc:79: int dh = >> first_rep.pixel_height() - >> second_rep.pixel_height(); >> drive-by question: where / in which process the rounding error is >> happening >> > now? > >> Just curious and having a pointer >> to such case in the comment would be helpful to understand >> for those who need to touch this code later. >> > > This patch addresses rounding errors on the tab strip. We see the issue > when > doing masking operations such as hover highlight, semi-transparency, etc. > Consider the process of rendering the left edge of a tab. We have an image > asset with a pixel size that is not an integer scale of the DIP size. For > the > background of the tab, we use an image tile operation which sets the 140P > representation based on scaling up the dip size. Is this because theme images are available only in 100P and have to be scaled up to match 140P? > The UX team can use different > rounding rules when scaling up an asset, thus we can get off by one > errors. Any > discrepancy larger than 1 pixel will get flags and show in red. > If the rounding errors come from the resources that UX team make offline, then UX team has to fix it and stick to the same rounding rule, unless there is some reason which make it difficult to do so. - oshima > > I can add a more terse explanation as part of the comment (e.g. resource > asset > size versus auto-scaled image size). > > > https://codereview.chromium.**org/12730010/<https://codereview.chromium.org/1... >
Updated comment explaining source of potential size mismatch.
Added comments inline... On 2013/03/13 15:24:43, oshima wrote: > On Wed, Mar 13, 2013 at 8:02 AM, <mailto:kevers@chromium.org> wrote: > > Is this because theme images are available only in 100P and > have to be scaled up to match 140P? > Some of the images are generated and not part of the resource bundle. Tiled backgrounds and hover effects fall into this category. Other resources are not necessarily available that the requested scale. Favicons and themes come to mind. > > If the rounding errors come from the resources that UX team make offline, > then > UX team has to fix it and stick to the same rounding rule, unless there is > some > reason which make it difficult to do so. > For 200%, we could simply enforce that each scaled asset have an even height and width. The optimal rounding is not so clear for 140% and 180%. Any rules we were to suggest to UX at this time might need refinement.
On 2013/03/13 15:37:08, kevers wrote: > Added comments inline... > > > On 2013/03/13 15:24:43, oshima wrote: > > On Wed, Mar 13, 2013 at 8:02 AM, <mailto:kevers@chromium.org> wrote: > > > > > Is this because theme images are available only in 100P and > > have to be scaled up to match 140P? > > > > Some of the images are generated and not part of the resource bundle. Tiled > backgrounds Background doesn't seem to be using ImageSkiaOperations. Am I wrong? > and hover effects fall into this category. When theme is used, right? Without theme, all images are supposed to have the right size. The reason I'm asking is that IF this is to handle some specific use cases (like theme), then we can avoid this type of hack by using the consistent rounding rule in both UX team and theme scaling. > Other resources are not necessarily available that the requested scale. > Favicons and themes come to mind. > > > > > If the rounding errors come from the resources that UX team make offline, > > then > > UX team has to fix it and stick to the same rounding rule, unless there is > > some > > reason which make it difficult to do so. > > > > For 200%, we could simply enforce that each scaled asset have an even height and > width. The optimal rounding is not so clear for 140% and 180%. Any rules we > were to suggest to UX at this time might need refinement. We use PRESUBMIT check to make sure they have proper size, and you can (and probably should) do the same if 140P images have the right size with the same rounding rule. (Unless UX team really wants to use different rounding rule for each images).
Imposed consistent rounding rules to avoid size mismatches.
https://chromiumcodereview.appspot.com/12730010/diff/32001/ui/gfx/image/image... File ui/gfx/image/image_skia_operations.cc (right): https://chromiumcodereview.appspot.com/12730010/diff/32001/ui/gfx/image/image... ui/gfx/image/image_skia_operations.cc:285: class ButtonImageSource: public BinaryImageSource { I vaguely remember that doing this caused some issues on 2x DSF/Pixel. I can't remember now (Sorry, it's my fault. I should have documented it) but could you please test this on 2x DSF and check if this doesn't cause any issue?
https://chromiumcodereview.appspot.com/12730010/diff/32001/ui/gfx/image/image... File ui/gfx/image/image_skia_operations.cc (right): https://chromiumcodereview.appspot.com/12730010/diff/32001/ui/gfx/image/image... ui/gfx/image/image_skia_operations.cc:285: class ButtonImageSource: public BinaryImageSource { On 2013/03/13 22:08:53, oshima wrote: > I vaguely remember that doing this caused some issues on 2x DSF/Pixel. > > I can't remember now (Sorry, it's my fault. I should have > documented it) but could you please test this on 2x DSF > and check if this doesn't cause any issue? Tested on Linux Chromeos with a forced scale factor. Do not see any visual defects.
https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:39: gfx::Rect& bounds_in_pixel) { Did you define this intentionally? Or can be in ExtractSubsetImageSource? https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:285: class ButtonImageSource: public BinaryImageSource { On 2013/03/14 15:49:46, kevers wrote: > On 2013/03/13 22:08:53, oshima wrote: > > I vaguely remember that doing this caused some issues on 2x DSF/Pixel. > > > > I can't remember now (Sorry, it's my fault. I should have > > documented it) but could you please test this on 2x DSF > > and check if this doesn't cause any issue? > > Tested on Linux Chromeos with a forced scale factor. Do not see any visual > defects. I now remember the reason. crbug.com/171725 In short, the image and mask can be different size. Can you handle this separately?
https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:285: class ButtonImageSource: public BinaryImageSource { On 2013/03/14 18:12:20, oshima wrote: > On 2013/03/14 15:49:46, kevers wrote: > > On 2013/03/13 22:08:53, oshima wrote: > > > I vaguely remember that doing this caused some issues on 2x DSF/Pixel. > > > > > > I can't remember now (Sorry, it's my fault. I should have > > > documented it) but could you please test this on 2x DSF > > > and check if this doesn't cause any issue? > > > > Tested on Linux Chromeos with a forced scale factor. Do not see any visual > > defects. > > I now remember the reason. crbug.com/171725 > > In short, the image and mask can be different size. Can you handle this > separately? Reverted ButtonImageSource change and added a comment.
https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:39: gfx::Rect& bounds_in_pixel) { On 2013/03/14 18:12:20, oshima wrote: > Did you define this intentionally? Or can be in ExtractSubsetImageSource? you probably missed this comment.
https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:39: gfx::Rect& bounds_in_pixel) { On 2013/03/14 19:51:17, oshima wrote: > On 2013/03/14 18:12:20, oshima wrote: > > Did you define this intentionally? Or can be in ExtractSubsetImageSource? > > you probably missed this comment. Oops..I did miss it. It was pulled out since it was being used elsewhere. Can move it back now that cropping is done in only one spot.
On Thu, Mar 14, 2013 at 1:18 PM, <kevers@chromium.org> wrote: > > https://codereview.chromium.**org/12730010/diff/32001/ui/** > gfx/image/image_skia_**operations.cc<https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_operations.cc> > File ui/gfx/image/image_skia_**operations.cc (right): > > https://codereview.chromium.**org/12730010/diff/32001/ui/** > gfx/image/image_skia_**operations.cc#newcode39<https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_operations.cc#newcode39> > ui/gfx/image/image_skia_**operations.cc:39: gfx::Rect& bounds_in_pixel) { > On 2013/03/14 19:51:17, oshima wrote: > >> On 2013/03/14 18:12:20, oshima wrote: >> > Did you define this intentionally? Or can be in >> > ExtractSubsetImageSource? > > you probably missed this comment. >> > > Oops..I did miss it. It was pulled out since it was being used > elsewhere. Can move it back now that cropping is done in only one spot. > > https://codereview.chromium.**org/12730010/<https://codereview.chromium.org/1... > Yeah, that's make it a bit easier to follow.
https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/12730010/diff/32001/ui/gfx/image/image_skia_o... ui/gfx/image/image_skia_operations.cc:39: gfx::Rect& bounds_in_pixel) { On 2013/03/14 18:12:20, oshima wrote: > Did you define this intentionally? Or can be in ExtractSubsetImageSource? Move back to ExtractSubsetImageSource.
LGTM
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/12730010/42001
Message was sent while issue was closed.
Change committed as 188434 |
