[PATCH weston 00/10] Implement screenshot-based testing with the headless renderer

[Bryce Harrington]

On Mon, May 11, 2015 at 03:41:44PM +0300, Pekka Paalanen wrote:
> On Sat, 9 May 2015 15:01:13 +0100
> Daniel Stone <daniel at fooishbar.org> wrote:
> 
> > Hi Bryce,
> > 
> > On 7 May 2015 at 01:44, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > > This series adds support for implementing test cases that can check
> > > rendering output without needing connection to a physical output.  It
> > > utilizes the pixman renderer in the headless backend to generate
> > > screenshots at the request of the test client.
> > >
> > > The test creates a shm buffer and passes it to the server via the
> > > Weston-Test protocol ('capture_screenshot').  The server then renders
> > > the display contents into the buffer and returns it as a response
> > > ('capture_screenshot_done').
> > >
> > > The test then loads a corresponding reference PNG from disk as a cairo
> > > surface, and then compares it with the captured screenshot.  Note that
> > > the screenshot includes the current time in the desktop clock, which
> > > will of course be different in the captured screenshot from the
> > > reference image.  So this checks a small clipped out section of each of
> > > the two images to verify congruence.
> > >
> > > By default, Wayland fades in the display and will show a patterned
> > > background.  The former feature causes a black or nearly-black image to
> > > be captured (the darkness of which may vary from system to system), and
> > > the latter merely adds noise in our comparison, so both features are
> > > disabled via a test-specific configuration .ini file.
> > 
> > Modulo the comments I had earlier (and loads of C++ comments in
> > 09/10), these do look good to me.
> > 
> > Thanks a lot for doing them - they're very useful, and I'd like to see
> > them get into 1.8 if possible.
> > 
> > They're certainly not perfect, and I think show up a lot of the
> > limitations of our current test 'infrastructure', but IMO that should
> > in no way block these tests. On the contrary, it gives us more data to
> > build our test framework on: it will make the conversion a little more
> > painful, but I think be a good guide for its development.
> 
> Hi Bryce,
> 
> the approach and design look good.
> 
> For patches 1, 2, 3, 5, 8, 
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> Patches 4, 6, 7 need fixing. Patch 9 should get minor changes. Patch 10
> would be better dropped as it is, since all that should be replaced by
> just a pixman_image_composite32() call. If there really was a reason to
> hand-code all that copying, I've long forgot it.
> 
> Should weston-tests-env also be setting WESTON_TEST_OUTPUT_PATH and
> WESTON_TEST_REFERENCE_PATH?
> 
> Right now by default, an out-of-tree 'make check' will fail for not
> finding the reference image.
> 
> Did you notice, that 'clientside-screenshot.png' ends up all black, if
> you do 'make check BACKEND=x11-backend.so'?
> Wonder why it does that, you're not intending to do anything
> backend-specific. Except the server command line arguments?
> 
> Jon, this series adds a test type that would be useful to run in several
> different setups: backends x renderers. I think that's something you
> could take into account.
> 
> 
> Thanks,
> pq

daniels, pq...

Thanks both of you for the reviews.  I've implemented some of the
changes you suggested:

 * Refactor cairo out of the test client backend code entirely
   by utilizing the weston test surface structure to carry the
   specific data that the check_ routines need.
 * Drop refactoring redundant pixel copying code to shared
 * Take stride and bpp into account in check functions
 * In test case, set the bpp for the screenshot
 * Fix screenshot protocol description
 * Convert C++ style comments to C
 * Don't leak screenshot object
 * s/ec/compositor/
 * Instead of spinning on client_roundtrip, check for client's wl_display
 * Drop unneeded wl_surface in test case
 * Restore missing weston_log in idle_launch_client()

Still needing to be done:

 * Split up test case code into several functions

You suggested a lot of other changes, which I've not done, but did spend
some time investigating:

 * Split the capture screenshot stuff out to its own interface, or 
   alternatively, use wl_callback to signal the completion.

   I did take a shot at this, but the code just kept getting more and
   more broken so I cut losses after a few hours banging my head on it.
   Code seems to work fine as implemented.

 * Use pixman_image_composite32 instead of existing pixel copy code.

 * Remove an internal abstraction that requires
   weston_test_screenshot_outcome

 * test_screenshot_frame_listener - drop indirection through the done
   pointer

 * handle or record the output transformation parameters

 * set a destroy_signal listener on the weston_buffer to be completely
   correct

 * also update src/screenshooter.c with the above changes

 * properly handle XRGB reference images

 * implement test framework self-tests.  

 * out of tree make check doesn't find the reference image.
   Legitimate issue, maybe follow up work.

 * Black output with 'make check BACKEND=x11-backend.so'

Most of the above is regarding existing screenshooter code that I'm just
appropriating and reusing in another spot.  I suppose most of those
changes are legitimate and I feel the pressure to make everything
perfect, but it wasn't obvious to me how to do them.  For the most part
a lot of this is just implementational details, which probably never
will be perfect anyway.  The reuse of the screenshooter code is also
generating a lot of refactoring opportunities, so to speak.

Bryce