| Message ID | 20220328165415.2102-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 |
If I count correctly, this is the fifth posting tagged "v2". Don't do that, please, as it's quite confusing.
On 29/03/22 12:12 pm, Markus Armbruster wrote: > If I count correctly, this is the fifth posting tagged "v2". Don't do > that, please, as it's quite confusing. > Thank you for your review and I apologise for that since I am fairly new to upstreaming. As per what I read version updates should be done only when there are significant design changes to the patch which didn't happen in the v2 version. Will update it to v3 and send the patch. Regards, Kshitij Suri
Kshitij Suri <kshitij.suri@nutanix.com> writes: > On 29/03/22 12:12 pm, Markus Armbruster wrote: >> If I count correctly, this is the fifth posting tagged "v2". Don't do >> that, please, as it's quite confusing. >> > Thank you for your review and I apologise for that since I am fairly > new to upstreaming. As per what I read version updates should be done > only when there are significant design changes to the patch which > didn't happen in the v2 version. Will update it to v3 and send the > patch. We all make mistakes :) The purpose of the version tag in the subject is to help humans with keeping track of patch submissions. Increment it for every submission. If you need to resend a submission completely unchanged for some reason, you may want to keep the tag and add "RESEND". A cover letter (git format-patch --cover-letter) lets you write an introduction to the whole series. Simple series may not need an introduction, but complex ones do. I always use one except when the "series" is a single patch. Keeping a change log in the cover letter helps people who already reviewed previous iterations. Check out https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03977.html for an example. Not every cover letter needs to be that verbose, of course. Likewise, the level of detail in change logs varies. A good way to get a feel for good cover letters and commit messages is to review patches. What kind of information helps you as a reviewer? That's the kind of information you want to provide with your submissions. Hope this helps!
On 29/03/22 1:29 pm, Markus Armbruster wrote: > Kshitij Suri <kshitij.suri@nutanix.com> writes: > >> On 29/03/22 12:12 pm, Markus Armbruster wrote: >>> If I count correctly, this is the fifth posting tagged "v2". Don't do >>> that, please, as it's quite confusing. >>> >> Thank you for your review and I apologise for that since I am fairly >> new to upstreaming. As per what I read version updates should be done >> only when there are significant design changes to the patch which >> didn't happen in the v2 version. Will update it to v3 and send the >> patch. > We all make mistakes :) > > The purpose of the version tag in the subject is to help humans with > keeping track of patch submissions. Increment it for every submission. > > If you need to resend a submission completely unchanged for some reason, > you may want to keep the tag and add "RESEND". > > A cover letter (git format-patch --cover-letter) lets you write an > introduction to the whole series. Simple series may not need an > introduction, but complex ones do. I always use one except when the > "series" is a single patch. > > Keeping a change log in the cover letter helps people who already > reviewed previous iterations. > > Check out > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D03_msg03977.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=D-rls6IoKGB-dpNu3-R_2PSp8HMjagXZU2NC6nqi6DhlurlWsW1vY9ip-7dSaSuv&s=kBAJFSYBE5TTs27r0Ycx7U2orcQfBt6L8Uslbk_X8xY&e= > > for an example. Not every cover letter needs to be that verbose, of > course. Likewise, the level of detail in change logs varies. > > A good way to get a feel for good cover letters and commit messages is > to review patches. What kind of information helps you as a reviewer? > That's the kind of information you want to provide with your > submissions. > > Hope this helps! This helps out a lot! Really grateful for this value packed mail! Will keep these points in mind while further upstreaming and versioning. Thank you! Regards, Kshitij Suri >
On Tue, Mar 29, 2022 at 09:59:55AM +0200, Markus Armbruster wrote: > Kshitij Suri <kshitij.suri@nutanix.com> writes: > > > On 29/03/22 12:12 pm, Markus Armbruster wrote: > >> If I count correctly, this is the fifth posting tagged "v2". Don't do > >> that, please, as it's quite confusing. > >> > > Thank you for your review and I apologise for that since I am fairly > > new to upstreaming. As per what I read version updates should be done > > only when there are significant design changes to the patch which > > didn't happen in the v2 version. Will update it to v3 and send the > > patch. > > We all make mistakes :) > > The purpose of the version tag in the subject is to help humans with > keeping track of patch submissions. Increment it for every submission. > > If you need to resend a submission completely unchanged for some reason, > you may want to keep the tag and add "RESEND". > > A cover letter (git format-patch --cover-letter) lets you write an > introduction to the whole series. Simple series may not need an > introduction, but complex ones do. I always use one except when the > "series" is a single patch. > > Keeping a change log in the cover letter helps people who already > reviewed previous iterations. FYI, using the 'git-publish' tool instead of 'git send-email' or 'git format-patch' helps you do all these things. It automatically sets & increments the version number, it prompts for a cover letter and remembers what you wrote next time. With regards, Daniel
On 29/03/22 2:31 pm, Daniel P. Berrangé wrote: > On Tue, Mar 29, 2022 at 09:59:55AM +0200, Markus Armbruster wrote: >> Kshitij Suri <kshitij.suri@nutanix.com> writes: >> >>> On 29/03/22 12:12 pm, Markus Armbruster wrote: >>>> If I count correctly, this is the fifth posting tagged "v2". Don't do >>>> that, please, as it's quite confusing. >>>> >>> Thank you for your review and I apologise for that since I am fairly >>> new to upstreaming. As per what I read version updates should be done >>> only when there are significant design changes to the patch which >>> didn't happen in the v2 version. Will update it to v3 and send the >>> patch. >> We all make mistakes :) >> >> The purpose of the version tag in the subject is to help humans with >> keeping track of patch submissions. Increment it for every submission. >> >> If you need to resend a submission completely unchanged for some reason, >> you may want to keep the tag and add "RESEND". >> >> A cover letter (git format-patch --cover-letter) lets you write an >> introduction to the whole series. Simple series may not need an >> introduction, but complex ones do. I always use one except when the >> "series" is a single patch. >> >> Keeping a change log in the cover letter helps people who already >> reviewed previous iterations. > FYI, using the 'git-publish' tool instead of 'git send-email' or > 'git format-patch' helps you do all these things. It automatically > sets & increments the version number, it prompts for a cover letter > and remembers what you wrote next time. > > With regards, > Daniel Oh didn't know about it, will try to use git-publish from now on. Thank you for your guidance for support throughout the issue! Regards, Kshitij Suri
On Tue, 29 Mar 2022 at 09:03, Markus Armbruster <armbru@redhat.com> wrote: > A cover letter (git format-patch --cover-letter) lets you write an > introduction to the whole series. Simple series may not need an > introduction, but complex ones do. I always use one except when the > "series" is a single patch. Note that a multi-patch series always needs a cover letter, even if its contents are quite brief. Some of the automatic tooling gets confused by multi-patch series with no cover letter. Conversely, single patches shouldn't have a cover letter, although getting that one wrong doesn't really have much ill effect. thanks -- PMM
On 29/03/22 3:18 pm, Peter Maydell wrote: > On Tue, 29 Mar 2022 at 09:03, Markus Armbruster <armbru@redhat.com> wrote: >> A cover letter (git format-patch --cover-letter) lets you write an >> introduction to the whole series. Simple series may not need an >> introduction, but complex ones do. I always use one except when the >> "series" is a single patch. > Note that a multi-patch series always needs a cover letter, > even if its contents are quite brief. Some of the automatic > tooling gets confused by multi-patch series with no cover letter. > Conversely, single patches shouldn't have a cover letter, although > getting that one wrong doesn't really have much ill effect. > > thanks > -- PMM Oh so regarding this PNG issue which has 2 patches, one to phase out VNC_PNG and one for core logic, should I send with a cover-letter? Thank you for our advice! Regards, Kshitij Suri
On Tue, 29 Mar 2022 at 10:55, Kshitij Suri <kshitij.suri@nutanix.com> wrote: > On 29/03/22 3:18 pm, Peter Maydell wrote: > > Note that a multi-patch series always needs a cover letter, > > even if its contents are quite brief. Some of the automatic > > tooling gets confused by multi-patch series with no cover letter. > > Conversely, single patches shouldn't have a cover letter, although > > getting that one wrong doesn't really have much ill effect. > Oh so regarding this PNG issue which has 2 patches, one to phase out > VNC_PNG and one for core logic, should I send with a cover-letter? Yes. There is more than one patch, so you must provide a cover letter. thanks -- PMM
diff --git a/hmp-commands.hx b/hmp-commands.hx index 8476277aa9..19b7cab595 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -244,11 +244,12 @@ 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'", - .cmd = hmp_screendump, + .args_type = "filename:F,format:s?,device:s?,head:i?", + .params = "filename [format] [device [head]]", + .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, }, diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 634968498b..2442bfa989 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1720,9 +1720,19 @@ 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_try_str(qdict, "format"); Error *err = NULL; + ImageFormat format; - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err); + format = qapi_enum_parse(&ImageFormat_lookup, input_format, + IMAGE_FORMAT_PPM, &err); + if (err) { + goto end; + } + + 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 664da9e462..24371fce05 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -157,12 +157,27 @@ ## { 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' } +## +# @ImageFormat: +# +# Supported image format types. +# +# @png: PNG format +# +# @ppm: PPM format +# +# Since: 7.1 +# +## +{ 'enum': 'ImageFormat', + 'data': ['ppm', 'png'] } + ## # @screendump: # -# Write a PPM of the VGA screen to a file. +# Capture the contents of a screen and write it 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) @@ -171,6 +186,8 @@ # 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. (default: ppm) (Since 7.1) +# # Returns: Nothing on success # # Since: 0.14 @@ -183,7 +200,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 da434ce1b2..f42f64d556 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 @@ -291,6 +294,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); @@ -329,7 +415,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; @@ -385,8 +472,16 @@ 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> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- hmp-commands.hx | 11 ++--- monitor/hmp-cmds.c | 12 +++++- qapi/ui.json | 24 +++++++++-- ui/console.c | 101 +++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 136 insertions(+), 12 deletions(-)