Bug 61942 – [EFL] Add ImageDiff implementation.

• Component: New Bugs
• Status: RESOLVED FIXED

[Leandro Pereira]

[EFL] Add ImageDiff implementation.

[Leandro Pereira]

Created attachment 95777 [details]
Patch

[WebKit Review Bot]

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.

[Leandro Pereira]

Created attachment 95779 [details]
Patch

[Eric Seidel (no email)]

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.

[Leandro Pereira]

(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.

[Raphael Kubo da Costa (:rakuco)]

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 :)

[Raphael Kubo da Costa (:rakuco)]

CC'ing eseidel.

[Eric Seidel (no email)]

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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Leandro Pereira]

Created attachment 97642 [details]
Patch

[Leandro Pereira]

(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.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 97642 [details]
Patch

Clearing flags on attachment: 97642

Committed r89175: <http://trac.webkit.org/changeset/89175>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.