Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1634103003:
Fix gfx::Canvas::DrawStringRectWithHalo (Closed)
DescriptionFix gfx::Canvas::DrawStringRectWithHalo
Crash fixed. DrawStringRectWithHalo is GPU-friendly now.
BUG=581358, 546564
R=tomhudson,ericrk,sky,chrishtr
TEST=PaintingWindowTest*
Committed: https://crrev.com/a757877ee245399ff048cca891e1bf7a1700a58f
Cr-Commit-Position: refs/heads/master@{#406374}
Patch Set 1 #
Total comments: 4
Patch Set 2 : pixel test #Patch Set 3 : small fixes #Patch Set 4 : Try to fix png loading #Patch Set 5 : rewrite test #
Total comments: 4
Patch Set 6 : remarks #Patch Set 7 : small fixes #Patch Set 8 : remove rebaseline #Patch Set 9 : try another font #Patch Set 10 : more tolerant compare #Patch Set 11 : robust test #Patch Set 12 : check halo pixels #
Total comments: 2
Patch Set 13 : fix memory leak #Patch Set 14 : fuzzy pixel tests #Patch Set 15 : fix fuzzy comparator's params #
Total comments: 8
Patch Set 16 : fixes #
Total comments: 3
Patch Set 17 : for try jobs only #Patch Set 18 : less tolerant compare #
Total comments: 2
Patch Set 19 : bots' baselines #
Total comments: 10
Patch Set 20 : small fixes #Messages
Total messages: 62 (9 generated)
Hello! It is look like we haven't tests for OnPaint for slimming paint. I fixed crash in DrawStringRectWithHalo. Made it GPU-friendly. Wrote tests.
Description was changed from ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* ========== to ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* ==========
sky@chromium.org changed reviewers: + danakj@chromium.org
+danakj
The bugs and the CL don't specify, what exactly was the crash?
danakj@chromium.org changed reviewers: + senorblanco@google.com
It's good to add tests, thanks, and it's great we're following [+senorblanco]'s advice and using a filter. https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... File ui/aura/painting_window_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... ui/aura/painting_window_unittest.cc:21: typedef void(*TestFunction)(gfx::Canvas*, const base::string16&, using, not typedef https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... ui/aura/painting_window_unittest.cc:60: // Snapshot test tests real drawing and readback, so needs pixel output. "Snapshot test tests"? I see no Snapshot tests/classes in this file. https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... ui/aura/painting_window_unittest.cc:156: WaitForDraw(); why wait for draw? SetupTestWindow already did? https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... ui/aura/painting_window_unittest.cc:179: EXPECT_NE(0u, count_blue); Few things about these tests.. 1) I'm not a big fan of unit tests which don't test something meaningful beyond "doesn't crash". It would be better if you want to test that text is being drawn, that you compare to a png and make sure the text was actually drawn? If that's too heavy handed is there something else you can test instead? 2) Why is this in ui/aura? It's not testing aura things. Can you put the test closer to the code it is testing? 3) Maybe you don't need a whole Compositor and aura::Window etc to test this, and just making a PaintContext and drawing a string of text would be enough? I can't tell cuz I'm not sure what the crash was though so I'm not sure what you're trying to test.
On 2016/01/26 19:19:42, danakj wrote: > The bugs and the CL don't specify, what exactly was the crash? There is only one place in chromium where DrawStringRectWithHalo is used. https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/d... But it doesn't involve slimming paint. So we have only slowdown here. But if you try to replace DrawStringRect in any OnPaint(gfx::Canvas* canvas) function ( for example, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... ) then browser crashes. I wrote test for this case.
On 2016/01/26 19:52:38, Lof wrote: > On 2016/01/26 19:19:42, danakj wrote: > > The bugs and the CL don't specify, what exactly was the crash? > There is only one place in chromium where DrawStringRectWithHalo is used. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/d... > But it doesn't involve slimming paint. So we have only slowdown here. > > But if you try to replace DrawStringRect in any OnPaint(gfx::Canvas* canvas) > function > ( for example, > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > ) > then browser crashes. > I wrote test for this case. What is the crash that happens exactly? Do you have a backtrace?
On 2016/01/26 19:54:31, danakj wrote: > What is the crash that happens exactly? Do you have a backtrace? DrawStringRectWithHalo() is one several parts of ui/gfx/ which assumes that the rendering is happening in software and that it can read back a buffer. I expect the problem is that skia::GetWritablePixels() fails; it *should* with a hardware call. I'd toyed with a different patch to Skiafy this particular calling point (any time we see ui/gfx/ iterating over raw pixels it smells like something we should perhaps be handling on their behalf), but I like the approach in this CL better, and it has a test! Like this CL, I'd used SkDilateImageFilter; we may want to switch to SkMaskFilter for performance reasons in the future, but this code path gets extraordinarily little use.
On 2016/01/26 20:11:36, tomhudson wrote: > DrawStringRectWithHalo() is one several parts of ui/gfx/ which assumes that the > rendering is happening in software and that it can read back a buffer. I expect > the problem is that skia::GetWritablePixels() fails; it *should* with a hardware > call. More specifically, skia::GetWritablePixels() should be returning an empty SkPixmap if the canvas you're trying to fetch from is on the GPU. We don't check for that in gfx::Canvas::DrawStringRectWithHalo(). During the normal operation of ui/gfx/ our canvas is never on the GPU, but if Yandex wants to use the function more generally or start supporting a GPU-backed UI this hardening is a good idea.
On 2016/01/26 19:41:53, danakj wrote: > "Snapshot test tests"? I see no Snapshot tests/classes in this file. https://code.google.com/p/chromium/codesearch#chromium/src/ui/snapshot/snapsh... I based on snapshot_aura_unittests. They are broken and don't run. I fixed them, but i think i have no chance for LGTM on https://code.google.com/p/chromium/codesearch#chromium/src/testing/buildbot/c... > https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... > ui/aura/painting_window_unittest.cc:156: WaitForDraw(); > why wait for draw? SetupTestWindow already did? You are right, i will fix it. > https://codereview.chromium.org/1634103003/diff/1/ui/aura/painting_window_uni... > ui/aura/painting_window_unittest.cc:179: EXPECT_NE(0u, count_blue); > Few things about these tests.. > > 1) I'm not a big fan of unit tests which don't test something meaningful beyond > "doesn't crash". It would be better if you want to test that text is being > drawn, that you compare to a png and make sure the text was actually drawn? If > that's too heavy handed is there something else you can test instead? Comparing png is very error prone method. So I try to check the quantities of background pixels and text pixels. There must be a lot of background pixels and few of text pixels. > 2) Why is this in ui/aura? It's not testing aura things. Can you put the test > closer to the code it is testing? I need the window due to OnPaint method. I try to write RealCompositor test a little, but it doesn't work. But I can try again :) > 3) Maybe you don't need a whole Compositor and aura::Window etc to test this, > and just making a PaintContext and drawing a string of text would be enough? I > can't tell cuz I'm not sure what the crash was though so I'm not sure what > you're trying to test. I need PlaybackToCanvas method. So I think compositor is necessary.
> What is the crash that happens exactly? Do you have a backtrace? Backtrace: S32A_Opaque_BlitRow32_SSE4 [0x6E619670+160] Sprite_D32_S32::blitRect [0x6E325889+153] SkScan::FillXRect [0x6E317871+705] SkScan::FillIRect [0x6E31713E+254] SkScan::FillIRect [0x6E3172AA+138] SkDraw::drawBitmap [0x6E26D618+648] SkBitmapDevice::drawBitmap [0x6E217116+22] SkCanvas::onDrawBitmap [0x6E250435+709] SkCanvas::drawBitmap [0x6E24C02B+59] SkRecords::Draw::operator()<SkRecords::DrawBitmap> [0x6E2E1321+65] SkRecord::Record::visit<void,SkRecords::Draw> [0x6E2E3B20+176] SkRecordDraw [0x6E2E4B5C+556] SkBigPicture::playback [0x6E20FABE+334] SkCanvas::drawPicture [0x6E24CEC8+232] cc::DrawingDisplayItem::Raster [0x6AFA13C3+179] cc::DisplayItemList::Raster [0x6AF9DA5B+251] cc::DisplayListRasterSource::RasterCommon [0x6AFA017B+379] cc::DisplayListRasterSource::PlaybackToCanvas [0x6AF9FAC0+64] cc::TileTaskWorkerPool::PlaybackToMemory [0x6AFBBFB6+566] cc::`anonymous namespace'::RasterBufferImpl::Playback [0x6AFAA19A+186] cc::`anonymous namespace'::RasterTaskImpl::Raster [0x6AFF8533+179] cc::`anonymous namespace'::RasterTaskImpl::RunOnWorkerThread [0x6AFF882C+284] cc::TaskGraphRunner::RunTaskWithLockAcquired [0x6AFB66D7+679] cc::TaskGraphRunner::Run [0x6AFB63EC+60] base::SimpleThread::ThreadMain [0x719F63C3+147] base::`anonymous namespace'::ThreadFunc [0x719EE722+146] BaseThreadInitThunk [0x77247C04+36] RtlInitializeExceptionChain [0x77C6AD1F+143] RtlInitializeExceptionChain [0x77C6ACEA+90]
We can't to record display list when we use temporary SkBitmap https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas_skia...
I rewrote to pixel test and fixed remarks. Now I try to write simple cc/playback test that reproduce the problem for better understanding. There is no test with OnPaint + Slimming paint, and on gfx::Canvas::DrawString functions. So I think painting_window_unittest has worth.
I rewrote test again. It's look like ok. And tests were passed successfully! I am still trying to write simple cc/playback test...
Thanks, doing this as a ui compositor test makes more sense, and looks much better. A few suggestions. https://codereview.chromium.org/1634103003/diff/80001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:53: using TestFunction = void(*)(gfx::Canvas*, const base::string16&, Can you give this a better name? What is this function meant to do? What are these arguments, some of them need names at the least. https://codereview.chromium.org/1634103003/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:112: func_(recorder.canvas(), text, font_list_, gfx::Rect(size())); how about just making an enum for the various ways this can draw text, and here do a switch on that enum? that'd be much easier to follow (no indirection) you can pass the enum value to the constructor and store that on the class https://codereview.chromium.org/1634103003/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:1287: WritePNGFile(bitmap, ref_img, true); If you write the PNG every time, then it will always match? I think you want to write it once, save the .png as part of the patch, and remove the Write from the CL? https://codereview.chromium.org/1634103003/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:1310: test_data_directory().AppendASCII("string.png"); string_faded.png?
It looks like draw text + pixel test = bad test.
On 2016/01/29 22:39:13, Lof wrote: > It looks like draw text + pixel test = bad test. ?
Appearance of text is vary from one platform to another. And even on Windows there are a lot of differences. Just see trybot results in 8, 9, 10 patch sets. I think it’s very hard to write robust pixel test with fuzzy comparator. If we can do it, eventually this test will be disabled. Bad test is much better than disabled test :) So I think it is better to check: 1) We have something drawn. 2) In test with halo text, to check existing of halo pixels.
lof84@yandex-team.ru changed reviewers: + enne@chromium.org
PTAL. I think this CL is important. I also reproduce original problem in simple test: https://codereview.chromium.org/1673193002 Summary: If we set immutable flag to HBitmap, then we can crash during PlaybackToCanvas.
On 2016/02/08 14:11:21, Lof wrote: > PTAL. I think this CL is important. I certainly like the idea of changing Canvas::DrawStringRectWithHalo(); I abandoned my own patch along those lines because yours seems more complete, but *something* along these lines will have to go through in the near future because we're deprecating the part of the Skia API used in the original code. But exact pixel tests on text are not very stable; I see you've rewritten that as a unit test, but I can't tell from your code what you're actually trying to test or why the expected results are what they are. And it's hard to argue that the CL is particularly important when there isn't actually a live bug; in practice we don't host those canvases on the GPU or set those bitmaps to immutable.
On 2016/02/08 14:17:54, tomhudson wrote: > I certainly like the idea of changing Canvas::DrawStringRectWithHalo(); I > abandoned my own patch along those lines because yours seems more complete, but > *something* along these lines will have to go through in the near future because > we're deprecating the part of the Skia API used in the original code. Sorry, can you explain me "but *something* along..." ? > But exact pixel tests on text are not very stable; I see you've rewritten that > as a unit test, but I can't tell from your code what you're actually trying to > test or why the expected results are what they are. Yes, test is not clear. But it's robust. 1) EXPECT_FALSE(CompareColor(bitmap, size, check_color)); // We have something drawn. 2) EXPECT_TRUE(HasColor(bitmap, size, halo_color)); // To check existing of halo pixels. It is invariant for all possible DrawStringRectWithHalo functions. So we check it :) > And it's hard to argue that the CL is particularly important when there isn't > actually a live bug; in practice we don't host those canvases on the GPU or set > those bitmaps to immutable. Sorry for my english. important => worth.
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/1634103003/diff/220001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/1634103003/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:278: paint_.setImageFilter(SkDilateImageFilter::Create(1, 1)); I think there's a memory leak here. Objects in Skia are created with a refcount of 1, and SkPaint::setImageFilter() takes another ref (making it 2). In Chrome and Blink, the usual pattern is something like this: skia::RefPtr<SkImageFilter> filter = skia::AdoptRef(SkDilateImageFilter::Create(1, 1)); paint.setImageFilter(filter.get());
https://codereview.chromium.org/1634103003/diff/220001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/1634103003/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:278: paint_.setImageFilter(SkDilateImageFilter::Create(1, 1)); On 2016/02/08 16:24:51, Stephen White wrote: > skia::RefPtr<SkImageFilter> filter = > skia::AdoptRef(SkDilateImageFilter::Create(1, 1)); (Or auto filter = skia::AdoptRef(SkDilateImageFilter::Create(1, 1)); to be more modern/concise, I suppose.)
On 2016/02/08 16:24:52, Stephen White wrote: > I think there's a memory leak here. You are right. I fixed it. Thank you!
What's the status of this CL?
On 2016/05/20 00:14:17, chrishtr wrote: > What's the status of this CL? Hello! Code works. But I don’t know how to write a good test. Exact pixel test fails, so I use heuristic in test. Reviewers don’t like it… PS. In Yandex.Browser code is working about 3 month in production.
On 2016/05/20 08:17:57, Lof wrote: > On 2016/05/20 00:14:17, chrishtr wrote: > > What's the status of this CL? > > Hello! > > Code works. > But I don’t know how to write a good test. Exact pixel test fails, so I use > heuristic in test. Reviewers don’t like it… > PS. In Yandex.Browser code is working about 3 month in production. For pixel tests with platform differences instead of cc::ExactPixelComparator you can use a fuzzy one: https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/pixel_comp...
On 2016/05/20 21:16:12, danakj wrote: > For pixel tests with platform differences instead of cc::ExactPixelComparator > you can use a fuzzy one: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/pixel_comp... Hi! Ok. I will try again. But there is no success story of using fuzzy comparator for test with fonts because of huge deltas.
I rewrote test. Try jobs succeeded.
https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:118: const base::string16 text = base::ASCIIToUTF16("Danakj"); lol how about "TEST" or something? https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1304: #if defined(OS_WIN) Why only OS_WIN?
https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:96: class DrawStringLayer : public Layer, public LayerDelegate { Why subclass Layer? This is just a LayerDelegate right? You can just use ui::Layer with this delegate?
https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1304: #if defined(OS_WIN) On 2016/05/23 19:42:55, danakj wrote: > Why only OS_WIN? We should probably run this on every platform, but I suspect the limitation here was because the function was only invoked on Windows - although it looks like TextExample::TextExampleView could call it on any platform, I don't think that's real code?
On 2016/05/23 19:49:05, tomhudson wrote: > https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... > File ui/compositor/layer_unittest.cc (right): > > https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... > ui/compositor/layer_unittest.cc:1304: #if defined(OS_WIN) > On 2016/05/23 19:42:55, danakj wrote: > > Why only OS_WIN? > > We should probably run this on every platform, but I suspect the limitation here > was because the function was only invoked on Windows - although it looks like > TextExample::TextExampleView could call it on any platform, I don't think that's > real code? Hi. I limited test only for WIN to reduce flakiness. Fonts on other platforms are different. Can I just run test without making a comparison on other platforms?
https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:96: class DrawStringLayer : public Layer, public LayerDelegate { On 2016/05/23 19:44:01, danakj wrote: > Why subclass Layer? This is just a LayerDelegate right? You can just use > ui::Layer with this delegate? Done. https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:118: const base::string16 text = base::ASCIIToUTF16("Danakj"); On 2016/05/23 19:42:55, danakj wrote: > lol how about "TEST" or something? Done. https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1304: #if defined(OS_WIN) On 2016/05/23 19:42:55, danakj wrote: > Why only OS_WIN? Done. https://codereview.chromium.org/1634103003/diff/280001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1304: #if defined(OS_WIN) On 2016/05/23 19:49:05, tomhudson wrote: > On 2016/05/23 19:42:55, danakj wrote: > > Why only OS_WIN? > > We should probably run this on every platform, but I suspect the limitation here > was because the function was only invoked on Windows - although it looks like > TextExample::TextExampleView could call it on any platform, I don't think that's > real code? Done.
https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1329: float percentage_pixels_large_error = 50.0f; // Don't check. What do you mean don't check? Surely the platform where you're generating the pngs doesn't need any error allowance? Normally we generate pngs on linux, and so we'd get exact results there, and allow some pixels to differ on other platforms.
On 2016/06/01 18:45:47, danakj wrote: > https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... > File ui/compositor/layer_unittest.cc (right): > > https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... > ui/compositor/layer_unittest.cc:1329: float percentage_pixels_large_error = > 50.0f; // Don't check. > What do you mean don't check? Surely the platform where you're generating the > pngs doesn't need any error allowance? Normally we generate pngs on linux, and > so we'd get exact results there, and allow some pixels to differ on other > platforms. My idea: on platforms != Windows we don't check result. We check only that code doesn't crash or doesn’t produce completely wrong result. And we check result on Windows . It is really hard to write pixel test on text rendering. There are a lot of reasons: 1) We can't choose font exactly. We can only choose preferred font. 2) We have a lot of system properties (clear type, aliasing, etc) which affect font appearance. 3) We have different glyph rendering system (DIrectWrite, etc). 4) We have even different font version (Arial 2.76 and Arial 5.00). 5) etc... That’s why we don’t have success story with pixel test with font.
On 2016/06/01 21:17:34, Lof wrote: > That’s why we don’t have success story with pixel test with font. This doesn't seem like a good candidate for a unit test, which is why my previous approach to this same problemlatic area of the code didn't bother adding any tests. If we want pixel tests across all platforms, we need to in some way recreate Blink's layout test infrastructure. Or, to create text pixel tests with any likelihood of staying uniform across all platforms, we could synthesize a non-scaling bitmap-only font and hardwire using that for the tests. Skia's put a lot of effort into pixel tests across most of Chrome's platforms (see gold.skia.org). Since this is replacing a bunch of hand-rolled pixel munging on deprecated APIs with a simple use of SkDilateImageFilter, why don't we rely on Skia's tests for output? (to get a sense of coverage, cd third_party/skia/gm; git grep SkDilateImageFilter) If the Chrome team then wants a unit test, it could be limited to verifying that DrawStringRectWithHalo() was calling into the render text functions with an SkPaint that has the image filter set?
Another alternative is to use the Ahem font. It consists of four simple glyphs and often renders the same on all platforms. I don't know if it would still cover correctness in this case, but it's an option. http://testthewebforward.org/docs/test-style-guidelines.html
https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1329: float percentage_pixels_large_error = 50.0f; // Don't check. On 2016/06/01 18:45:47, danakj wrote: > What do you mean don't check? Surely the platform where you're generating the > pngs doesn't need any error allowance? Normally we generate pngs on linux, and > so we'd get exact results there, and allow some pixels to differ on other > platforms. OK then just #ifdef windows around the whole test, no point in running the test and not doing anything in it. I think this is better than nothing and don't want to see this go around a lot more rounds. LGTM with a few more comments. https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1333: int large_error_allowed = 256; Can you narrow this? Surely the pixels are off by only a little in each channel. The test output will actually say the maximum error amount in each of r/g/b when it fails, so you can use that to choose the minimum needed. Same for the average allowed, it prints out in a failure what it found so you can use something close to that instead? Ditto for the other test.
On 2016/06/22 23:14:04, danakj wrote: > LGTM with a few more comments. (please get senorblanco to review the non-test parts and approve it too though.)
On Mon, Jun 6, 2016 at 8:38 AM, <tomhudson@google.com> wrote: > On 2016/06/01 21:17:34, Lof wrote: > > That’s why we don’t have success story with pixel test with font. > > This doesn't seem like a good candidate for a unit test, which is why my > previous approach to this same problemlatic area of the code didn't bother > adding any tests. > > If we want pixel tests across all platforms, we need to in some way > recreate > Blink's layout test infrastructure. > > Or, to create text pixel tests with any likelihood of staying uniform > across all > platforms, we could synthesize a non-scaling bitmap-only font and hardwire > using > that for the tests. > > Skia's put a lot of effort into pixel tests across most of Chrome's > platforms > (see gold.skia.org). Since this is replacing a bunch of hand-rolled pixel > munging on deprecated APIs with a simple use of SkDilateImageFilter, why > don't > we rely on Skia's tests for output? (to get a sense of coverage, cd > third_party/skia/gm; git grep SkDilateImageFilter) > Skia's tests are awesome, but I don't think we need that level of attention to pixel correctness here. I simply want to know that some halo is being added to the text, etc. Running tests on all platforms is nice, and I think we could to that, but I'd rather land this with a test on windows than go in circles forever. > > If the Chrome team then wants a unit test, it could be limited to > verifying that > DrawStringRectWithHalo() was calling into the render text functions with an > SkPaint that has the image filter set? > That'd be possible too, but then we have to introspect skia types to verify all the fields are right or something. Fuzzy pixel testing seems like a reasonable compromise in this space. -- 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 2016/06/22 23:19:21, danakj wrote: > That'd be possible too, but then we have to introspect skia types to verify > all the fields are right or something. Fuzzy pixel testing seems like a > reasonable compromise in this space. Yuck. Given how much pain fuzzy pixel testing has given us in the past I tried to sketch up a CL, but it looks like we can't introspect farther than verifying that *some* image filter is attached to the text draw - they're opaque on purpose.
On 2016/06/22 23:14:04, danakj wrote: > LGTM with a few more comments. > > https://codereview.chromium.org/1634103003/diff/300001/ui/compositor/layer_un... > ui/compositor/layer_unittest.cc:1333: int large_error_allowed = 256; > Can you narrow this? Surely the pixels are off by only a little in each channel. > The test output will actually say the maximum error amount in each of r/g/b when > it fails, so you can use that to choose the minimum needed. > > Same for the average allowed, it prints out in a failure what it found so you > can use something close to that instead? > > Ditto for the other test. Hello Dana! "Surely the pixels are off by only a little in each channel." Sorry, but it is not correct. Just because it is text rendering :( From log: [ RUN ] LayerWithRealCompositorTest.CanvasDrawStringRectWithShadows [196:6784:0623/054438:25125786:ERROR:pixel_comparator.cc(177)] Percentage of pixels with an error: 7.36 [196:6784:0623/054438:25125786:ERROR:pixel_comparator.cc(179)] Percentage of pixels with errors not greater than 0: 0 [196:6784:0623/054438:25125786:ERROR:pixel_comparator.cc(182)] Average absolute error (excluding identical pixels): R=58.7228 G=0 B=48.6522 A=0 [196:6784:0623/054438:25125786:ERROR:pixel_comparator.cc(187)] Largest absolute error: R=246 G=0 B=196 A=0 [196:6784:0623/054438:25125801:ERROR:pixel_comparator.cc(205)] Error Bounding Box : 0,20 39x11 [196:6784:0623/054438:25125801:ERROR:pixel_test_utils.cc(79)] Pixels do not match! [196:6784:0623/054438:25125801:ERROR:pixel_test_utils.cc(80)] Actual: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAADEElEQVRoge3ZQWsbZxDG8d+uldgpKb3E1kpJ7OSWQ5rQQ0HYFAq9tPQSjMmnKP0wpZ8iuCaXUnoolGIct5fSJofcEjeOtFJ8KWlxnEi7PVhaS7Jsy25Mt2YfEGjfmZ13/vvODFoUkKbOgML/OoG3pQIkbypA8qYCJG8qQPKmAiRvKkDypgIkbypA8qYCJG8KUo58Z5+0IxFmn9NWKjChk12Ps2cQ6qRVdRUNVXX33bFkeSBIIlRX1VBRVz0WzEmSSgWuepZdj7NnadRiVf1YQY7SSeJVNLLvsehkIMNB7rszYB8utVQw0v7GOfDMVTBvzZZLtl04NB6D8L+5fSgElHplE4uyG371QeZwz13v+FskVtZU1rJjUs26hooNc855o1eekTizX/PEU9d94WsVDZ/71pp51zwBkeaAf690IZQcmfwACPY1cXvooKZtmfFCRZyt9T+xyzZFmtnaohWROJsiEzoDiZU1hZIsXs//3wyTkaU1rA1z+9Zm/SGUWLDqqevgS19pmdFWOjChj/zkZ7Wx/cfVWCCLVlyx6bLnHnof3PSom0BgwaqylhWL4IbH2kpZj+wWXQW79V7zQFVjpP+pnkivLBKh185LhDZd6Y7kSFNZR8miFfDYDfPWsnrv9G3z2nkNVQQj/bdcOj2QYS1bsmRZLPLAvCnbEhNCiRktyGoeAom0+/2VCybtCKQj/VOBKds+9f3bBbnopb+8O7D2nj/NaGkrmbXh1dA4/diPbnqUXX9jCdx1z0Uv7Zg80D8V2DCnZj2zj1Nu+0Am7fjMdwNBatZV1VXVxSKf+MHe2C6btZFNrZK2SJyV44LV7lhuuuV3z1V96Jdsag37w5Rttzw8MvlDQXoJ9tRWyhp1HPX6qNdDLdPdsToxcrwO+8eifXue6EQS4UCQjtIAWNSd/XG3yRtdW2B3KPTbG92z6Ch1O2IX6IXp7J5h/7pq5nscBaP+seo/5l7AvR8QexOs/wkfZu+3jbrnoHjDORwb5P+oM/NiVYDkTQVI3lSA5E0FSN5UgORNBUjedGZA/gFoNEKIpUKSQQAAAABJRU5ErkJggg== [196:6784:0623/054438:25125801:ERROR:pixel_test_utils.cc(81)] Expected: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAAD3ElEQVRoge3ZXYiUVRgH8N+M06xCkF7ozoyumHhlpklhxBIiEbYq0cdu2IVeVAQJXWQQeBERpkbQB16IF5IUbfjRioEl1oXRCi114W5+JQVarvOxrrmSkI27M13MO9Pu7NeMCE4yfzjwvuf5P+c8//M+5zlnmBD5vDsA4dsdwK1CXUitoS6k1lAXUmuoC6k11IXUGupCag11IbWGupBaQ11IrSF8VEh+wtYpKitiUFjuJqbosrsq/rvyQs7ICQetEkQeMyQhKS4lYa92Xc57zVsBIadfi8OSElLikhJyFX/IPa44qNvnJY9KfSOSEsFzJXNGxuqMVjnIxMhruInxIlLiwXNabFKfMax5USnxoIX96R1P+dEyFzQZstUnI1Jtjytl6fiLrIh2Vzxvun0e0eS0bk0uiGofh18cb54BxS9SaJWkVyQnLCkhLabHWTecd81Sx8GAV71skaPmm6tRxkve8KIBl7X5yE96dOpxw/ogPWP2a9fgV10esN3PjshosU+X1U454ppTzlmLmMwI/nJxSUNBaNXlQaToUnCLICwvYhB871Gb7TffLClxaV9ZbaVN2jTba0DYPJf0istISKLZOs1y0hpdFfIPhoJVzWOBfhlxOXHpMn5YzgBaDVQhoiRkPKxxDp9p86a2MtvfTppjoWNatXpPK9horWUGRcZZz4U6bbPGTmvG5TfJVCliUiEF7Paxe3TrNdtFJ9wPFjkpJm2XDls0atRph032BAF22GCjC/IaZMUdD7buNlctdFrCd2Pw75KTxhJ9VQqZMA0PuReHPBxstpywrKjrpuo1p1SSC2V5hQ0OeMY2vT7wkD4h5EwNUrXQsqJSEuPw8/otkRaqUsakJ/sKnTrc55jlpb6DvvChV8SkfesHX3o7KBaFMtlvhmZ/SYuZ5rIO00tV54Dr3rc54I7mp8Ww0uu2l47DSs+dCTFDi990WeDZUt/TOn3tBX0GPWiunf4wzbqSfZVuW+xyUkLCWazS6oCtnvOku51xTYP1Y/LJ+9Tvdjivy9qAUYmYUPnfClFZLQ6X3r/xuMVOlGp68XD6r2w3olBKE5KlJJqlT685MmYFZTkjJu2wJ2RFhYOqVc6/aLYeiy12ohTDcUsNTrLmo6zFAIsYHHHGTo7iPiruoT4zA+FTxkyVcn5abNScld0Exghk+CBDI249xKRRuDZkNEoFthDCciPsqeBbDInIl3J+iktmlnzK+UmJErcajEotQUDDhRX7ht9Gc2WbcSL7cNtYPuONVx5D1UL+j7hzfljd7gBuFepCag11IbWGupBaQ11IraEupNZQF1Jr+BeajpxfNjpzGgAAAABJRU5ErkJggg== e:\b\build\slave\win\build\src\ui\compositor\layer_unittest.cc(1434): error: Value of: MatchesPNGFile(bitmap, ref_img, cc::FuzzyPixelComparator( true, percentage_pixels_large_error, percentage_pixels_small_error, average_error_allowed_in_bad_pixels, large_error_allowed, small_error_allowed)) Actual: false Expected: true [ FAILED ] LayerWithRealCompositorTest.CanvasDrawStringRectWithShadows (67 ms) Funny, but I didn't change shadows rendering, only halo. And halo test passed well! So I narrowed it for halo test. PS. Sorry for controversial CL.
Thanks! One last thought: it might make sense to use the baselines that the bots are generating (the expected image that you pasted) rather than the ones youre generating on your machine. Idk why your machine is giving different results, but other devs are hopefully more likely to get what the bots get? It doesn't realllly make a diff tho if there are machines that will get what you're getting and we expect it to pass there too. https://codereview.chromium.org/1634103003/diff/340001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/340001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1421: float percentage_pixels_large_error = 8.0f; // 200px / (50*50) can be smaller even 7.4 or somethin https://codereview.chromium.org/1634103003/diff/340001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1424: int large_error_allowed = 255; Can be 246 then based on what you showed, it's not awesome but better than 255. (Largest absolute error: R=246 G=0 B=196 A=0)
On 2016/06/23 18:43:28, danakj wrote: > .... Done!
non-test parts LGTM, but sky@ (or some other gfx/ person) should probably take a look at this also.
just nits https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:96: class DrawStringLayerDelegate: public LayerDelegate { Add description, also 'DrawStringLayerDelegate:' -> DrawStringLayerDelegate :'. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:154: SkColor background_color_; nit: use const where applicable. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:159: }; nit: DISALLOW... https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:212: Layer* CreateDrawStringLayer(const gfx::Rect& bounds, nit: make return type std::unique_ptr https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1336: #if defined(OS_WIN) Document why windows only.
https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:96: class DrawStringLayerDelegate: public LayerDelegate { On 2016/07/18 15:02:02, sky wrote: > Add description, also 'DrawStringLayerDelegate:' -> DrawStringLayerDelegate :'. Done. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:154: SkColor background_color_; On 2016/07/18 15:02:02, sky wrote: > nit: use const where applicable. Done. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:159: }; On 2016/07/18 15:02:02, sky wrote: > nit: DISALLOW... Done. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:212: Layer* CreateDrawStringLayer(const gfx::Rect& bounds, On 2016/07/18 15:02:02, sky wrote: > nit: make return type std::unique_ptr Done. https://codereview.chromium.org/1634103003/diff/360001/ui/compositor/layer_un... ui/compositor/layer_unittest.cc:1336: #if defined(OS_WIN) On 2016/07/18 15:02:02, sky wrote: > Document why windows only. Done.
LGTM
The CQ bit was checked by lof84@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1634103003/#ps380001 (title: "small fixes")
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 ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* ========== to ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* ========== to ========== Fix gfx::Canvas::DrawStringRectWithHalo Crash fixed. DrawStringRectWithHalo is GPU-friendly now. BUG=581358, 546564 R=tomhudson,ericrk,sky,chrishtr TEST=PaintingWindowTest* Committed: https://crrev.com/a757877ee245399ff048cca891e1bf7a1700a58f Cr-Commit-Position: refs/heads/master@{#406374} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/a757877ee245399ff048cca891e1bf7a1700a58f Cr-Commit-Position: refs/heads/master@{#406374} |
