[v1,weston,07/11] tests: Add a fadein test

Submitted by Bryce Harrington on Nov. 19, 2014, 11:06 p.m.

Details

Message ID 1416438386-4783-8-git-send-email-bryce@osg.samsung.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Bryce Harrington Nov. 19, 2014, 11:06 p.m.
This also serves as a proof of concept of the screen capture
functionality and as a demo for snapshot-based rendering verification.

Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
---
 Makefile.am         |  7 +++++-
 tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 tests/fadein-test.c

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 26dd473..c3dfa10 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -852,7 +852,8 @@  weston_tests =					\
 	text.weston				\
 	presentation.weston			\
 	roles.weston				\
-	subsurface.weston
+	subsurface.weston			\
+	fadein.weston
 
 
 AM_TESTS_ENVIRONMENT = \
@@ -954,6 +955,10 @@  subsurface_weston_SOURCES = tests/subsurface-test.c
 subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
 subsurface_weston_LDADD = libtest-client.la
 
+fadein_weston_SOURCES = tests/fadein-test.c
+fadein_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
+fadein_weston_LDADD = libtest-client.la
+
 presentation_weston_SOURCES = tests/presentation-test.c
 nodist_presentation_weston_SOURCES =		\
 	protocol/presentation_timing-protocol.c	\
diff --git a/tests/fadein-test.c b/tests/fadein-test.c
new file mode 100644
index 0000000..a6c284f
--- /dev/null
+++ b/tests/fadein-test.c
@@ -0,0 +1,64 @@ 
+/*
+ * Copyright © 2014 Samsung
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and
+ * its documentation for any purpose is hereby granted without fee, provided
+ * that the above copyright notice appear in all copies and that both that
+ * copyright notice and this permission notice appear in supporting
+ * documentation, and that the name of the copyright holders not be used in
+ * advertising or publicity pertaining to distribution of the software
+ * without specific, written prior permission.  The copyright holders make
+ * no representations about the suitability of this software for any
+ * purpose.  It is provided "as is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
+ * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
+ * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
+ * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
+ * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
+ * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include "config.h"
+
+#include <unistd.h>
+#include <stdio.h>
+
+#include "weston-test-client-helper.h"
+
+char *server_parameters="--use-pixman --width=320 --height=240";
+
+static char*
+output_filename(const char* basename) {
+	static const char *path = "./";
+	char *filename;
+
+        if (asprintf(&filename, "%s%s", path, basename) < 0)
+		filename = NULL;
+
+	return filename;
+}
+
+TEST(fadein)
+{
+	struct client *client;
+	char basename[32];
+	char *out_path;
+	int i;
+
+	client = client_create(100, 100, 100, 100);
+	assert(client);
+
+	for (i = 0; i < 6; i++) {
+		snprintf(basename, sizeof basename, "fadein-%02d", i);
+		out_path = output_filename(basename);
+
+		wl_test_record_screenshot(client->test->wl_test, out_path);
+		client_roundtrip(client);
+		free (out_path);
+
+		usleep(250000);
+	}
+
+}

Comments

On Wed, 19 Nov 2014 15:06:22 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> This also serves as a proof of concept of the screen capture
> functionality and as a demo for snapshot-based rendering verification.
> 
> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> ---
>  Makefile.am         |  7 +++++-
>  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 tests/fadein-test.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 26dd473..c3dfa10 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -852,7 +852,8 @@ weston_tests =					\
>  	text.weston				\
>  	presentation.weston			\
>  	roles.weston				\
> -	subsurface.weston
> +	subsurface.weston			\
> +	fadein.weston
>  
>  
>  AM_TESTS_ENVIRONMENT = \
> @@ -954,6 +955,10 @@ subsurface_weston_SOURCES = tests/subsurface-test.c
>  subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
>  subsurface_weston_LDADD = libtest-client.la
>  
> +fadein_weston_SOURCES = tests/fadein-test.c
> +fadein_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> +fadein_weston_LDADD = libtest-client.la
> +
>  presentation_weston_SOURCES = tests/presentation-test.c
>  nodist_presentation_weston_SOURCES =		\
>  	protocol/presentation_timing-protocol.c	\
> diff --git a/tests/fadein-test.c b/tests/fadein-test.c
> new file mode 100644
> index 0000000..a6c284f
> --- /dev/null
> +++ b/tests/fadein-test.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright © 2014 Samsung
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission.  The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose.  It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include "config.h"
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +
> +#include "weston-test-client-helper.h"
> +
> +char *server_parameters="--use-pixman --width=320 --height=240";
> +
> +static char*
> +output_filename(const char* basename) {
> +	static const char *path = "./";
> +	char *filename;
> +
> +        if (asprintf(&filename, "%s%s", path, basename) < 0)
> +		filename = NULL;
> +
> +	return filename;
> +}
> +
> +TEST(fadein)
> +{
> +	struct client *client;
> +	char basename[32];
> +	char *out_path;
> +	int i;
> +
> +	client = client_create(100, 100, 100, 100);
> +	assert(client);
> +
> +	for (i = 0; i < 6; i++) {
> +		snprintf(basename, sizeof basename, "fadein-%02d", i);
> +		out_path = output_filename(basename);
> +
> +		wl_test_record_screenshot(client->test->wl_test, out_path);
> +		client_roundtrip(client);
> +		free (out_path);
> +
> +		usleep(250000);
> +	}
> +
> +}

Hi,

where is the verification promised in the commit message? ;-)

I think it is bad to rely on delays/timers. We need something
more deterministic: a protocol to drive the (headless) repaint.

We probably need the headless repaint to not run on its own, but
strictly when requested by the test client. Additionally, we probably
want to carry a "page flip completion" timestamp in that request, and
have the compositor use the timestamp. That way the test client can
actually drive the in-compositor animations, because it is in control
of the clock and framerate.

That should make the fade-in test reliable, and it will help with
Presentation testing too.

The client driven repaint likely needs to be enabled per-test. If a
test attempts to enable it, but the compositor still does not advertise
the support(!) in protocol, the test should skip. This way real
hardware backends can skip these tests automatically, as they cannot
(or not implemented yet) support client driven repaint.

How would that sound?

As a first proof of concept screenshooting test, I'd prefer something
that doesn't depend on timings at all, so we can add the screenshooting
sooner, and the client driven repaint later if that needs more time.
Which probably calls for an option to skip the fade-in in Weston... oh
but we already have 'startup-animation=none' from weston.ini.


Thanks,
pq
On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
> On Wed, 19 Nov 2014 15:06:22 -0800
> Bryce Harrington <bryce@osg.samsung.com> wrote:
> 
> > This also serves as a proof of concept of the screen capture
> > functionality and as a demo for snapshot-based rendering verification.
> > 
> > Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> > ---
> >  Makefile.am         |  7 +++++-
> >  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 70 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/fadein-test.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 26dd473..c3dfa10 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -852,7 +852,8 @@ weston_tests =					\
> >  	text.weston				\
> >  	presentation.weston			\
> >  	roles.weston				\
> > -	subsurface.weston
> > +	subsurface.weston			\
> > +	fadein.weston
> >  
> >  
> >  AM_TESTS_ENVIRONMENT = \
> > @@ -954,6 +955,10 @@ subsurface_weston_SOURCES = tests/subsurface-test.c
> >  subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> >  subsurface_weston_LDADD = libtest-client.la
> >  
> > +fadein_weston_SOURCES = tests/fadein-test.c
> > +fadein_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > +fadein_weston_LDADD = libtest-client.la
> > +
> >  presentation_weston_SOURCES = tests/presentation-test.c
> >  nodist_presentation_weston_SOURCES =		\
> >  	protocol/presentation_timing-protocol.c	\
> > diff --git a/tests/fadein-test.c b/tests/fadein-test.c
> > new file mode 100644
> > index 0000000..a6c284f
> > --- /dev/null
> > +++ b/tests/fadein-test.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright © 2014 Samsung
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and
> > + * its documentation for any purpose is hereby granted without fee, provided
> > + * that the above copyright notice appear in all copies and that both that
> > + * copyright notice and this permission notice appear in supporting
> > + * documentation, and that the name of the copyright holders not be used in
> > + * advertising or publicity pertaining to distribution of the software
> > + * without specific, written prior permission.  The copyright holders make
> > + * no representations about the suitability of this software for any
> > + * purpose.  It is provided "as is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> > + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> > + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> > + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#include "config.h"
> > +
> > +#include <unistd.h>
> > +#include <stdio.h>
> > +
> > +#include "weston-test-client-helper.h"
> > +
> > +char *server_parameters="--use-pixman --width=320 --height=240";
> > +
> > +static char*
> > +output_filename(const char* basename) {
> > +	static const char *path = "./";
> > +	char *filename;
> > +
> > +        if (asprintf(&filename, "%s%s", path, basename) < 0)
> > +		filename = NULL;
> > +
> > +	return filename;
> > +}
> > +
> > +TEST(fadein)
> > +{
> > +	struct client *client;
> > +	char basename[32];
> > +	char *out_path;
> > +	int i;
> > +
> > +	client = client_create(100, 100, 100, 100);
> > +	assert(client);
> > +
> > +	for (i = 0; i < 6; i++) {
> > +		snprintf(basename, sizeof basename, "fadein-%02d", i);
> > +		out_path = output_filename(basename);
> > +
> > +		wl_test_record_screenshot(client->test->wl_test, out_path);
> > +		client_roundtrip(client);
> > +		free (out_path);
> > +
> > +		usleep(250000);
> > +	}
> > +
> > +}
> 
> Hi,
> 
> where is the verification promised in the commit message? ;-)
> 
> I think it is bad to rely on delays/timers. We need something
> more deterministic: a protocol to drive the (headless) repaint.
> 
> We probably need the headless repaint to not run on its own, but
> strictly when requested by the test client. Additionally, we probably
> want to carry a "page flip completion" timestamp in that request, and
> have the compositor use the timestamp. That way the test client can
> actually drive the in-compositor animations, because it is in control
> of the clock and framerate.

Yes, that would definitely be helpful.

Can you elaborate on how this should be implemented?

> That should make the fade-in test reliable, and it will help with
> Presentation testing too.
> 
> The client driven repaint likely needs to be enabled per-test. If a
> test attempts to enable it, but the compositor still does not advertise
> the support(!) in protocol, the test should skip. This way real
> hardware backends can skip these tests automatically, as they cannot
> (or not implemented yet) support client driven repaint.
> 
> How would that sound?

Sure, although I suspect all tests that care about the rendered output
will need to set this.  Unless they care only about the initial frame.

> As a first proof of concept screenshooting test, I'd prefer something
> that doesn't depend on timings at all, so we can add the screenshooting
> sooner, and the client driven repaint later if that needs more time.
> Which probably calls for an option to skip the fade-in in Weston... oh
> but we already have 'startup-animation=none' from weston.ini.

Ah, I wondered if there was a switch to disable that, thanks.  Only
reason I did fadein as the test was because I couldn't get around the
timed fade in.  ;-)

Bryce

> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Mon, 24 Nov 2014 18:48:51 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
> > On Wed, 19 Nov 2014 15:06:22 -0800
> > Bryce Harrington <bryce@osg.samsung.com> wrote:
> > 
> > > This also serves as a proof of concept of the screen capture
> > > functionality and as a demo for snapshot-based rendering verification.
> > > 
> > > Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> > > ---
> > >  Makefile.am         |  7 +++++-
> > >  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 70 insertions(+), 1 deletion(-)

> > > +TEST(fadein)
> > > +{
> > > +	struct client *client;
> > > +	char basename[32];
> > > +	char *out_path;
> > > +	int i;
> > > +
> > > +	client = client_create(100, 100, 100, 100);
> > > +	assert(client);
> > > +
> > > +	for (i = 0; i < 6; i++) {
> > > +		snprintf(basename, sizeof basename, "fadein-%02d", i);
> > > +		out_path = output_filename(basename);
> > > +
> > > +		wl_test_record_screenshot(client->test->wl_test, out_path);
> > > +		client_roundtrip(client);
> > > +		free (out_path);
> > > +
> > > +		usleep(250000);
> > > +	}
> > > +
> > > +}
> > 
> > Hi,
> > 
> > where is the verification promised in the commit message? ;-)
> > 
> > I think it is bad to rely on delays/timers. We need something
> > more deterministic: a protocol to drive the (headless) repaint.
> > 
> > We probably need the headless repaint to not run on its own, but
> > strictly when requested by the test client. Additionally, we probably
> > want to carry a "page flip completion" timestamp in that request, and
> > have the compositor use the timestamp. That way the test client can
> > actually drive the in-compositor animations, because it is in control
> > of the clock and framerate.
> 
> Yes, that would definitely be helpful.
> 
> Can you elaborate on how this should be implemented?

I don't have a clear picture in my mind yet.

Client driven repaint would probably need to be a new global in the
protocol, another test extension. Exposing it would be conditional to
both the test module, and the current backend supporting it.

It is difficult to call module functions from another module, so you
likely need some function pointers in struct weston_compositor. One
module sets them, and if the other module finds them non-NULL, it knows
the other one supports client driven repaint.

But, because for starters it would be supported only by the headless
backend, which is not used for anything but testing, it's likely fine
to make the first implementation in the headless backend and always
expose it directly from there. No need to hook it up in the test module
for now.

Once we see how it works for headless, we can plan how to let other
backends support it, if needed.

> > That should make the fade-in test reliable, and it will help with
> > Presentation testing too.
> > 
> > The client driven repaint likely needs to be enabled per-test. If a
> > test attempts to enable it, but the compositor still does not advertise
> > the support(!) in protocol, the test should skip. This way real
> > hardware backends can skip these tests automatically, as they cannot
> > (or not implemented yet) support client driven repaint.
> > 
> > How would that sound?
> 
> Sure, although I suspect all tests that care about the rendered output
> will need to set this.  Unless they care only about the initial frame.

To the contrary, I would expect most tests to only care about the final
stabilised image when all animations have ended (preferably never even
started).

I think that should be perfectly implementable without client driven
repaint. Client driven repaint is only needed for timing sensitive
tests, like for animations and testing Presentation extension.

You post damage to a surface and wait for the
presentation_feedback.presented event. Then do the screenshot. Or
actually you shouldn't even need to wait, because completing a
screenshot likely requires one more repaint cycle, so shooting itself
already guarantees a repaint.


Thanks,
pq
On 25/11/14 04:11 AM, Pekka Paalanen wrote:
> On Mon, 24 Nov 2014 18:48:51 -0800
> Bryce Harrington <bryce@osg.samsung.com> wrote:
> 
>> On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
>>> On Wed, 19 Nov 2014 15:06:22 -0800
>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
>>>
>>>> This also serves as a proof of concept of the screen capture
>>>> functionality and as a demo for snapshot-based rendering verification.
>>>>
>>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
>>>> ---
>>>>  Makefile.am         |  7 +++++-
>>>>  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
>>>> +TEST(fadein)
>>>> +{
>>>> +	struct client *client;
>>>> +	char basename[32];
>>>> +	char *out_path;
>>>> +	int i;
>>>> +
>>>> +	client = client_create(100, 100, 100, 100);
>>>> +	assert(client);
>>>> +
>>>> +	for (i = 0; i < 6; i++) {
>>>> +		snprintf(basename, sizeof basename, "fadein-%02d", i);
>>>> +		out_path = output_filename(basename);
>>>> +
>>>> +		wl_test_record_screenshot(client->test->wl_test, out_path);
>>>> +		client_roundtrip(client);
>>>> +		free (out_path);
>>>> +
>>>> +		usleep(250000);
>>>> +	}
>>>> +
>>>> +}
>>>
>>> Hi,
>>>
>>> where is the verification promised in the commit message? ;-)
>>>
>>> I think it is bad to rely on delays/timers. We need something
>>> more deterministic: a protocol to drive the (headless) repaint.
>>>
>>> We probably need the headless repaint to not run on its own, but
>>> strictly when requested by the test client. Additionally, we probably
>>> want to carry a "page flip completion" timestamp in that request, and
>>> have the compositor use the timestamp. That way the test client can
>>> actually drive the in-compositor animations, because it is in control
>>> of the clock and framerate.
>>
>> Yes, that would definitely be helpful.
>>
>> Can you elaborate on how this should be implemented?
> 
> I don't have a clear picture in my mind yet.
> 
> Client driven repaint would probably need to be a new global in the
> protocol, another test extension. Exposing it would be conditional to
> both the test module, and the current backend supporting it.
> 
> It is difficult to call module functions from another module, so you
> likely need some function pointers in struct weston_compositor. One
> module sets them, and if the other module finds them non-NULL, it knows
> the other one supports client driven repaint.
> 
> But, because for starters it would be supported only by the headless
> backend, which is not used for anything but testing, it's likely fine
> to make the first implementation in the headless backend and always
> expose it directly from there. No need to hook it up in the test module
> for now.
> 
> Once we see how it works for headless, we can plan how to let other
> backends support it, if needed.

I've been thinking about this a bit lately, here's some random musing.  :)

We could wrap any clock_gettime usage within weston (if we only care
about the headless compositor that's one call) - if we're using a test
clock it would return the time as controlled by the client, otherwise
it'd just call clock_gettime.  I need to look more into how the timerfd_
stuff works, but I think the fds will need to be manually messed with on
clock change.

The client would advance the clock by a specified number of
(nano?)seconds (monotonically, always increasing) via protocol.

headless renderer would need changes to fake a refresh rate - I don't
think we'd want every clock advance to trigger a repaint.  I think we're
better off with client driving the clock, and repaint being indirectly
controlled.  Right now it just waits 16ms after a repaint then repaints
again.

In any event we do need to control the clock (ie: not just gate repaint)
to get deterministic snapshots of animations/presentation - if that's
really our end goal.

Catching the server fade-in would be problematic - the clock frobbing
would have to be turned on at some point, I'm not sure we want it to
default to on even for the headless compositor (I guess this could be a
command line parameter).


I've wondered a bit about sharing this fake clock with clients -
wl_clock_gettime() or such, the first call would query the presentation
clock id, on every subsequent call it'd just call clock_gettime() on
that id if it's a normal clock.  If it's the test clock it would query
the time from the server.

Is this too invasive?  There's no weston client library, so I'm not
really sure exactly where this would have to be implemented.  I think
jamming this into libwayland-client might be frowned upon.

Similar effects could be achieved with LD_PRELOAD and replacing
clock_gettime when running a test, but that's pretty nasty.

If we're going to do anything for normal clients I guess it should be
sorted before the presentation extension is finalized, since it relies
on clients calling clock_gettime() directly.

(as a complete tangent, I'm a little confused that the presentation
protocol is part of weston - does this mean that video players will
target specific compositors and not generically "wayland"?)

>>> That should make the fade-in test reliable, and it will help with
>>> Presentation testing too.
>>>
>>> The client driven repaint likely needs to be enabled per-test. If a
>>> test attempts to enable it, but the compositor still does not advertise
>>> the support(!) in protocol, the test should skip. This way real
>>> hardware backends can skip these tests automatically, as they cannot
>>> (or not implemented yet) support client driven repaint.
>>>
>>> How would that sound?
>>
>> Sure, although I suspect all tests that care about the rendered output
>> will need to set this.  Unless they care only about the initial frame.
> 
> To the contrary, I would expect most tests to only care about the final
> stabilised image when all animations have ended (preferably never even
> started).
>
> I think that should be perfectly implementable without client driven
> repaint. Client driven repaint is only needed for timing sensitive
> tests, like for animations and testing Presentation extension.

Right - if we're not specifically testing animations we should probably
just disable them.

If we are testing animations it's possible we can write test apps that
just display the frame we're interested in for a really long time
anyway, which may be less direct but with care could produce valid results.

Some aspects of presentation can be tested by playing the same video
frame for a very long time as well...

Is client driven repaint too much work for too little gain?  I'm
inclined to think it's worthwhile, but don't want to go too far towards
implementation if the general concensus is no. :)

It gets a bit easier if we don't care at all about messing with the
clocks in non-test clients.

> You post damage to a surface and wait for the
> presentation_feedback.presented event. Then do the screenshot. Or
> actually you shouldn't even need to wait, because completing a
> screenshot likely requires one more repaint cycle, so shooting itself
> already guarantees a repaint.

Hmm, I'm not sure why the screenshot would require an extra repaint cycle?
On Tue, 25 Nov 2014 10:15:04 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 25/11/14 04:11 AM, Pekka Paalanen wrote:
> > On Mon, 24 Nov 2014 18:48:51 -0800
> > Bryce Harrington <bryce@osg.samsung.com> wrote:
> > 
> >> On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
> >>> On Wed, 19 Nov 2014 15:06:22 -0800
> >>> Bryce Harrington <bryce@osg.samsung.com> wrote:
> >>>
> >>>> This also serves as a proof of concept of the screen capture
> >>>> functionality and as a demo for snapshot-based rendering verification.
> >>>>
> >>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> >>>> ---
> >>>>  Makefile.am         |  7 +++++-
> >>>>  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 70 insertions(+), 1 deletion(-)
> > 
> >>>> +TEST(fadein)
> >>>> +{
> >>>> +	struct client *client;
> >>>> +	char basename[32];
> >>>> +	char *out_path;
> >>>> +	int i;
> >>>> +
> >>>> +	client = client_create(100, 100, 100, 100);
> >>>> +	assert(client);
> >>>> +
> >>>> +	for (i = 0; i < 6; i++) {
> >>>> +		snprintf(basename, sizeof basename, "fadein-%02d", i);
> >>>> +		out_path = output_filename(basename);
> >>>> +
> >>>> +		wl_test_record_screenshot(client->test->wl_test, out_path);
> >>>> +		client_roundtrip(client);
> >>>> +		free (out_path);
> >>>> +
> >>>> +		usleep(250000);
> >>>> +	}
> >>>> +
> >>>> +}
> >>>
> >>> Hi,
> >>>
> >>> where is the verification promised in the commit message? ;-)
> >>>
> >>> I think it is bad to rely on delays/timers. We need something
> >>> more deterministic: a protocol to drive the (headless) repaint.
> >>>
> >>> We probably need the headless repaint to not run on its own, but
> >>> strictly when requested by the test client. Additionally, we probably
> >>> want to carry a "page flip completion" timestamp in that request, and
> >>> have the compositor use the timestamp. That way the test client can
> >>> actually drive the in-compositor animations, because it is in control
> >>> of the clock and framerate.
> >>
> >> Yes, that would definitely be helpful.
> >>
> >> Can you elaborate on how this should be implemented?
> > 
> > I don't have a clear picture in my mind yet.
> > 
> > Client driven repaint would probably need to be a new global in the
> > protocol, another test extension. Exposing it would be conditional to
> > both the test module, and the current backend supporting it.
> > 
> > It is difficult to call module functions from another module, so you
> > likely need some function pointers in struct weston_compositor. One
> > module sets them, and if the other module finds them non-NULL, it knows
> > the other one supports client driven repaint.
> > 
> > But, because for starters it would be supported only by the headless
> > backend, which is not used for anything but testing, it's likely fine
> > to make the first implementation in the headless backend and always
> > expose it directly from there. No need to hook it up in the test module
> > for now.
> > 
> > Once we see how it works for headless, we can plan how to let other
> > backends support it, if needed.
> 
> I've been thinking about this a bit lately, here's some random musing.  :)
> 
> We could wrap any clock_gettime usage within weston (if we only care
> about the headless compositor that's one call) - if we're using a test
> clock it would return the time as controlled by the client, otherwise
> it'd just call clock_gettime.  I need to look more into how the timerfd_
> stuff works, but I think the fds will need to be manually messed with on
> clock change.

Yes, controlling the current time is yet another request to add, it
shouldn't come directly from the repaint completion.

There are several categories of clock_gettime() calls:

All calls of the like
	clock_gettime(output->compositor->presentation_clock, &ts);
can be wrapped in a weston_compositor_read_presentation_clock() or
something.

Then there are the crash-too-early/often checks in desktop-shell/ that
I don't think we need to override.

And... that's all. Any other clock_gettime() calls in the compositor
can likely be regarded as bugs.

Timers I imagine we would not care about. Animations are not driven
by timers. There are things like idle timeout, which for testing
probably just want to be disabled, unless testing the idle timeout
itself.

> The client would advance the clock by a specified number of
> (nano?)seconds (monotonically, always increasing) via protocol.

Let's make it absolutely clear what happens: always send absolute time.
We even have some rare cases where weston actually tests for "monotonic"
time going backwards to detect broken things, IIRC.

> headless renderer would need changes to fake a refresh rate - I don't
> think we'd want every clock advance to trigger a repaint.  I think we're
> better off with client driving the clock, and repaint being indirectly
> controlled.  Right now it just waits 16ms after a repaint then repaints
> again.

I'd suggest driving both independently from the test client. That way
we have the most control, and the compositor side is simpler.

It's up to the test to drive it right.

But, when we are not using client driven clock and repaint, then the
headless backend would indeed benefit from a timer to fake a refresh
rate.

> In any event we do need to control the clock (ie: not just gate repaint)
> to get deterministic snapshots of animations/presentation - if that's
> really our end goal.

Yes.

> Catching the server fade-in would be problematic - the clock frobbing
> would have to be turned on at some point, I'm not sure we want it to
> default to on even for the headless compositor (I guess this could be a
> command line parameter).

Indeed, or any other way we configure weston.

> I've wondered a bit about sharing this fake clock with clients -
> wl_clock_gettime() or such, the first call would query the presentation
> clock id, on every subsequent call it'd just call clock_gettime() on
> that id if it's a normal clock.  If it's the test clock it would query
> the time from the server.
> 
> Is this too invasive?  There's no weston client library, so I'm not
> really sure exactly where this would have to be implemented.  I think
> jamming this into libwayland-client might be frowned upon.
> 
> Similar effects could be achieved with LD_PRELOAD and replacing
> clock_gettime when running a test, but that's pretty nasty.

I can't see yet, why we would need such get-clock API. We also don't
have a place to put in either.

> If we're going to do anything for normal clients I guess it should be
> sorted before the presentation extension is finalized, since it relies
> on clients calling clock_gettime() directly.

What we are planning here should be strictly for test clients.

If we ever have desktop components that use clocks for special things
that we would need to frob, let's look at that then. For now, just
ignore it.

All we are planning here is basically for non-realtime testing, e.g.
testing in build-bots and such. We have had some talk about also doing
automated realtime testing on real hardware, like does Weston keep on
reaching the full refresh rate when playing a 1080p video on, say, RPi
if it once did. Relying on a particular real hardware platform allows
testing much more, including timing sensitive operations (hw video
decode combined with compositing) and driver/hw operations (does this
video surface actually go to a hardware overlay). So far these are just
plans, but this kind of things would never be part of the non-realtime
test suite of 'make check'.

Just making a point of limiting the scope here, so we don't try to
bite too much at once.

> (as a complete tangent, I'm a little confused that the presentation
> protocol is part of weston - does this mean that video players will
> target specific compositors and not generically "wayland"?)

Protocols usually get in Weston first when they are experimental, so we
can have some wider testing on them. Most, including Presentation and
xdg_shell, will move into the Wayland repository when they are deemed
stable enough. Sub-surfaces already moved. The move is the final point
where we can theoretically make compatibility-breaking changes.

> >>> That should make the fade-in test reliable, and it will help with
> >>> Presentation testing too.
> >>>
> >>> The client driven repaint likely needs to be enabled per-test. If a
> >>> test attempts to enable it, but the compositor still does not advertise
> >>> the support(!) in protocol, the test should skip. This way real
> >>> hardware backends can skip these tests automatically, as they cannot
> >>> (or not implemented yet) support client driven repaint.
> >>>
> >>> How would that sound?
> >>
> >> Sure, although I suspect all tests that care about the rendered output
> >> will need to set this.  Unless they care only about the initial frame.
> > 
> > To the contrary, I would expect most tests to only care about the final
> > stabilised image when all animations have ended (preferably never even
> > started).
> >
> > I think that should be perfectly implementable without client driven
> > repaint. Client driven repaint is only needed for timing sensitive
> > tests, like for animations and testing Presentation extension.
> 
> Right - if we're not specifically testing animations we should probably
> just disable them.

Yeah. So first, let's do things that do not need any clock games.

> If we are testing animations it's possible we can write test apps that
> just display the frame we're interested in for a really long time
> anyway, which may be less direct but with care could produce valid results.

Except the animations we usually would want to test are
compositor-internal.

> Some aspects of presentation can be tested by playing the same video
> frame for a very long time as well...

No, not really, I think. Presentation is all about timings.

> Is client driven repaint too much work for too little gain?  I'm
> inclined to think it's worthwhile, but don't want to go too far towards
> implementation if the general concensus is no. :)
> 
> It gets a bit easier if we don't care at all about messing with the
> clocks in non-test clients.

Yes, let's restrict the clock games solely to test clients.

> > You post damage to a surface and wait for the
> > presentation_feedback.presented event. Then do the screenshot. Or
> > actually you shouldn't even need to wait, because completing a
> > screenshot likely requires one more repaint cycle, so shooting itself
> > already guarantees a repaint.
> 
> Hmm, I'm not sure why the screenshot would require an extra repaint cycle?

That's the way it's implemented. If you read the current frame on
screen, it may already be outdated if the scenegraph state has changed.
Your protocol stream just before the record_screenshot request
may include state changes. There might be also some technical reasons I
forget, but IIRC we read out the image just before pushing it to the
display hw.


Thanks,
pq
On 26/11/14 02:43 AM, Pekka Paalanen wrote:
> On Tue, 25 Nov 2014 10:15:04 -0600
> Derek Foreman <derekf@osg.samsung.com> wrote:
> 
>> On 25/11/14 04:11 AM, Pekka Paalanen wrote:
>>> On Mon, 24 Nov 2014 18:48:51 -0800
>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
>>>
>>>> On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
>>>>> On Wed, 19 Nov 2014 15:06:22 -0800
>>>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
>>>>>
>>>>>> This also serves as a proof of concept of the screen capture
>>>>>> functionality and as a demo for snapshot-based rendering verification.
>>>>>>
>>>>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
>>>>>> ---
>>>>>>  Makefile.am         |  7 +++++-
>>>>>>  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 70 insertions(+), 1 deletion(-)
>>>
>>>>>> +TEST(fadein)

<snip>

>>> Client driven repaint would probably need to be a new global in the
>>> protocol, another test extension. Exposing it would be conditional to
>>> both the test module, and the current backend supporting it.
>>>
>>> It is difficult to call module functions from another module, so you
>>> likely need some function pointers in struct weston_compositor. One
>>> module sets them, and if the other module finds them non-NULL, it knows
>>> the other one supports client driven repaint.
>>>
>>> But, because for starters it would be supported only by the headless
>>> backend, which is not used for anything but testing, it's likely fine
>>> to make the first implementation in the headless backend and always
>>> expose it directly from there. No need to hook it up in the test module
>>> for now.
>>>
>>> Once we see how it works for headless, we can plan how to let other
>>> backends support it, if needed.
>>
>> I've been thinking about this a bit lately, here's some random musing.  :)
>>
>> We could wrap any clock_gettime usage within weston (if we only care
>> about the headless compositor that's one call) - if we're using a test
>> clock it would return the time as controlled by the client, otherwise
>> it'd just call clock_gettime.  I need to look more into how the timerfd_
>> stuff works, but I think the fds will need to be manually messed with on
>> clock change.
> 
> Yes, controlling the current time is yet another request to add, it
> shouldn't come directly from the repaint completion.
> 
> There are several categories of clock_gettime() calls:
> 
> All calls of the like
> 	clock_gettime(output->compositor->presentation_clock, &ts);
> can be wrapped in a weston_compositor_read_presentation_clock() or
> something.
> 
> Then there are the crash-too-early/often checks in desktop-shell/ that
> I don't think we need to override.

Mmhm, good point, I don't think we actually need an automated test for
those checks...

> And... that's all. Any other clock_gettime() calls in the compositor
> can likely be regarded as bugs.
> 
> Timers I imagine we would not care about. Animations are not driven
> by timers. There are things like idle timeout, which for testing
> probably just want to be disabled, unless testing the idle timeout
> itself.

The only timer I was concerned about was the one in the headless backend
frame completion stuff - which I think will have to be replaced by
something that isn't actually a timer when we're controlling the clock
anyway.

So yeah, I think you're right and timers aren't going to need any attention.

>> The client would advance the clock by a specified number of
>> (nano?)seconds (monotonically, always increasing) via protocol.
> 
> Let's make it absolutely clear what happens: always send absolute time.
> We even have some rare cases where weston actually tests for "monotonic"
> time going backwards to detect broken things, IIRC.

Ok, I'll do it that way.  :)

>> headless renderer would need changes to fake a refresh rate - I don't
>> think we'd want every clock advance to trigger a repaint.  I think we're
>> better off with client driving the clock, and repaint being indirectly
>> controlled.  Right now it just waits 16ms after a repaint then repaints
>> again.
> 
> I'd suggest driving both independently from the test client. That way
> we have the most control, and the compositor side is simpler.
> 
> It's up to the test to drive it right.
> 
> But, when we are not using client driven clock and repaint, then the
> headless backend would indeed benefit from a timer to fake a refresh
> rate.

Right now it does have a 16ms timer to fake a refresh rate - do you see
any reason this needs to be configurable or more complicated than it is now?

For now I'm inclined to leave the un-client-driven repaint loop driven
by the simple 16ms timer.

>> In any event we do need to control the clock (ie: not just gate repaint)
>> to get deterministic snapshots of animations/presentation - if that's
>> really our end goal.
> 
> Yes.
> 
>> Catching the server fade-in would be problematic - the clock frobbing
>> would have to be turned on at some point, I'm not sure we want it to
>> default to on even for the headless compositor (I guess this could be a
>> command line parameter).
> 
> Indeed, or any other way we configure weston.
> 
>> I've wondered a bit about sharing this fake clock with clients -
>> wl_clock_gettime() or such, the first call would query the presentation
>> clock id, on every subsequent call it'd just call clock_gettime() on
>> that id if it's a normal clock.  If it's the test clock it would query
>> the time from the server.
>>
>> Is this too invasive?  There's no weston client library, so I'm not
>> really sure exactly where this would have to be implemented.  I think
>> jamming this into libwayland-client might be frowned upon.
>>
>> Similar effects could be achieved with LD_PRELOAD and replacing
>> clock_gettime when running a test, but that's pretty nasty.
> 
> I can't see yet, why we would need such get-clock API. We also don't
> have a place to put in either.

Ok, will stick to clock control for test clients only.

>> If we're going to do anything for normal clients I guess it should be
>> sorted before the presentation extension is finalized, since it relies
>> on clients calling clock_gettime() directly.
> 
> What we are planning here should be strictly for test clients.
> 
> If we ever have desktop components that use clocks for special things
> that we would need to frob, let's look at that then. For now, just
> ignore it.
> 
> All we are planning here is basically for non-realtime testing, e.g.
> testing in build-bots and such. We have had some talk about also doing
> automated realtime testing on real hardware, like does Weston keep on
> reaching the full refresh rate when playing a 1080p video on, say, RPi
> if it once did. Relying on a particular real hardware platform allows
> testing much more, including timing sensitive operations (hw video
> decode combined with compositing) and driver/hw operations (does this
> video surface actually go to a hardware overlay). So far these are just
> plans, but this kind of things would never be part of the non-realtime
> test suite of 'make check'.

Very cool, except we run into a situation where make check can fail if a
cron job wakes up in the background.  :D

Testing if something actually ended up in a plane would probably need
additional test protocol to query such things?

Anyway, I'm getting ahead of myself, that's an interesting problem for
another day. :)

> Just making a point of limiting the scope here, so we don't try to
> bite too much at once.

gotcha.

>> (as a complete tangent, I'm a little confused that the presentation
>> protocol is part of weston - does this mean that video players will
>> target specific compositors and not generically "wayland"?)
> 
> Protocols usually get in Weston first when they are experimental, so we
> can have some wider testing on them. Most, including Presentation and
> xdg_shell, will move into the Wayland repository when they are deemed
> stable enough. Sub-surfaces already moved. The move is the final point
> where we can theoretically make compatibility-breaking changes.

Thanks for the explanation.

>>>>> That should make the fade-in test reliable, and it will help with
>>>>> Presentation testing too.
>>>>>
>>>>> The client driven repaint likely needs to be enabled per-test. If a
>>>>> test attempts to enable it, but the compositor still does not advertise
>>>>> the support(!) in protocol, the test should skip. This way real
>>>>> hardware backends can skip these tests automatically, as they cannot
>>>>> (or not implemented yet) support client driven repaint.
>>>>>
>>>>> How would that sound?
>>>>
>>>> Sure, although I suspect all tests that care about the rendered output
>>>> will need to set this.  Unless they care only about the initial frame.
>>>
>>> To the contrary, I would expect most tests to only care about the final
>>> stabilised image when all animations have ended (preferably never even
>>> started).
>>>
>>> I think that should be perfectly implementable without client driven
>>> repaint. Client driven repaint is only needed for timing sensitive
>>> tests, like for animations and testing Presentation extension.
>>
>> Right - if we're not specifically testing animations we should probably
>> just disable them.
> 
> Yeah. So first, let's do things that do not need any clock games.

Maybe our first tests should be for all output transforms - gives a good
way to validate the big transforms refactor...

I think just drawing two rectangles of different colors in appropriate
places and then using your suggested "check for solid color" functions
to verify they're moved appropriately...

(why two regions?  because if we do just one we can false positive if
the whole screen ends up that color by mistake - bad zoom)

>> If we are testing animations it's possible we can write test apps that
>> just display the frame we're interested in for a really long time
>> anyway, which may be less direct but with care could produce valid results.
> 
> Except the animations we usually would want to test are
> compositor-internal.
> 
>> Some aspects of presentation can be tested by playing the same video
>> frame for a very long time as well...
> 
> No, not really, I think. Presentation is all about timings.

Ok.

>> Is client driven repaint too much work for too little gain?  I'm
>> inclined to think it's worthwhile, but don't want to go too far towards
>> implementation if the general concensus is no. :)
>>
>> It gets a bit easier if we don't care at all about messing with the
>> clocks in non-test clients.
> 
> Yes, let's restrict the clock games solely to test clients.

Will do.

>>> You post damage to a surface and wait for the
>>> presentation_feedback.presented event. Then do the screenshot. Or
>>> actually you shouldn't even need to wait, because completing a
>>> screenshot likely requires one more repaint cycle, so shooting itself
>>> already guarantees a repaint.
>>
>> Hmm, I'm not sure why the screenshot would require an extra repaint cycle?
> 
> That's the way it's implemented. If you read the current frame on
> screen, it may already be outdated if the scenegraph state has changed.
> Your protocol stream just before the record_screenshot request
> may include state changes. There might be also some technical reasons I
> forget, but IIRC we read out the image just before pushing it to the
> display hw.

Ah, ok.  I see this now in the screenshooter code.  That's not how my
current (direct to file) implementation operates.

I'll follow screenshooter's example for the next revision.

Thanks!
Derek
On Wed, 26 Nov 2014 10:49:08 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 26/11/14 02:43 AM, Pekka Paalanen wrote:
> > On Tue, 25 Nov 2014 10:15:04 -0600
> > Derek Foreman <derekf@osg.samsung.com> wrote:
> > 
> >> On 25/11/14 04:11 AM, Pekka Paalanen wrote:
> >>> On Mon, 24 Nov 2014 18:48:51 -0800
> >>> Bryce Harrington <bryce@osg.samsung.com> wrote:
> >>>
> >>>> On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
> >>>>> On Wed, 19 Nov 2014 15:06:22 -0800
> >>>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
> >>>>>
> >>>>>> This also serves as a proof of concept of the screen capture
> >>>>>> functionality and as a demo for snapshot-based rendering verification.
> >>>>>>
> >>>>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> >>>>>> ---
> >>>>>>  Makefile.am         |  7 +++++-
> >>>>>>  tests/fadein-test.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  2 files changed, 70 insertions(+), 1 deletion(-)
> >>>
> >>>>>> +TEST(fadein)

> > Timers I imagine we would not care about. Animations are not driven
> > by timers. There are things like idle timeout, which for testing
> > probably just want to be disabled, unless testing the idle timeout
> > itself.
> 
> The only timer I was concerned about was the one in the headless backend
> frame completion stuff - which I think will have to be replaced by
> something that isn't actually a timer when we're controlling the clock
> anyway.

Correct. That timer would be replaced maybe by an idle task/callback in
the main event loop. When we use client driven repaint, the repaint
request should cause the finish_frame hook to run, but I'm not sure
it's safe to do from a request handler. So, the request handler would
schedule an idle task that would do exactly what the timer task would
do, but with the fake timestamp from the request. Something like that.

> So yeah, I think you're right and timers aren't going to need any attention.
> 
> >> The client would advance the clock by a specified number of
> >> (nano?)seconds (monotonically, always increasing) via protocol.
> > 
> > Let's make it absolutely clear what happens: always send absolute time.
> > We even have some rare cases where weston actually tests for "monotonic"
> > time going backwards to detect broken things, IIRC.
> 
> Ok, I'll do it that way.  :)
> 
> >> headless renderer would need changes to fake a refresh rate - I don't
> >> think we'd want every clock advance to trigger a repaint.  I think we're
> >> better off with client driving the clock, and repaint being indirectly
> >> controlled.  Right now it just waits 16ms after a repaint then repaints
> >> again.
> > 
> > I'd suggest driving both independently from the test client. That way
> > we have the most control, and the compositor side is simpler.
> > 
> > It's up to the test to drive it right.
> > 
> > But, when we are not using client driven clock and repaint, then the
> > headless backend would indeed benefit from a timer to fake a refresh
> > rate.
> 
> Right now it does have a 16ms timer to fake a refresh rate - do you see
> any reason this needs to be configurable or more complicated than it is now?
> 
> For now I'm inclined to leave the un-client-driven repaint loop driven
> by the simple 16ms timer.

Yeah, that's fine. I just forgot it had a timer already. :-)

Btw. do check, that the headless backend reports the wl_output refresh
rate as 60000, not 60 mHz. I fixed that already once, but I forget if I
ever pushed it. (A free commit for you if so! ;-)

I was baffled when some tests of mine were taking a very long time to
complete, because something was sleeping based on the reported refresh
rate...


> > All we are planning here is basically for non-realtime testing, e.g.
> > testing in build-bots and such. We have had some talk about also doing
> > automated realtime testing on real hardware, like does Weston keep on
> > reaching the full refresh rate when playing a 1080p video on, say, RPi
> > if it once did. Relying on a particular real hardware platform allows
> > testing much more, including timing sensitive operations (hw video
> > decode combined with compositing) and driver/hw operations (does this
> > video surface actually go to a hardware overlay). So far these are just
> > plans, but this kind of things would never be part of the non-realtime
> > test suite of 'make check'.
> 
> Very cool, except we run into a situation where make check can fail if a
> cron job wakes up in the background.  :D

It would be a full OS image, completely repeatable, and no funny
automatic random stuff running, flashed/booted by a test box controller
in a completely automated way. We have some infrastructure for such
things at Collabora, but not for Weston yet. Nor RPi, AFAIK.

So that's all just ponies atm. but planned.

> Testing if something actually ended up in a plane would probably need
> additional test protocol to query such things?

There is likely going to be a bit in the
presentation_feedback.presented flags for that. We've already done it
once at Collabora as a hack. I just need to those flags sorted out
before Christmas so Weston 1.7 won't have a handicapped Presentation...

Mario Kleiner was very interested in presentation_feedback telling
exactly how the presentation was done, so it fits there quite nicely.

> Anyway, I'm getting ahead of myself, that's an interesting problem for
> another day. :)

Indeed. ;-)


> Maybe our first tests should be for all output transforms - gives a good
> way to validate the big transforms refactor...

Yeah.

> I think just drawing two rectangles of different colors in appropriate
> places and then using your suggested "check for solid color" functions
> to verify they're moved appropriately...
> 
> (why two regions?  because if we do just one we can false positive if
> the whole screen ends up that color by mistake - bad zoom)

Or a 2x2 grid, with a distinct background.

> >>> You post damage to a surface and wait for the
> >>> presentation_feedback.presented event. Then do the screenshot. Or
> >>> actually you shouldn't even need to wait, because completing a
> >>> screenshot likely requires one more repaint cycle, so shooting itself
> >>> already guarantees a repaint.
> >>
> >> Hmm, I'm not sure why the screenshot would require an extra repaint cycle?
> > 
> > That's the way it's implemented. If you read the current frame on
> > screen, it may already be outdated if the scenegraph state has changed.
> > Your protocol stream just before the record_screenshot request
> > may include state changes. There might be also some technical reasons I
> > forget, but IIRC we read out the image just before pushing it to the
> > display hw.
> 
> Ah, ok.  I see this now in the screenshooter code.  That's not how my
> current (direct to file) implementation operates.
> 
> I'll follow screenshooter's example for the next revision.

Right. Btw. it will interact with the client driven repaint, so you
need to carefully think what triggers what, and what will end up in the
screenshot both client state wise and the clock controls for
flip/current time.

Going with the current code initially is a good plan.


Thanks,
pq