[PATCH 0/5] qemu: Prefer PNG for domain screenshots
Whilst preparing for libvirt-php release, I want to ditch its imagick dependency which is needed to convert PPM to PNG. But I've learned that QEMU is able to return PNG already which looks more versatile than PPM anyways. Michal Prívozník (5): qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG qemu_monitor: Debug print all arguments in qemuMonitorScreendump() qemu_monitor: Extend qemuMonitorScreendump() for @format qemu: Prefer PNG for domain screenshots NEWS: Document change of screenshot format for QEMU NEWS.rst | 4 ++++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++++++++-- src/qemu/qemu_monitor.c | 6 ++++-- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 2 +- 12 files changed, 30 insertions(+), 5 deletions(-) -- 2.37.4
In its v7.1.0-rc0~125^2~6 commit, QEMU gained support for taking screenshots in PNG format. Track this capability. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 + 5 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 965af45cb2..2553b5b3ad 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -679,6 +679,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "query-stats-schemas", /* QEMU_CAPS_QUERY_STATS_SCHEMAS */ "sgx-epc", /* QEMU_CAPS_SGX_EPC */ "thread-context", /* QEMU_CAPS_THREAD_CONTEXT */ + "screenshot-format-png", /* QEMU_CAPS_SCREENSHOT_FORMAT_PNG */ ); @@ -1562,6 +1563,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "query-display-options/ret-type/+dbus", QEMU_CAPS_DISPLAY_DBUS }, { "object-add/arg-type/+iothread/thread-pool-max", QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX }, { "query-migrate/ret-type/blocked-reasons", QEMU_CAPS_MIGRATION_BLOCKED_REASONS }, + { "screendump/arg-type/format/^png", QEMU_CAPS_SCREENSHOT_FORMAT_PNG }, }; typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b70c02c05b..cc8b3759ea 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -658,6 +658,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_QUERY_STATS_SCHEMAS, /* accepts query-stats-schemas */ QEMU_CAPS_SGX_EPC, /* -object sgx-epc,... */ QEMU_CAPS_THREAD_CONTEXT, /* -object thread-context */ + QEMU_CAPS_SCREENSHOT_FORMAT_PNG, /* screendump command supports png format */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml index 2cd47a7770..1f43612703 100644 --- a/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml @@ -166,6 +166,7 @@ <flag name='migration.blocked-reasons'/> <flag name='query-stats'/> <flag name='query-stats-schemas'/> + <flag name='screenshot-format-png'/> <version>7001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900244</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml index e9210dfd44..8a2ed2236a 100644 --- a/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml @@ -198,6 +198,7 @@ <flag name='migration.blocked-reasons'/> <flag name='query-stats'/> <flag name='query-stats-schemas'/> + <flag name='screenshot-format-png'/> <version>7001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100244</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml index 0fa042a339..6bc739065f 100644 --- a/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml @@ -200,6 +200,7 @@ <flag name='query-stats'/> <flag name='query-stats-schemas'/> <flag name='thread-context'/> + <flag name='screenshot-format-png'/> <version>7001091</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100245</microcodeVersion> -- 2.37.4
For some reason, only @file argument is printed into debug logs. The rest of arguments was left out. Include all arguments. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 80f262cec7..e697ef2518 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2855,7 +2855,8 @@ qemuMonitorScreendump(qemuMonitor *mon, unsigned int head, const char *file) { - VIR_DEBUG("file=%s", file); + VIR_DEBUG("device=%s head=%u file=%s", + device, head, file); QEMU_CHECK_MONITOR(mon); -- 2.37.4
The 'screendump' command has new argument 'format'. Let's expose this on our QMP level so that callers can specify the format, if they wish so. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_monitor.c | 7 ++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 2 +- 6 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 25a1f6e0fd..f6683cbb0a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3350,7 +3350,7 @@ qemuDomainScreenshot(virDomainPtr dom, qemuSecurityDomainSetPathLabel(driver, vm, tmp, false); qemuDomainObjEnterMonitor(vm); - if (qemuMonitorScreendump(priv->mon, videoAlias, screen, tmp) < 0) { + if (qemuMonitorScreendump(priv->mon, videoAlias, screen, NULL, tmp) < 0) { qemuDomainObjExitMonitor(vm); goto endjob; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e697ef2518..734364e070 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2853,14 +2853,15 @@ int qemuMonitorScreendump(qemuMonitor *mon, const char *device, unsigned int head, + const char *format, const char *file) { - VIR_DEBUG("device=%s head=%u file=%s", - device, head, file); + VIR_DEBUG("device=%s head=%u format=%s file=%s", + device, head, NULLSTR(format), file); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONScreendump(mon, device, head, file); + return qemuMonitorJSONScreendump(mon, device, head, format, file); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c690fc3655..906a919f52 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -987,6 +987,7 @@ int qemuMonitorInjectNMI(qemuMonitor *mon); int qemuMonitorScreendump(qemuMonitor *mon, const char *device, unsigned int head, + const char *format, const char *file); int qemuMonitorSendKey(qemuMonitor *mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 39f313c2af..9822097bd7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4151,6 +4151,7 @@ int qemuMonitorJSONSendKey(qemuMonitor *mon, int qemuMonitorJSONScreendump(qemuMonitor *mon, const char *device, unsigned int head, + const char *format, const char *file) { g_autoptr(virJSONValue) cmd = NULL; @@ -4160,6 +4161,7 @@ int qemuMonitorJSONScreendump(qemuMonitor *mon, "s:filename", file, "S:device", device, "p:head", head, + "S:format", format, NULL); if (!cmd) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 93789480c5..484cb09830 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -337,6 +337,7 @@ int qemuMonitorJSONScreendump(qemuMonitor *mon, const char *device, unsigned int head, + const char *format, const char *file); int diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 238c6c1813..59f7322711 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1212,7 +1212,7 @@ GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONBlockdevMirror, "jobname", true, "vdb", "targetnode", 1024, 1234, 31234, true, true) GEN_TEST_FUNC(qemuMonitorJSONBlockStream, "vdb", "jobname", "backingnode", "backingfilename", 1024) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname", "topnode", "basenode", "backingfilename", 1024) -GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, "/foo/bar") +GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, NULL, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", "export", true, "bitmap") GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1") -- 2.37.4
Historically, QEMU took screenshots in PPM. While this might use to be popular format, as of v7.1.0-rc0~125^2~6 it is possible to take screenshots in PNG. This is more popular and renders almost everywhere, which is not the case for PPM (for instance, modern browsers do not render it). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f6683cbb0a..d509582719 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3291,6 +3291,7 @@ qemuDomainScreenshot(virDomainPtr dom, const char *videoAlias = NULL; char *ret = NULL; bool unlink_tmp = false; + const char *format = NULL; virCheckFlags(0, NULL); @@ -3339,6 +3340,10 @@ qemuDomainScreenshot(virDomainPtr dom, } } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SCREENSHOT_FORMAT_PNG)) { + format = "png"; + } + tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", priv->libDir); if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { @@ -3350,7 +3355,7 @@ qemuDomainScreenshot(virDomainPtr dom, qemuSecurityDomainSetPathLabel(driver, vm, tmp, false); qemuDomainObjEnterMonitor(vm); - if (qemuMonitorScreendump(priv->mon, videoAlias, screen, NULL, tmp) < 0) { + if (qemuMonitorScreendump(priv->mon, videoAlias, screen, format, tmp) < 0) { qemuDomainObjExitMonitor(vm); goto endjob; } @@ -3367,7 +3372,11 @@ qemuDomainScreenshot(virDomainPtr dom, goto endjob; } - ret = g_strdup("image/x-portable-pixmap"); + if (STREQ_NULLABLE(format, "png")) { + ret = g_strdup("image/png"); + } else { + ret = g_strdup("image/x-portable-pixmap"); + } endjob: VIR_FORCE_CLOSE(tmp_fd); -- 2.37.4
On Wed, Dec 07, 2022 at 12:20:52PM +0100, Michal Privoznik wrote:
Historically, QEMU took screenshots in PPM. While this might use to be popular format, as of v7.1.0-rc0~125^2~6 it is possible to take screenshots in PNG. This is more popular and renders almost everywhere, which is not the case for PPM (for instance, modern browsers do not render it).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f6683cbb0a..d509582719 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3291,6 +3291,7 @@ qemuDomainScreenshot(virDomainPtr dom, const char *videoAlias = NULL; char *ret = NULL; bool unlink_tmp = false; + const char *format = NULL;
virCheckFlags(0, NULL);
@@ -3339,6 +3340,10 @@ qemuDomainScreenshot(virDomainPtr dom, } }
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SCREENSHOT_FORMAT_PNG)) { + format = "png"; + } +
Curly brackets are discouraged for one-line condition bodies IIRC ;)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index ffffbc61b9..39f508a6ce 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -19,6 +19,10 @@ v9.0.0 (unreleased) * **Improvements** + * qemu: Prefer PNG for domain screenshots + + With sufficiently new QEMU (v7.1.0) screenshots change format from PPM to PNG. + * **Bug fixes** -- 2.37.4
On Wed, Dec 07, 2022 at 12:20:48PM +0100, Michal Privoznik wrote:
Whilst preparing for libvirt-php release, I want to ditch its imagick dependency which is needed to convert PPM to PNG. But I've learned that QEMU is able to return PNG already which looks more versatile than PPM anyways.
Michal Prívozník (5): qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG qemu_monitor: Debug print all arguments in qemuMonitorScreendump() qemu_monitor: Extend qemuMonitorScreendump() for @format qemu: Prefer PNG for domain screenshots NEWS: Document change of screenshot format for QEMU
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
NEWS.rst | 4 ++++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++++++++-- src/qemu/qemu_monitor.c | 6 ++++-- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 2 +- 12 files changed, 30 insertions(+), 5 deletions(-)
-- 2.37.4
On a Wednesday in 2022, Michal Privoznik wrote:
Whilst preparing for libvirt-php release, I want to ditch its imagick dependency which is needed to convert PPM to PNG. But I've learned that QEMU is able to return PNG already which looks more versatile than PPM anyways.
Michal Prívozník (5): qemu_caps: Introduce QEMU_CAPS_SCREENSHOT_FORMAT_PNG qemu_monitor: Debug print all arguments in qemuMonitorScreendump() qemu_monitor: Extend qemuMonitorScreendump() for @format qemu: Prefer PNG for domain screenshots NEWS: Document change of screenshot format for QEMU
NEWS.rst | 4 ++++ src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++++++++-- src/qemu/qemu_monitor.c | 6 ++++-- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemucapabilitiesdata/caps_7.1.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_7.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_7.2.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 2 +- 12 files changed, 30 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Martin Kletzander -
Michal Privoznik