[PATCH 2/4] xwayland: Remove a useless out-of-memory check

[Dima Ryazanov]

snprintf does not allocate memory, so we can never get an out-of-memory error.

(Also, the error handler would free xwl_output after it was already registered
as an event listener.)

Signed-off-by: Dima Ryazanov 
---
 hw/xwayland/xwayland-output.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index 1d75d0b..4949363 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -167,11 +167,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t 
id)
   &wl_output_interface, 2);
 wl_output_add_listener(xwl_output->output, &output_listener, xwl_output);
 
-if (snprintf(name, sizeof name, "XWAYLAND%d", serial++) < 0) {
-ErrorF("create_output ENOMEM\n");
-free(xwl_output);
-return NULL;
-}
+snprintf(name, sizeof name, "XWAYLAND%d", serial++);
 
 xwl_output->xwl_screen = xwl_screen;
 xwl_output->randr_crtc = RRCrtcCreate(xwl_screen->screen, xwl_output);
-- 
2.4.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH 3/4] xwayland: Keep a list of wayland globals

[Dima Ryazanov]

The logic is pretty much copied from weston's clients/window.c.

Signed-off-by: Dima Ryazanov 
---
 hw/xwayland/xwayland.c | 25 -
 hw/xwayland/xwayland.h |  8 
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 7e8d667..e99fbac 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -383,6 +383,17 @@ registry_global(void *data, struct wl_registry *registry, 
uint32_t id,
 const char *interface, uint32_t version)
 {
 struct xwl_screen *xwl_screen = data;
+struct xwl_global *xwl_global;
+
+xwl_global = calloc(sizeof *xwl_global, 1);
+if (xwl_global == NULL) {
+ErrorF("registry_global ENOMEM\n");
+return;
+}
+xwl_global->name = id;
+xwl_global->interface = strdup(interface);
+xwl_global->version = version;
+xorg_list_add(&xwl_global->link, &xwl_screen->global_list);
 
 if (strcmp(interface, "wl_compositor") == 0) {
 xwl_screen->compositor =
@@ -410,7 +421,18 @@ registry_global(void *data, struct wl_registry *registry, 
uint32_t id,
 static void
 global_remove(void *data, struct wl_registry *registry, uint32_t name)
 {
-/* Nothing to do here, wl_compositor and wl_shm should not be removed */
+struct xwl_screen *xwl_screen = data;
+struct xwl_global *xwl_global, *next_xwl_global;
+
+xorg_list_for_each_entry_safe(xwl_global, next_xwl_global,
+  &xwl_screen->global_list, link) {
+if (xwl_global->name != name)
+continue;
+
+xorg_list_del(&xwl_global->link);
+free(xwl_global->interface);
+free(xwl_global);
+}
 }
 
 static const struct wl_registry_listener registry_listener = {
@@ -562,6 +584,7 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 listen_on_fds(xwl_screen);
 }
 
+xorg_list_init(&xwl_screen->global_list);
 xorg_list_init(&xwl_screen->output_list);
 xorg_list_init(&xwl_screen->seat_list);
 xorg_list_init(&xwl_screen->damage_window_list);
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index cfb343d..2ba5312 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -64,6 +64,7 @@ struct xwl_screen {
 UnrealizeWindowProcPtr UnrealizeWindow;
 XYToWindowProcPtr XYToWindow;
 
+struct xorg_list global_list;
 struct xorg_list output_list;
 struct xorg_list seat_list;
 struct xorg_list damage_window_list;
@@ -95,6 +96,13 @@ struct xwl_screen {
 struct glamor_context *glamor_ctx;
 };
 
+struct xwl_global {
+uint32_t name;
+char *interface;
+uint32_t version;
+struct xorg_list link;
+};
+
 struct xwl_window {
 struct xwl_screen *xwl_screen;
 struct wl_surface *surface;
-- 
2.4.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH 4/4] xwayland: Destroy xwl_output when wl_output gets removed

[Dima Ryazanov]

This makes Xwayland correctly handle a monitor getting unplugged.

Signed-off-by: Dima Ryazanov 
---
 hw/xwayland/xwayland-output.c |  1 +
 hw/xwayland/xwayland.c| 16 
 hw/xwayland/xwayland.h|  1 +
 3 files changed, 18 insertions(+)

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index 4949363..4ee74fb 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -165,6 +165,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t 
id)
 
 xwl_output->output = wl_registry_bind(xwl_screen->registry, id,
   &wl_output_interface, 2);
+xwl_output->server_output_id = id;
 wl_output_add_listener(xwl_output->output, &output_listener, xwl_output);
 
 snprintf(name, sizeof name, "XWAYLAND%d", serial++);
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index e99fbac..a763b02 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -379,6 +379,19 @@ xwl_screen_post_damage(struct xwl_screen *xwl_screen)
 }
 
 static void
+xwl_screen_destroy_output(struct xwl_screen *xwl_screen, uint32_t id)
+{
+struct xwl_output *xwl_output;
+
+xorg_list_for_each_entry(xwl_output, &xwl_screen->output_list, link) {
+if (xwl_output->server_output_id == id) {
+xwl_output_destroy(xwl_output);
+break;
+}
+}
+}
+
+static void
 registry_global(void *data, struct wl_registry *registry, uint32_t id,
 const char *interface, uint32_t version)
 {
@@ -429,6 +442,9 @@ global_remove(void *data, struct wl_registry *registry, 
uint32_t name)
 if (xwl_global->name != name)
 continue;
 
+if (strcmp(xwl_global->interface, "wl_output") == 0)
+xwl_screen_destroy_output(xwl_screen, name);
+
 xorg_list_del(&xwl_global->link);
 free(xwl_global->interface);
 free(xwl_global);
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 2ba5312..c4342a4 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -138,6 +138,7 @@ struct xwl_seat {
 struct xwl_output {
 struct xorg_list link;
 struct wl_output *output;
+uint32_t server_output_id;
 struct xwl_screen *xwl_screen;
 RROutputPtr randr_output;
 RRCrtcPtr randr_crtc;
-- 
2.4.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH 1/4] xwayland: Remove the output from the list after destroying it

[Dima Ryazanov]

Signed-off-by: Dima Ryazanov 
---
 hw/xwayland/xwayland-output.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index 155cbc1..1d75d0b 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -190,6 +190,7 @@ xwl_output_destroy(struct xwl_output *xwl_output)
 wl_output_destroy(xwl_output->output);
 RRCrtcDestroy(xwl_output->randr_crtc);
 RROutputDestroy(xwl_output->randr_output);
+xorg_list_del(&xwl_output->link);
 free(xwl_output);
 }
 
-- 
2.4.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Tue, 12 May 2015 10:47:08 +0300
Pekka Paalanen  wrote:

> On Mon, 11 May 2015 16:07:26 +0100
> Daniel Stone  wrote:
> 
> > Hi,
> > 
> > On 11 May 2015 at 13:41, Pekka Paalanen  wrote:
> > > 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?
> > 
> > €10 says this is only on uncomposited X11 servers, where you require
> > Expose events ... ?
> 
> I find that hard to believe...
> 
> x11_output_repaint_shm() ->
> pixman_renderer_repaint_output() does a repaint to the shadow, then
> copy_to_hw_buffer() and just before returning it emits the
> output->frame_signal.
> 
> Output->frame_signal should lead to a call to
> weston_renderer::read_pixels, which reads from the hw_buffer, which is
> only a pixman_image_t...
> 
> So all of rendering and read_pixels should be already done when control
> returns to x11_output_repaint_shm(), so there is no room for X11 calls
> in between.
> 
> The hw_surface (hw_buffer) is created by pixman_image_create_bits()
> which is handed a pointer to a piece of memory from shmat().
> 
> Looks like screenshooting does not go through X11 protocol, so I'm
> puzzled why it doesn't seem to work. Also, the screenshooter (super+s)
> works just fine.

Seems to be a fun race somehow. If I add a sleep(1) in the test client
just before it asks the server to take a screenshot, the saved image
turns up fine instead of black.

Oh I know. The test runs before weston-desktop-shell has initialized.
That means there is no background to be shot yet.

I do not think adding the necessary infrastructure to wait for
weston-desktop-shell to come up is necessary at this point (it might
be useful later in more advanced tests). What you could do is have the
test client post a surface with know position and a known test pattern
as the content. That can reliably be expected to work.

The test still fails though, even if the captured image seems ok:

Loading reference image 
/home/pq/git/weston/tests/reference/internal-screenshot-00.png
Screenshot different from reference image
Clip: 50,50 20 x 20
Bytewise comparison inside clip
Mismatched image on row 50
50,0:ee 
50,1:   
50,2:   29   29 
50,3:   1b   1b 
50,4:ee 
50,5:   
50,6:   29   29 
50,7:   1b   1b 
50,8:ee 
50,9:   
50,10:   29 ffb2  <---
50,11:   1b ffae  <---
50,12:e ffa9  <---
50,13:   
50,14:   29 ffa2  <---
50,15:   1b ffa0  <---
50,16:e ff9e  <---
50,17:   
50,18:   29   10  <---
50,19:   1ba  <---
Screenshot doesn't match reference image in clipped area

My guess is the comparison fails due to bugs in bpp/stride handling
etc. The saved PNG files look identical apart from the date in the
panel.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 09/10] tests: Add internal test for the weston test screenshot capability

[Pekka Paalanen]

On Mon, 11 May 2015 15:24:28 +0300
Pekka Paalanen  wrote:

> On Wed,  6 May 2015 17:44:28 -0700
> Bryce Harrington  wrote:
> 
> > This also serves as a proof of concept of the screen capture
> > functionality and as a demo for snapshot-based rendering verification.
> > Implements screenshot saving clientside in the test itself.
> > 
> > This also demonstrates use of test-specific configuration files, in this
> > case to disable fadein animations and background images.
> > 
> > Signed-off-by: Bryce Harrington 
> > ---
> >  Makefile.am|  19 +++-
> >  tests/internal-screenshot-test.c   | 151 
> > +
> >  tests/internal-screenshot.ini  |   3 +
> >  tests/reference/internal-screenshot-00.png | Bin 0 -> 5149 bytes
> >  4 files changed, 172 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/internal-screenshot-test.c
> >  create mode 100644 tests/internal-screenshot.ini
> >  create mode 100644 tests/reference/internal-screenshot-00.png

> > +char *server_parameters="--use-pixman --width=320 --height=240";
> > +
> > +TEST(internal_screenshot)
> > +{
> > +   struct client *client;
> > +   struct surface *screenshot = NULL;
> > +   struct rectangle clip;
> > +   const char *fname;
> > +   cairo_surface_t *reference;
> > +   cairo_status_t status;
> > +   bool match = false;
> > +   bool dump_all_images = true;
> > +
> > +   printf("Starting test\n");
> > +
> > +   /* Create the client */
> > +   client = create_client_and_test_surface(100, 100, 100, 100);
> > +   assert(client);
> > +   printf("Client created\n");
> > +
> > +   /* Create a surface to hold the screenshot */
> > +   screenshot = xzalloc(sizeof *screenshot);
> 
> I think you are leaking this. Could be allocated from stack just as
> well.
> 
> > +   assert(screenshot);
> > +   screenshot->wl_surface = 
> > wl_compositor_create_surface(client->wl_compositor);
> > +   assert(screenshot->wl_surface);
> > +   printf("Screenshot surface created\n");

Oh, and this wl_surface is not needed for anything. Right?
It's also leaked from what I can tell.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Mon, 11 May 2015 16:07:26 +0100
Daniel Stone  wrote:

> Hi,
> 
> On 11 May 2015 at 13:41, Pekka Paalanen  wrote:
> > 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?
> 
> €10 says this is only on uncomposited X11 servers, where you require
> Expose events ... ?

I find that hard to believe...

x11_output_repaint_shm() ->
pixman_renderer_repaint_output() does a repaint to the shadow, then
copy_to_hw_buffer() and just before returning it emits the
output->frame_signal.

Output->frame_signal should lead to a call to
weston_renderer::read_pixels, which reads from the hw_buffer, which is
only a pixman_image_t...

So all of rendering and read_pixels should be already done when control
returns to x11_output_repaint_shm(), so there is no room for X11 calls
in between.

The hw_surface (hw_buffer) is created by pixman_image_create_bits()
which is handed a pointer to a piece of memory from shmat().

Looks like screenshooting does not go through X11 protocol, so I'm
puzzled why it doesn't seem to work. Also, the screenshooter (super+s)
works just fine.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 03/10] tests: Add an xmalloc helper function

[Pekka Paalanen]

On Tue, 12 May 2015 09:08:41 +0200
Quentin Glidic  wrote:

> On 2015-05-12 06:49, Bryce Harrington wrote:
> > On Mon, May 11, 2015 at 01:39:29PM +0300, Pekka Paalanen wrote:
> >> Hi,
> >>
> >> so the motivation for this patch is to make alloc failures just exit
> >> the process with EXIT_FAILURE than calling abort()? Or is to stop
> >> relying on assert()? Or to add the printf? Or just code consistency?
> >>
> >> Would be nice to know why... maybe I'll learn in the later patches.
> > 
> > Actually, the original motivation was that I needed an xmalloc.  I
> > noticed there was xzalloc already defined but it was done differently
> > from the one in clients/window.c.  So this patch is making the
> > definitions (and behavior) consistent across the codebase.
> > 
> > I considered moving the functions to the shared directory but felt like
> > they were not yet being used widely enough to justify that.  window.c
> > actually has a set of alloc routines that would be handy to make more
> > convenient for test writers.
> > 
> > The assert-vs-exit difference didn't seem like a big concern here.
> > Either way the test harness will count it as a failure.

Hi,

ok, so no complicated motivation behind it, just consistency. That's
good.

> From Automake manual[1]:
> “When no test protocol is in use, an exit status of 0 from a test script
> will denote a success, an exit status of 77 a skipped test, an exit
> status of 99 an hard error, and any other exit status will denote a
> failure.”

Sure, why not.

> I think these tests should use these values to make it clear what is
> happening. It also allows to have working XFAILS_TESTS to abort for real
> on a malloc failure, if in the future we want to have such tests.
> 
> [1]
> 
> 

Does XFAIL mean the test "should pass one day, but currently we expect
it to fail, because our code is known broken" or "this test is always
intended to fail/crash, because it checks a condition that must
fail/crash"?

I find the latter interpretation hard to use, because it is hard to
guarantee the particular expected failure is the only failure that can
occur.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 03/10] tests: Add an xmalloc helper function

[id3]

On 2015-05-12 06:49, Bryce Harrington wrote:
> On Mon, May 11, 2015 at 01:39:29PM +0300, Pekka Paalanen wrote:
>> Hi,
>>
>> so the motivation for this patch is to make alloc failures just exit
>> the process with EXIT_FAILURE than calling abort()? Or is to stop
>> relying on assert()? Or to add the printf? Or just code consistency?
>>
>> Would be nice to know why... maybe I'll learn in the later patches.
> 
> Actually, the original motivation was that I needed an xmalloc.  I
> noticed there was xzalloc already defined but it was done differently
> from the one in clients/window.c.  So this patch is making the
> definitions (and behavior) consistent across the codebase.
> 
> I considered moving the functions to the shared directory but felt like
> they were not yet being used widely enough to justify that.  window.c
> actually has a set of alloc routines that would be handy to make more
> convenient for test writers.
> 
> The assert-vs-exit difference didn't seem like a big concern here.
> Either way the test harness will count it as a failure.

From Automake manual[1]:
“When no test protocol is in use, an exit status of 0 from a test script
will denote a success, an exit status of 77 a skipped test, an exit
status of 99 an hard error, and any other exit status will denote a
failure.”

I think these tests should use these values to make it clear what is
happening. It also allows to have working XFAILS_TESTS to abort for real
on a malloc failure, if in the future we want to have such tests.

[1]


-- 

Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel