diff mbox

[RFC,v4,4/9] qxl: screen_dump in vga: do a single ppm_save

Message ID 20120222122616.GB607@garlic.redhat.com
State New
Headers show

Commit Message

Alon Levy Feb. 22, 2012, 12:26 p.m. UTC 
On Wed, Feb 22, 2012 at 12:10:08PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, Alon Levy wrote:
> > Using vga->screen_dump results in a number of calls to ppm_save,
> > instead of a single one.
> 
> Can you investigate why?

oh, I know why. vga_screen_dump implementation:

    screen_dump_filename = filename;
    vga_invalidate_display(s);
->  vga_hw_update();
    screen_dump_filename = NULL;

And the only user of screen_dump_filename is:

static void vga_save_dpy_update(DisplayState *ds,
                                int x, int y, int w, int h)
{
    if (screen_dump_filename) {
        ppm_save(screen_dump_filename, ds->surface);
    }
}

vga_save_dpy_update is called on any dpy_update, registered after the
first vga_screen_dump call.

Since there are a number of callers to dpy_update: vga_draw_text calls
it potentially once every line, vga_draw_graphic the same.
vga_draw_blank calls it once.

> 
> > Lacking time to test all the possible users of
> > vga->screen_dump, avoid the redundant calls by doing the vga_hw_update+
> > ppm_save in qxl_hw_screen_dump.
> 
> I'd prefer to fix the root cause instead of adding workarounds.
> 

This seems to work, only tested with -vga qxl and in vga mode:



> cheers,
>   Gerd

Comments

Gerd Hoffmann Feb. 22, 2012, 1:58 p.m. UTC | #1 
> And the only user of screen_dump_filename is:
> 
> static void vga_save_dpy_update(DisplayState *ds,
>                                 int x, int y, int w, int h)
> {
>     if (screen_dump_filename) {
>         ppm_save(screen_dump_filename, ds->surface);

upstream/master this here:

          screen_dump_filename = NULL;

>     }
> }

> This seems to work, only tested with -vga qxl and in vga mode:

The corner case where this fails is when console switching is needed,
i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
command there ...
Alon Levy Feb. 22, 2012, 2:25 p.m. UTC | #2 
On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
> > And the only user of screen_dump_filename is:
> > 
> > static void vga_save_dpy_update(DisplayState *ds,
> >                                 int x, int y, int w, int h)
> > {
> >     if (screen_dump_filename) {
> >         ppm_save(screen_dump_filename, ds->surface);
> 
> upstream/master this here:
> 
>           screen_dump_filename = NULL;
> 

That's wrong, you'll get the screenshot after the first update, who's to
say it is fully rendered?

> >     }
> > }
> 
> > This seems to work, only tested with -vga qxl and in vga mode:
> 
> The corner case where this fails is when console switching is needed,
> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
> command there ...

Are you talking about sdl console or linux guest console? ctrl-alt-2 or
ctrl-alt-F2? I can try both.

> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Gerd Hoffmann Feb. 22, 2012, 2:37 p.m. UTC | #3 
On 02/22/12 15:25, Alon Levy wrote:
> On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
>>> And the only user of screen_dump_filename is:
>>>
>>> static void vga_save_dpy_update(DisplayState *ds,
>>>                                 int x, int y, int w, int h)
>>> {
>>>     if (screen_dump_filename) {
>>>         ppm_save(screen_dump_filename, ds->surface);
>>
>> upstream/master this here:
>>
>>           screen_dump_filename = NULL;
>>
> 
> That's wrong, you'll get the screenshot after the first update, who's to
> say it is fully rendered?

vga code actually does a single, fullscreen update after
vga_invalidate_display(), so it should work fine.

>> The corner case where this fails is when console switching is needed,
>> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
>> command there ...
> 
> Are you talking about sdl console or linux guest console? ctrl-alt-2 or
> ctrl-alt-F2? I can try both.

ctrl-alt-2 sdl console.

cheers,
  Gerd
Alon Levy Feb. 22, 2012, 3:27 p.m. UTC | #4 
On Wed, Feb 22, 2012 at 03:37:46PM +0100, Gerd Hoffmann wrote:
> On 02/22/12 15:25, Alon Levy wrote:
> > On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
> >>> And the only user of screen_dump_filename is:
> >>>
> >>> static void vga_save_dpy_update(DisplayState *ds,
> >>>                                 int x, int y, int w, int h)
> >>> {
> >>>     if (screen_dump_filename) {
> >>>         ppm_save(screen_dump_filename, ds->surface);
> >>
> >> upstream/master this here:
> >>
> >>           screen_dump_filename = NULL;
> >>
> > 
> > That's wrong, you'll get the screenshot after the first update, who's to
> > say it is fully rendered?
> 
> vga code actually does a single, fullscreen update after
> vga_invalidate_display(), so it should work fine.

ok, so I'll just use your one liner and see.

> 
> >> The corner case where this fails is when console switching is needed,
> >> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
> >> command there ...
> > 
> > Are you talking about sdl console or linux guest console? ctrl-alt-2 or
> > ctrl-alt-F2? I can try both.
> 
> ctrl-alt-2 sdl console.
> 
> cheers,
>   Gerd
> 
>
diff mbox

Patch

diff --git a/hw/vga.c b/hw/vga.c
index c1029db..51f20c1 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -163,8 +163,6 @@  static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
 static void vga_screen_dump(void *opaque, const char *filename);
-static const char *screen_dump_filename;
-static DisplayChangeListener *screen_dump_dcl;
 
 static void vga_update_memory_access(VGACommonState *s)
 {
@@ -2364,22 +2362,6 @@  void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 /********************************************************/
 /* vga screen dump */
 
-static void vga_save_dpy_update(DisplayState *ds,
-                                int x, int y, int w, int h)
-{
-    if (screen_dump_filename) {
-        ppm_save(screen_dump_filename, ds->surface);
-    }
-}
-
-static void vga_save_dpy_resize(DisplayState *s)
-{
-}
-
-static void vga_save_dpy_refresh(DisplayState *s)
-{
-}
-
 int ppm_save(const char *filename, struct DisplaySurface *ds)
 {
     FILE *f;
@@ -2389,10 +2371,12 @@  int ppm_save(const char *filename, struct DisplaySurface *ds)
     uint8_t r, g, b;
     int ret;
     char *linebuf, *pbuf;
+    static int calls;
 
     f = fopen(filename, "wb");
     if (!f)
         return -1;
+    fprintf(stderr, "ppm_save %d\n", ++calls);
     fprintf(f, "P6\n%d %d\n%d\n",
             ds->width, ds->height, 255);
     linebuf = g_malloc(ds->width * 3);
@@ -2423,29 +2407,13 @@  int ppm_save(const char *filename, struct DisplaySurface *ds)
     return 0;
 }
 
-static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
-{
-    DisplayChangeListener *dcl;
-
-    dcl = g_malloc0(sizeof(DisplayChangeListener));
-    dcl->dpy_update = vga_save_dpy_update;
-    dcl->dpy_resize = vga_save_dpy_resize;
-    dcl->dpy_refresh = vga_save_dpy_refresh;
-    register_displaychangelistener(ds, dcl);
-    return dcl;
-}
-
 /* save the vga display in a PPM image even if no display is
    available */
 static void vga_screen_dump(void *opaque, const char *filename)
 {
     VGACommonState *s = opaque;
 
-    if (!screen_dump_dcl)
-        screen_dump_dcl = vga_screen_dump_init(s->ds);
-
-    screen_dump_filename = filename;
     vga_invalidate_display(s);
     vga_hw_update();
-    screen_dump_filename = NULL;
+    ppm_save(filename, s->ds->surface);
 }