Bug 170608 – ImageDiff: Add CG implementation for new ImageDiff

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Garcia Campos]

In bug #85299 there's a patch to move GTK+ port to use a cairo only image diff. While working on it I realized we have 3 implementations with a lot of duplicated code, so as part of the patch I refactored the code to share all the common code. On top of that patch adding the CG implementations is easy.

[Carlos Garcia Campos]

Created attachment 306522 [details]
Patch

[Carlos Garcia Campos]

Created attachment 306683 [details]
Rebased patch

[Carlos Garcia Campos]

Created attachment 306687 [details]
Rebased patch

[Carlos Garcia Campos]

hmm, the debug failure is because of the ASSERT, doesn't image diff link to WTF already? or am I missing a header include? I can easily fix the build by removing the ASSERT, I guess.

[Carlos Garcia Campos]

Created attachment 306807 [details]
Fix debug build

I have no idea why it fails to build so I've just removed the assert.

[Carlos Garcia Campos]

Created attachment 306812 [details]
Include win changes

I've realized that the patch would break windows, because it removes the ImageDiffCG from DRT. So, this new patch also includes the windows changed that was going to do in a follow up patch. I have no way to test windows changes, so I don't know whether it works or not.

[Build Bot]

Attachment 306812 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Ping reviewers

[Brent Fulgham]

This builds and seems to work locally.

[Brent Fulgham]

Comment on attachment 306812 [details]
Include win changes

Looks good, and works properly on Windows (at least locally). Let's land it.

[Carlos Garcia Campos]

(In reply to Brent Fulgham from comment #10)
> Comment on attachment 306812 [details]
> Include win changes
> 
> Looks good, and works properly on Windows (at least locally). Let's land it.

Thanks! I hope it also works in WinCairo . . .

[WebKit Commit Bot]

Comment on attachment 306812 [details]
Include win changes

Rejecting attachment 306812 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
eC /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.build/Objects-normal/x86_64/PlatformImage.o /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.build/Objects-normal/x86_64/ImageDiff.o /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(2 failures)

Full output: http://webkit-queues.webkit.org/results/3582981

[Carlos Garcia Campos]

(In reply to WebKit Commit Bot from comment #12)
> Comment on attachment 306812 [details]
> Include win changes
> 
> Rejecting attachment 306812 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build',
> '--no-clean', '--no-update', '--build-style=release', '--port=mac']"
> exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
> 
> Last 500 characters of output:
> eC
> /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.
> build/Objects-normal/x86_64/PlatformImage.o
> /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++
> com.apple.compilers.llvm.clang.1_0.compiler
> 	CompileC
> /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.
> build/Objects-normal/x86_64/ImageDiff.o
> /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++
> com.apple.compilers.llvm.clang.1_0.compiler
> (2 failures)
> 
> Full output: http://webkit-queues.webkit.org/results/3582981

#include <WebCore/PlatformExportMacros.h>
         ^
1 error generated.

I have no idea why this is failing to build now.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to WebKit Commit Bot from comment #12)
> > Comment on attachment 306812 [details]
> > Include win changes
> > 
> > Rejecting attachment 306812 [details] from commit-queue.
> > 
> > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> > '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build',
> > '--no-clean', '--no-update', '--build-style=release', '--port=mac']"
> > exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
> > 
> > Last 500 characters of output:
> > eC
> > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.
> > build/Objects-normal/x86_64/PlatformImage.o
> > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++
> > com.apple.compilers.llvm.clang.1_0.compiler
> > 	CompileC
> > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.
> > build/Objects-normal/x86_64/ImageDiff.o
> > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++
> > com.apple.compilers.llvm.clang.1_0.compiler
> > (2 failures)
> > 
> > Full output: http://webkit-queues.webkit.org/results/3582981
> 
> #include <WebCore/PlatformExportMacros.h>
>          ^
> 1 error generated.
> 
> I have no idea why this is failing to build now.

Could someone help me with this? I don't know why this is failing now. It worked for me locally and in EWS, but now it's failing, and I don't know what have changed since then.

[Alex Christensen]

It builds fine for me locally, and EWS liked it.  I'd say commit it and see what happens.  I'm not sure why the commit queue didn't like it.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #15)
> It builds fine for me locally, and EWS liked it.  I'd say commit it and see
> what happens.  I'm not sure why the commit queue didn't like it.

The thing is that I tried yesterday in a mac and got the same error than EWS.

[Carlos Garcia Campos]

I'm trying to rebase this patch now that ImageDiff has been moved from DRT to its own project, but I don't see the ImageDiff now in Xcode to do the changes. Is there any magic I need to do in Xcode for the new project to show up?

[Carlos Garcia Campos]

Manually adding a new group to WebKit.xcworkspace/contents.xcworkspacedata made it appear in XCode.

[Carlos Garcia Campos]

Created attachment 309491 [details]
Rebased patch

[Build Bot]

Attachment 309491 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 309491 [details]
Rebased patch

Attachment 309491 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3705722

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html
fast/forms/file/intrinsic-min-width-overrides-width.html

[Build Bot]

Created attachment 309498 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Created attachment 309500 [details]
Rebase again after r216504

[Build Bot]

Attachment 309500 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 309501 [details]
Try to fix win build

[Build Bot]

Attachment 309501 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 309491 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309491&action=review

r=me.  There's room for improvement, but this is an improvement because it unites the code from the different platforms.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:116
> +PlatformImage::PlatformImage(CGImageRef image)
> +    : m_image(image)
> +{
> +}
> +
> +PlatformImage::~PlatformImage()
> +{
> +    CFRelease(m_image);
> +    free(m_buffer);
> +}

I think it would be nicer to have the constructor call CFRetain and have the callers properly release their images.  We shouldn't rely on the caller knowing that this object adopts it without any annotation.  I guess it's just a small tool that nobody looks at often, but it should still make sense.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:145
> +    m_buffer = calloc(height, rowBytes);

It would be nice if we could use std::unique_ptr<uint8_t[]> or something so we could avoid all this manual memory management.  Just glancing at this makes it hard to tell if we should check m_buffer for something already there and free it if there is.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #27)
> Comment on attachment 309491 [details]
> Rebased patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309491&action=review
> 
> r=me.  There's room for improvement, but this is an improvement because it
> unites the code from the different platforms.

Yes, I tried to keep existing code as mush as possible, just removing all the duplications.

> > Tools/ImageDiff/cg/PlatformImageCG.cpp:116
> > +PlatformImage::PlatformImage(CGImageRef image)
> > +    : m_image(image)
> > +{
> > +}
> > +
> > +PlatformImage::~PlatformImage()
> > +{
> > +    CFRelease(m_image);
> > +    free(m_buffer);
> > +}
> 
> I think it would be nicer to have the constructor call CFRetain and have the
> callers properly release their images.  We shouldn't rely on the caller
> knowing that this object adopts it without any annotation.  I guess it's
> just a small tool that nobody looks at often, but it should still make sense.

My previous patch used RetainPtr everywhere and the constructor received a &&, but then ImageDiff was made standalone and WTF dependency removed, so I had to go back to manually release everything as existing code does. Note that I'm not familiar with CG APIs.

> > Tools/ImageDiff/cg/PlatformImageCG.cpp:145
> > +    m_buffer = calloc(height, rowBytes);
> 
> It would be nice if we could use std::unique_ptr<uint8_t[]> or something so
> we could avoid all this manual memory management.  Just glancing at this
> makes it hard to tell if we should check m_buffer for something already
> there and free it if there is.

I agree, but again this is just moving existing code. We can add further improvements in follow up patches. I don't think we need to null check m_buffer since free is null safe and m_buffer is initialized to nullptr in the header.

[Carlos Garcia Campos]

Created attachment 309510 [details]
Try to fix win build

[Build Bot]

Attachment 309510 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 309510 [details]
Try to fix win build

Clearing flags on attachment: 309510

Committed r216576: <http://trac.webkit.org/changeset/216576>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.