[Spice-devel] [vdagent-win PATCH v3 2/5] Initial rewrite of image conversion code

Pavel Grunt pgrunt at redhat.com
Mon Jul 17 06:55:39 UTC 2017


On Fri, 2017-07-14 at 11:42 -0400, Frediano Ziglio wrote:
> > 
> > On Fri, 2017-07-14 at 13:57 +0100, Frediano Ziglio wrote:
> > > Remove CxImage linking.
> > > Support Windows BMP format.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  Makefile.am                 |   4 +-
> > >  configure.ac                |   4 +-
> > >  mingw-spice-vdagent.spec.in |   2 -
> > >  vdagent/image.cpp           | 182
> > >  ++++++++++++++++++++++++++++++++++---------
> > > -
> > >  vdagent/image.h             |  13 ++++
> > >  5 files changed, 159 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 868199e..b35dd57 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -23,8 +23,8 @@ LIBS = -lversion
> > >  
> > >  bin_PROGRAMS = vdagent vdservice
> > >  
> > > -vdagent_LDADD = -lwtsapi32 $(CXIMAGE_LIBS) vdagent_rc.$(OBJEXT)
> > > -vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(CXIMAGE_CFLAGS)
> > > +vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
> > > +vdagent_CXXFLAGS = $(AM_CXXFLAGS)
> > >  vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
> > >  vdagent_SOURCES =			\
> > >  	common/vdcommon.cpp             \
> > > diff --git a/configure.ac b/configure.ac
> > > index ff489cc..4eac4b4 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -42,6 +42,7 @@ AC_DEFINE_UNQUOTED([BUILD_YEAR], "$BUILD_YEAR", [Build
> > > year])
> > >  # Check for programs
> > >  AC_PROG_CC
> > >  AC_PROG_CXX
> > > +AX_CXX_COMPILE_STDCXX_11
> > >  AM_PROG_CC_C_O
> > >  AC_PROG_INSTALL
> > >  AC_CHECK_TOOL(WINDRES, [windres])
> > > @@ -100,9 +101,6 @@ dnl
> > > ------------------------------------------------------
> > > ---------------------
> > >  dnl - Check library dependencies
> > >  dnl
> > >  -----------------------------------------------------------------------
> > > ----
> > >  
> > > -PKG_CHECK_MODULES(CXIMAGE, [cximage])
> > > -CXIMAGE_LIBS=`$PKG_CONFIG --static --libs cximage`
> > > -
> > >  dnl
> > >  -----------------------------------------------------------------------
> > > ----
> > >  dnl - Makefiles, etc.
> > >  dnl
> > >  -----------------------------------------------------------------------
> > > ----
> > > diff --git a/mingw-spice-vdagent.spec.in b/mingw-spice-vdagent.spec.in
> > > index 563341d..8cfd01a 100644
> > > --- a/mingw-spice-vdagent.spec.in
> > > +++ b/mingw-spice-vdagent.spec.in
> > > @@ -13,8 +13,6 @@ Source0:        vdagent-win-
> > > %{version}%{?_version_suffix}.tar.xz
> > >  
> > >  BuildRequires:  mingw32-filesystem >= 23
> > >  BuildRequires:  mingw64-filesystem >= 23
> > > -BuildRequires:  mingw32-cximage-static
> > > -BuildRequires:  mingw64-cximage-static
> > >  BuildRequires:  mingw32-jasper-static
> > >  BuildRequires:  mingw64-jasper-static
> > >  BuildRequires:  mingw32-libjpeg-turbo-static
> > > diff --git a/vdagent/image.cpp b/vdagent/image.cpp
> > > index 598a742..6f7608d 100644
> > > --- a/vdagent/image.cpp
> > > +++ b/vdagent/image.cpp
> > > @@ -16,71 +16,175 @@
> > >  */
> > >  
> > >  #include <spice/macros.h>
> > > +#include <memory>
> > > +#include <vector>
> > >  
> > >  #include "vdcommon.h"
> > >  #include "image.h"
> > >  
> > > -#include "ximage.h"
> > > +ImageCoder *create_bitmap_coder();
> > > +ImageCoder *create_png_coder();
> > >  
> > > -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)
> > > +static ImageCoder *get_coder(uint32_t vdagent_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;
> > > -        }
> > > +    switch (vdagent_type) {
> > > +    case VD_AGENT_CLIPBOARD_IMAGE_BMP:
> > > +        return create_bitmap_coder();
> > > +    case VD_AGENT_CLIPBOARD_IMAGE_PNG:
> > > +        return create_png_coder();
> > >      }
> > > -    return 0;
> > > +    return NULL;
> > >  }
> > >  
> > >  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();
> > > +    std::unique_ptr<ImageCoder> coder(get_coder(clipboard.type));
> > > +    if (!coder) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    format = CF_DIB;
> > 
> > so it is just an output parameter (and always set to CF_DIB)
> > 
> 
> Currently only CF_DIB is used. But potentially other clipboard types
> can be used for images.
> 
> > > +    size_t dib_size = coder->get_dib_size(clipboard.data, size);
> > > +    if (!dib_size) {
> > > +        return NULL;
> > > +    }
> > > +    HANDLE clip_data = GlobalAlloc(GMEM_MOVEABLE, dib_size);
> > > +    if (clip_data) {
> > > +        uint8_t* dst = (uint8_t*)GlobalLock(clip_data);
> > > +        if (!dst) {
> > > +            GlobalFree(clip_data);
> > > +            return NULL;
> > > +        }
> > > +        coder->get_dib_data(dst, clipboard.data, size);
> > > +        GlobalUnlock(clip_data);
> > > +    }
> > >      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;
> > > +    if (GetObjectType(clip_data) != OBJ_BITMAP) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    std::unique_ptr<ImageCoder> coder(get_coder(clipboard_request.type));
> > > +    if (!coder) {
> > > +        return NULL;
> > > +    }
> > >  
> > > -    ASSERT(cximage_format);
> > > +    HPALETTE pal = 0;
> > >      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;
> > > +
> > > +    // extract DIB
> > > +    BITMAP bitmap;
> > > +    GetObject(clip_data, sizeof(bitmap), &bitmap);
> > > +
> > > +    struct {
> > > +        BITMAPINFOHEADER head;
> > > +        RGBQUAD colors[256];
> > > +    } info;
> > > +
> > > +    BITMAPINFOHEADER& head(info.head);
> > > +    memset(&head, 0, sizeof(head));
> > > +    head.biSize = sizeof(head);
> > > +    head.biWidth = bitmap.bmWidth;
> > > +    head.biHeight = bitmap.bmHeight;
> > > +    head.biPlanes = bitmap.bmPlanes;
> > > +    head.biBitCount = bitmap.bmBitsPixel >= 16 ? 24 : bitmap.bmBitsPixel;
> > > +    head.biCompression = BI_RGB;
> > > +
> > > +    HDC dc = GetDC(NULL);
> > > +    HPALETTE old_pal = NULL;
> > > +    if (pal) {
> > > +        old_pal = (HPALETTE)SelectObject(dc, pal);
> > > +        RealizePalette(dc);
> > > +    }
> > > +    size_t stride = ((head.biWidth * head.biBitCount + 31u) & ~31u) / 8u;
> > > +    std::vector<uint8_t> bits(stride * head.biHeight);
> > > +    int res = GetDIBits(dc, (HBITMAP) clip_data, 0, head.biHeight,
> > > +                        &bits[0], (LPBITMAPINFO)&info, DIB_RGB_COLORS);
> > > +    if (pal) {
> > > +        SelectObject(dc, old_pal);
> > >      }
> > > -    if (!image.Encode(new_data, new_size, cximage_format)) {
> > > -        vd_printf("Image encode to type %u failed",
> > > clipboard_request.type);
> > > +    ReleaseDC(NULL, dc);
> > > +    if (!res) {
> > >          return NULL;
> > >      }
> > > -    vd_printf("Image encoded to %lu bytes", new_size);
> > > -    return new_data;
> > > +
> > > +    // convert DIB to desired format
> > > +    return coder->from_bitmap(*(LPBITMAPINFO)&info, &bits[0], new_size);
> > >  }
> > >  
> > >  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);
> > > +    free(data);
> > > +}
> > > +
> > > +class BitmapCoder: public ImageCoder
> > > +{
> > > +public:
> > > +    BitmapCoder() {};
> > > +    size_t get_dib_size(const uint8_t *data, size_t size);
> > > +    void get_dib_data(uint8_t *dib, const uint8_t *data, size_t size);
> > > +    uint8_t *from_bitmap(const BITMAPINFO& info, const void *bits, long
> > > &size);
> > > +};
> > > +
> > > +size_t BitmapCoder::get_dib_size(const uint8_t *data, size_t size)
> > > +{
> > > +    if (*(const WORD*)data == 'B'+'M'*256u)
> 
> what about memcmp(data, "BM", 2) == 0 ??

yes, memcmp is more readable

> 
> > isn't nicer to compare byte by byte instead of casting and multiplication ?
> > (or
> > do the shift instead of the multiplication???).
> > 
> > > +        return size > 14 ? size - 14 : 0;
> > > +    return size;
> > > +}
> > > +
> > > +void BitmapCoder::get_dib_data(uint8_t *dib, const uint8_t *data, size_t
> > > size)
> > > +{
> > > +    // just strip the file header if present, images can be either BMP or
> > > DIB
> > > +    size_t new_size = get_dib_size(data, size);
> > > +    memcpy(dib, data + (size - new_size), new_size);
> > > +}
> > > +
> > > +uint8_t *BitmapCoder::from_bitmap(const BITMAPINFO& info, const void
> > > *bits,
> > > long &size)
> > > +{
> > > +    const BITMAPINFOHEADER& head(info.bmiHeader);
> > > +
> > > +    size_t palette_size = 0;
> > > +    DWORD max_colors = 0;
> > > +    switch (head.biBitCount) {
> > > +    case 1:
> > > +        max_colors = 2;
> > > +        break;
> > > +    case 4:
> > > +        max_colors = 16;
> > > +        break;
> > > +    case 8:
> > > +        max_colors = 256;
> > > +        break;
> > 
> > what about other values? cannot be reached?
> > 
> 
> true colors, so no palette and max_colors will be 0. Maybe
> 
>     switch (head.biBitCount) {
>     case 1:
>     case 4:
>     case 8:
>         max_colors = 1 << head.biBitCount;
>         break;
>     default:
>         max_colors = 0;
>         break;
it should return NULL 


>     }
> 
> ?



> 
> > > +    }
> > > +    palette_size = sizeof(RGBQUAD) * std::min(head.biClrUsed,
> > > max_colors);
> > > +
> > > +    const size_t stride = ((head.biWidth * head.biBitCount + 31u) & ~31u)
> > > /
> > > 8u;
> > 
> > maybe it can be a function to "hide" the math (and avoid repeating it)
> > 
> 
> make sense. Not clear where I should put such function. Maybe image.h ?
> 
> size_t compute_dib_stride(unsigned width, unsigned bit_count) ?

looks good, it is general enough to be in the image.h

> 
> > > +    const size_t image_size = stride * head.biHeight;
> > 
> > also it is possible to return the image_size. the stride is not used after
> > that
> > (same in get_raw_clipboard_image())
> > 
> 
> other code uses stride so a function just computing the stride would
> be good.

ok

thanks,
Pavel

> 
> > > +    size = sizeof(head) + palette_size + image_size;
> > > +
> > > +    uint8_t *data = (uint8_t *) malloc(size);
> > > +    if (!data) {
> > > +        return NULL;
> > > +    }
> > > +    memcpy(data, &info, sizeof(head) + palette_size);
> > > +    memcpy(data + sizeof(head) + palette_size, bits, image_size);
> > > +    return data;
> > > +}
> > > +
> > > +ImageCoder *create_bitmap_coder()
> > > +{
> > > +    return new BitmapCoder();
> > > +}
> > > +
> > > +// TODO
> > > +ImageCoder *create_png_coder()
> > > +{
> > > +    return NULL;
> > >  }
> > > diff --git a/vdagent/image.h b/vdagent/image.h
> > > index 379c5cc..75e2268 100644
> > > --- a/vdagent/image.h
> > > +++ b/vdagent/image.h
> > > @@ -18,6 +18,19 @@
> > >  #ifndef VDAGENT_IMAGE_H_
> > >  #define VDAGENT_IMAGE_H_
> > >  
> > > +class ImageCoder
> > > +{
> > > +public:
> > > +    ImageCoder() {};
> > > +    virtual ~ImageCoder() {}
> > > +    virtual size_t get_dib_size(const uint8_t *data, size_t size)=0;
> > > +    virtual void get_dib_data(uint8_t *dib, const uint8_t *data, size_t
> > > size)=0;
> > > +    virtual uint8_t *from_bitmap(const BITMAPINFO& info, const void
> > > *bits,
> > > long &size)=0;
> > > +private:
> > > +    ImageCoder(const ImageCoder& rhs);
> > > +    void operator=(const ImageCoder &rhs);
> > > +};
> > > +
> > >  /**
> > >   * Returns image to put in the clipboard.
> > >   *
> > 
> > It is a nice improvement.
> > 
> > Thanks,
> > Pavel
> > 
> > 
> > 
> 
> Frediano


More information about the Spice-devel mailing list