[EFL] Add ImageDiff implementation.
Created attachment 95777 [details] Patch
Attachment 95777 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/e..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 95779 [details] Patch
Comment on attachment 95779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95779&action=review Same troubles. We're 2011, not 1980. This code doesn't need to compile on my old DEC. > Tools/DumpRenderTree/efl/ImageDiff.cpp:144 > + else if (getenv("TEMP")) Do we need to release returns from getenv? > Tools/DumpRenderTree/efl/ImageDiff.cpp:165 > + int tempImageFd; Sigh.
(In reply to comment #4) > (From update of attachment 95779 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95779&action=review > > Same troubles. We're 2011, not 1980. This code doesn't need to compile on > my old DEC. > I agree so much with you, I'm probably making some t-shirts with these words. Ping me if you want one. :P > > > Tools/DumpRenderTree/efl/ImageDiff.cpp:144 > > + else if (getenv("TEMP")) > > Do we need to release returns from getenv? > Nope -- strings returned by getenv() can't be modified in any way, as they're pointing to the process' environment. It should really be returning const char*, but I guess they didn't need const pointers on a PDP. :) > > Tools/DumpRenderTree/efl/ImageDiff.cpp:165 > > + int tempImageFd; > > Sigh. Yeah, I know. Unfortunately, Evas will only allow saving canvas objects to a file. I could close the file descriptor returned by mkstemps(), but that would create a (highly unlikely) race condition if the temporary file got removed somehow. I could dump directly to the standard output, but then I'd have no way to know in advance the file size. Couldn't think of anything much different from this. Unless, of course, you're sighing to something else and I missed your point.
Created attachment 97460 [details] ImageDiff, take 2 New attempt at upstreaming ImageDiff. Hopefully all the concerns have been solved, and there are no free() calls :)
CC'ing eseidel.
Comment on attachment 97460 [details] ImageDiff, take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=97460&action=review It looks OK, not great. > Tools/DumpRenderTree/efl/ImageDiff.cpp:83 > + printf("Error, test and reference image have different properties.\n"); I think you meant different sizes. > Tools/DumpRenderTree/efl/ImageDiff.cpp:105 > + for (int x = 0; x < width; x++) { > + for (int y = 0; y < height; y++) { This entire loop could be a helper. > Tools/DumpRenderTree/efl/ImageDiff.cpp:112 > + float red = (actualPixel[0] - basePixel[0]) / std::max<float>(255 - basePixel[0], basePixel[0]); > + float green = (actualPixel[1] - basePixel[1]) / std::max<float>(255 - basePixel[1], basePixel[1]); > + float blue = (actualPixel[2] - basePixel[2]) / std::max<float>(255 - basePixel[2], basePixel[2]); > + float alpha = (actualPixel[3] - basePixel[3]) / std::max<float>(255 - basePixel[3], basePixel[3]); I woudl have written/used a helper function here. > Tools/DumpRenderTree/efl/ImageDiff.cpp:129 > + // Compute the difference as a percentage combining both the number of > + // different pixels and their difference amount i.e. the average distance > + // over the entire image > + float difference = 0; > + if (count > 0.0f) This whole calculation could be a helper. > Tools/DumpRenderTree/efl/ImageDiff.cpp:147 > + if (getenv("TMPDIR")) > + snprintf(fileName, fileNameLength, "%s/ImageDiffXXXXXX.png", getenv("TMPDIR")); > + else if (getenv("TEMP")) > + snprintf(fileName, fileNameLength, "%s/ImageDiffXXXXXX.png", getenv("TEMP")); Really? Do we need to copy/paste those lines just to change out TMPDIR vs. TEMP? char* tempDirectory = getenv("TMPDIR"); if (!tempDirectory) tempDirectory = getEnv("TEMP"); if (tempDirectory) // printf else { // other thing. > Tools/DumpRenderTree/efl/ImageDiff.cpp:255 > + ecore_evas_free(ecoreEvas); manual memory management sucks. > Tools/DumpRenderTree/efl/ImageDiff.cpp:290 > + abortWithErrorMessage("could not read image from standard input"); Is it EFL style not to use capitals or periods? > Tools/DumpRenderTree/efl/ImageDiff.cpp:306 > + evas_object_del(*actualImage); > + evas_object_del(*baselineImage); manual memory management sucks.
(In reply to comment #8) > (From update of attachment 97460 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97460&action=review > > It looks OK, not great. > > > Tools/DumpRenderTree/efl/ImageDiff.cpp:83 > > + printf("Error, test and reference image have different properties.\n"); > > I think you meant different sizes. That's what the cg, gtk, qt and win DRTs say, so we were just being consistent :) Plus it is also considering the alpha besides the size in the comparison. > > Tools/DumpRenderTree/efl/ImageDiff.cpp:105 > > + for (int x = 0; x < width; x++) { > > + for (int y = 0; y < height; y++) { > > This entire loop could be a helper. IMHO it would make the code unnecessarily bigger -- I would need to pass at least five different parameters to it (buffer, width, height, diffPixels and rowStride) just to put the loop elsewhere with no apparent gain. I'm fine with it if you really feel it should be done this way, though. > > + if (getenv("TMPDIR")) > > + snprintf(fileName, fileNameLength, "%s/ImageDiffXXXXXX.png", getenv("TMPDIR")); > > + else if (getenv("TEMP")) > > + snprintf(fileName, fileNameLength, "%s/ImageDiffXXXXXX.png", getenv("TEMP")); > > Really? Do we need to copy/paste those lines just to change out TMPDIR vs. TEMP? > > char* tempDirectory = getenv("TMPDIR"); > if (!tempDirectory) > tempDirectory = getEnv("TEMP"); > if (tempDirectory) > // printf > else { > // other thing. OK, done. > > Tools/DumpRenderTree/efl/ImageDiff.cpp:290 > > + abortWithErrorMessage("could not read image from standard input"); > > Is it EFL style not to use capitals or periods? I've added periods to the error messages. They don't have capitals because they're appended to another string in abortWithErrorMessage(). > > Tools/DumpRenderTree/efl/ > > + ecore_evas_free(ecoreEvas); > > manual memory management sucks. > > > Tools/DumpRenderTree/efl/ImageDiff.cpp:306 > > + evas_object_del(*actualImage); > > + evas_object_del(*baselineImage); > > manual memory management sucks. I think it's better to talk about it again in bug 62642 -- please see comment 28 in it.
Created attachment 97642 [details] Patch
(In reply to comment #10) > Created an attachment (id=97642) [details] > Patch Third time is the charm. Hopefully all concerns raised by Eric have been taken care of.
Comment on attachment 97642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97642&action=review Thank you. > Tools/DumpRenderTree/efl/ImageDiff.cpp:80 > + diffPixel[Red] = diffPixel[Green] = diffPixel[Blue] = *buffer++; Oh, this is nice. thank you. > Tools/DumpRenderTree/efl/ImageDiff.cpp:100 > + const float red = computeDistanceBetweenPixelComponents(actualPixel, basePixel, Red); Very nice! > Tools/DumpRenderTree/efl/ImageDiff.cpp:118 > + OwnArrayPtr<unsigned char> diffBuffer = adoptArrayPtr(new unsigned char[width * height]); I might have typedefed unsigned char given how often you're typing it in this file. But this looks fine.
Comment on attachment 97642 [details] Patch Clearing flags on attachment: 97642 Committed r89175: <http://trac.webkit.org/changeset/89175>
All reviewed patches have been landed. Closing bug.