Bug 160023 - [GTK] Fix some video/canvas tests that should be passing
Summary: [GTK] Fix some video/canvas tests that should be passing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-21 08:07 PDT by Miguel Gomez
Modified: 2016-07-27 09:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.60 KB, patch)
2016-07-21 08:39 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (808.99 KB, application/zip)
2016-07-21 09:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.51 MB, application/zip)
2016-07-21 11:48 PDT, Build Bot
no flags Details
Patch (17.86 KB, patch)
2016-07-22 08:19 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.54 MB, application/zip)
2016-07-22 09:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (810.80 KB, application/zip)
2016-07-22 09:07 PDT, Build Bot
no flags Details
Patch (15.63 KB, patch)
2016-07-26 07:55 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miguel Gomez 2016-07-21 08:07:45 PDT
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.
Comment 1 Miguel Gomez 2016-07-21 08:39:50 PDT
Created attachment 284217 [details]
Patch
Comment 2 Carlos Garcia Campos 2016-07-21 08:56:15 PDT
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.
Comment 3 Build Bot 2016-07-21 09:26:40 PDT
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
Comment 4 Build Bot 2016-07-21 09:26:43 PDT
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
Comment 5 Carlos Garcia Campos 2016-07-21 09:30:05 PDT
Btw, is this really GTK specific? or is it GStreamer specific?
Comment 6 Build Bot 2016-07-21 11:48:18 PDT
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
Comment 7 Build Bot 2016-07-21 11:48:21 PDT
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
Comment 8 Michael Catanzaro 2016-07-21 14:00:49 PDT
Comment on attachment 284217 [details]
Patch

It breaks the tests on the Mac bots; please investigate that!
Comment 9 Chris Dumez 2016-07-21 14:04:56 PDT
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?
Comment 10 Miguel Gomez 2016-07-22 05:58:46 PDT
(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 :)
Comment 11 Michael Catanzaro 2016-07-22 07:59:46 PDT
Another option would be to increase the tolerance level on all platforms to the max needed on any platform.
Comment 12 Miguel Gomez 2016-07-22 08:19:41 PDT
Created attachment 284333 [details]
Patch
Comment 13 Miguel Gomez 2016-07-22 08:23:57 PDT
(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.
Comment 14 Build Bot 2016-07-22 09:05:06 PDT
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
Comment 15 Build Bot 2016-07-22 09:05:10 PDT
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
Comment 16 Build Bot 2016-07-22 09:07:07 PDT
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
Comment 17 Build Bot 2016-07-22 09:07:11 PDT
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
Comment 18 Miguel Gomez 2016-07-22 09:10:08 PDT
Stupid me, DRT uses its own TestRunner, that's why the tests fail there. I'll fix it.
Comment 19 Chris Dumez 2016-07-22 09:10:29 PDT
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.
Comment 20 Carlos Garcia Campos 2016-07-23 02:46:15 PDT
(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.
Comment 21 Miguel Gomez 2016-07-26 07:55:35 PDT
Created attachment 284588 [details]
Patch
Comment 22 Miguel Gomez 2016-07-26 07:59:12 PDT
(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.
Comment 23 Chris Dumez 2016-07-27 08:35:49 PDT
Comment on attachment 284588 [details]
Patch

lgtm
Comment 24 WebKit Commit Bot 2016-07-27 09:01:29 PDT
Comment on attachment 284588 [details]
Patch

Clearing flags on attachment: 284588

Committed r203777: <http://trac.webkit.org/changeset/203777>
Comment 25 WebKit Commit Bot 2016-07-27 09:01:35 PDT
All reviewed patches have been landed.  Closing bug.