85292 2012-05-01 09:32:39 -0700 ImageDiff should be run inside a properly established environment 2012-05-03 05:12:35 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zan zan abarth dpranke mrobinson ojan pnormand webkit.review.bot oldest_to_newest 613463 0 zan 2012-05-01 09:32:39 -0700 As with DumpRenderTree, WebKitTestRunner and even http and websocket servers, ImageDiff should be ran inside an environment that was created with port.setup_environment_for_server() function. 613467 1 zan 2012-05-01 09:35:23 -0700 (In reply to comment #0) > As with DumpRenderTree, WebKitTestRunner and even http and websocket servers, ImageDiff should be ran inside an environment that was created with port.setup_environment_for_server() function. Specifically for the Gtk port, the lack of doing this resulted in ImageDiff crashing because it was not being able to open the display. 613469 2 139640 zan 2012-05-01 09:37:32 -0700 Created attachment 139640 Patch 613482 3 139640 pnormand 2012-05-01 09:45:53 -0700 Comment on attachment 139640 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139640&action=review This approach makes sense to me, however: > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:183 > + environment = self._port.setup_environ_for_server('ImageDiff') I'm afraid this doesn't set the DISPLAY env var in GTK :( It's set only in the GtkDriver._start method or did I miss something? 613496 4 zan 2012-05-01 10:12:57 -0700 (In reply to comment #3) > (From update of attachment 139640 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139640&action=review > > This approach makes sense to me, however: > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:183 > > + environment = self._port.setup_environ_for_server('ImageDiff') > > I'm afraid this doesn't set the DISPLAY env var in GTK :( It's set only in the GtkDriver._start method or did I miss something? It uses the display in which new-run-webkit-tests is run. This is done for all the ports in base.py[1]. In the case of the Gtk bots, I can see both 64-bit bots having a DISPLAY env of value ':10', while the 32-bit bot doesn't have that env set (which might cause problems). In GtkDriver, a new display is spawn for each driver starting, with none of these drivers being connected to the WebKitPort object, so it might be difficult to get a proper display number. Also, I just realized the patch is wrong - setup_environ_for_server should be called on self, not self._port. [1] http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py#L759 613499 5 139647 zan 2012-05-01 10:17:31 -0700 Created attachment 139647 Patch Fixed patch 613509 6 pnormand 2012-05-01 10:33:55 -0700 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 139640 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=139640&action=review > > > > This approach makes sense to me, however: > > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:183 > > > + environment = self._port.setup_environ_for_server('ImageDiff') > > > > I'm afraid this doesn't set the DISPLAY env var in GTK :( It's set only in the GtkDriver._start method or did I miss something? > > It uses the display in which new-run-webkit-tests is run. This is done for all the ports in base.py[1]. In the case of the Gtk bots, I can see both 64-bit bots having a DISPLAY env of value ':10', while the 32-bit bot doesn't have that env set (which might cause problems). > They shouldn't have a pre-configured DISPLAY, I think. What about spawning a new xvfb process if needed by ImageDiff? The :1 value used in base.py looks hasardous, how are we sure a X server is running on :1 ? > In GtkDriver, a new display is spawn for each driver starting, with none of these drivers being connected to the WebKitPort object, so it might be difficult to get a proper display number. > Maybe that code can be refactored then. 613555 7 mrobinson 2012-05-01 11:32:32 -0700 (In reply to comment #1) > (In reply to comment #0) > > As with DumpRenderTree, WebKitTestRunner and even http and websocket servers, ImageDiff should be ran inside an environment that was created with port.setup_environment_for_server() function. > > Specifically for the Gtk port, the lack of doing this resulted in ImageDiff crashing because it was not being able to open the display. Hrm. ImageDiff should probably be rewritten such that it doesn't need an X11 display. 614015 8 zan 2012-05-01 23:14:34 -0700 (In reply to comment #7) > (In reply to comment #1) > > (In reply to comment #0) > > > As with DumpRenderTree, WebKitTestRunner and even http and websocket servers, ImageDiff should be ran inside an environment that was created with port.setup_environment_for_server() function. > > > > Specifically for the Gtk port, the lack of doing this resulted in ImageDiff crashing because it was not being able to open the display. > > Hrm. ImageDiff should probably be rewritten such that it doesn't need an X11 display. While I find the current implementation with the GdkPixbufLoader extremely elegant, I guess the alternative would be to use Cairo (and possibly share most of the code with the Win port). Another possibility is to rewrite the ImageDiff tool in Python, making it platform-independent. Looking across current ImageDiff implementations, only Chromium's differs a bit in functionality, while basically all are using the same difference algorithm. While it would be nice to unify the behavior in a cross-platform implementation, I don't think this would be that simple without introducing another python dependency on a specific imaging library. While the diff algorithm is basically the same in every current implementation, the PNG data is still gathered in a platform-dependent way, making that data possibly vary in the format of row strides, alpha channels etc, making way to problems. As a current workaround, I've modified the GtkDriver to contain a static list of open displays, with GtkPort, when setting up environment for ImageDiff, choosing the latest opened display (an open display should always be available when running ImageDiff). But it's just a work-in-progress workaround that's to be abandoned if we can soon get to a consensus on what's to be done. 614383 9 ojan 2012-05-02 09:54:46 -0700 (In reply to comment #8) > Another possibility is to rewrite the ImageDiff tool in Python, making it platform-independent. Looking across current ImageDiff implementations, only Chromium's differs a bit in functionality, while basically all are using the same difference algorithm. Historically, the concern with a python implementation is performance. Having this code be platform-independent would obviously be great. If someone puts together a python implementation and shows that it has negligible impact on the time it takes to run the full test suite, then I think everyone would be happier with that. 614600 10 dpranke 2012-05-02 13:26:45 -0700 I've been playing around with pure-python PNG logic, and the best I've found seems to be buggy so far (at least for computing the checksums), and is definitely slower (but I don't know how significant the impact is yet). See bug 68039 for more on this. 614602 11 dpranke 2012-05-02 13:27:40 -0700 Also, for the record, this change looks fine to me but I'll let mrobinson or pnormand give the final blessing if they like. 615169 12 zan 2012-05-03 05:06:06 -0700 Thanks for the review. It so happens that after a bit more looking around I've found that Gtk's ImageDiff crashes can easily be fixed. I've opened bug #85476 for that. 615175 13 139647 zan 2012-05-03 05:12:27 -0700 Comment on attachment 139647 Patch Clearing flags on attachment: 139647 Committed r115962: <http://trac.webkit.org/changeset/115962> 615177 14 zan 2012-05-03 05:12:35 -0700 All reviewed patches have been landed. Closing bug. 139640 2012-05-01 09:37:32 -0700 2012-05-01 10:17:21 -0700 Patch bug-85292-20120501183731.patch text/plain 1718 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1NzI4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOTgyY2Q0YzU4ZmQ0MjRiZjNiZGUzNGQ1NDYxNDZlYWQ3 ZmM3NDZkMS4uZGFlMDRlYzMyODE4ODk4NGE0NWU3MDE0MTJmOWNkODRjM2Q3YTRmMiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2 IEBACisyMDEyLTA1LTAxICBaYW4gRG9iZXJzZWsgIDx6YW5kb2JlcnNla0BnbWFpbC5jb20+CisK KyAgICAgICAgSW1hZ2VEaWZmIHNob3VsZCBiZSBydW4gaW5zaWRlIGEgcHJvcGVybHkgZXN0YWJs aXNoZWQgZW52aXJvbm1lbnQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTg1MjkyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgV2hlbiBjcmVhdGluZyB0aGUgSW1hZ2VEaWZmIHNlcnZlciBwcm9jZXNzLCBwYXNz IGFsb25nIGEgZnJlc2hseS1zZXQtdXAgZW52aXJvbm1lbnQKKyAgICAgICAgaW4gd2hpY2ggdGhl IHByb2Nlc3Mgc2hvdWxkIGJlIGV4ZWN1dGVkIGluLgorCisgICAgICAgICogU2NyaXB0cy93ZWJr aXRweS9sYXlvdXRfdGVzdHMvcG9ydC93ZWJraXQucHk6CisgICAgICAgIChXZWJLaXRQb3J0Ll9z dGFydF9pbWFnZV9kaWZmX3Byb2Nlc3MpOgorCiAyMDEyLTA1LTAxICBLZW5uZXRoIFJvaGRlIENo cmlzdGlhbnNlbiAgPGtlbm5ldGhAd2Via2l0Lm9yZz4KIAogICAgICAgICBbUXRdIEFkZCBhbiBl eHBlcmltZW50YWwgZXh0ZW5zaW9uIHRvIHNldCB0aGUgbWluLiBjb250ZW50cyB3aWR0aApkaWZm IC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC93ZWJraXQu cHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtpdC5weQpp bmRleCA3OWY5YWQwOGU1NWRlZTczZDNiMjI3ZmU5YjlhMzZiYzM4YmFlOWI1Li41YjBkOGUyNDk3 MmYxYjJmYTEzMzYyM2NhODI0NjZmN2M2MDAzNWYzIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtpdC5weQorKysgYi9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtpdC5weQpAQCAtMTgwLDcgKzE4MCw4IEBA IGNsYXNzIFdlYktpdFBvcnQoUG9ydCk6CiAgICAgICAgICAgICBlbHNlOgogICAgICAgICAgICAg ICAgIHRvbGVyYW5jZSA9IDAuMQogICAgICAgICBjb21tYW5kID0gW3NlbGYuX3BhdGhfdG9faW1h Z2VfZGlmZigpLCAnLS10b2xlcmFuY2UnLCBzdHIodG9sZXJhbmNlKV0KLSAgICAgICAgcHJvY2Vz cyA9IHNlcnZlcl9wcm9jZXNzLlNlcnZlclByb2Nlc3Moc2VsZiwgJ0ltYWdlRGlmZicsIGNvbW1h bmQpCisgICAgICAgIGVudmlyb25tZW50ID0gc2VsZi5fcG9ydC5zZXR1cF9lbnZpcm9uX2Zvcl9z ZXJ2ZXIoJ0ltYWdlRGlmZicpCisgICAgICAgIHByb2Nlc3MgPSBzZXJ2ZXJfcHJvY2Vzcy5TZXJ2 ZXJQcm9jZXNzKHNlbGYsICdJbWFnZURpZmYnLCBjb21tYW5kLCBlbnZpcm9ubWVudCkKIAogICAg ICAgICBwcm9jZXNzLndyaXRlKCdDb250ZW50LUxlbmd0aDogJWRcbiVzQ29udGVudC1MZW5ndGg6 ICVkXG4lcycgJSAoCiAgICAgICAgICAgICBsZW4oYWN0dWFsX2NvbnRlbnRzKSwgYWN0dWFsX2Nv bnRlbnRzLAo= 139647 2012-05-01 10:17:31 -0700 2012-05-03 05:12:27 -0700 Patch bug-85292-20120501191730.patch text/plain 1746 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1NzMzCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggN2Y2NzVmOWMzMTJmMWMwMjZhNTVkZWMxM2JlNzc3MWY2 M2U5Nzc1ZS4uZTRiYjkwMzdiNzAzMmU1ZmIxYTUxMTYzN2VmZWZlNzJkODAzMjkyNiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4 IEBACiAyMDEyLTA1LTAxICBaYW4gRG9iZXJzZWsgIDx6YW5kb2JlcnNla0BnbWFpbC5jb20+CiAK KyAgICAgICAgSW1hZ2VEaWZmIHNob3VsZCBiZSBydW4gaW5zaWRlIGEgcHJvcGVybHkgZXN0YWJs aXNoZWQgZW52aXJvbm1lbnQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTg1MjkyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgV2hlbiBjcmVhdGluZyB0aGUgSW1hZ2VEaWZmIHNlcnZlciBwcm9jZXNzLCBwYXNz IGFsb25nIGEgZnJlc2hseS1zZXQtdXAgZW52aXJvbm1lbnQKKyAgICAgICAgaW4gd2hpY2ggdGhl IHByb2Nlc3Mgc2hvdWxkIGJlIGV4ZWN1dGVkIGluLgorCisgICAgICAgICogU2NyaXB0cy93ZWJr aXRweS9sYXlvdXRfdGVzdHMvcG9ydC93ZWJraXQucHk6CisgICAgICAgIChXZWJLaXRQb3J0Ll9z dGFydF9pbWFnZV9kaWZmX3Byb2Nlc3MpOgorCisyMDEyLTA1LTAxICBaYW4gRG9iZXJzZWsgIDx6 YW5kb2JlcnNla0BnbWFpbC5jb20+CisKICAgICAgICAgUHJpbnQgb3V0IHN0ZGVyciBvdXRwdXQg b2YgSW1hZ2VEaWZmIGlmIGl0IGlzIHByZXNlbnQKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTg1Mjg1CiAKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvd2Via2l0LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJr aXRweS9sYXlvdXRfdGVzdHMvcG9ydC93ZWJraXQucHkKaW5kZXggOWQ4YmI5ZmY4N2ZhOWU5OWE4 ZDczNjdiODc0ZTVmOTJiMzllODE2Yy4uYTQxZDYyZTMyODM0ODU1ZjA2YTRiYzg0OTQyN2Y1YzY1 YjMyOTFkMyAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv cG9ydC93ZWJraXQucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv cG9ydC93ZWJraXQucHkKQEAgLTE4MCw3ICsxODAsOCBAQCBjbGFzcyBXZWJLaXRQb3J0KFBvcnQp OgogICAgICAgICAgICAgZWxzZToKICAgICAgICAgICAgICAgICB0b2xlcmFuY2UgPSAwLjEKICAg ICAgICAgY29tbWFuZCA9IFtzZWxmLl9wYXRoX3RvX2ltYWdlX2RpZmYoKSwgJy0tdG9sZXJhbmNl Jywgc3RyKHRvbGVyYW5jZSldCi0gICAgICAgIHByb2Nlc3MgPSBzZXJ2ZXJfcHJvY2Vzcy5TZXJ2 ZXJQcm9jZXNzKHNlbGYsICdJbWFnZURpZmYnLCBjb21tYW5kKQorICAgICAgICBlbnZpcm9ubWVu dCA9IHNlbGYuc2V0dXBfZW52aXJvbl9mb3Jfc2VydmVyKCdJbWFnZURpZmYnKQorICAgICAgICBw cm9jZXNzID0gc2VydmVyX3Byb2Nlc3MuU2VydmVyUHJvY2VzcyhzZWxmLCAnSW1hZ2VEaWZmJywg Y29tbWFuZCwgZW52aXJvbm1lbnQpCiAKICAgICAgICAgcHJvY2Vzcy53cml0ZSgnQ29udGVudC1M ZW5ndGg6ICVkXG4lc0NvbnRlbnQtTGVuZ3RoOiAlZFxuJXMnICUgKAogICAgICAgICAgICAgbGVu KGFjdHVhbF9jb250ZW50cyksIGFjdHVhbF9jb250ZW50cywK