> On 18 Jul 2017, at 17:54, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>
> On Mon, Jul 17, 2017 at 09:54:04AM +0100, Frediano Ziglio wrote:
>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>> ---
>> Makefile.am | 6 +-
>> configure.ac | 3 +
>> vdagent/image.cpp | 8 +-
>> vdagent/imagepng.cpp | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> vdagent/imagepng.h | 25 ++++++
>> 5 files changed, 277 insertions(+), 9 deletions(-)
>> create mode 100644 vdagent/imagepng.cpp
>> create mode 100644 vdagent/imagepng.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index b35dd57..175d8f7 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -23,8 +23,8 @@ LIBS = -lversion
>>
>> bin_PROGRAMS = vdagent vdservice
>>
>> -vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
>> -vdagent_CXXFLAGS = $(AM_CXXFLAGS)
>> +vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
>> +vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
>> vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
>> vdagent_SOURCES = \
>> common/vdcommon.cpp \
>> @@ -44,6 +44,8 @@ vdagent_SOURCES = \
>> vdagent/as_user.h \
>> vdagent/image.cpp \
>> vdagent/image.h \
>> + vdagent/imagepng.cpp \
>> + vdagent/imagepng.h \
>> $(NULL)
>>
>> vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
>> diff --git a/configure.ac b/configure.ac
>> index 4eac4b4..bb33075 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -101,6 +101,9 @@ dnl ---------------------------------------------------------------------------
>> dnl - Check library dependencies
>> dnl ---------------------------------------------------------------------------
>>
>> +PKG_CHECK_MODULES(LIBPNG, [libpng])
>> +PKG_CHECK_MODULES(ZLIB, [zlib])
>> +
>> dnl ---------------------------------------------------------------------------
>> dnl - Makefiles, etc.
>> dnl ---------------------------------------------------------------------------
>> diff --git a/vdagent/image.cpp b/vdagent/image.cpp
>> index b32da6f..c968217 100644
>> --- a/vdagent/image.cpp
>> +++ b/vdagent/image.cpp
>> @@ -21,9 +21,9 @@
>>
>> #include "vdcommon.h"
>> #include "image.h"
>> +#include "imagepng.h"
>>
>> ImageCoder *create_bitmap_coder();
>> -ImageCoder *create_png_coder();
>>
>> static ImageCoder *get_coder(uint32_t vdagent_type)
>> {
>> @@ -172,9 +172,3 @@ ImageCoder *create_bitmap_coder()
>> {
>> return new BitmapCoder();
>> }
>> -
>> -// TODO
>> -ImageCoder *create_png_coder()
>> -{
>> - return NULL;
>> -}
>> diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
>> new file mode 100644
>> index 0000000..19d9efc
>> --- /dev/null
>> +++ b/vdagent/imagepng.cpp
>> @@ -0,0 +1,244 @@
>> +/*
>> + Copyright (C) 2017 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 "vdcommon.h"
>> +
>> +#include <png.h>
>> +#include <algorithm>
>> +
>> +#include "imagepng.h"
>> +
>> +class PngCoder: public ImageCoder
>> +{
>> +public:
>> + PngCoder() {};
>> + 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);
>> +private:
>> + size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t size);
>> +};
>> +
>> +struct BufferIo {
>> + uint8_t *buf;
>> + uint32_t pos, size;
>> +};
>> +
>> +static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
>> +{
>> + BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
As long as you do a cast form a pointer, I would rather use a pointer to BufferIo here.
>> + if (io.pos + size >= io.size)
>> + png_error(png, "read past end");
>> + memcpy(out, io.buf+io.pos, size);
This memcpy should be in ‘else’, unless png_error interrupts the flow of control (I saw a png_jumpbuf below that frightens me).
>> + io.pos += size;
>> +}
>> +
>> +size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
>> +{
>> + return convert_to_dib(NULL, data, size);
>> +}
>> +
>> +typedef void line_fixup_t(uint8_t *line, unsigned width);
>> +
>> +static void line_fixup_none(uint8_t *line, unsigned width)
>> +{
>> +}
>> +
>> +static void line_fixup_2to4(uint8_t *line, unsigned width)
>
> Might be slightly more readable with 'bpp' in the name,
> line_fixup_2bpp_to_4bpp()
>
>> +{
>> + width = (width + 3) / 4u;
>> + while (width--) {
>> + uint8_t from = line[width];
>> + line[width*2+1] = ((from & 0x03) << 0) | ((from & 0x0c) << 2);
>> + line[width*2+0] = ((from & 0x30) >> 4) | ((from & 0xc0) >> 2);
>> + }
>> +}
>> +
>> +static void line_fixup_rgb2bgr(uint8_t *line, unsigned width)
>
> 'unsigned int' (applies to several parts of the file).
I’m curious why you think this is an improvement? ‘unsigned’ always means ‘unsigned int’, and two-words types always feel weird.
>
>> +{
>> + for (; width; --width) {
>> + std::swap(line[0], line[2]);
>> + line += 3;
>> + }
>
> Not really a fan of the over short for() loop, could just be
> for (unsigned int i = 0; i < width; i++),
If not initializing, it’s just while(width—).
> or something closer to what
> you do in line_fixup_2to4 (which I'm don't like much either ;)).
> And I don't think it's really useful to use std::swap to swap two uint8_t.
std::swap is there. Why not use it? In this day and age, manually swapping stuff in C++ is considered quite inelegant, even for standard types.
>
>
>
>> +}
>> +
>> +size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t size)
>> +{
>> + BufferIo io;
>> + io.buf = (uint8_t *)data;
>> + io.pos = 0;
>> + io.size = size;
Since you are using C++, consider adding a constructor to BufferIo.
>> +
>> + png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL);
>> + if (!png)
>> + return 0;
>> +
>> + png_infop info = png_create_info_struct(png);
>> + if (!info) {
>> + png_destroy_read_struct(&png, &info, NULL);
>> + return 0;
>> + }
>> +
>> + if (setjmp(png_jmpbuf(png))) {
>> + png_destroy_read_struct(&png, &info, NULL);
>> + return 0;
>> + }
>> +
>> + png_set_read_fn(png, &io, read_from_bufio);
>> +
>> + png_read_info(png, info);
>> +
>> + // not so much precision is supported
>> + unsigned bits = png_get_bit_depth(png, info);
>> + if (bits == 16)
>> + png_set_strip_16(png);
>> +
>> + unsigned out_bits;
>> + bool is_gray = false;
>> + line_fixup_t *line_fixup = line_fixup_none;
>> + switch (png_get_color_type(png, info)) {
>> + case PNG_COLOR_TYPE_GRAY:
>> + is_gray = true;
>> + out_bits = bits == 16 ? 8 : bits;
>> + if (bits == 2) {
>
> Can you rework these 2 lines? I would not use a ternary '?' here when
> you need to test on multiple different values of 'bits' (2 or 16).
> Regular if, or a switch/case should do the trick and be more readable.
>
>> +#if 0
>> + png_set_expand_gray_1_2_4_to_8(png);
>> + out_bits = 8;
>> +#else
>> + line_fixup = line_fixup_2to4;
>> + out_bits = 4;
>> +#endif
>
> Is the #if 0 intentional?
>
>
>> + }
>> + break;
>> + case PNG_COLOR_TYPE_PALETTE:
>> + // should return 1, 2, 4 and 8, BMP does not support 2
>
> Is the first '2' unwanted? Since you say later 2 is unsupported
>
>> + out_bits = bits;
>> + if (bits == 2) {
>> + line_fixup = line_fixup_2to4;
>> + out_bits = 4;
>> + }
>> + break;
>> + case PNG_COLOR_TYPE_RGB:
>> + line_fixup = line_fixup_rgb2bgr;
>> + out_bits = 24;
>> + break;
>> + case PNG_COLOR_TYPE_RGB_ALPHA:
>> + line_fixup = line_fixup_rgb2bgr;
>> + out_bits = 24;
>> + png_set_strip_alpha(png);
>> + break;
>> + case PNG_COLOR_TYPE_GRAY_ALPHA:
>> + is_gray = true;
>> + // gray with alpha should always be 8 bit but make it sure
>> + // in case format change
>> + png_set_expand_gray_1_2_4_to_8(png);
>> + out_bits = 8;
>> + png_set_strip_alpha(png);
>> + break;
>> + default:
>> + longjmp(png_jmpbuf(png), 1);
Ouch.
Some png_error maybe?
>> + break;
>> + }
>> +
>> + const unsigned width = png_get_image_width(png, info);
>> + const unsigned height = png_get_image_height(png, info);
>> + const size_t stride = compute_dib_stride(width, out_bits);
>> + const size_t image_size = stride * height;
>> + const size_t palette_size = out_bits > 8 ? 0 : sizeof(RGBQUAD) * (1 << out_bits);
>> + const size_t dib_size = sizeof(BITMAPINFOHEADER) + palette_size + image_size;
>> + if (!out_buf) {
>> + png_destroy_read_struct(&png, &info, NULL);
>> + return dib_size;
>> + }
>> +
>> + // TODO tests
>> + // bits, 1, 2, 4, 8, 16
>> + // all color types
>> + // alpha/not alpha
>> + // indexed with not all colors
>> +
>> + // fill header
>> + BITMAPINFOHEADER& head(*(BITMAPINFOHEADER *)out_buf);
>> + memset(&head, 0, sizeof(head));
>> + head.biSize = sizeof(head);
And that’s the fourth coding style in this file alone, camelcase hungarian ;-)
>> + head.biWidth = width;
>> + head.biHeight = height;
>> + head.biPlanes = 1;
>> + head.biBitCount = out_bits;
>> + head.biCompression = BI_RGB;
>> + head.biSizeImage = image_size;
>> +
>> + // copy palette
>> + RGBQUAD *rgb = (RGBQUAD *)(out_buf + sizeof(BITMAPINFOHEADER));
>> + if (is_gray) {
>> + const unsigned colors = 1 << out_bits;
>> + const unsigned mult = 255 / (colors - 1);
>> + for (unsigned color = 0; color < colors; ++color) {
>> + rgb->rgbBlue = rgb->rgbGreen = rgb->rgbRed = color * mult;
>> + rgb->rgbReserved = 0;
>> + ++rgb;
>> + }
>> + head.biClrUsed = colors;
>> + } else if (out_bits <= 8) {
>> + png_colorp palette = NULL;
>> + int num_palette;
>> + if (!png_get_PLTE(png, info, &palette, &num_palette)) {
>> + png_error(png, "error getting palette");
>> + }
>> + const int colors = 1 << out_bits;
>> + for (int color = 0; color < colors; ++color) {
>> + if (color < num_palette) {
>> + rgb->rgbBlue = palette->blue;
>> + rgb->rgbGreen = palette->green;
>> + rgb->rgbRed = palette->red;
>> + } else {
>> + rgb->rgbBlue = rgb->rgbGreen = rgb->rgbRed = 0;
>> + }
>> + rgb->rgbReserved = 0;
>> + ++rgb;
>> + ++palette;
>> + }
>> + head.biClrUsed = colors;
>> + }
>> +
>> + // now do the actual conversion!
>> + uint8_t *dst = out_buf + sizeof(BITMAPINFOHEADER) + palette_size + image_size;
>> + for (unsigned row = 0; row < height; ++row) {
>> + ((uint32_t*)dst)[-1] = 0; // padding
>> + dst -= stride;
>> + png_read_row(png, dst, NULL);
>> + line_fixup(dst, width);
>> + }
>> +
>> + png_destroy_read_struct(&png, &info, NULL);
>> + return dib_size;
>> +}
>> +
>> +void PngCoder::get_dib_data(uint8_t *dib, const uint8_t *data, size_t size)
>> +{
>> + convert_to_dib(dib, data, size);
>> +}
>> +
>> +uint8_t *PngCoder::from_bitmap(const BITMAPINFO& info, const void *bits, long &size)
>> +{
>> + return NULL;
>> +}
>
> Fwiw, I would have added a comment as to why it's returning NULL
> (encoding to be implemented)
>
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel