[Spice-devel] [vdagent-win PATCH 1/3] Make BitmapCoder::from_bitmap return a raw file format

[Uri Lublin]

Hi Frediano,

The patch looks good to me. Some minor comments below.

Consider replace "raw file format" with "bmp file format".
Or maybe even mention the file-header in the subject.

On 08/22/2017 03:01 PM, Frediano Ziglio wrote:
> The raw data are used to return raw data that are going to
> the network.

This sentence is a bit confusing. I'd remove it. The following
lines explains what's going on.


> The network expect the format of the data to match a file
> format so prepending DIB data with BITMAPFILEHEADER change
> the format from DIB to BMP file.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Ack.

> ---
>   vdagent/image.cpp | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/vdagent/image.cpp b/vdagent/image.cpp
> index 82cfb0e..727f295 100644
> --- a/vdagent/image.cpp
> +++ b/vdagent/image.cpp
> @@ -150,6 +150,8 @@ void BitmapCoder::get_dib_data(uint8_t *dib, const uint8_t *data, size_t size)
>   
>   uint8_t *BitmapCoder::from_bitmap(const BITMAPINFO& info, const void *bits, long &size)
>   {
> +    BITMAPFILEHEADER file_hdr;
> +
>       const BITMAPINFOHEADER& head(info.bmiHeader);
>   
>       const DWORD max_palette_colors = head.biBitCount <= 8 ? 1 << head.biBitCount : 0;
> @@ -157,14 +159,21 @@ uint8_t *BitmapCoder::from_bitmap(const BITMAPINFO& info, const void *bits, long
>   
>       const size_t stride = compute_dib_stride(head.biWidth, head.biBitCount);
>       const size_t image_size = stride * head.biHeight;
> -    size = sizeof(head) + palette_size + image_size;
> +    size = sizeof(file_hdr) + sizeof(head) + palette_size + image_size;
> +
> +    file_hdr.bfType = 'B'+'M'*256u;

Consider adding spaces before/after the + sign.

Thanks,
     Uri.


> +    file_hdr.bfSize = size;
> +    file_hdr.bfReserved1 = 0;
> +    file_hdr.bfReserved2 = 0;
> +    file_hdr.bfOffBits = sizeof(file_hdr) + sizeof(head) + palette_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);
> +    memcpy(data, &file_hdr, sizeof(file_hdr));
> +    memcpy(data + sizeof(file_hdr), &info, sizeof(head) + palette_size);
> +    memcpy(data + sizeof(file_hdr) + sizeof(head) + palette_size, bits, image_size);
>       return data;
>   }
>   
>