Re: [PATCH v2 2/2] Added parameter to take screenshot with screendump as PNG.

[Kshitij Suri <kshitij.suri@nutanix.com>]

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
>