Bug 160023 – [GTK] Fix some video/canvas tests that should be passing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

The functionality to draw video frames into a canvas is working, but the tests

- fast/canvas/canvas-createPattern-video-loading.html
- fast/canvas/canvas-createPattern-video-modify.html
- media/video-canvas-createPattern.html

are failing because when comparing the expected pixel colors with the actual ones, the tolerance used is too low.

The default value for this tolerance is 2, but we need to increase it to 6 to make these tests pass.

[Miguel Gomez]

Created attachment 284217 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 284217 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:104
> +    , m_videoCanvasPixelComparisonTolerance(6)
> +#else
> +    , m_videoCanvasPixelComparisonTolerance(2)
>  #endif

I think this should be initialized in platformInitialize() instead of using platform ifdefs. We need this ifdef here because of the timer only.

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:64
> +        var tolerance;
> +        if (window.testRunner)
> +            tolerance = testRunner.videoCanvasPixelComparisonTolerance;
> +        else
> +            tolerance = 2;

Maybe this could be converted into a function and moved to a common .js file, so that we can do videoCanvasPixelComparisonTolerance() directly in the tests instead of duplicating this in all tests.

[Build Bot]

Comment on attachment 284217 [details]
Patch

Attachment 284217 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1725529

New failing tests:
fast/canvas/canvas-createPattern-video-modify.html
media/video-canvas-createPattern.html
fast/canvas/canvas-createPattern-video-loading.html

[Build Bot]

Created attachment 284219 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Carlos Garcia Campos]

Btw, is this really GTK specific? or is it GStreamer specific?

[Build Bot]

Comment on attachment 284217 [details]
Patch

Attachment 284217 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1726390

New failing tests:
fast/canvas/canvas-createPattern-video-modify.html
media/video-canvas-createPattern.html
fast/canvas/canvas-createPattern-video-loading.html

[Build Bot]

Created attachment 284240 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Michael Catanzaro]

Comment on attachment 284217 [details]
Patch

It breaks the tests on the Mac bots; please investigate that!

[Chris Dumez]

Comment on attachment 284217 [details]
Patch

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

It likely fails on Mac-WebKit1 because only WebKitTestRunner was updated, not DumpRenderTree.

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:62
> +            tolerance = testRunner.videoCanvasPixelComparisonTolerance;

This is pretty ugly, can't you simply have a GTK-specification version of these tests under platform/gtk?

[Miguel Gomez]

(In reply to comment #9)
> Comment on attachment 284217 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284217&action=review
> 
> It likely fails on Mac-WebKit1 because only WebKitTestRunner was updated,
> not DumpRenderTree.

Yeah, my fault. I run the tests in elcapitan but forgot about the platforms that use DRT.
 
> > LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:62
> > +            tolerance = testRunner.videoCanvasPixelComparisonTolerance;
> 
> This is pretty ugly, can't you simply have a GTK-specification version of
> these tests under platform/gtk?

The problem with these tests is that we cannot just create specific expectations for gtk, as the test output includes whether it's failure or success, and also two of them include the tolerance value in the output. We would need to add as expectation an output of the tests that indicates failure, which is weird. So we need to modify the tests somehow to increase that tolerance in the comparison somehow when using GStreamer and Cairo.

But then, from what I've been testing, it's not possible to override a general test with another one placed in platform/gtk (at least I haven't found a way to do so, please correct me if I'm wrong). Maybe adding those tests to the TestExpectations as expected failures and then adding new ones in platform/gtk, but this would mean that we would need to copy every new test that gets added that performs a similar comparison, which is a pain.

I think this approach is a nice solution, having a tolerance value defined for each platform and make the tests to use it. This patch had some problems, but I'll send a new improved version that I think should also work for platforms that use DRT.

As I don't have those platforms to test, I'll the EWS test it for me.

Is that ok for you Chris? Or am I missing something? Comments are very welcome :)

[Michael Catanzaro]

Another option would be to increase the tolerance level on all platforms to the max needed on any platform.

[Miguel Gomez]

Created attachment 284333 [details]
Patch

[Miguel Gomez]

(In reply to comment #12)
> Created attachment 284333 [details]
> Patch

I've refactored the usage of the tolerance value in the tests, as suggested by Carlos, and moved the initialization of the value to the platform TestRunners. For bonus points, I've also added the same fix for the EFL port, as it uses gstreamer and cairo as well.

This version should not break the tests in the platforms that use DRT, but let's see EWS's opinion about that.

[Build Bot]

Comment on attachment 284333 [details]
Patch

Attachment 284333 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1734343

New failing tests:
fast/canvas/canvas-createPattern-video-modify.html
media/video-canvas-createPattern.html
fast/canvas/canvas-createPattern-video-loading.html

[Build Bot]

Created attachment 284336 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284333 [details]
Patch

Attachment 284333 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1734383

New failing tests:
fast/canvas/canvas-createPattern-video-modify.html
media/video-canvas-createPattern.html
fast/canvas/canvas-createPattern-video-loading.html

[Build Bot]

Created attachment 284337 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Miguel Gomez]

Stupid me, DRT uses its own TestRunner, that's why the tests fail there. I'll fix it.

[Chris Dumez]

Comment on attachment 284333 [details]
Patch

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

> LayoutTests/media/video-canvas.js:4
> +        return window.testRunner.videoCanvasPixelComparisonTolerance;

Crazy thought, is it possible to detect WebKitGTK from JavaScript (e.g. using user-agent string?). If so, it may be simpler to return 6 if we detect WebKitGTK / WebKitEFL and 2 otherwise. It would be pure JS and would not require any TestRunner functionality.

[Carlos Garcia Campos]

(In reply to comment #19)
> Comment on attachment 284333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284333&action=review
> 
> > LayoutTests/media/video-canvas.js:4
> > +        return window.testRunner.videoCanvasPixelComparisonTolerance;
> 
> Crazy thought, is it possible to detect WebKitGTK from JavaScript (e.g.
> using user-agent string?). If so, it may be simpler to return 6 if we detect
> WebKitGTK / WebKitEFL and 2 otherwise. It would be pure JS and would not
> require any TestRunner functionality.

I don't think so, because of websites parsing UA string and assuming there are only 3 browsers in the world, we pretend to be safari in our UA. Of course we could do some hacks like checking if there's Linux in the UA, and assume that's EFL/GTK+, but I don't see how that's better than exposing the property in the TestRunner. What we could do, though is modify the user agent from WTR using WKPageSetApplicationNameForUserAgent, appending something like WebKitTestRunnerGTK, WebKitTestRunnerEFL, WebKitTestRunnerFoo and then parse the UA string from tests to detect the port. Good thing of this is that it would be more generic solution, we could add helper functions to the common js files isGtk(), isEfl(), etc.

[Miguel Gomez]

Created attachment 284588 [details]
Patch

[Miguel Gomez]

(In reply to comment #21)
> Created attachment 284588 [details]
> Patch

Ok, followed your comments and added a custom platform identifier to the TestRunner's user agent using WKPageSetApplicationNameForUserAgent. I did this for both GTK and EFL.
Then I use js to identify the platform and return the appropriate value for the comparison.

[Chris Dumez]

Comment on attachment 284588 [details]
Patch

lgtm

[WebKit Commit Bot]

Comment on attachment 284588 [details]
Patch

Clearing flags on attachment: 284588

Committed r203777: <http://trac.webkit.org/changeset/203777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.