OSDN Git Service

Add error check for float parsing and fix tests
authorDoris Liu <tianliu@google.com>
Thu, 12 Nov 2015 23:57:45 +0000 (15:57 -0800)
committerDoris Liu <tianliu@google.com>
Fri, 13 Nov 2015 23:58:49 +0000 (15:58 -0800)
Change-Id: Ia4f4863d415536b3796edbcdb83c951b6cff02cf

core/jni/android_util_PathParser.cpp
libs/hwui/PathParser.cpp
libs/hwui/PathParser.h
libs/hwui/VectorDrawablePath.cpp
libs/hwui/microbench/PathParserBench.cpp
libs/hwui/unit_tests/PathParserTests.cpp

index bf6ffc5..245aa0f 100644 (file)
@@ -19,6 +19,7 @@
 #include <PathParser.h>
 #include <SkPath.h>
 
+#include <android/log.h>
 #include "core_jni_helpers.h"
 
 namespace android {
@@ -27,10 +28,14 @@ static bool parseStringForPath(JNIEnv* env, jobject, jlong skPathHandle, jstring
         jint strLength) {
     const char* pathString = env->GetStringUTFChars(inputPathStr, NULL);
     SkPath* skPath = reinterpret_cast<SkPath*>(skPathHandle);
-    bool hasValidData = android::uirenderer::PathParser::parseStringForSkPath(skPath, pathString,
-            strLength);
+
+    android::uirenderer::PathParser::ParseResult result;
+    android::uirenderer::PathParser::parseStringForSkPath(skPath, &result, pathString, strLength);
     env->ReleaseStringUTFChars(inputPathStr, pathString);
-    return hasValidData;
+    if (result.failureOccurred) {
+        ALOGE(result.failureMessage.c_str());
+    }
+    return !result.failureOccurred;
 }
 
 static const JNINativeMethod gMethods[] = {
index 61b9d21..35230ff 100644 (file)
@@ -18,6 +18,7 @@
 
 #include "jni.h"
 
+#include <errno.h>
 #include <utils/Log.h>
 #include <sstream>
 #include <stdlib.h>
@@ -97,14 +98,31 @@ static void extract(int* outEndPosition, bool* outEndWithNegOrDot, const char* s
     *outEndPosition = currentIndex;
 }
 
+static float parseFloat(PathParser::ParseResult* result, const char* startPtr, size_t expectedLength) {
+    char* endPtr = NULL;
+    float currentValue = strtof(startPtr, &endPtr);
+    if ((currentValue == HUGE_VALF || currentValue == -HUGE_VALF) && errno == ERANGE) {
+        result->failureOccurred = true;
+        result->failureMessage = "Float out of range:  ";
+        result->failureMessage.append(startPtr, expectedLength);
+    }
+    if (currentValue == 0 && endPtr == startPtr) {
+        // No conversion is done.
+        result->failureOccurred = true;
+        result->failureMessage = "Float format error when parsing: ";
+        result->failureMessage.append(startPtr, expectedLength);
+    }
+    return currentValue;
+}
+
 /**
-* Parse the floats in the string.
-* This is an optimized version of parseFloat(s.split(",|\\s"));
-*
-* @param s the string containing a command and list of floats
-* @return array of floats
-*/
-static void getFloats(std::vector<float>* outPoints, const char* pathStr, int start, int end) {
+ * Parse the floats in the string.
+ *
+ * @param s the string containing a command and list of floats
+ * @return true on success
+ */
+static void getFloats(std::vector<float>* outPoints, PathParser::ParseResult* result,
+        const char* pathStr, int start, int end) {
 
     if (pathStr[start] == 'z' || pathStr[start] == 'Z') {
         return;
@@ -120,7 +138,12 @@ static void getFloats(std::vector<float>* outPoints, const char* pathStr, int st
         extract(&endPosition, &endWithNegOrDot, pathStr, startPosition, end);
 
         if (startPosition < endPosition) {
-            outPoints->push_back(strtof(&pathStr[startPosition], NULL));
+            float currentValue = parseFloat(result, &pathStr[startPosition],
+                    end - startPosition);
+            if (result->failureOccurred) {
+                return;
+            }
+            outPoints->push_back(currentValue);
         }
 
         if (endWithNegOrDot) {
@@ -130,10 +153,14 @@ static void getFloats(std::vector<float>* outPoints, const char* pathStr, int st
             startPosition = endPosition + 1;
         }
     }
+    return;
 }
 
-void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size_t strLen) {
+void PathParser::getPathDataFromString(PathData* data, ParseResult* result,
+        const char* pathStr, size_t strLen) {
     if (pathStr == NULL) {
+        result->failureOccurred = true;
+        result->failureMessage = "Path string cannot be NULL.";
         return;
     }
 
@@ -143,7 +170,10 @@ void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size
     while (end < strLen) {
         end = nextStart(pathStr, strLen, end);
         std::vector<float> points;
-        getFloats(&points, pathStr, start, end);
+        getFloats(&points, result, pathStr, start, end);
+        if (result->failureOccurred) {
+            return;
+        }
         data->verbs.push_back(pathStr[start]);
         data->verbSizes.push_back(points.size());
         data->points.insert(data->points.end(), points.begin(), points.end());
@@ -151,16 +181,11 @@ void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size
         end++;
     }
 
-    if ((end - start) == 1 && pathStr[start] != '\0') {
+    if ((end - start) == 1 && start < strLen) {
         data->verbs.push_back(pathStr[start]);
         data->verbSizes.push_back(0);
     }
-
-    int i = 0;
-    while(pathStr[i] != '\0') {
-       i++;
-    }
-
+    return;
 }
 
 void PathParser::dump(const PathData& data) {
@@ -169,6 +194,7 @@ void PathParser::dump(const PathData& data) {
     for (size_t i = 0; i < data.verbs.size(); i++) {
         std::ostringstream os;
         os << data.verbs[i];
+        os << ", verb size: " << data.verbSizes[i];
         for (size_t j = 0; j < data.verbSizes[i]; j++) {
             os << " " << data.points[start + j];
         }
@@ -183,16 +209,20 @@ void PathParser::dump(const PathData& data) {
     ALOGD("points are : %s", os.str().c_str());
 }
 
-bool PathParser::parseStringForSkPath(SkPath* skPath, const char* pathStr, size_t strLen) {
+void PathParser::parseStringForSkPath(SkPath* skPath, ParseResult* result, const char* pathStr, size_t strLen) {
     PathData pathData;
-    getPathDataFromString(&pathData, pathStr, strLen);
-
+    getPathDataFromString(&pathData, result, pathStr, strLen);
+    if (result->failureOccurred) {
+        return;
+    }
     // Check if there is valid data coming out of parsing the string.
     if (pathData.verbs.size() == 0) {
-        return false;
+        result->failureOccurred = true;
+        result->failureMessage = "No verbs found in the string for pathData";
+        return;
     }
     VectorDrawablePath::verbsToPath(skPath, &pathData);
-    return true;
+    return;
 }
 
 }; // namespace uirenderer
index d30bb0f..a9c1e60 100644 (file)
 #include <android/log.h>
 #include <cutils/compiler.h>
 
+#include <string>
+
 namespace android {
 namespace uirenderer {
 
+
 class PathParser {
 public:
+    struct ANDROID_API ParseResult {
+        bool failureOccurred = false;
+        std::string failureMessage;
+    };
     /**
      * Parse the string literal and create a Skia Path. Return true on success.
      */
-    ANDROID_API static bool parseStringForSkPath(SkPath* outPath, const char* pathStr,
-            size_t strLength);
-    static void getPathDataFromString(PathData* outData, const char* pathStr, size_t strLength);
+    ANDROID_API static void parseStringForSkPath(SkPath* outPath, ParseResult* result,
+            const char* pathStr, size_t strLength);
+    static void getPathDataFromString(PathData* outData, ParseResult* result,
+            const char* pathStr, size_t strLength);
     static void dump(const PathData& data);
 };
 
index 115435c..05ea2da 100644 (file)
@@ -37,8 +37,11 @@ public:
 };
 
 VectorDrawablePath::VectorDrawablePath(const char* pathStr, size_t strLength) {
-    PathParser::getPathDataFromString(&mData, pathStr, strLength);
-    verbsToPath(&mSkPath, &mData);
+    PathParser::ParseResult result;
+    PathParser::getPathDataFromString(&mData, &result, pathStr, strLength);
+    if (!result.failureOccurred) {
+        verbsToPath(&mSkPath, &mData);
+    }
 }
 
 VectorDrawablePath::VectorDrawablePath(const PathData& data) {
@@ -80,7 +83,7 @@ void VectorDrawablePath::verbsToPath(SkPath* outPath, const PathData* data) {
     for (unsigned int i = 0; i < data->verbs.size(); i++) {
         size_t verbSize = data->verbSizes[i];
         resolver.addCommand(outPath, previousCommand, data->verbs[i], &data->points, start,
-                start + verbSize - 1u);
+                start + verbSize);
         previousCommand = data->verbs[i];
         start += verbSize;
     }
@@ -254,7 +257,7 @@ static void drawArc(SkPath* p,
     arcToBezier(p, cx, cy, a, b, x0, y0, thetaD, eta0, sweep);
 }
 
-
+// Use the given verb, and points in the range [start, end) to insert a command into the SkPath.
 void PathResolver::addCommand(SkPath* outPath, char previousCmd,
         char cmd, const std::vector<float>* points, size_t start, size_t end) {
 
@@ -305,7 +308,7 @@ void PathResolver::addCommand(SkPath* outPath, char previousCmd,
         break;
     }
 
-    for (unsigned int k = start; k <= end; k += incr) {
+    for (unsigned int k = start; k < end; k += incr) {
         switch (cmd) {
         case 'm': // moveto - Start a new sub-path (relative)
             currentX += points->at(k + 0);
index 198035e..171078d 100644 (file)
@@ -28,9 +28,10 @@ void BM_PathParser_parseStringPath::Run(int iter) {
     const char* pathString = "M 1 1 m 2 2, l 3 3 L 3 3 H 4 h4 V5 v5, Q6 6 6 6 q 6 6 6 6t 7 7 T 7 7 C 8 8 8 8 8 8 c 8 8 8 8 8 8 S 9 9 9 9 s 9 9 9 9 A 10 10 0 1 1 10 10 a 10 10 0 1 1 10 10";
     SkPath skPath;
     size_t length = strlen(pathString);
+    PathParser::ParseResult result;
     StartBenchmarkTiming();
     for (int i = 0; i < iter; i++) {
-        PathParser::parseStringForSkPath(&skPath, pathString, length);
+        PathParser::parseStringForSkPath(&skPath, &result, pathString, length);
     }
     StopBenchmarkTiming();
 }
index 60ea219..c99d7b0 100644 (file)
@@ -164,16 +164,37 @@ const static TestData sTestDataSet[] = {
 
 };
 
+struct StringPath {
+    const char* stringPath;
+    bool isValid;
+};
+
+const StringPath sStringPaths[] = {
+    {"3e...3", false},
+    {"L.M.F.A.O", false},
+    {"m 1 1", true},
+    {"z", true},
+    {"1-2e34567", false}
+};
+
 TEST(PathParser, parseStringForData) {
     for (TestData testData: sTestDataSet) {
+        PathParser::ParseResult result;
         // Test generated path data against the given data.
         PathData pathData;
         size_t length = strlen(testData.pathString);
-        PathParser::getPathDataFromString(&pathData, testData.pathString, length);
-        PathParser::dump(pathData);
+        PathParser::getPathDataFromString(&pathData, &result, testData.pathString, length);
         EXPECT_EQ(testData.pathData, pathData);
     }
 
+    for (StringPath stringPath : sStringPaths) {
+        PathParser::ParseResult result;
+        PathData pathData;
+        SkPath skPath;
+        PathParser::getPathDataFromString(&pathData, &result,
+                stringPath.stringPath, strlen(stringPath.stringPath));
+        EXPECT_EQ(stringPath.isValid, !result.failureOccurred);
+    }
 }
 
 TEST(PathParser, createSkPathFromPathData) {
@@ -188,21 +209,25 @@ TEST(PathParser, createSkPathFromPathData) {
 
 TEST(PathParser, parseStringForSkPath) {
     for (TestData testData: sTestDataSet) {
+        PathParser::ParseResult result;
         size_t length = strlen(testData.pathString);
         // Check the return value as well as the SkPath generated.
         SkPath actualPath;
-        bool hasValidData = PathParser::parseStringForSkPath(&actualPath, testData.pathString,
-                length);
+        PathParser::parseStringForSkPath(&actualPath, &result, testData.pathString, length);
+        bool hasValidData = !result.failureOccurred;
         EXPECT_EQ(hasValidData, testData.pathData.verbs.size() > 0);
         SkPath expectedPath;
         testData.skPathLamda(&expectedPath);
         EXPECT_EQ(expectedPath, actualPath);
     }
-    SkPath path;
-    EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "l", 1));
-    EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "1 1", 3));
-    EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "LMFAO", 5));
-    EXPECT_TRUE(PathParser::parseStringForSkPath(&path, "m1 1", 4));
+
+    for (StringPath stringPath : sStringPaths) {
+        PathParser::ParseResult result;
+        SkPath skPath;
+        PathParser::parseStringForSkPath(&skPath, &result, stringPath.stringPath,
+                strlen(stringPath.stringPath));
+        EXPECT_EQ(stringPath.isValid, !result.failureOccurred);
+    }
 }
 
 }; // namespace uirenderer