ImageDiff should be run inside a properly established environment

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

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.

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 139640 [details]
Patch

[Philippe Normand]

Comment on attachment 139640 [details]
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?

[Zan Dobersek]

(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

[Zan Dobersek]

Created attachment 139647 [details]
Patch

Fixed patch

[Philippe Normand]

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

[Martin Robinson]

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

[Zan Dobersek]

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

[Ojan Vafai]

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

[Dirk Pranke]

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.

[Dirk Pranke]

Also, for the record, this change looks fine to me but I'll let mrobinson or pnormand give the final blessing if they like.

[Zan Dobersek]

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.

[Zan Dobersek]

Comment on attachment 139647 [details]
Patch

Clearing flags on attachment: 139647

Committed r115962: <http://trac.webkit.org/changeset/115962>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.