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

[Daniel Stone]

Hi,

On 13 May 2015 at 13:19, Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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()

Gosh, thanks!

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

Fair enough. Should be pretty easy to fix; if you want to post a WIP
to do this as a followup, I'm sure you'd find someone to walk through
and figure out what was wrong. I think having this in 1.8 is more
important than having it literally perfect first go around.

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

These I'm fine with deferring too, as long as we have some way (I was
going to say 'more or less sophisticated than a comment in the code'
here, but surely not less sophisticated ... ?) of tracking these for
follow-up work.

>  * 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'

These two are pretty bad. The first one not just because I do
out-of-tree builds (multiple architectures as well as just finding
when it breaks), but also because distcheck requires it. The second
one because, well, we need this stuff to work on x11.

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

Sorry, I really need to work on my communication a bit it seems. For
avoidance of doubt:
  * review is a conversation, not a series of judgements given from on high
  * the review comments are expressing the perfect world we'd like to
end up in; how many of them are crucial to fix before landing are
entirely context-dependent, and a lot of them can be fixed after
landing, if not flat-out disagreed with ('it would be nice if you did
this instead', 'no, I don't want to because X/Y/Z', 'fair enough')
  * in this context especially, landing a very useful test framework
for 1.8 is much more useful than technical perfection; it's not like
protocol review where we're screwed if we land something imperfect
  * I've got worse code in before, some of which I'm pretty sure
persists in the tree to this day

Thanks for sticking with this! If you have a v2 which fixes the
out-of-tree/X11 issues, I'm all ears.

Cheers,
Daniel