From f6bfccd562f4ac071db9902097ad4bea83294085 Mon Sep 17 00:00:00 2001 From: benjaminwagner Date: Thu, 25 Feb 2016 10:28:11 -0800 Subject: [PATCH] Cleanups related to SkFixed. This CL relands https://codereview.chromium.org/1683743005/ with two fixes: - Removing the test for vertical text. Vertical text doesn't work on Windows and breakText isn't supported for non-trivial text. - Omit PaintBreakTextTest in Google3 because we don't have the correct font setup yet. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1683743005 Committed: https://skia.googlesource.com/skia/+/7ea5cb18389db11a94175de95c9db2b44972630c patch from issue 1683743005 at patchset 120001 (http://crrev.com/1683743005#ps120001) Review URL: https://codereview.chromium.org/1739453002 --- include/core/SkFixed.h | 1 - include/core/SkRect.h | 3 +- public.bzl | 2 +- samplecode/SampleText.cpp | 37 -------- src/core/SkBitmapFilter.h | 12 --- src/core/SkPaint.cpp | 2 +- .../gradients/SkTwoPointConicalGradient.h | 1 + tests/PaintBreakTextTest.cpp | 84 +++++++++++++++++++ 8 files changed, 88 insertions(+), 54 deletions(-) create mode 100644 tests/PaintBreakTextTest.cpp diff --git a/include/core/SkFixed.h b/include/core/SkFixed.h index f6dd3d6002..1541e32105 100644 --- a/include/core/SkFixed.h +++ b/include/core/SkFixed.h @@ -22,7 +22,6 @@ typedef int32_t SkFixed; #define SK_FixedHalf (1 << 15) #define SK_FixedMax (0x7FFFFFFF) #define SK_FixedMin (-SK_FixedMax) -#define SK_FixedNaN ((int) 0x80000000) #define SK_FixedPI (0x3243F) #define SK_FixedSqrt2 (92682) #define SK_FixedTanPIOver8 (0x6A0A) diff --git a/include/core/SkRect.h b/include/core/SkRect.h index 4f649b7c73..3ebe099ae6 100644 --- a/include/core/SkRect.h +++ b/include/core/SkRect.h @@ -463,8 +463,7 @@ struct SK_API SkRect { /** * Returns true iff all values in the rect are finite. If any are - * infinite or NaN (or SK_FixedNaN when SkScalar is fixed) then this - * returns false. + * infinite or NaN then this returns false. */ bool isFinite() const { float accum = 0; diff --git a/public.bzl b/public.bzl index 0fb0ceaf7d..986bfd9342 100644 --- a/public.bzl +++ b/public.bzl @@ -464,7 +464,7 @@ def DM_ARGS(base_dir): "--nogpu", "--verbose", # TODO(mtklein): maybe investigate why these fail? - "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~RecordDraw_TextBounds", + "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~RecordDraw_TextBounds ~PaintBreakText", "--resourcePath %s/resources" % base_dir, "--images %s/resources" % base_dir, ] diff --git a/samplecode/SampleText.cpp b/samplecode/SampleText.cpp index f5f8dbefa0..87ca91406b 100644 --- a/samplecode/SampleText.cpp +++ b/samplecode/SampleText.cpp @@ -27,41 +27,6 @@ #include "SkStream.h" #include "SkXMLParser.h" -static void test_breakText() { - SkPaint paint; - const char* text = "sdfkljAKLDFJKEWkldfjlk#$%&sdfs.dsj"; - size_t length = strlen(text); - SkScalar width = paint.measureText(text, length); - - SkScalar mm = 0; - SkScalar nn = 0; - for (SkScalar w = 0; w <= width; w += SK_Scalar1) { - SkScalar m; - size_t n = paint.breakText(text, length, w, &m); - - SkASSERT(n <= length); - SkASSERT(m <= width); - - if (n == 0) { - SkASSERT(m == 0); - } else { - // now assert that we're monotonic - if (n == nn) { - SkASSERT(m == mm); - } else { - SkASSERT(n > nn); - SkASSERT(m > mm); - } - } - nn = SkIntToScalar((unsigned int)n); - mm = m; - } - - SkDEBUGCODE(size_t length2 =) paint.breakText(text, length, width, &mm); - SkASSERT(length2 == length); - SkASSERT(mm == width); -} - static const struct { const char* fName; uint32_t fFlags; @@ -109,8 +74,6 @@ public: TextSpeedView() { fHints = 0; fClickX = 0; - - test_breakText(); } protected: diff --git a/src/core/SkBitmapFilter.h b/src/core/SkBitmapFilter.h index 6fa8edde34..ca3e0930f2 100644 --- a/src/core/SkBitmapFilter.h +++ b/src/core/SkBitmapFilter.h @@ -28,15 +28,6 @@ public: } virtual ~SkBitmapFilter() {} - SkFixed lookup(float x) const { - if (!fPrecomputed) { - precomputeTable(); - } - int filter_idx = int(sk_float_abs(x * fLookupMultiplier)); - SkASSERT(filter_idx < SKBITMAP_FILTER_TABLE_SIZE); - return fFilterTable[filter_idx]; - } - SkScalar lookupScalar(float x) const { if (!fPrecomputed) { precomputeTable(); @@ -67,19 +58,16 @@ protected: float fLookupMultiplier; mutable bool fPrecomputed; - mutable SkFixed fFilterTable[SKBITMAP_FILTER_TABLE_SIZE]; mutable SkScalar fFilterTableScalar[SKBITMAP_FILTER_TABLE_SIZE]; private: void precomputeTable() const { fPrecomputed = true; - SkFixed *ftp = fFilterTable; SkScalar *ftpScalar = fFilterTableScalar; for (int x = 0; x < SKBITMAP_FILTER_TABLE_SIZE; ++x) { float fx = ((float)x + .5f) * this->width() / SKBITMAP_FILTER_TABLE_SIZE; float filter_value = evaluate(fx); *ftpScalar++ = filter_value; - *ftp++ = SkFloatToFixed(filter_value); } } }; diff --git a/src/core/SkPaint.cpp b/src/core/SkPaint.cpp index d3384a628d..638e251b36 100644 --- a/src/core/SkPaint.cpp +++ b/src/core/SkPaint.cpp @@ -925,7 +925,7 @@ size_t SkPaint::breakText(const void* textD, size_t length, SkScalar maxWidth, GlyphCacheProc glyphCacheProc = paint.getGlyphCacheProc(false); const int xyIndex = paint.isVerticalText() ? 1 : 0; // use 64bits for our accumulator, to avoid overflowing 16.16 - Sk48Dot16 max = SkScalarToFixed(maxWidth); + Sk48Dot16 max = SkScalarTo48Dot16(maxWidth); Sk48Dot16 width = 0; SkAutoKern autokern; diff --git a/src/effects/gradients/SkTwoPointConicalGradient.h b/src/effects/gradients/SkTwoPointConicalGradient.h index f468de8007..954b09698e 100644 --- a/src/effects/gradients/SkTwoPointConicalGradient.h +++ b/src/effects/gradients/SkTwoPointConicalGradient.h @@ -15,6 +15,7 @@ // Should only be initialized once via init(). Immutable afterwards. struct TwoPtRadial { enum { + // This value is outside the range SK_FixedMin to SK_FixedMax. kDontDrawT = 0x80000000 }; diff --git a/tests/PaintBreakTextTest.cpp b/tests/PaintBreakTextTest.cpp new file mode 100644 index 0000000000..474bbf686a --- /dev/null +++ b/tests/PaintBreakTextTest.cpp @@ -0,0 +1,84 @@ +/* + * Copyright 2011-2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "SkPaint.h" +#include "Test.h" + +static void test_monotonic(skiatest::Reporter* reporter, + const SkPaint& paint, + const char* msg) { + const char* text = "sdfkljAKLDFJKEWkldfjlk#$%&sdfs.dsj"; + const size_t length = strlen(text); + const SkScalar width = paint.measureText(text, length); + + SkScalar mm = 0; + size_t nn = 0; + const SkScalar step = SkMaxScalar(width / 10, SK_Scalar1); + for (SkScalar w = 0; w <= width; w += step) { + SkScalar m; + const size_t n = paint.breakText(text, length, w, &m); + + REPORTER_ASSERT_MESSAGE(reporter, n <= length, msg); + REPORTER_ASSERT_MESSAGE(reporter, m <= width, msg); + + if (n == 0) { + REPORTER_ASSERT_MESSAGE(reporter, m == 0, msg); + } else { + // now assert that we're monotonic + if (n == nn) { + REPORTER_ASSERT_MESSAGE(reporter, m == mm, msg); + } else { + REPORTER_ASSERT_MESSAGE(reporter, n > nn, msg); + REPORTER_ASSERT_MESSAGE(reporter, m > mm, msg); + } + } + nn = n; + mm = m; + } +} + +static void test_eq_measure_text(skiatest::Reporter* reporter, + const SkPaint& paint, + const char* msg) { + const char* text = "The ultimate measure of a man is not where he stands in moments of comfort " + "and convenience, but where he stands at times of challenge and controversy."; + const size_t length = strlen(text); + const SkScalar width = paint.measureText(text, length); + + SkScalar mm; + const size_t length2 = paint.breakText(text, length, width, &mm); + REPORTER_ASSERT_MESSAGE(reporter, length2 == length, msg); + REPORTER_ASSERT_MESSAGE(reporter, mm == width, msg); +} + +static void test_long_text(skiatest::Reporter* reporter, + const SkPaint& paint, + const char* msg) { + static const int kSize = 16 * 1024; + SkAutoMalloc block(kSize); + memset(block.get(), 'a', kSize - 1); + char* text = static_cast(block.get()); + text[kSize - 1] = '\0'; + const SkScalar width = paint.measureText(text, kSize); + + SkScalar mm; + const size_t length = paint.breakText(text, kSize, width, &mm); + REPORTER_ASSERT_MESSAGE(reporter, length == kSize, msg); + REPORTER_ASSERT_MESSAGE(reporter, mm == width, msg); +} + +DEF_TEST(PaintBreakText, reporter) { + SkPaint paint; + test_monotonic(reporter, paint, "default"); + test_eq_measure_text(reporter, paint, "default"); + test_long_text(reporter, paint, "default"); + paint.setTextSize(SkIntToScalar(1 << 17)); + test_monotonic(reporter, paint, "huge text size"); + test_eq_measure_text(reporter, paint, "huge text size"); + paint.setTextSize(0); + test_monotonic(reporter, paint, "zero text size"); +}