>
> > 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.
>
Yes and no. You are working with a C library that only understand
pointers and you know the pointer is the object (always existing)
you saved.
> >> + 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).
>
yes, longjmp is strongly used by libpng.
> >> + 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—).
>
by the way, removed using png_set_bgr.
> > 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.
>
done
> >> +
> >> + 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?
>
yes, I'll do
> >> + 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
> ;-)
>
I cannot rename system structure fields :-)
>
> >> + 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
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel