[Spice-devel] [vdagent-win PATCH 3/7] Move image handling to a separate file

Pavel Grunt pgrunt at redhat.com
Mon Jul 10 13:11:39 UTC 2017


On Thu, 2017-07-06 at 14:32 +0100, Frediano Ziglio wrote:
> This will make easier to change code that handle images.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  Makefile.am         |  2 ++
>  vdagent/image.cpp   | 86
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  vdagent/image.h     | 46 ++++++++++++++++++++++++++++
>  vdagent/vdagent.cpp | 57 +++++------------------------------
>  4 files changed, 142 insertions(+), 49 deletions(-)
>  create mode 100644 vdagent/image.cpp
>  create mode 100644 vdagent/image.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index b60a718..868199e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,8 @@ vdagent_SOURCES =			\
>  	vdagent/vdagent.cpp		\
>  	vdagent/as_user.cpp		\
>  	vdagent/as_user.h		\
> +	vdagent/image.cpp		\
> +	vdagent/image.h			\
>  	$(NULL)
>  
>  vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
> diff --git a/vdagent/image.cpp b/vdagent/image.cpp
> new file mode 100644
> index 0000000..fb19dbc
> --- /dev/null
> +++ b/vdagent/image.cpp
> @@ -0,0 +1,86 @@
> +/*
> +   Copyright (C) 2013-2016 Red Hat, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU General Public License as
> +   published by the Free Software Foundation; either version 2 of
> +   the License, or (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <spice/macros.h>
> +
> +#include "vdcommon.h"
> +#include "image.h"
> +
> +#include "ximage.h"


(it is going to be removed, but it is a system header, per our style the order
should be different)

> +
> +typedef struct ImageType {
> +    uint32_t type;
> +    DWORD cximage_format;
> +} ImageType;
> +
> +static const ImageType image_types[] = {
> +    {VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
> +    {VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
> +};
> +
> +static DWORD get_cximage_format(uint32_t type)
> +{
> +    for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
> +        if (image_types[i].type == type) {
> +            return image_types[i].cximage_format;
> +        }
> +    }
> +    return 0;
> +}
> +
> +HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size,
> UINT &format)
> +{
> +    HANDLE clip_data;
> +    DWORD cximage_format = get_cximage_format(clipboard.type);
> +    ASSERT(cximage_format);
> +    CxImage image((BYTE*)clipboard.data, size, cximage_format);
> +    clip_data = image.CopyToHandle();
> +    return clip_data;
> +}
> +
> +uint8_t* get_raw_clipboard_image(const VDAgentClipboardRequest&
> clipboard_request,
> +                                 HANDLE clip_data, long& new_size)
> +{
> +    CxImage image;
> +    uint8_t *new_data = NULL;
> +    DWORD cximage_format = get_cximage_format(clipboard_request.type);
> +    HPALETTE pal = 0;
> +
> +    ASSERT(cximage_format);
> +    if (IsClipboardFormatAvailable(CF_PALETTE)) {
> +        pal = (HPALETTE)GetClipboardData(CF_PALETTE);
> +    }
> +    if (!image.CreateFromHBITMAP((HBITMAP)clip_data, pal)) {
> +        vd_printf("Image create from handle failed");
> +        return NULL;
> +    }
> +    if (!image.Encode(new_data, new_size, cximage_format)) {
> +        vd_printf("Image encode to type %u failed", clipboard_request.type);
> +        return NULL;
> +    }
> +    vd_printf("Image encoded to %lu bytes", new_size);
> +    return new_data;
> +}
> +
> +void free_raw_clipboard_image(uint8_t *data)
> +{
> +    // this is really just a free however is better to make
> +    // the free from CxImage code as on Windows the free
> +    // can be different between libraries
> +    CxImage image;
> +    image.FreeMemory(data);
> +}
> diff --git a/vdagent/image.h b/vdagent/image.h
> new file mode 100644
> index 0000000..9ba6003
> --- /dev/null
> +++ b/vdagent/image.h
> @@ -0,0 +1,46 @@
> +/*
> +   Copyright (C) 2013-2016 Red Hat, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU General Public License as
> +   published by the Free Software Foundation; either version 2 of
> +   the License, or (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef VDAGENT_IMAGE_H_
> +#define VDAGENT_IMAGE_H_
> +
> +/**
> + * Returns image to put in the clipboard.
> + * @param clipboard  data to write in the clipboard
> + * @param size       size of data
> + * @param format     format decided. This can be changed by the function to
> + *                   reflect a better format
> + */

so doxygen. Doesn't it require an empty line between the description(s) and the
list of parameters? Maybe it is not required, but in my opinion is more readable
:)

I don't understand the description of "@param format". Is it input or output
parameter?

> +HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size,
> UINT& format);
> +
> +/**
> + * Return raw data got from the clipboard.
> + * Function could use clip_data or get new data from the clipboard.
> + * You should free data returned with free_raw_clipboard_image.
> + * @param clipboard_request  request
> + * @param clip_data          clipboard data
> + * @param new_size           size of returned data
> + */
> +uint8_t* get_raw_clipboard_image(const VDAgentClipboardRequest&
> clipboard_request,
> +                                 HANDLE clip_data, long& new_size);
> +
> +/**
> + * Free data returned by get_raw_clipboard_image
> + */
> +void free_raw_clipboard_image(uint8_t *data);
> +
> +#endif
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 6cb889f..cd49755 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -19,7 +19,7 @@
>  #include "desktop_layout.h"
>  #include "display_setting.h"
>  #include "file_xfer.h"
> -#include "ximage.h"
> +#include "image.h"
ok, i see, it's copied from here

>  #undef max
>  #undef min
>  #include <spice/macros.h>
> @@ -55,16 +55,6 @@ static const VDClipboardFormat clipboard_formats[] = {
>  
>  #define clipboard_formats_count SPICE_N_ELEMENTS(clipboard_formats)
>  
> -typedef struct ImageType {
> -    uint32_t type;
> -    DWORD cximage_format;
> -} ImageType;
> -
> -static const ImageType image_types[] = {
> -    {VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
> -    {VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
> -};
> -
>  typedef struct ALIGN_VC VDIChunk {
>      VDIChunkHeader hdr;
>      uint8_t data[0];
> @@ -725,23 +715,19 @@ bool VDAgent::handle_clipboard(VDAgentClipboard*
> clipboard, uint32_t size)
>      if (clipboard->type == VD_AGENT_CLIPBOARD_NONE) {
>          goto fin;
>      }
> +    format = get_clipboard_format(clipboard->type);
>      switch (clipboard->type) {
>      case VD_AGENT_CLIPBOARD_UTF8_TEXT:
>          clip_data = utf8_alloc((LPCSTR)clipboard->data, size);
>          break;
>      case VD_AGENT_CLIPBOARD_IMAGE_PNG:
> -    case VD_AGENT_CLIPBOARD_IMAGE_BMP: {
> -        DWORD cximage_format = get_cximage_format(clipboard->type);
> -        ASSERT(cximage_format);
> -        CxImage image(clipboard->data, size, cximage_format);
> -        clip_data = image.CopyToHandle();
> +    case VD_AGENT_CLIPBOARD_IMAGE_BMP:
> +        clip_data = get_image_handle(*clipboard, size, format);
>          break;
> -    }
>      default:
>          vd_printf("Unsupported clipboard type %u", clipboard->type);
>          goto fin;
>      }
> -    format = get_clipboard_format(clipboard->type);
>      if (format == 0) {
>          vd_printf("Unknown clipboard format, type %u", clipboard->type);
>          goto fin;
> @@ -1104,7 +1090,6 @@ bool
> VDAgent::handle_clipboard_request(VDAgentClipboardRequest* clipboard_reques
>      uint8_t* new_data = NULL;
>      long new_size = 0;
>      size_t len = 0;
> -    CxImage image;
>      VDAgentClipboard* clipboard = NULL;
>  
>      if (_clipboard_owner != owner_guest) {
> @@ -1135,28 +1120,12 @@ bool
> VDAgent::handle_clipboard_request(VDAgentClipboardRequest* clipboard_reques
>          new_size = WideCharToMultiByte(CP_UTF8, 0, (LPCWSTR)new_data,
> (int)len, NULL, 0, NULL, NULL);
>          break;
>      case VD_AGENT_CLIPBOARD_IMAGE_PNG:
> -    case VD_AGENT_CLIPBOARD_IMAGE_BMP: {
> -        DWORD cximage_format = get_cximage_format(clipboard_request->type);
> -        HPALETTE pal = 0;
> -
> -        ASSERT(cximage_format);
> -        if (IsClipboardFormatAvailable(CF_PALETTE)) {
> -            pal = (HPALETTE)GetClipboardData(CF_PALETTE);
> -        }
> -        if (!image.CreateFromHBITMAP((HBITMAP)clip_data, pal)) {
> -            vd_printf("Image create from handle failed");
> -            break;
> -        }
> -        if (!image.Encode(new_data, new_size, cximage_format)) {
> -            vd_printf("Image encode to type %u failed", clipboard_request-
> >type);
> -            break;
> -        }
> -        vd_printf("Image encoded to %lu bytes", new_size);
> +    case VD_AGENT_CLIPBOARD_IMAGE_BMP:
> +        new_data = get_raw_clipboard_image(*clipboard_request, clip_data,
> new_size);
>          break;
>      }
> -    }
>  
> -    if (!new_size) {
> +    if (!new_size || !new_data) {
>          vd_printf("clipboard is empty");
>          goto handle_clipboard_request_fail;
>      }
> @@ -1184,7 +1153,7 @@ bool
> VDAgent::handle_clipboard_request(VDAgentClipboardRequest* clipboard_reques
>      case VD_AGENT_CLIPBOARD_IMAGE_PNG:
>      case VD_AGENT_CLIPBOARD_IMAGE_BMP:
>          memcpy(clipboard->data, new_data, new_size);
> -        image.FreeMemory(new_data);
> +        free_raw_clipboard_image(new_data);
>          break;
>      }
>      CloseClipboard();
> @@ -1242,16 +1211,6 @@ uint32_t VDAgent::get_clipboard_type(uint32_t format)
> const
>      return 0;
>  }
>  
> -DWORD VDAgent::get_cximage_format(uint32_t type) const
> -{
> -    for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
> -        if (image_types[i].type == type) {
> -            return image_types[i].cximage_format;
> -        }
> -    }
> -    return 0;
> -}
> -
>  void VDAgent::set_clipboard_owner(int new_owner)
>  {
>      // FIXME: Clear requests, clipboard data and state

Pavel



More information about the Spice-devel mailing list