| Message ID | 20220301064424.136234-2-kshitij.suri@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG | expand |
Hi, Hope this mail finds you well. I have updated the code as required and request you to review and suggest changes that are needed to be implemented. In case no change is required, please do let me know the next steps for the same. Regards, Kshitij Suri On 01/03/22 12:14 pm, Kshitij Suri wrote: > Currently screendump only supports PPM format, which is un-compressed and not > standard. Added a "format" parameter to qemu monitor screendump capabilites > to support PNG image capture using libpng. The param was added in QAPI schema > of screendump present in ui.json along with png_save() function which converts > pixman_image to PNG. HMP command equivalent was also modified to support the > feature. > > Example usage: > { "execute": "screendump", "arguments": { "filename": "/tmp/image", > "format":"png" } } > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/718 > > Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> > --- > diff to v1: > - Removed repeated alpha conversion operation. > - Modified logic to mirror png conversion in vnc-enc-tight.c file. > - Added a new CONFIG_PNG parameter for libpng support. > - Changed input format to enum instead of string. > - Improved error handling. > hmp-commands.hx | 11 ++--- > monitor/hmp-cmds.c | 19 ++++++++- > qapi/ui.json | 25 +++++++++-- > ui/console.c | 102 +++++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 145 insertions(+), 12 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 70a9136ac2..5eda4eeb24 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -244,17 +244,18 @@ ERST > > { > .name = "screendump", > - .args_type = "filename:F,device:s?,head:i?", > - .params = "filename [device [head]]", > - .help = "save screen from head 'head' of display device 'device' " > - "into PPM image 'filename'", > + .args_type = "filename:F,device:s?,head:i?,format:f?", > + .params = "filename [device [head]] [format]", > + .help = "save screen from head 'head' of display device 'device'" > + "in specified format 'format' as image 'filename'." > + "Currently only 'png' and 'ppm' formats are supported.", > .cmd = hmp_screendump, > .coroutine = true, > }, > > SRST > ``screendump`` *filename* > - Save screen into PPM image *filename*. > + Save screen as an image *filename*, with default format of PPM. > ERST > > { > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 8c384dc1b2..9a640146eb 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1677,9 +1677,26 @@ hmp_screendump(Monitor *mon, const QDict *qdict) > const char *filename = qdict_get_str(qdict, "filename"); > const char *id = qdict_get_try_str(qdict, "device"); > int64_t head = qdict_get_try_int(qdict, "head", 0); > + const char *input_format = qdict_get_str(qdict, "format"); > Error *err = NULL; > + ImageFormat format; > > - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err); > + int val = qapi_enum_parse(&ImageFormat_lookup, input_format, -1, &err); > + if (val < 0) { > + goto end; > + } > + > + switch (val) { > + case IMAGE_FORMAT_PNG: > + format = IMAGE_FORMAT_PNG; > + break; > + default: > + format = IMAGE_FORMAT_PPM; > + } > + > + qmp_screendump(filename, id != NULL, id, id != NULL, head, > + input_format != NULL, format, &err); > +end: > hmp_handle_error(mon, err); > } > > diff --git a/qapi/ui.json b/qapi/ui.json > index 9354f4c467..6aa0dd7c1b 100644 > --- a/qapi/ui.json > +++ b/qapi/ui.json > @@ -73,12 +73,27 @@ > ## > { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} } > > +## > +# @ImageFormat: > +# > +# Available list of supported types. > +# > +# @png: PNG format > +# > +# @ppm: PPM format > +# > +# Since: 7.0 > +# > +## > +{ 'enum': 'ImageFormat', > + 'data': ['ppm', 'png'] } > + > ## > # @screendump: > # > -# Write a PPM of the VGA screen to a file. > +# Write a screenshot of the VGA screen to a file. > # > -# @filename: the path of a new PPM file to store the image > +# @filename: the path of a new file to store the image > # > # @device: ID of the display device that should be dumped. If this parameter > # is missing, the primary display will be used. (Since 2.12) > @@ -87,6 +102,9 @@ > # parameter is missing, head #0 will be used. Also note that the head > # can only be specified in conjunction with the device ID. (Since 2.12) > # > +# @format: image format for screendump is specified. ppm is the set as the > +# default format. (Since 7.0) > +# > # Returns: Nothing on success > # > # Since: 0.14 > @@ -99,7 +117,8 @@ > # > ## > { 'command': 'screendump', > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int', > + '*format': 'ImageFormat'}, > 'coroutine': true } > > ## > diff --git a/ui/console.c b/ui/console.c > index 40eebb6d2c..aab561e1ac 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -37,6 +37,9 @@ > #include "exec/memory.h" > #include "io/channel-file.h" > #include "qom/object.h" > +#ifdef CONFIG_PNG > +#include <png.h> > +#endif > > #define DEFAULT_BACKSCROLL 512 > #define CONSOLE_CURSOR_PERIOD 500 > @@ -289,6 +292,89 @@ void graphic_hw_invalidate(QemuConsole *con) > } > } > > +#ifdef CONFIG_PNG > +/** > + * png_save: Take a screenshot as PNG > + * > + * Saves screendump as a PNG file > + * > + * Returns true for success or false for error. > + * > + * @fd: File descriptor for PNG file. > + * @image: Image data in pixman format. > + * @errp: Pointer to an error. > + */ > +static bool png_save(int fd, pixman_image_t *image, Error **errp) > +{ > + int width = pixman_image_get_width(image); > + int height = pixman_image_get_height(image); > + g_autofree png_struct *png_ptr = NULL; > + g_autofree png_info *info_ptr = NULL; > + g_autoptr(pixman_image_t) linebuf = > + qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, width); > + uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf); > + FILE *f = fdopen(fd, "wb"); > + int y; > + if (!f) { > + error_setg_errno(errp, errno, > + "Failed to create file from file descriptor"); > + return false; > + } > + > + png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, > + NULL, NULL); > + if (!png_ptr) { > + error_setg(errp, "PNG creation failed. Unable to write struct"); > + fclose(f); > + return false; > + } > + > + info_ptr = png_create_info_struct(png_ptr); > + > + if (!info_ptr) { > + error_setg(errp, "PNG creation failed. Unable to write info"); > + fclose(f); > + png_destroy_write_struct(&png_ptr, &info_ptr); > + return false; > + } > + > + png_init_io(png_ptr, f); > + > + png_set_IHDR(png_ptr, info_ptr, width, height, 8, > + PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, > + PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); > + > + png_write_info(png_ptr, info_ptr); > + > + for (y = 0; y < height; ++y) { > + qemu_pixman_linebuf_fill(linebuf, image, width, 0, y); > + png_write_row(png_ptr, buf); > + } > + qemu_pixman_image_unref(linebuf); > + > + png_write_end(png_ptr, NULL); > + > + png_destroy_write_struct(&png_ptr, &info_ptr); > + > + if (fclose(f) != 0) { > + error_setg_errno(errp, errno, > + "PNG creation failed. Unable to close file"); > + return false; > + } > + > + return true; > +} > + > +#else /* no png support */ > + > +static bool png_save(int fd, pixman_image_t *image, Error **errp) > +{ > + error_setg(errp, "Enable PNG support with libpng for screendump"); > + return false; > +} > + > +#endif /* CONFIG_PNG */ > + > static bool ppm_save(int fd, pixman_image_t *image, Error **errp) > { > int width = pixman_image_get_width(image); > @@ -327,7 +413,8 @@ static void graphic_hw_update_bh(void *con) > /* Safety: coroutine-only, concurrent-coroutine safe, main thread only */ > void coroutine_fn > qmp_screendump(const char *filename, bool has_device, const char *device, > - bool has_head, int64_t head, Error **errp) > + bool has_head, int64_t head, bool has_format, > + ImageFormat format, Error **errp) > { > g_autoptr(pixman_image_t) image = NULL; > QemuConsole *con; > @@ -383,8 +470,17 @@ qmp_screendump(const char *filename, bool has_device, const char *device, > * yields and releases the BQL. It could produce corrupted dump, but > * it should be otherwise safe. > */ > - if (!ppm_save(fd, image, errp)) { > - qemu_unlink(filename); > + > + if (has_format && format == IMAGE_FORMAT_PNG) { > + /* PNG format specified for screendump */ > + if (!png_save(fd, image, errp)) { > + qemu_unlink(filename); > + } > + } else { > + /* PPM format specified/default for screendump */ > + if (!ppm_save(fd, image, errp)) { > + qemu_unlink(filename); > + } > } > } >
Kshitij Suri <kshitij.suri@nutanix.com> writes: > Currently screendump only supports PPM format, which is un-compressed and not > standard. Added a "format" parameter to qemu monitor screendump capabilites > to support PNG image capture using libpng. The param was added in QAPI schema > of screendump present in ui.json along with png_save() function which converts > pixman_image to PNG. HMP command equivalent was also modified to support the > feature. > > Example usage: > { "execute": "screendump", "arguments": { "filename": "/tmp/image", > "format":"png" } } > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/718 > > Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> > --- [...] > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 8c384dc1b2..9a640146eb 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1677,9 +1677,26 @@ hmp_screendump(Monitor *mon, const QDict *qdict) > const char *filename = qdict_get_str(qdict, "filename"); > const char *id = qdict_get_try_str(qdict, "device"); > int64_t head = qdict_get_try_int(qdict, "head", 0); > + const char *input_format = qdict_get_str(qdict, "format"); > Error *err = NULL; > + ImageFormat format; > > - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err); > + int val = qapi_enum_parse(&ImageFormat_lookup, input_format, -1, &err); > + if (val < 0) { > + goto end; > + } > + > + switch (val) { > + case IMAGE_FORMAT_PNG: > + format = IMAGE_FORMAT_PNG; > + break; > + default: > + format = IMAGE_FORMAT_PPM; > + } > + > + qmp_screendump(filename, id != NULL, id, id != NULL, head, > + input_format != NULL, format, &err); > +end: > hmp_handle_error(mon, err); > } > > diff --git a/qapi/ui.json b/qapi/ui.json > index 9354f4c467..6aa0dd7c1b 100644 > --- a/qapi/ui.json > +++ b/qapi/ui.json > @@ -73,12 +73,27 @@ > ## > { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} } > > +## > +# @ImageFormat: > +# > +# Available list of supported types. This is just a hair better than "Lorem ipsum" :) Suggest: Supported image format types. > +# > +# @png: PNG format > +# > +# @ppm: PPM format > +# > +# Since: 7.0 > +# > +## > +{ 'enum': 'ImageFormat', > + 'data': ['ppm', 'png'] } > + > ## > # @screendump: > # > -# Write a PPM of the VGA screen to a file. > +# Write a screenshot of the VGA screen to a file. Is "VGA screen" accurate? Or does this work for other displays, too? > # > -# @filename: the path of a new PPM file to store the image > +# @filename: the path of a new file to store the image > # > # @device: ID of the display device that should be dumped. If this parameter > # is missing, the primary display will be used. (Since 2.12) > @@ -87,6 +102,9 @@ > # parameter is missing, head #0 will be used. Also note that the head > # can only be specified in conjunction with the device ID. (Since 2.12) > # > +# @format: image format for screendump is specified. ppm is the set as the > +# default format. (Since 7.0) I figure you mean "is set as the default". Suggest to replace the sentence by "(default: ppm)". > +# > # Returns: Nothing on success > # > # Since: 0.14 > @@ -99,7 +117,8 @@ > # > ## > { 'command': 'screendump', > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int', > + '*format': 'ImageFormat'}, > 'coroutine': true } > > ## [...]
On 11/03/22 5:50 pm, Markus Armbruster wrote: > Kshitij Suri <kshitij.suri@nutanix.com> writes: > >> Currently screendump only supports PPM format, which is un-compressed and not >> standard. Added a "format" parameter to qemu monitor screendump capabilites >> to support PNG image capture using libpng. The param was added in QAPI schema >> of screendump present in ui.json along with png_save() function which converts >> pixman_image to PNG. HMP command equivalent was also modified to support the >> feature. >> >> Example usage: >> { "execute": "screendump", "arguments": { "filename": "/tmp/image", >> "format":"png" } } >> >> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e= >> >> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> >> --- > [...] > >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index 8c384dc1b2..9a640146eb 100644 >> --- a/monitor/hmp-cmds.c >> +++ b/monitor/hmp-cmds.c >> @@ -1677,9 +1677,26 @@ hmp_screendump(Monitor *mon, const QDict *qdict) >> const char *filename = qdict_get_str(qdict, "filename"); >> const char *id = qdict_get_try_str(qdict, "device"); >> int64_t head = qdict_get_try_int(qdict, "head", 0); >> + const char *input_format = qdict_get_str(qdict, "format"); >> Error *err = NULL; >> + ImageFormat format; >> >> - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err); >> + int val = qapi_enum_parse(&ImageFormat_lookup, input_format, -1, &err); >> + if (val < 0) { >> + goto end; >> + } >> + >> + switch (val) { >> + case IMAGE_FORMAT_PNG: >> + format = IMAGE_FORMAT_PNG; >> + break; >> + default: >> + format = IMAGE_FORMAT_PPM; >> + } >> + >> + qmp_screendump(filename, id != NULL, id, id != NULL, head, >> + input_format != NULL, format, &err); >> +end: >> hmp_handle_error(mon, err); >> } >> >> diff --git a/qapi/ui.json b/qapi/ui.json >> index 9354f4c467..6aa0dd7c1b 100644 >> --- a/qapi/ui.json >> +++ b/qapi/ui.json >> @@ -73,12 +73,27 @@ >> ## >> { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} } >> >> +## >> +# @ImageFormat: >> +# >> +# Available list of supported types. > This is just a hair better than "Lorem ipsum" :) > > Suggest: Supported image format types. Will add in the updated patch > >> +# >> +# @png: PNG format >> +# >> +# @ppm: PPM format >> +# >> +# Since: 7.0 >> +# >> +## >> +{ 'enum': 'ImageFormat', >> + 'data': ['ppm', 'png'] } >> + >> ## >> # @screendump: >> # >> -# Write a PPM of the VGA screen to a file. >> +# Write a screenshot of the VGA screen to a file. > Is "VGA screen" accurate? Or does this work for other displays, too? The patch didn't modify any display changes and VGA screen was previously supported display type. > >> # >> -# @filename: the path of a new PPM file to store the image >> +# @filename: the path of a new file to store the image >> # >> # @device: ID of the display device that should be dumped. If this parameter >> # is missing, the primary display will be used. (Since 2.12) >> @@ -87,6 +102,9 @@ >> # parameter is missing, head #0 will be used. Also note that the head >> # can only be specified in conjunction with the device ID. (Since 2.12) >> # >> +# @format: image format for screendump is specified. ppm is the set as the >> +# default format. (Since 7.0) > I figure you mean "is set as the default". Suggest to replace the > sentence by "(default: ppm)". Will add in the updated patch. > >> +# >> # Returns: Nothing on success >> # >> # Since: 0.14 >> @@ -99,7 +117,8 @@ >> # >> ## >> { 'command': 'screendump', >> - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, >> + 'data': {'filename': 'str', '*device': 'str', '*head': 'int', >> + '*format': 'ImageFormat'}, >> 'coroutine': true } >> >> ## > [...] Thank you for your review! Regards, Kshitij Suri >
Kshitij Suri <kshitij.suri@nutanix.com> writes: > On 11/03/22 5:50 pm, Markus Armbruster wrote: >> Kshitij Suri <kshitij.suri@nutanix.com> writes: >> >>> Currently screendump only supports PPM format, which is un-compressed and not >>> standard. Added a "format" parameter to qemu monitor screendump capabilites >>> to support PNG image capture using libpng. The param was added in QAPI schema >>> of screendump present in ui.json along with png_save() function which converts >>> pixman_image to PNG. HMP command equivalent was also modified to support the >>> feature. >>> >>> Example usage: >>> { "execute": "screendump", "arguments": { "filename": "/tmp/image", >>> "format":"png" } } >>> >>> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e= >>> >>> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> [...] >>> diff --git a/qapi/ui.json b/qapi/ui.json >>> index 9354f4c467..6aa0dd7c1b 100644 >>> --- a/qapi/ui.json >>> +++ b/qapi/ui.json [...] >>> ## >>> # @screendump: >>> # >>> -# Write a PPM of the VGA screen to a file. >>> +# Write a screenshot of the VGA screen to a file. >> >> Is "VGA screen" accurate? Or does this work for other displays, too? > > The patch didn't modify any display changes and VGA screen was > previously supported display type. Let me rephrase my question: was "VGA screen" accurate before your patch? [...]
On Tue, Mar 15, 2022 at 11:06:31AM +0100, Markus Armbruster wrote: > Kshitij Suri <kshitij.suri@nutanix.com> writes: > > > On 11/03/22 5:50 pm, Markus Armbruster wrote: > >> Kshitij Suri <kshitij.suri@nutanix.com> writes: > >> > >>> Currently screendump only supports PPM format, which is un-compressed and not > >>> standard. Added a "format" parameter to qemu monitor screendump capabilites > >>> to support PNG image capture using libpng. The param was added in QAPI schema > >>> of screendump present in ui.json along with png_save() function which converts > >>> pixman_image to PNG. HMP command equivalent was also modified to support the > >>> feature. > >>> > >>> Example usage: > >>> { "execute": "screendump", "arguments": { "filename": "/tmp/image", > >>> "format":"png" } } > >>> > >>> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e= > >>> > >>> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> > > [...] > > >>> diff --git a/qapi/ui.json b/qapi/ui.json > >>> index 9354f4c467..6aa0dd7c1b 100644 > >>> --- a/qapi/ui.json > >>> +++ b/qapi/ui.json > > [...] > > >>> ## > >>> # @screendump: > >>> # > >>> -# Write a PPM of the VGA screen to a file. > >>> +# Write a screenshot of the VGA screen to a file. > >> > >> Is "VGA screen" accurate? Or does this work for other displays, too? > > > > The patch didn't modify any display changes and VGA screen was > > previously supported display type. > > Let me rephrase my question: was "VGA screen" accurate before your > patch? No, it would be better phrased as "Capture the specified screen contents and write it to a file" In a multi-head scenario, it can be any of the output heads, and whether the head is in a VGA mode or not is irrelevant to the command functionality. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Mar 15, 2022 at 11:06:31AM +0100, Markus Armbruster wrote: >> Kshitij Suri <kshitij.suri@nutanix.com> writes: >> >> > On 11/03/22 5:50 pm, Markus Armbruster wrote: >> >> Kshitij Suri <kshitij.suri@nutanix.com> writes: >> >> >> >>> Currently screendump only supports PPM format, which is un-compressed and not >> >>> standard. Added a "format" parameter to qemu monitor screendump capabilites >> >>> to support PNG image capture using libpng. The param was added in QAPI schema >> >>> of screendump present in ui.json along with png_save() function which converts >> >>> pixman_image to PNG. HMP command equivalent was also modified to support the >> >>> feature. >> >>> >> >>> Example usage: >> >>> { "execute": "screendump", "arguments": { "filename": "/tmp/image", >> >>> "format":"png" } } >> >>> >> >>> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e= >> >>> >> >>> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> >> >> [...] >> >> >>> diff --git a/qapi/ui.json b/qapi/ui.json >> >>> index 9354f4c467..6aa0dd7c1b 100644 >> >>> --- a/qapi/ui.json >> >>> +++ b/qapi/ui.json >> >> [...] >> >> >>> ## >> >>> # @screendump: >> >>> # >> >>> -# Write a PPM of the VGA screen to a file. >> >>> +# Write a screenshot of the VGA screen to a file. >> >> >> >> Is "VGA screen" accurate? Or does this work for other displays, too? >> > >> > The patch didn't modify any display changes and VGA screen was >> > previously supported display type. >> >> Let me rephrase my question: was "VGA screen" accurate before your >> patch? > > No, it would be better phrased as > > "Capture the specified screen contents and write it to a file" > > In a multi-head scenario, it can be any of the output heads, and > whether the head is in a VGA mode or not is irrelevant to the > command functionality. Makes sense to me.
On 15/03/22 3:49 pm, Daniel P. Berrangé wrote: > On Tue, Mar 15, 2022 at 11:06:31AM +0100, Markus Armbruster wrote: >> Kshitij Suri <kshitij.suri@nutanix.com> writes: >> >>> On 11/03/22 5:50 pm, Markus Armbruster wrote: >>>> Kshitij Suri <kshitij.suri@nutanix.com> writes: >>>> >>>>> Currently screendump only supports PPM format, which is un-compressed and not >>>>> standard. Added a "format" parameter to qemu monitor screendump capabilites >>>>> to support PNG image capture using libpng. The param was added in QAPI schema >>>>> of screendump present in ui.json along with png_save() function which converts >>>>> pixman_image to PNG. HMP command equivalent was also modified to support the >>>>> feature. >>>>> >>>>> Example usage: >>>>> { "execute": "screendump", "arguments": { "filename": "/tmp/image", >>>>> "format":"png" } } >>>>> >>>>> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e= >>>>> >>>>> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> >> [...] >> >>>>> diff --git a/qapi/ui.json b/qapi/ui.json >>>>> index 9354f4c467..6aa0dd7c1b 100644 >>>>> --- a/qapi/ui.json >>>>> +++ b/qapi/ui.json >> [...] >> >>>>> ## >>>>> # @screendump: >>>>> # >>>>> -# Write a PPM of the VGA screen to a file. >>>>> +# Write a screenshot of the VGA screen to a file. >>>> Is "VGA screen" accurate? Or does this work for other displays, too? >>> The patch didn't modify any display changes and VGA screen was >>> previously supported display type. >> Let me rephrase my question: was "VGA screen" accurate before your >> patch? > No, it would be better phrased as > > "Capture the specified screen contents and write it to a file" > > In a multi-head scenario, it can be any of the output heads, and > whether the head is in a VGA mode or not is irrelevant to the > command functionality. > > Regards, > Daniel Thank you! Will modify in the upcoming patch. Regards, Kshitij Suri
diff to v1: - Removed repeated alpha conversion operation. - Modified logic to mirror png conversion in vnc-enc-tight.c file. - Added a new CONFIG_PNG parameter for libpng support. - Changed input format to enum instead of string. - Improved error handling. hmp-commands.hx | 11 ++--- monitor/hmp-cmds.c | 19 ++++++++- qapi/ui.json | 25 +++++++++-- ui/console.c | 102 +++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 145 insertions(+), 12 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index 70a9136ac2..5eda4eeb24 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -244,17 +244,18 @@ ERST { .name = "screendump", - .args_type = "filename:F,device:s?,head:i?", - .params = "filename [device [head]]", - .help = "save screen from head 'head' of display device 'device' " - "into PPM image 'filename'", + .args_type = "filename:F,device:s?,head:i?,format:f?", + .params = "filename [device [head]] [format]", + .help = "save screen from head 'head' of display device 'device'" + "in specified format 'format' as image 'filename'." + "Currently only 'png' and 'ppm' formats are supported.", .cmd = hmp_screendump, .coroutine = true, }, SRST ``screendump`` *filename* - Save screen into PPM image *filename*. + Save screen as an image *filename*, with default format of PPM. ERST { diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 8c384dc1b2..9a640146eb 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1677,9 +1677,26 @@ hmp_screendump(Monitor *mon, const QDict *qdict) const char *filename = qdict_get_str(qdict, "filename"); const char *id = qdict_get_try_str(qdict, "device"); int64_t head = qdict_get_try_int(qdict, "head", 0); + const char *input_format = qdict_get_str(qdict, "format"); Error *err = NULL; + ImageFormat format; - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err); + int val = qapi_enum_parse(&ImageFormat_lookup, input_format, -1, &err); + if (val < 0) { + goto end; + } + + switch (val) { + case IMAGE_FORMAT_PNG: + format = IMAGE_FORMAT_PNG; + break; + default: + format = IMAGE_FORMAT_PPM; + } + + qmp_screendump(filename, id != NULL, id, id != NULL, head, + input_format != NULL, format, &err); +end: hmp_handle_error(mon, err); } diff --git a/qapi/ui.json b/qapi/ui.json index 9354f4c467..6aa0dd7c1b 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -73,12 +73,27 @@ ## { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} } +## +# @ImageFormat: +# +# Available list of supported types. +# +# @png: PNG format +# +# @ppm: PPM format +# +# Since: 7.0 +# +## +{ 'enum': 'ImageFormat', + 'data': ['ppm', 'png'] } + ## # @screendump: # -# Write a PPM of the VGA screen to a file. +# Write a screenshot of the VGA screen to a file. # -# @filename: the path of a new PPM file to store the image +# @filename: the path of a new file to store the image # # @device: ID of the display device that should be dumped. If this parameter # is missing, the primary display will be used. (Since 2.12) @@ -87,6 +102,9 @@ # parameter is missing, head #0 will be used. Also note that the head # can only be specified in conjunction with the device ID. (Since 2.12) # +# @format: image format for screendump is specified. ppm is the set as the +# default format. (Since 7.0) +# # Returns: Nothing on success # # Since: 0.14 @@ -99,7 +117,8 @@ # ## { 'command': 'screendump', - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, + 'data': {'filename': 'str', '*device': 'str', '*head': 'int', + '*format': 'ImageFormat'}, 'coroutine': true } ## diff --git a/ui/console.c b/ui/console.c index 40eebb6d2c..aab561e1ac 100644 --- a/ui/console.c +++ b/ui/console.c @@ -37,6 +37,9 @@ #include "exec/memory.h" #include "io/channel-file.h" #include "qom/object.h" +#ifdef CONFIG_PNG +#include <png.h> +#endif #define DEFAULT_BACKSCROLL 512 #define CONSOLE_CURSOR_PERIOD 500 @@ -289,6 +292,89 @@ void graphic_hw_invalidate(QemuConsole *con) } } +#ifdef CONFIG_PNG +/** + * png_save: Take a screenshot as PNG + * + * Saves screendump as a PNG file + * + * Returns true for success or false for error. + * + * @fd: File descriptor for PNG file. + * @image: Image data in pixman format. + * @errp: Pointer to an error. + */ +static bool png_save(int fd, pixman_image_t *image, Error **errp) +{ + int width = pixman_image_get_width(image); + int height = pixman_image_get_height(image); + g_autofree png_struct *png_ptr = NULL; + g_autofree png_info *info_ptr = NULL; + g_autoptr(pixman_image_t) linebuf = + qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, width); + uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf); + FILE *f = fdopen(fd, "wb"); + int y; + if (!f) { + error_setg_errno(errp, errno, + "Failed to create file from file descriptor"); + return false; + } + + png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, + NULL, NULL); + if (!png_ptr) { + error_setg(errp, "PNG creation failed. Unable to write struct"); + fclose(f); + return false; + } + + info_ptr = png_create_info_struct(png_ptr); + + if (!info_ptr) { + error_setg(errp, "PNG creation failed. Unable to write info"); + fclose(f); + png_destroy_write_struct(&png_ptr, &info_ptr); + return false; + } + + png_init_io(png_ptr, f); + + png_set_IHDR(png_ptr, info_ptr, width, height, 8, + PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, + PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); + + png_write_info(png_ptr, info_ptr); + + for (y = 0; y < height; ++y) { + qemu_pixman_linebuf_fill(linebuf, image, width, 0, y); + png_write_row(png_ptr, buf); + } + qemu_pixman_image_unref(linebuf); + + png_write_end(png_ptr, NULL); + + png_destroy_write_struct(&png_ptr, &info_ptr); + + if (fclose(f) != 0) { + error_setg_errno(errp, errno, + "PNG creation failed. Unable to close file"); + return false; + } + + return true; +} + +#else /* no png support */ + +static bool png_save(int fd, pixman_image_t *image, Error **errp) +{ + error_setg(errp, "Enable PNG support with libpng for screendump"); + return false; +} + +#endif /* CONFIG_PNG */ + static bool ppm_save(int fd, pixman_image_t *image, Error **errp) { int width = pixman_image_get_width(image); @@ -327,7 +413,8 @@ static void graphic_hw_update_bh(void *con) /* Safety: coroutine-only, concurrent-coroutine safe, main thread only */ void coroutine_fn qmp_screendump(const char *filename, bool has_device, const char *device, - bool has_head, int64_t head, Error **errp) + bool has_head, int64_t head, bool has_format, + ImageFormat format, Error **errp) { g_autoptr(pixman_image_t) image = NULL; QemuConsole *con; @@ -383,8 +470,17 @@ qmp_screendump(const char *filename, bool has_device, const char *device, * yields and releases the BQL. It could produce corrupted dump, but * it should be otherwise safe. */ - if (!ppm_save(fd, image, errp)) { - qemu_unlink(filename); + + if (has_format && format == IMAGE_FORMAT_PNG) { + /* PNG format specified for screendump */ + if (!png_save(fd, image, errp)) { + qemu_unlink(filename); + } + } else { + /* PPM format specified/default for screendump */ + if (!ppm_save(fd, image, errp)) { + qemu_unlink(filename); + } } }
Currently screendump only supports PPM format, which is un-compressed and not standard. Added a "format" parameter to qemu monitor screendump capabilites to support PNG image capture using libpng. The param was added in QAPI schema of screendump present in ui.json along with png_save() function which converts pixman_image to PNG. HMP command equivalent was also modified to support the feature. Example usage: { "execute": "screendump", "arguments": { "filename": "/tmp/image", "format":"png" } } Resolves: https://gitlab.com/qemu-project/qemu/-/issues/718 Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com> ---