Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Frediano Ziglio]

> 
> On Tue, Jul 25, 2017 at 07:20:23PM +0200, Christophe Fergeau wrote:
> > On Tue, Jul 25, 2017 at 07:09:22AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > > >>> I see several benefits to doing this:
> > > > > >>> 
> > > > > >>> 1. We always know exactly which component and branch is being
> > > > > >>> patched
> > > > > >>> 
> > > > > > 
> > > > > > As long as contributor keep pinging or resending his series, this
> > > > > > is
> > > > > > already the case.
> > > > > 
> > > > > As Frediano said at the beginning of the series, “I’m tired of
> > > > > hearing this
> > > > > reply”.
> > > > 
> > > > And this is not an actionable answer... My perception is that there
> > > > rarely are 'ping' on old series. Does this mean we are doing a good job
> > > > at reviews? (I doubt it or we would not have this conversation) Does
> > > > this mean patch senders do not want to do that? Why? Does this mean
> > > > it's
> > > > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > > > things work", with nothing specific.
> > > > 
> > > 
> > > Patch series are getting old (even years) repeatedly pinged (5/6 times)
> > > but they continue to not getting any feedback/ack/comment.
> > > If you can't remember any... this just confirms the problem.
> > 
> > So I went through my mails (searched for mails containing 'ping'), in
> > the last months
> 
> Forgot a number here, it's since April 2016, so in the last 15 months or so.
> 
> > I found 24 series which needed a ping, among these, 3
> > needed several pings, and it stopped at 2 pings. Maybe I missed some.
> 

As I said with ping you don't count multiple submissions and even
if you got a 0 ping count this does not mean that series get not
reviewed and should be. For me the fact that you have to search is
another confirmation that a tool would be helpful as memory and
patchwork are not enough.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe Fergeau]

On Tue, Jul 25, 2017 at 07:20:23PM +0200, Christophe Fergeau wrote:
> On Tue, Jul 25, 2017 at 07:09:22AM -0400, Frediano Ziglio wrote:
> > > 
> > > > >>> I see several benefits to doing this:
> > > > >>> 
> > > > >>> 1. We always know exactly which component and branch is being 
> > > > >>> patched
> > > > >>> 
> > > > > 
> > > > > As long as contributor keep pinging or resending his series, this is
> > > > > already the case.
> > > > 
> > > > As Frediano said at the beginning of the series, “I’m tired of hearing 
> > > > this
> > > > reply”.
> > > 
> > > And this is not an actionable answer... My perception is that there
> > > rarely are 'ping' on old series. Does this mean we are doing a good job
> > > at reviews? (I doubt it or we would not have this conversation) Does
> > > this mean patch senders do not want to do that? Why? Does this mean it's
> > > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > > things work", with nothing specific.
> > > 
> > 
> > Patch series are getting old (even years) repeatedly pinged (5/6 times)
> > but they continue to not getting any feedback/ack/comment.
> > If you can't remember any... this just confirms the problem.
> 
> So I went through my mails (searched for mails containing 'ping'), in
> the last months

Forgot a number here, it's since April 2016, so in the last 15 months or so.

> I found 24 series which needed a ping, among these, 3
> needed several pings, and it stopped at 2 pings. Maybe I missed some.


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v12 3/6] Write code to decode PNG format

[Frediano Ziglio]

> 
> On Mon, Jul 24, 2017 at 01:39:35PM +0100, Frediano Ziglio wrote:
> > +size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data,
> > size_t size)
> > +{
> [...]
> > +const unsigned int width = png_get_image_width(png, info);
> > +const unsigned int 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 int palette_colors = [=]() {
> 
> If you want to use a function, then give it a name, if you don't want to
> use a function, then this can just go in convert_to_dib() body rather
> than through a lambda (I know the latter means removing the 'const').

Lambdas are functions.
I personally think the purpose of that code was clear even without
a name.
Using a "standard" function requires to declare parameters and pass
them manually, something that lambda capture can do automatically.

> 
> Christophe
> 

By the way, got new version with more old style code.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Frediano Ziglio]

> 
> On Tue, Jul 25, 2017 at 07:09:22AM -0400, Frediano Ziglio wrote:
> > > 
> > > > >>> I see several benefits to doing this:
> > > > >>> 
> > > > >>> 1. We always know exactly which component and branch is being
> > > > >>> patched
> > > > >>> 
> > > > > 
> > > > > As long as contributor keep pinging or resending his series, this is
> > > > > already the case.
> > > > 
> > > > As Frediano said at the beginning of the series, “I’m tired of hearing
> > > > this
> > > > reply”.
> > > 
> > > And this is not an actionable answer... My perception is that there
> > > rarely are 'ping' on old series. Does this mean we are doing a good job
> > > at reviews? (I doubt it or we would not have this conversation) Does
> > > this mean patch senders do not want to do that? Why? Does this mean it's
> > > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > > things work", with nothing specific.
> > > 
> > 
> > Patch series are getting old (even years) repeatedly pinged (5/6 times)
> > but they continue to not getting any feedback/ack/comment.
> > If you can't remember any... this just confirms the problem.
> 
> So I went through my mails (searched for mails containing 'ping'), in
> the last months I found 24 series which needed a ping, among these, 3
> needed several pings, and it stopped at 2 pings. Maybe I missed some.
> 

As I said there are series that last years, not months and you
should consider the multiple sending of them too.

> > 
> > > > 
> > > > > 
> > > > >>> 3. If you want to test, a git checkout is enough to test (assuming
> > > > >>> you
> > > > >>> did
> > > > >>> the git fetch above). Simpler IMO than git am (even if I assume
> > > > >>> most of
> > > > >>> us
> > > > >>> have scripts to process incoming mail)
> > > > > 
> > > > > qemu uses patchew, I think it would be worth to consider it for
> > > > > spice as well. It applies the patches on a mailing list, run some
> > > > > tests, gives you a stable URL, tracks the review (and the various
> > > > > iteration recently iirc)…
> > > > 
> > > > Good tool, but different problem.
> > > 
> > > I personally don't know which exact problem we are tackling ;) I see
> > > some people having issues keeping track of pending series, others saying
> > > that CI would be nice, ... Are we trying to solve one single very
> > > specific
> > > problem? If yes, what is it? Patch reviews not being done in a timely
> > > manner? Patch series being forgotten? Patch series status hard to know
> > > by email? Something else? (note that you said "problem", not "problems"
> > > :)
> > > 
> > > Christophe
> > > 
> > 
> > If the review is done months after been sent to the ML the author
> > has to review again the patches, not counting the time he has to
> > spend to update the series and test it again (as the master in the
> > meantime has moved).
> > 
> > Just for instance the png patches were sent on November and
> > are being seriously reviewed this month (so 8 months after).
> > I agree in this case where not pinged that much there were
> > also different discussion on them about.
> > There are a lot of stuff that get lost if the review is done
> > so late. Think about the searches done on the web, the experiments
> > before coming to that version, the commands used which could be
> > helpful again, talks with other people.
> 
> Ok, I take from that email that the main issue is untimely reviews, with
> which I agree. However, what we are currently discussing is how to
> better keep track of pending patch series. If the reason for the
> untimely reviews is that people are not aware that series A and B need
> reviewing, then I agree this is going to help. If the reason for the
> delayed reviews is because we don't have enough reviewers, no tools are
> going to help us here.
> 
> Christophe
> 

I think technically the problem is tracking the series, yes.
patchwork and patchew seems good at spotting them and offer different
features but still you end up with a long list of series with no
status where is easy to loose them.
I don't fully agree such a tool is useless even if the real problem is
few people are able to review patches fast enough. Having the status
of pending work helps choosing the pace of new development and having
a short list of pending series helps people reuse pending patches
if needed.
Spending more time on later reviews is also not helping much too.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe Fergeau]

On Tue, Jul 25, 2017 at 02:26:36PM +0200, Christophe de Dinechin wrote:
> >>> As long as contributor keep pinging or resending his series, this is 
> >>> already the case.
> >> 
> >> As Frediano said at the beginning of the series, “I’m tired of hearing 
> >> this reply”.
> > 
> > And this is not an actionable answer... My perception is that there
> > rarely are 'ping' on old series. Does this mean we are doing a good job
> > at reviews?
> 
> I think that we (but not I) are doing an OK job at reviews, but we apparently 
> drop
> some reviews, e.g. because they were too complex, or did not represent the
> priority of the time.
> 
> That being said, I observe that there are better ways to track WIP than pure 
> mail.
> Redmine, JIRA, pull requests, whatever. All well known solutions to the 
> problems
> we complain about.
> 
> As an aside, these tools typically solve many other problems too, like being 
> able to
> record things to be done *before* there is a patch for them, or CI, or 
> priorities, etc.
> Frankly, Bugzilla + Mail brings me back a good 15 years ago or something.
> 
> I don’t care much about which tool we use. I do mind that we have none.

The patch submitter's mind who sends ping when the series gets too old
can be seen as such a tool, with the added benefit that they know if the
series is still relevant, they can solve complex conflicts when
rebasing, ... :)

> > (I doubt it or we would not have this conversation) Does
> > this mean patch senders do not want to do that? Why? Does this mean it's
> > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > things work", with nothing specific.
> 
> It’s funny, because
> a) I never said I was unhappy, and
> b) I gave a very simple, very specific suggestion for action, which was to 
> add a
> URL to a branch with the name review// on freedesktop.org to 
> the
> cover letter or patch description.
> 
> So how you turn that into “I’m not happy with how things work with nothing 
> specific”
> is a bit beyond my understanding.

The quote (from you) on top of this part of my answer was
« As Frediano said at the beginning of the series, “I’m tired of hearing
this reply”. »
I was specifically referring to that, which is non-specific, and which I
interpret as unhappiness. I'll blame written media for any
misinterpretation here :)

> > If yes, what is it? Patch reviews not being done in a timely
> > manner? Patch series being forgotten? Patch series status hard to know
> > by email? Something else? (note that you said "problem", not "problems"
> > :)
> 
> Starting with the first in your list, but with the full knowledge of the 
> state of the art
> for tools solving this problem solves other problems that we discussed 
> earlier at the
> same time, and listing these problems as well.

Yes, but it's good to know what is the main issue people are having with
the current workflow, so that we make sure this really is fixed by any change
in tools and processes. The other features would then just be additional
niceties.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

Remove CxImage linking.
Support Windows BMP format.

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 Makefile.am |   4 +-
 configure.ac|   4 +-
 mingw-spice-vdagent.spec.in |  10 +--
 vdagent/image.cpp   | 172 +---
 vdagent/image.h |  21 ++
 5 files changed, 158 insertions(+), 53 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..f874e66 100644
--- a/mingw-spice-vdagent.spec.in
+++ b/mingw-spice-vdagent.spec.in
@@ -13,16 +13,10 @@ 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
-BuildRequires:  mingw64-libjpeg-turbo-static
+BuildRequires:  mingw32-gcc-c++
+BuildRequires:  mingw64-gcc-c++
 BuildRequires:  mingw32-libpng-static
 BuildRequires:  mingw64-libpng-static
-BuildRequires:  mingw32-libtiff-static
-BuildRequires:  mingw64-libtiff-static
 BuildRequires:  mingw32-zlib-static
 BuildRequires:  mingw64-zlib-static
 BuildRequires:  mingw32-winpthreads-static
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
index 960058d..faec18f 100644
--- a/vdagent/image.cpp
+++ b/vdagent/image.cpp
@@ -16,39 +16,48 @@
 */
 
 #include 
+#include 
+#include 
 
 #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&)
+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 coder(get_coder(clipboard.type));
+if (!coder) {
+return NULL;
+}
+
+format = CF_DIB;
+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;
 }
 
@@ -57,32 +66,115 @@ uint8_t* get_raw_clipboard_image(const 
VDAgentClipboardRequest& clipboard_reques
 {
 new_size = 0;
 
-CxImage image;
-uint8_t 

[Spice-devel] [vdagent-win PATCH v13 4/6] Support encoding PNG images

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 vdagent/imagepng.cpp | 118 +--
 1 file changed, 115 insertions(+), 3 deletions(-)

diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
index a89b7f7..78b4188 100644
--- a/vdagent/imagepng.cpp
+++ b/vdagent/imagepng.cpp
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "imagepng.h"
 
@@ -50,6 +51,43 @@ static void read_from_bufio(png_structp png, png_bytep out, 
png_size_t size)
 io.pos += size;
 }
 
+struct WriteBufferIo {
+uint8_t *buf;
+uint32_t pos, size;
+WriteBufferIo():
+buf(NULL), pos(0), size(0)
+{}
+~WriteBufferIo() { free(buf); }
+uint8_t *release() {
+uint8_t *res = buf;
+buf = NULL;
+pos = size = 0;
+return res;
+}
+};
+
+static void write_to_bufio(png_structp png, png_bytep in, png_size_t size)
+{
+WriteBufferIo& io(*(WriteBufferIo*)png_get_io_ptr(png));
+if (io.pos + size > io.size) {
+uint32_t new_size = io.size ? io.size * 2 : 4096;
+while (io.pos + size >= new_size) {
+new_size *= 2;
+}
+uint8_t *p = (uint8_t*) realloc(io.buf, new_size);
+if (!p)
+png_error(png, "out of memory");
+io.buf = p;
+io.size = new_size;
+}
+memcpy(io.buf+io.pos, in, size);
+io.pos += size;
+}
+
+static void flush_bufio(png_structp png)
+{
+}
+
 size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
 {
 return convert_to_dib(NULL, data, size);
@@ -233,10 +271,84 @@ 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 
)
+uint8_t *PngCoder::from_bitmap(const BITMAPINFO& bmp_info, const void *bits, 
long )
 {
-// TODO not implemented
-return NULL;
+// this vector is here to avoid leaking resources if libpng use 
setjmp/longjmp
+std::vector palette;
+WriteBufferIo io;
+
+png_structp png = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if (!png)
+return 0;
+
+png_infop info = png_create_info_struct(png);
+if (!info) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+if (setjmp(png_jmpbuf(png))) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+png_set_write_fn(png, , write_to_bufio, flush_bufio);
+
+const BITMAPINFOHEADER& head(bmp_info.bmiHeader);
+int color_type;
+int out_bits = head.biBitCount;
+switch (out_bits) {
+case 1:
+case 4:
+case 8:
+color_type = PNG_COLOR_TYPE_PALETTE;
+break;
+case 24:
+case 32:
+png_set_bgr(png);
+color_type = PNG_COLOR_TYPE_RGB;
+break;
+default:
+png_error(png, "BMP bit count not supported");
+break;
+}
+// TODO detect gray
+png_set_IHDR(png, info, head.biWidth, head.biHeight,
+ out_bits > 8 ? 8 : out_bits, color_type,
+ PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
+ PNG_FILTER_TYPE_DEFAULT);
+
+// palette
+if (color_type == PNG_COLOR_TYPE_PALETTE) {
+palette.resize(head.biClrUsed);
+const RGBQUAD *rgb = bmp_info.bmiColors;
+for (unsigned int color = 0; color < head.biClrUsed; ++color) {
+palette[color].red = rgb->rgbRed;
+palette[color].green = rgb->rgbGreen;
+palette[color].blue = rgb->rgbBlue;
+++rgb;
+}
+png_set_PLTE(png, info, [0], palette.size());
+}
+
+png_write_info(png, info);
+
+const unsigned int width = head.biWidth;
+const unsigned int height = head.biHeight;
+const size_t stride = compute_dib_stride(width, out_bits);
+const size_t image_size = stride * height;
+
+// now do the actual conversion!
+const uint8_t *src = (const uint8_t*)bits + image_size;
+for (unsigned int row = 0; row < height; ++row) {
+src -= stride;
+png_write_row(png, src);
+}
+png_write_end(png, NULL);
+
+png_destroy_write_struct(, );
+size = io.pos;
+return io.release();
 }
 
 ImageCoder *create_png_coder()
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-win PATCH v13 1/6] Move image handling to a separate file

[Frediano Ziglio]

This will make easier to change code that handle images.

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 Makefile.am |  2 ++
 vdagent/image.cpp   | 88 +
 vdagent/image.h | 48 +
 vdagent/vdagent.cpp | 57 +-
 4 files changed, 146 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 000..960058d
--- /dev/null
+++ b/vdagent/image.cpp
@@ -0,0 +1,88 @@
+/*
+   Copyright (C) 2013-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 .
+*/
+
+#include 
+
+#include "vdcommon.h"
+#include "image.h"
+
+#include "ximage.h"
+
+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&)
+{
+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)
+{
+new_size = 0;
+
+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 000..b70f53a
--- /dev/null
+++ b/vdagent/image.h
@@ -0,0 +1,48 @@
+/*
+   Copyright (C) 2013-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 .
+*/
+
+#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[in,out] format suggested clipboard format. This can be 

[Spice-devel] [vdagent-win PATCH v13 6/6] spec: run tests during RPM build if possible

[Frediano Ziglio]

Currently to fully run checks we need Wine for both 32 and 64 bit.
Some distros (like RHEL 7) don't seem to allow installing
both 32 and 64 bit versions so turn on checks based on distro
versions.

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 mingw-spice-vdagent.spec.in | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/mingw-spice-vdagent.spec.in b/mingw-spice-vdagent.spec.in
index f874e66..d25ea31 100644
--- a/mingw-spice-vdagent.spec.in
+++ b/mingw-spice-vdagent.spec.in
@@ -2,6 +2,19 @@
 
 #define _version_suffix -e198
 
+%if "%{_build_arch}" == "x86_64" && (0%{?fedora} && 0%{?fedora} >= 24)
+%define can_do_check_x64 1
+%define can_do_check_x86 1
+%else
+%if "%{_build_arch}" == "x86_64" && (0%{?epel} && 0%{?epel} >= 7)
+%define can_do_check_x64 1
+%define can_do_check_x86 0
+%else
+%define can_do_check_x64 0
+%define can_do_check_x86 0
+%endif
+%endif
+
 Name:   mingw-spice-vdagent
 Version:@VERSION@
 Release:1%{?dist}%{?extra_release}
@@ -22,6 +35,12 @@ BuildRequires:  mingw64-zlib-static
 BuildRequires:  mingw32-winpthreads-static
 BuildRequires:  mingw64-winpthreads-static
 BuildRequires:  pkgconfig
+%if 0%{can_do_check_x64}
+BuildRequires:  ImageMagick wine-core(x86-64)
+%endif
+%if 0%{can_do_check_x86}
+BuildRequires:  ImageMagick wine-core(x86-32)
+%endif
 
 BuildArch:  noarch
 
@@ -73,6 +92,14 @@ Features:
 %mingw_configure --enable-debug
 %mingw_make %{?_smp_mflags} V=1
 
+%if 0%{can_do_check_x86} || 0%{can_do_check_x64}
+%check
+%define mingw_build_win32 %{can_do_check_x86}
+%define mingw_build_win64 %{can_do_check_x64}
+%mingw_make check
+%define mingw_build_win32 1
+%define mingw_build_win64 1
+%endif
 
 %install
 %mingw_make_install DESTDIR=$RPM_BUILD_ROOT
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-win PATCH v13 5/6] Add test for PNG files

[Frediano Ziglio]

Test various image and formats.
The idea is to decode and encode again an image and
check for differences. ImageMagick is used to create
some test image and compare results.
Wine is used to execute a test helper.

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 Makefile.am   | 20 +
 test-png  | 58 
 vdagent/imagetest.cpp | 82 +++
 3 files changed, 160 insertions(+)
 create mode 100755 test-png
 create mode 100644 vdagent/imagetest.cpp

diff --git a/Makefile.am b/Makefile.am
index 175d8f7..411bf0d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -68,6 +68,26 @@ vdservice_rc.$(OBJEXT): vdservice/vdservice.rc
 
 MAINTAINERCLEANFILES += vdservice_rc.$(OBJEXT)
 
+check_PROGRAMS = imagetest
+
+imagetest_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32
+imagetest_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
+imagetest_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,console
+imagetest_SOURCES =\
+   common/vdcommon.cpp \
+   common/vdcommon.h   \
+   common/vdlog.cpp\
+   common/vdlog.h  \
+   vdagent/imagetest.cpp   \
+   vdagent/image.cpp   \
+   vdagent/image.h \
+   vdagent/imagepng.cpp\
+   vdagent/imagepng.h  \
+   $(NULL)
+
+TESTS = test-png
+EXTRA_DIST += test-png
+
 deps.txt:
$(AM_V_GEN)rpm -qa | grep $(host_os) | sort | unix2dos > $@
 
diff --git a/test-png b/test-png
new file mode 100755
index 000..ee9d86e
--- /dev/null
+++ b/test-png
@@ -0,0 +1,58 @@
+#!/bin/bash
+
+VERBOSE=${VERBOSE:-0}
+
+IN=in.png
+OUT=out.png
+OUT_BMP=out.bmp
+
+error() {
+echo "$*" >&2
+exit 1
+}
+
+verbose() {
+if [ $VERBOSE != 0 ]; then
+"$@"
+fi
+}
+
+compare_images() {
+DIFF=$(compare -metric AE $1 $2 - 2>&1 > /dev/null || true)
+if [ "$DIFF" != "0" ]; then
+error "Images $1 and $2 are too different, diff $DIFF"
+fi
+}
+
+do_test() {
+echo "Running image $IMAGE with '$*'..."
+convert $IMAGE "$@" $IN
+wine imagetest.exe $IN $OUT_BMP $OUT
+verbose ls -lh $IN
+verbose identify $IN
+verbose identify $OUT_BMP
+compare_images $IN $OUT
+compare_images $IN $OUT_BMP
+rm -f $IN $OUT $OUT_BMP
+}
+
+do_test_all() {
+IMAGE="$1"
+do_test
+do_test -type Palette
+do_test -type Palette -colors 14
+do_test -type Palette -colors 3
+do_test -type TrueColor
+do_test -type Grayscale -depth 8
+do_test -type Grayscale -depth 2
+do_test -type Grayscale -depth 4
+}
+
+set -e
+# test with small image
+do_test_all rose:
+# test with larger image
+do_test_all logo:
+rm -f $IN $OUT $OUT_BMP
+
+verbose echo Finish
diff --git a/vdagent/imagetest.cpp b/vdagent/imagetest.cpp
new file mode 100644
index 000..319b188
--- /dev/null
+++ b/vdagent/imagetest.cpp
@@ -0,0 +1,82 @@
+/*
+   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 .
+*/
+
+#undef NDEBUG
+#include 
+#include 
+
+#include "vdcommon.h"
+#include "image.h"
+#include "imagepng.h"
+
+int main(int argc, char **argv)
+{
+ImageCoder *coder = create_png_coder();
+
+assert(coder);
+if (argc < 2) {
+fprintf(stderr, "Usage: %s  [ []]\n", 
argv[0]);
+return 1;
+}
+
+// read all file into memory
+FILE *f = fopen(argv[1], "rb");
+assert(f);
+assert(fseek(f, 0, SEEK_END) == 0);
+long len = ftell(f);
+assert(len > 0);
+assert(fseek(f, 0, SEEK_SET) == 0);
+
+std::vector data(len);
+assert(fread([0], 1, len, f) == (unsigned long) len);
+fclose(f);
+
+size_t dib_size = coder->get_dib_size([0], len);
+assert(dib_size);
+std::vector out(dib_size);
+memset([0], 0xcc, dib_size);
+coder->get_dib_data([0], [0], len);
+
+// looks like many tools wants this header so craft it
+BITMAPFILEHEADER head;
+memset(, 0, sizeof(head));
+head.bfType = 'B'+'M'*256u;
+head.bfSize = sizeof(head) + dib_size;
+BITMAPINFOHEADER& info(*(BITMAPINFOHEADER*)[0]);
+head.bfOffBits = sizeof(head) + sizeof(BITMAPINFOHEADER) + 4 * 
info.biClrUsed;
+
+f = fopen(argc > 2 ? argv[2] : "out.bmp", "wb");
+assert(f);
+

[Spice-devel] [vdagent-win PATCH v13 3/6] Write code to decode PNG format

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 Makefile.am  |   6 +-
 configure.ac |   3 +
 vdagent/image.cpp|   8 +-
 vdagent/imagepng.cpp | 245 +++
 vdagent/imagepng.h   |  25 ++
 5 files changed, 278 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 faec18f..82cfb0e 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 000..a89b7f7
--- /dev/null
+++ b/vdagent/imagepng.cpp
@@ -0,0 +1,245 @@
+/*
+   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 .
+*/
+
+#include "vdcommon.h"
+
+#include 
+#include 
+
+#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 );
+private:
+size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t size);
+};
+
+struct ReadBufferIo {
+const uint8_t *buf;
+uint32_t pos, size;
+ReadBufferIo(const uint8_t *_buf, uint32_t _size):
+buf(_buf), pos(0), size(_size)
+{}
+};
+
+static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
+{
+ReadBufferIo& io(*(ReadBufferIo*)png_get_io_ptr(png));
+if (io.pos + size > io.size)
+png_error(png, "read past end");
+memcpy(out, io.buf+io.pos, size);
+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 int width);
+
+static void line_fixup_none(uint8_t *line, unsigned int width)
+{
+}
+
+static void line_fixup_2bpp_to_4bpp(uint8_t *line, unsigned int width)
+{
+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);
+}
+}
+
+size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t 
size)
+{
+ReadBufferIo io(data, size);
+
+png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if (!png)
+return 0;
+
+png_infop info = 

[Spice-devel] [vdagent-win PATCH v13 0/6] Rewrite image support

[Frediano Ziglio]

CxImage is used for image conversion for clipboard support.
CxImage have currently some issue:
- library is old and unsupported;
- required an old libpng library.
Currently the MingW binary we distribute due to some
issue have PNG disabled (so no clipboard image support).
Note that currently we support (before and after this patch)
only BMP and PNG. PNG is required as the default agent format.
However Windows programs wants to have BMP (specifically DIB)
in the clipboard.
This patch remove CxImage and use directly libpng (BMP is
supported directly by Windows APIs).
Tested with various formats and the compiled agent with a
Linux client.
The main concern is actually the possible problem to
support some weird/old BMP file formats however I don't
think any actual program will handle BMP2 (BMP3 was
introduced with Windows 3 probably 30 years ago).

The test patch add a test helper executable and a script
to test PNG encoding.

Changes since v12:
- change code style.

Changes since v11:
- change code to compute palette colors.

Changes since v10:
- use 2 buffer classes to make implementation easier;
- better format a long statement;
- remove RFC from spec check patch.

Changes since v9:
- add some more tests for different image bits (2,4,8) and gray;
- fix 2 bit gray images;
- make some comments more clear.

Changes since v8:
- test intermediate image too. This test single PNG -> BMP conversions.

Changes since v7:
- use png_set_bgr to avoid manually swap pixel components;
- use "unsigned int" instead of unsigned;
- use png_error instead of a manual longjmp;
- moved some code hunks in early patches;
- minor style changes.

Changes since v6:
- minor style changes;
- merge small change to imagetest from Uri (usage error).

Changes since v5:
- remove jpeg dependencies, were used just to satisfy
  CxImage but not used in the code.

Changes since v4:
- remove tiff dependency, was just for CxImage.

Changes since v3:
- reuse code to compute DIB stride;
- update documentation comments;
- update copyright lines.

Changes since v2:
- updated copyright lines;
- update some function documentation;
- fixed 2 bit PNGs;
- removed RFC from tests;
- improved test adding different images;
- merged Uri fix for memory.

Frediano Ziglio (6):
  Move image handling to a separate file
  Initial rewrite of image conversion code
  Write code to decode PNG format
  Support encoding PNG images
  Add test for PNG files
  spec: run tests during RPM build if possible

 Makefile.am |  28 +++-
 configure.ac|   5 +-
 mingw-spice-vdagent.spec.in |  37 -
 test-png|  58 +++
 vdagent/image.cpp   | 174 +
 vdagent/image.h |  69 +
 vdagent/imagepng.cpp| 357 
 vdagent/imagepng.h  |  25 
 vdagent/imagetest.cpp   |  82 ++
 vdagent/vdagent.cpp |  57 +--
 10 files changed, 831 insertions(+), 61 deletions(-)
 create mode 100755 test-png
 create mode 100644 vdagent/image.cpp
 create mode 100644 vdagent/image.h
 create mode 100644 vdagent/imagepng.cpp
 create mode 100644 vdagent/imagepng.h
 create mode 100644 vdagent/imagetest.cpp

-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe Fergeau]

On Tue, Jul 25, 2017 at 07:09:22AM -0400, Frediano Ziglio wrote:
> > 
> > > >>> I see several benefits to doing this:
> > > >>> 
> > > >>> 1. We always know exactly which component and branch is being patched
> > > >>> 
> > > > 
> > > > As long as contributor keep pinging or resending his series, this is
> > > > already the case.
> > > 
> > > As Frediano said at the beginning of the series, “I’m tired of hearing 
> > > this
> > > reply”.
> > 
> > And this is not an actionable answer... My perception is that there
> > rarely are 'ping' on old series. Does this mean we are doing a good job
> > at reviews? (I doubt it or we would not have this conversation) Does
> > this mean patch senders do not want to do that? Why? Does this mean it's
> > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > things work", with nothing specific.
> > 
> 
> Patch series are getting old (even years) repeatedly pinged (5/6 times)
> but they continue to not getting any feedback/ack/comment.
> If you can't remember any... this just confirms the problem.

So I went through my mails (searched for mails containing 'ping'), in
the last months I found 24 series which needed a ping, among these, 3
needed several pings, and it stopped at 2 pings. Maybe I missed some.

> 
> > > 
> > > > 
> > > >>> 3. If you want to test, a git checkout is enough to test (assuming you
> > > >>> did
> > > >>> the git fetch above). Simpler IMO than git am (even if I assume most 
> > > >>> of
> > > >>> us
> > > >>> have scripts to process incoming mail)
> > > > 
> > > > qemu uses patchew, I think it would be worth to consider it for
> > > > spice as well. It applies the patches on a mailing list, run some
> > > > tests, gives you a stable URL, tracks the review (and the various
> > > > iteration recently iirc)…
> > > 
> > > Good tool, but different problem.
> > 
> > I personally don't know which exact problem we are tackling ;) I see
> > some people having issues keeping track of pending series, others saying
> > that CI would be nice, ... Are we trying to solve one single very specific
> > problem? If yes, what is it? Patch reviews not being done in a timely
> > manner? Patch series being forgotten? Patch series status hard to know
> > by email? Something else? (note that you said "problem", not "problems"
> > :)
> > 
> > Christophe
> > 
> 
> If the review is done months after been sent to the ML the author
> has to review again the patches, not counting the time he has to
> spend to update the series and test it again (as the master in the
> meantime has moved).
> 
> Just for instance the png patches were sent on November and
> are being seriously reviewed this month (so 8 months after).
> I agree in this case where not pinged that much there were
> also different discussion on them about.
> There are a lot of stuff that get lost if the review is done
> so late. Think about the searches done on the web, the experiments
> before coming to that version, the commands used which could be
> helpful again, talks with other people.

Ok, I take from that email that the main issue is untimely reviews, with
which I agree. However, what we are currently discussing is how to
better keep track of pending patch series. If the reason for the
untimely reviews is that people are not aware that series A and B need
reviewing, then I agree this is going to help. If the reason for the
delayed reviews is because we don't have enough reviewers, no tools are
going to help us here.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v12 3/6] Write code to decode PNG format

[Christophe Fergeau]

On Mon, Jul 24, 2017 at 01:39:35PM +0100, Frediano Ziglio wrote:
> +size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, 
> size_t size)
> +{
[...]
> +const unsigned int width = png_get_image_width(png, info);
> +const unsigned int 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 int palette_colors = [=]() {

If you want to use a function, then give it a name, if you don't want to
use a function, then this can just go in convert_to_dib() body rather
than through a lambda (I know the latter means removing the 'const').

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v11 4/6] Support encoding PNG images

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Mon, Jul 24, 2017 at 01:10:24PM +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> Acked-by: Christophe Fergeau 
> ---
>  vdagent/imagepng.cpp | 118 
> +--
>  1 file changed, 115 insertions(+), 3 deletions(-)
> 
> diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
> index f966cc2..d15f2a8 100644
> --- a/vdagent/imagepng.cpp
> +++ b/vdagent/imagepng.cpp
> @@ -19,6 +19,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "imagepng.h"
>  
> @@ -50,6 +51,43 @@ static void read_from_bufio(png_structp png, png_bytep 
> out, png_size_t size)
>  io.pos += size;
>  }
>  
> +struct WriteBufferIo {
> +uint8_t *buf;
> +uint32_t pos, size;
> +WriteBufferIo():
> +buf(NULL), pos(0), size(0)
> +{}
> +~WriteBufferIo() { free(buf); }
> +uint8_t *release() {
> +uint8_t *res = buf;
> +buf = NULL;
> +pos = size = 0;
> +return res;
> +}
> +};
> +
> +static void write_to_bufio(png_structp png, png_bytep in, png_size_t size)
> +{
> +WriteBufferIo& io(*(WriteBufferIo*)png_get_io_ptr(png));
> +if (io.pos + size > io.size) {
> +uint32_t new_size = io.size ? io.size * 2 : 4096;
> +while (io.pos + size >= new_size) {
> +new_size *= 2;
> +}
> +uint8_t *p = (uint8_t*) realloc(io.buf, new_size);
> +if (!p)
> +png_error(png, "out of memory");
> +io.buf = p;
> +io.size = new_size;
> +}
> +memcpy(io.buf+io.pos, in, size);
> +io.pos += size;
> +}
> +
> +static void flush_bufio(png_structp png)
> +{
> +}
> +
>  size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
>  {
>  return convert_to_dib(NULL, data, size);
> @@ -226,10 +264,84 @@ 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 )
> +uint8_t *PngCoder::from_bitmap(const BITMAPINFO& bmp_info, const void *bits, 
> long )
>  {
> -// TODO not implemented
> -return NULL;
> +// this vector is here to avoid leaking resources if libpng use 
> setjmp/longjmp
> +std::vector palette;
> +WriteBufferIo io;
> +
> +png_structp png = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, 
> NULL, NULL);
> +if (!png)
> +return 0;
> +
> +png_infop info = png_create_info_struct(png);
> +if (!info) {
> +png_destroy_write_struct(, );
> +return 0;
> +}
> +
> +if (setjmp(png_jmpbuf(png))) {
> +png_destroy_write_struct(, );
> +return 0;
> +}
> +
> +png_set_write_fn(png, , write_to_bufio, flush_bufio);
> +
> +const BITMAPINFOHEADER& head(bmp_info.bmiHeader);
> +int color_type;
> +int out_bits = head.biBitCount;
> +switch (out_bits) {
> +case 1:
> +case 4:
> +case 8:
> +color_type = PNG_COLOR_TYPE_PALETTE;
> +break;
> +case 24:
> +case 32:
> +png_set_bgr(png);
> +color_type = PNG_COLOR_TYPE_RGB;
> +break;
> +default:
> +png_error(png, "BMP bit count not supported");
> +break;
> +}
> +// TODO detect gray
> +png_set_IHDR(png, info, head.biWidth, head.biHeight,
> + out_bits > 8 ? 8 : out_bits, color_type,
> + PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
> + PNG_FILTER_TYPE_DEFAULT);
> +
> +// palette
> +if (color_type == PNG_COLOR_TYPE_PALETTE) {
> +palette.resize(head.biClrUsed);
> +const RGBQUAD *rgb = bmp_info.bmiColors;
> +for (unsigned int color = 0; color < head.biClrUsed; ++color) {
> +palette[color].red = rgb->rgbRed;
> +palette[color].green = rgb->rgbGreen;
> +palette[color].blue = rgb->rgbBlue;
> +++rgb;
> +}
> +png_set_PLTE(png, info, [0], palette.size());
> +}
> +
> +png_write_info(png, info);
> +
> +const unsigned int width = head.biWidth;
> +const unsigned int height = head.biHeight;
> +const size_t stride = compute_dib_stride(width, out_bits);
> +const size_t image_size = stride * height;
> +
> +// now do the actual conversion!
> +const uint8_t *src = (const uint8_t*)bits + image_size;
> +for (unsigned int row = 0; row < height; ++row) {
> +src -= stride;
> +png_write_row(png, src);
> +}
> +png_write_end(png, NULL);
> +
> +png_destroy_write_struct(, );
> +size = io.pos;
> +return io.release();
>  }
>  
>  ImageCoder *create_png_coder()
> -- 
> 2.13.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature

Re: [Spice-devel] [spice-gtk v3] gstreamer: Take into account stride information

[Uri Lublin]

On 07/25/2017 06:30 PM, Pavel Grunt wrote:

On Tue, 2017-07-25 at 16:20 +0100, Frediano Ziglio wrote:

Using hardware encoders/decoders is possible that the output
stride of the image cannot be computed with a fixed formula
(width * 4). GStreamer in this case should set GstVideoMeta
information with the correct stride value.
Consider this value if present.
This fix a problem using NvEnc encoder and Intel decoder.

Signed-off-by: Frediano Ziglio 
---
  src/channel-display-gst.c   | 9 -
  src/channel-display-mjpeg.c | 2 +-
  src/channel-display-priv.h  | 3 ++-
  src/channel-display.c   | 6 --
  4 files changed, 15 insertions(+), 5 deletions(-)

Changes since v2:
- use spice_ prefix for new function.

Changes since v1:
- use a mnemonic value for unknown stride;
- better check for stride in GstVideoMeta.

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 03c6044..338e7d2 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -26,6 +26,7 @@
  #include 
  #include 
  #include 
+#include 
  
  
  /* GStreamer decoder implementation */

@@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
  
  static void schedule_frame(SpiceGstDecoder *decoder);
  
+static int spice_gst_buffer_get_stride(GstBuffer *buffer)

+{
+GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
+return video && video->n_planes > 0 ? video->stride[0] :
SPICE_UNKNOWN_STRIDE;
+}
+
  /* main context */
  static gboolean display_frame(gpointer video_decoder)
  {
@@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
  }
  
  stream_display_frame(decoder->base.stream, gstframe->frame,

- width, height, mapinfo.data);
+ width, height, spice_gst_buffer_get_stride(buffer),
mapinfo.data);
  gst_buffer_unmap(buffer, );
  
   error:

diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ba098c..563dc1b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
  
  /* Display the frame and dispose of it */

  stream_display_frame(decoder->base.stream, decoder->cur_frame,
- width, height, decoder->out_frame);
+ width, height, SPICE_UNKNOWN_STRIDE, decoder-

out_frame);

  free_spice_frame(decoder->cur_frame);
  decoder->cur_frame = NULL;
  decoder->timer_id = 0;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 04cb4d1..a0a420c 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -192,7 +192,8 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
  
  guint32 stream_get_time(display_stream *st);

  void stream_dropped_frame_on_playback(display_stream *st);
-void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
width, uint32_t height, uint8_t* data);
+#define SPICE_UNKNOWN_STRIDE 0
+void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
width, uint32_t height, int stride, uint8_t* data);
  gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
  
  
diff --git a/src/channel-display.c b/src/channel-display.c

index 06ed18a..45742a6 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream
*st)
  /* main context */
  G_GNUC_INTERNAL
  void stream_display_frame(display_stream *st, SpiceFrame *frame,
-  uint32_t width, uint32_t height, uint8_t *data)
+  uint32_t width, uint32_t height, int stride,
uint8_t *data)
  {
-int stride = width * sizeof(uint32_t);
+if (stride == SPICE_UNKNOWN_STRIDE) {
+stride = width * sizeof(uint32_t);
+}
  if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
  data += stride * (height - 1);
  stride = -stride;


Looks good to me, tested


Looks good to me too. I also tested this.
Thanks,
   Uri.



Acked-by: Pavel Grunt 

Thanks,
Pavel


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v3] gstreamer: Take into account stride information

[Frediano Ziglio]

Using hardware encoders/decoders is possible that the output
stride of the image cannot be computed with a fixed formula
(width * 4). GStreamer in this case should set GstVideoMeta
information with the correct stride value.
Consider this value if present.
This fix a problem using NvEnc encoder and Intel decoder.

Signed-off-by: Frediano Ziglio 
---
 src/channel-display-gst.c   | 9 -
 src/channel-display-mjpeg.c | 2 +-
 src/channel-display-priv.h  | 3 ++-
 src/channel-display.c   | 6 --
 4 files changed, 15 insertions(+), 5 deletions(-)

Changes since v2:
- use spice_ prefix for new function.

Changes since v1:
- use a mnemonic value for unknown stride;
- better check for stride in GstVideoMeta.

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 03c6044..338e7d2 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /* GStreamer decoder implementation */
@@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
 
 static void schedule_frame(SpiceGstDecoder *decoder);
 
+static int spice_gst_buffer_get_stride(GstBuffer *buffer)
+{
+GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
+return video && video->n_planes > 0 ? video->stride[0] : 
SPICE_UNKNOWN_STRIDE;
+}
+
 /* main context */
 static gboolean display_frame(gpointer video_decoder)
 {
@@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
 }
 
 stream_display_frame(decoder->base.stream, gstframe->frame,
- width, height, mapinfo.data);
+ width, height, spice_gst_buffer_get_stride(buffer), 
mapinfo.data);
 gst_buffer_unmap(buffer, );
 
  error:
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ba098c..563dc1b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer 
video_decoder)
 
 /* Display the frame and dispose of it */
 stream_display_frame(decoder->base.stream, decoder->cur_frame,
- width, height, decoder->out_frame);
+ width, height, SPICE_UNKNOWN_STRIDE, 
decoder->out_frame);
 free_spice_frame(decoder->cur_frame);
 decoder->cur_frame = NULL;
 decoder->timer_id = 0;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 04cb4d1..a0a420c 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -192,7 +192,8 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= 
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
 
 guint32 stream_get_time(display_stream *st);
 void stream_dropped_frame_on_playback(display_stream *st);
-void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, uint8_t* data);
+#define SPICE_UNKNOWN_STRIDE 0
+void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, int stride, uint8_t* data);
 gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
 
 
diff --git a/src/channel-display.c b/src/channel-display.c
index 06ed18a..45742a6 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream *st)
 /* main context */
 G_GNUC_INTERNAL
 void stream_display_frame(display_stream *st, SpiceFrame *frame,
-  uint32_t width, uint32_t height, uint8_t *data)
+  uint32_t width, uint32_t height, int stride, uint8_t 
*data)
 {
-int stride = width * sizeof(uint32_t);
+if (stride == SPICE_UNKNOWN_STRIDE) {
+stride = width * sizeof(uint32_t);
+}
 if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
 data += stride * (height - 1);
 stride = -stride;
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v2] gstreamer: Take into account stride information

[Frediano Ziglio]

Using hardware encoders/decoders is possible that the output
stride of the image cannot be computed with a fixed formula
(width * 4). GStreamer in this case should set GstVideoMeta
information with the correct stride value.
Consider this value if present.
This fix a problem using NvEnc encoder and Intel decoder.

Signed-off-by: Frediano Ziglio 
---
 src/channel-display-gst.c   | 9 -
 src/channel-display-mjpeg.c | 2 +-
 src/channel-display-priv.h  | 3 ++-
 src/channel-display.c   | 6 --
 4 files changed, 15 insertions(+), 5 deletions(-)

Changes since v1:
- use a mnemonic value for unknown stride;
- better check for stride in GstVideoMeta.

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 03c6044..6f738be 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /* GStreamer decoder implementation */
@@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
 
 static void schedule_frame(SpiceGstDecoder *decoder);
 
+static int gst_buffer_get_stride(GstBuffer *buffer)
+{
+GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
+return video && video->n_planes > 0 ? video->stride[0] : 
SPICE_UNKNOWN_STRIDE;
+}
+
 /* main context */
 static gboolean display_frame(gpointer video_decoder)
 {
@@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
 }
 
 stream_display_frame(decoder->base.stream, gstframe->frame,
- width, height, mapinfo.data);
+ width, height, gst_buffer_get_stride(buffer), 
mapinfo.data);
 gst_buffer_unmap(buffer, );
 
  error:
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ba098c..563dc1b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer 
video_decoder)
 
 /* Display the frame and dispose of it */
 stream_display_frame(decoder->base.stream, decoder->cur_frame,
- width, height, decoder->out_frame);
+ width, height, SPICE_UNKNOWN_STRIDE, 
decoder->out_frame);
 free_spice_frame(decoder->cur_frame);
 decoder->cur_frame = NULL;
 decoder->timer_id = 0;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 04cb4d1..a0a420c 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -192,7 +192,8 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= 
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
 
 guint32 stream_get_time(display_stream *st);
 void stream_dropped_frame_on_playback(display_stream *st);
-void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, uint8_t* data);
+#define SPICE_UNKNOWN_STRIDE 0
+void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, int stride, uint8_t* data);
 gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
 
 
diff --git a/src/channel-display.c b/src/channel-display.c
index 06ed18a..45742a6 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream *st)
 /* main context */
 G_GNUC_INTERNAL
 void stream_display_frame(display_stream *st, SpiceFrame *frame,
-  uint32_t width, uint32_t height, uint8_t *data)
+  uint32_t width, uint32_t height, int stride, uint8_t 
*data)
 {
-int stride = width * sizeof(uint32_t);
+if (stride == SPICE_UNKNOWN_STRIDE) {
+stride = width * sizeof(uint32_t);
+}
 if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
 data += stride * (height - 1);
 stride = -stride;
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] gstreamer: Take into account stride information

[Pavel Grunt]

On Tue, 2017-07-25 at 10:50 -0400, Frediano Ziglio wrote:
> > 
> > Hi Frediano,
> > 
> > On Tue, 2017-07-25 at 15:31 +0100, Frediano Ziglio wrote:
> > > Using hardware encoders/decoders is possible that the output
> > > stride of the image cannot be computed with a fixed formula
> > > (width * 4). GStreamer in this case should set GstVideoMeta
> > > information with the correct stride value.
> > > Consider this value if present.
> > > This fix a problem using NvDec encoder and Intel decoder.
> > 
> > you mean NVENC encoder
> > 
> 
> correct :)
> 
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  src/channel-display-gst.c   | 9 -
> > >  src/channel-display-mjpeg.c | 2 +-
> > >  src/channel-display-priv.h  | 2 +-
> > >  src/channel-display.c   | 6 --
> > >  4 files changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 03c6044..0abe706 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -26,6 +26,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  
> > >  /* GStreamer decoder implementation */
> > > @@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
> > >  
> > >  static void schedule_frame(SpiceGstDecoder *decoder);
> > >  
> > > +static int gst_buffer_get_stride(GstBuffer *buffer)
> > 
> > I'd prefer to do in our namespace instead on gstreamer's
> > > +{
> > > +GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
> > > +return video ? video->stride[0] : 0;
> > 
> > The documentation of GstVideoMeta says that the value of the stride may be
> > invalid - do you know what is the value in that case?
> > 
> > What about using some constant instead of 0 (respectively the invalid value)
> > (SPICE_UNKNOWN_STRIDE)
> > 
> 
> No idea the gstreamer invalid value. Maybe n_planes == 0 ?
> Where do you suggest to define it?
> 
> #define SPICE_UNKNOWN_STRIDE 0
> 
> in src/channel-display-priv.h ?
> 
yes

> > 
> > > +}
> > > +
> > >  /* main context */
> > >  static gboolean display_frame(gpointer video_decoder)
> > >  {
> > > @@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
> > >  }
> > >  
> > >  stream_display_frame(decoder->base.stream, gstframe->frame,
> > > - width, height, mapinfo.data);
> > > + width, height, gst_buffer_get_stride(buffer),
> > > mapinfo.data);
> > >  gst_buffer_unmap(buffer, );
> > >  
> > >   error:
> > > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > > index 3ba098c..6082511 100644
> > > --- a/src/channel-display-mjpeg.c
> > > +++ b/src/channel-display-mjpeg.c
> > > @@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > > video_decoder)
> > >  
> > >  /* Display the frame and dispose of it */
> > >  stream_display_frame(decoder->base.stream, decoder->cur_frame,
> > > - width, height, decoder->out_frame);
> > > + width, height, 0, decoder->out_frame);
> > 
> > same here
> > 
> > >  free_spice_frame(decoder->cur_frame);
> > >  decoder->cur_frame = NULL;
> > >  decoder->timer_id = 0;
> > > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > > index 04cb4d1..72e0840 100644
> > > --- a/src/channel-display-priv.h
> > > +++ b/src/channel-display-priv.h
> > > @@ -192,7 +192,7 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
> > > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > >  
> > >  guint32 stream_get_time(display_stream *st);
> > >  void stream_dropped_frame_on_playback(display_stream *st);
> > > -void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> > > width, uint32_t height, uint8_t* data);
> > > +void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> > > width, uint32_t height, int stride, uint8_t* data);
> > >  gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream
> > > *st);
> > >  
> > >  
> > > diff --git a/src/channel-display.c b/src/channel-display.c
> > > index 06ed18a..4eb665f 100644
> > > --- a/src/channel-display.c
> > > +++ b/src/channel-display.c
> > > @@ -1312,9 +1312,11 @@ void
> > > stream_dropped_frame_on_playback(display_stream
> > > *st)
> > >  /* main context */
> > >  G_GNUC_INTERNAL
> > >  void stream_display_frame(display_stream *st, SpiceFrame *frame,
> > > -  uint32_t width, uint32_t height, uint8_t *data)
> > > +  uint32_t width, uint32_t height, int stride,
> > > uint8_t *data)
> > >  {
> > > -int stride = width * sizeof(uint32_t);
> > > +if (stride == 0) {
> > 
> > and here
> > > +stride = width * sizeof(uint32_t);
> > > +}
> > >  if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
> > >  data += stride * (height - 1);
> > >  stride = -stride;
> 
> Frediano

Re: [Spice-devel] [spice-gtk] gstreamer: Take into account stride information

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> On Tue, 2017-07-25 at 15:31 +0100, Frediano Ziglio wrote:
> > Using hardware encoders/decoders is possible that the output
> > stride of the image cannot be computed with a fixed formula
> > (width * 4). GStreamer in this case should set GstVideoMeta
> > information with the correct stride value.
> > Consider this value if present.
> > This fix a problem using NvDec encoder and Intel decoder.
> you mean NVENC encoder
> 

correct :)

> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/channel-display-gst.c   | 9 -
> >  src/channel-display-mjpeg.c | 2 +-
> >  src/channel-display-priv.h  | 2 +-
> >  src/channel-display.c   | 6 --
> >  4 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 03c6044..0abe706 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  
> >  /* GStreamer decoder implementation */
> > @@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
> >  
> >  static void schedule_frame(SpiceGstDecoder *decoder);
> >  
> > +static int gst_buffer_get_stride(GstBuffer *buffer)
> I'd prefer to do in our namespace instead on gstreamer's
> > +{
> > +GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
> > +return video ? video->stride[0] : 0;
> The documentation of GstVideoMeta says that the value of the stride may be
> invalid - do you know what is the value in that case?
> 
> What about using some constant instead of 0 (respectively the invalid value)
> (SPICE_UNKNOWN_STRIDE)
> 

No idea the gstreamer invalid value. Maybe n_planes == 0 ?
Where do you suggest to define it?

#define SPICE_UNKNOWN_STRIDE 0

in src/channel-display-priv.h ?

> 
> > +}
> > +
> >  /* main context */
> >  static gboolean display_frame(gpointer video_decoder)
> >  {
> > @@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
> >  }
> >  
> >  stream_display_frame(decoder->base.stream, gstframe->frame,
> > - width, height, mapinfo.data);
> > + width, height, gst_buffer_get_stride(buffer),
> > mapinfo.data);
> >  gst_buffer_unmap(buffer, );
> >  
> >   error:
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index 3ba098c..6082511 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > video_decoder)
> >  
> >  /* Display the frame and dispose of it */
> >  stream_display_frame(decoder->base.stream, decoder->cur_frame,
> > - width, height, decoder->out_frame);
> > + width, height, 0, decoder->out_frame);
> 
> same here
> 
> >  free_spice_frame(decoder->cur_frame);
> >  decoder->cur_frame = NULL;
> >  decoder->timer_id = 0;
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 04cb4d1..72e0840 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -192,7 +192,7 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> >  
> >  guint32 stream_get_time(display_stream *st);
> >  void stream_dropped_frame_on_playback(display_stream *st);
> > -void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> > width, uint32_t height, uint8_t* data);
> > +void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> > width, uint32_t height, int stride, uint8_t* data);
> >  gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
> >  
> >  
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 06ed18a..4eb665f 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream
> > *st)
> >  /* main context */
> >  G_GNUC_INTERNAL
> >  void stream_display_frame(display_stream *st, SpiceFrame *frame,
> > -  uint32_t width, uint32_t height, uint8_t *data)
> > +  uint32_t width, uint32_t height, int stride,
> > uint8_t *data)
> >  {
> > -int stride = width * sizeof(uint32_t);
> > +if (stride == 0) {
> and here
> > +stride = width * sizeof(uint32_t);
> > +}
> >  if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
> >  data += stride * (height - 1);
> >  stride = -stride;
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] gstreamer: Take into account stride information

[Pavel Grunt]

Hi Frediano,

On Tue, 2017-07-25 at 15:31 +0100, Frediano Ziglio wrote:
> Using hardware encoders/decoders is possible that the output
> stride of the image cannot be computed with a fixed formula
> (width * 4). GStreamer in this case should set GstVideoMeta
> information with the correct stride value.
> Consider this value if present.
> This fix a problem using NvDec encoder and Intel decoder.
you mean NVENC encoder

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/channel-display-gst.c   | 9 -
>  src/channel-display-mjpeg.c | 2 +-
>  src/channel-display-priv.h  | 2 +-
>  src/channel-display.c   | 6 --
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 03c6044..0abe706 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  
>  /* GStreamer decoder implementation */
> @@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
>  
>  static void schedule_frame(SpiceGstDecoder *decoder);
>  
> +static int gst_buffer_get_stride(GstBuffer *buffer)
I'd prefer to do in our namespace instead on gstreamer's
> +{
> +GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
> +return video ? video->stride[0] : 0;
The documentation of GstVideoMeta says that the value of the stride may be
invalid - do you know what is the value in that case?

What about using some constant instead of 0 (respectively the invalid value)
(SPICE_UNKNOWN_STRIDE)


> +}
> +
>  /* main context */
>  static gboolean display_frame(gpointer video_decoder)
>  {
> @@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
>  }
>  
>  stream_display_frame(decoder->base.stream, gstframe->frame,
> - width, height, mapinfo.data);
> + width, height, gst_buffer_get_stride(buffer),
> mapinfo.data);
>  gst_buffer_unmap(buffer, );
>  
>   error:
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 3ba098c..6082511 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
>  
>  /* Display the frame and dispose of it */
>  stream_display_frame(decoder->base.stream, decoder->cur_frame,
> - width, height, decoder->out_frame);
> + width, height, 0, decoder->out_frame);

same here

>  free_spice_frame(decoder->cur_frame);
>  decoder->cur_frame = NULL;
>  decoder->timer_id = 0;
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 04cb4d1..72e0840 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -192,7 +192,7 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
> SPICE_VIDEO_CODEC_TYPE_ENUM_END);
>  
>  guint32 stream_get_time(display_stream *st);
>  void stream_dropped_frame_on_playback(display_stream *st);
> -void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> width, uint32_t height, uint8_t* data);
> +void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> width, uint32_t height, int stride, uint8_t* data);
>  gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
>  
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 06ed18a..4eb665f 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream
> *st)
>  /* main context */
>  G_GNUC_INTERNAL
>  void stream_display_frame(display_stream *st, SpiceFrame *frame,
> -  uint32_t width, uint32_t height, uint8_t *data)
> +  uint32_t width, uint32_t height, int stride,
> uint8_t *data)
>  {
> -int stride = width * sizeof(uint32_t);
> +if (stride == 0) {
and here
> +stride = width * sizeof(uint32_t);
> +}
>  if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
>  data += stride * (height - 1);
>  stride = -stride;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk] gstreamer: Take into account stride information

[Frediano Ziglio]

Using hardware encoders/decoders is possible that the output
stride of the image cannot be computed with a fixed formula
(width * 4). GStreamer in this case should set GstVideoMeta
information with the correct stride value.
Consider this value if present.
This fix a problem using NvDec encoder and Intel decoder.

Signed-off-by: Frediano Ziglio 
---
 src/channel-display-gst.c   | 9 -
 src/channel-display-mjpeg.c | 2 +-
 src/channel-display-priv.h  | 2 +-
 src/channel-display.c   | 6 --
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 03c6044..0abe706 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /* GStreamer decoder implementation */
@@ -102,6 +103,12 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
 
 static void schedule_frame(SpiceGstDecoder *decoder);
 
+static int gst_buffer_get_stride(GstBuffer *buffer)
+{
+GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
+return video ? video->stride[0] : 0;
+}
+
 /* main context */
 static gboolean display_frame(gpointer video_decoder)
 {
@@ -145,7 +152,7 @@ static gboolean display_frame(gpointer video_decoder)
 }
 
 stream_display_frame(decoder->base.stream, gstframe->frame,
- width, height, mapinfo.data);
+ width, height, gst_buffer_get_stride(buffer), 
mapinfo.data);
 gst_buffer_unmap(buffer, );
 
  error:
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ba098c..6082511 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -176,7 +176,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer 
video_decoder)
 
 /* Display the frame and dispose of it */
 stream_display_frame(decoder->base.stream, decoder->cur_frame,
- width, height, decoder->out_frame);
+ width, height, 0, decoder->out_frame);
 free_spice_frame(decoder->cur_frame);
 decoder->cur_frame = NULL;
 decoder->timer_id = 0;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 04cb4d1..72e0840 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -192,7 +192,7 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= 
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
 
 guint32 stream_get_time(display_stream *st);
 void stream_dropped_frame_on_playback(display_stream *st);
-void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, uint8_t* data);
+void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t 
width, uint32_t height, int stride, uint8_t* data);
 gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream *st);
 
 
diff --git a/src/channel-display.c b/src/channel-display.c
index 06ed18a..4eb665f 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1312,9 +1312,11 @@ void stream_dropped_frame_on_playback(display_stream *st)
 /* main context */
 G_GNUC_INTERNAL
 void stream_display_frame(display_stream *st, SpiceFrame *frame,
-  uint32_t width, uint32_t height, uint8_t *data)
+  uint32_t width, uint32_t height, int stride, uint8_t 
*data)
 {
-int stride = width * sizeof(uint32_t);
+if (stride == 0) {
+stride = width * sizeof(uint32_t);
+}
 if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
 data += stride * (height - 1);
 stride = -stride;
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe de Dinechin]

> On 25 Jul 2017, at 12:36, Christophe Fergeau  wrote:
> 
> On Tue, Jul 25, 2017 at 12:23:34PM +0200, Christophe de Dinechin wrote:
>>> and there are plenty of public places you can push your work.
>> 
>> So plenty of places, but by no means a shared one? The point is that
>> we need a shared one, to be able to view pending reviews at a glance.
> 
> Note that "we need a way to easily view pending reviews" does not imply
> "this has to be done through review/ branches in a shared repository”

No, it does not. I’m just pointing out one possible way to do it while 
maintaining
the mail-based process that some people here seem to love.

> 
> 
>>> If necessary, a cover-letter should say on which commit the series applies. 
>>> There are tools like patchew that may also help. 
>> 
>> My point is that 1) this is not the case today, and 2) a git link is
>> the best way to summarize the history (faster and more reliable than
>> a cover letter)
> 
> A git branch won't tell you the changes between different versions of
> the same series.

Just like a single mail won’t tell you the changes between this patch and
the previous one. You have to look at the history to find it. As an aside,
git-publish uses tags to preserve the previous versions of a patch in the
git history.


> You can use -v1, -v2 suffixes, but then you are keeping
> obsolete stuff around.

You are not keeping obsolete stuff around, you are keeping a history.
A mailing list is not a better way to record the history of changes than git 
itself.
But at least, you can cleanup branches once the patch is merged (knowing
that the actual history of the patch is recorded in the mail should we need it).
With patches, it’s much harder to look at an e-mail and say “that was merged”.

So no, I don’t think you’d keep obsolete stuff around. You’d keep
“not yet reviewed stuff” around, which is a feature, not a bug.


> And then, a diff between 2 such branches is not
> the best way of summarizing what changed (we would not need commit logs
> if a diff was expressive enough).

I don’t understand what you are saying here.


> And a cover letter usually contains more than just history summary, so still 
> useful ;)

I do not advocate we don’t send patch or a cover letter, I advocate that we send
a git URL along with the patch (or any other practical way to track if that 
thing
was reviewed and actioned upon).


> 
> 
> I see several benefits to doing this:
> 
> 1. We always know exactly which component and branch is being patched
> 
>>> 
>>> As long as contributor keep pinging or resending his series, this is 
>>> already the case.
>> 
>> As Frediano said at the beginning of the series, “I’m tired of hearing this 
>> reply”.
> 
> And this is not an actionable answer... My perception is that there
> rarely are 'ping' on old series. Does this mean we are doing a good job
> at reviews?

I think that we (but not I) are doing an OK job at reviews, but we apparently 
drop
some reviews, e.g. because they were too complex, or did not represent the
priority of the time.

That being said, I observe that there are better ways to track WIP than pure 
mail.
Redmine, JIRA, pull requests, whatever. All well known solutions to the problems
we complain about.

As an aside, these tools typically solve many other problems too, like being 
able to
record things to be done *before* there is a patch for them, or CI, or 
priorities, etc.
Frankly, Bugzilla + Mail brings me back a good 15 years ago or something.

I don’t care much about which tool we use. I do mind that we have none.


> (I doubt it or we would not have this conversation) Does
> this mean patch senders do not want to do that? Why? Does this mean it's
> done a lot, but to no avail? All I'm reading is "I'm not happy with how
> things work", with nothing specific.

It’s funny, because
a) I never said I was unhappy, and
b) I gave a very simple, very specific suggestion for action, which was to add a
URL to a branch with the name review// on freedesktop.org to the
cover letter or patch description.

So how you turn that into “I’m not happy with how things work with nothing 
specific”
is a bit beyond my understanding.

As a new team member, I’m find it unnecessarily difficult to know what a
given patch applies to, or to reconstruct code that actually compiles for 
testing,
or to see how it was tested before I look at the code, etc. I noticed that the 
patch
URL would *also* solve that problem. That’s about it.

As an aside, I find it strange to have private repos of half a dozen team 
members
in my history already, instead of a shared repo for things we are working on.


> 
>>> Eventually, the contributor can send a WIP series for some input without 
>>> detailed review.
>> 
>> I think you meant “eventually” to mean the same thing as
>> “eventuellement” in French (if necessary or possibly), not in the
>> english sense of “at the end”?
>> 
>> I did that with my early recorder WIP. You apparently 

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe Fergeau]

On Tue, Jul 25, 2017 at 12:23:34PM +0200, Christophe de Dinechin wrote:
> > and there are plenty of public places you can push your work.
> 
> So plenty of places, but by no means a shared one? The point is that
> we need a shared one, to be able to view pending reviews at a glance.

Note that "we need a way to easily view pending reviews" does not imply
"this has to be done through review/ branches in a shared repository"


> > If necessary, a cover-letter should say on which commit the series applies. 
> > There are tools like patchew that may also help. 
> 
> My point is that 1) this is not the case today, and 2) a git link is
> the best way to summarize the history (faster and more reliable than
> a cover letter)

A git branch won't tell you the changes between different versions of
the same series. You can use -v1, -v2 suffixes, but then you are keeping
obsolete stuff around. And then, a diff between 2 such branches is not
the best way of summarizing what changed (we would not need commit logs
if a diff was expressive enough). And a cover letter usually contains more
than just history summary, so still useful ;)


> >>> I see several benefits to doing this:
> >>> 
> >>> 1. We always know exactly which component and branch is being patched
> >>> 
> > 
> > As long as contributor keep pinging or resending his series, this is 
> > already the case.
> 
> As Frediano said at the beginning of the series, “I’m tired of hearing this 
> reply”.

And this is not an actionable answer... My perception is that there
rarely are 'ping' on old series. Does this mean we are doing a good job
at reviews? (I doubt it or we would not have this conversation) Does
this mean patch senders do not want to do that? Why? Does this mean it's
done a lot, but to no avail? All I'm reading is "I'm not happy with how
things work", with nothing specific.

> > Eventually, the contributor can send a WIP series for some input without 
> > detailed review.
> 
> I think you meant “eventually” to mean the same thing as
> “eventuellement” in French (if necessary or possibly), not in the
> english sense of “at the end”?
> 
> I did that with my early recorder WIP. You apparently did not like
> that way of proceeding.

Iirc you sent a link to a git branch without the accompanying patch
series, and this was what Marc-André asked you not to do.

> 
> > 
> >>> 3. If you want to test, a git checkout is enough to test (assuming you did
> >>> the git fetch above). Simpler IMO than git am (even if I assume most of us
> >>> have scripts to process incoming mail)
> > 
> > qemu uses patchew, I think it would be worth to consider it for
> > spice as well. It applies the patches on a mailing list, run some
> > tests, gives you a stable URL, tracks the review (and the various
> > iteration recently iirc)…
> 
> Good tool, but different problem.

I personally don't know which exact problem we are tackling ;) I see
some people having issues keeping track of pending series, others saying
that CI would be nice, ... Are we trying to solve one single very specific
problem? If yes, what is it? Patch reviews not being done in a timely
manner? Patch series being forgotten? Patch series status hard to know
by email? Something else? (note that you said "problem", not "problems"
:)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe de Dinechin]

> On 24 Jul 2017, at 16:06, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> - Original Message -
>>> 
 On 21 Jul 2017, at 12:41, Frediano Ziglio  wrote:
 
> 
> On Fri, Jul 21, 2017 at 06:18:49AM -0400, Frediano Ziglio wrote:
>> Beside that I wonder why I had to wait 8 months for these reviews,
>> not counting the time to decide to rewrite this part of code
>> (with all the wasted time trying to not do it) and the time we
>> waited to fix a known bug which is also a regression (image copy
>> paste are not working) and potentially a security risk (the library
>> versions used contain security bugs but actually the code is disabled).
>> Looks like that if a Red Hat client is not pushing for a fix
>> solutions are not that important.
> 
> People can be busy with other things, and patches can fall through the
> cracks, it's ok to send a 'ping' message once in a while to bring a
> patch series  back to people's attention.
> 
> Christophe
> 
 
 I'm really getting tired of this reply..
>>> 
>>> Hmmm, maybe we can do a few things to reduce that problem.
>>> It would be nice if we had a way to get back to unreviewed patches
>>> without having to browse through the mailing list.
>>> 
>>> Also, as a newbie, I must say I often spend quite some time finding
>>> the base for a patch, which is a shame, because that’s a problem
>>> that git URLs solve quite nicely. The problem is amplified by the
>>> fact that we have multiple components, and patches sometimes
>>> don’t really specify which component they apply to.
>>> 
>>> So maybe we can fix the two problems with one stone. What about
>>> 
>>> 1. Pushing branches for review under a consistent name, e.g. something
>>> like ‘review/c3d/recorder’
>>> 
> 
> You forgot to say that you mean 'pushing in upstream official repo’.

No, in some repository shared by the team (at large), so that we can, at a 
glance, look at what has not been reviewed yet.

> This is not how 'distributed' is supposed to work imho. If every contributor 
> should start pushing is branch, and leave it outdated for ages..

The problem of outdated patches is what we have today, see Frediano’s message 
that started the thread.

> What are the rules about cleaning up those old personal branches? Who should 
> be allowed to push, when etc? no, thanks, use git

How is using git URLs not “using git”?

> and there are plenty of public places you can push your work.

So plenty of places, but by no means a shared one? The point is that we need a 
shared one, to be able to view pending reviews at a glance.

> Then you can use the mailing list to announce it, or to send your patches for 
> review.

Last time I shared a private link as an RFC, you told me this was not the right 
way to do things. Which is why I am now suggesting we also keep the mail patch.


> 
>>> 2. Adding the git URL in the patch, e.g.
>>> https://cgit.freedesktop.org/spice/spice-gtk/log/?h=review/c3d/recorder 
>>> .
>>> 
> 
> That you can already do in a mail, although such url tends to become obsolete 
> pretty quickly. patches series are better archived.

It gets obsolete even faster if you remove the branch I push for illustration 
seconds after I push it ;-)

> If necessary, a cover-letter should say on which commit the series applies. 
> There are tools like patchew that may also help. 

My point is that 1) this is not the case today, and 2) a git link is the best 
way to summarize the history (faster and more reliable than  a cover letter)

> 
>>> 3. Once the patch has been committed, simply do a ‘git push freedesktop
>>> :review/c3d/recorder’
>>> to delete the branch.
> 
> This is extra burden, and will leave broken links

A broken link is useful information. It means “this work has been merged”. As 
for the burden, it’s totally negligible relative to figuring out today if a 
specific patch has been merged.

> 
>>> I see several benefits to doing this:
>>> 
>>> 1. We always know exactly which component and branch is being patched
>>> 
> 
> As long as contributor keep pinging or resending his series, this is already 
> the case.

As Frediano said at the beginning of the series, “I’m tired of hearing this 
reply”.

> 
> A series that is no longer being proposed is basically considered abandoned. 
> I think that rule of thumb works fine in practice.

Some of us disagree.


> 
>>> 2. When you work on branch, a daily “git fetch” will fetch all the new
>>> “review”
>>> branches, so a simple git branch -a will show you what needs to be
>>> reviewed.
> 
> As a developer or as a maintainer, you are not often interested by the 
> various iterations of a work. It's enough to be on CC of a series to review.

If you are not interested, don’t look at the ‘review’ branches then. Where’s 
the problem?

> Eventually, the contributor can send a WIP 

Re: [Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Frediano Ziglio]

> 
> Hi
> 
> - Original Message -
> > ...
> > 
> > > 
> > > > > 3. If you want to test, a git checkout is enough to test (assuming
> > > > > you
> > > > > did
> > > > > the git fetch above). Simpler IMO than git am (even if I assume most
> > > > > of
> > > > > us
> > > > > have scripts to process incoming mail)
> > > 
> > > qemu uses patchew, I think it would be worth to consider it for spice as
> > > well. It applies the patches on a mailing list, run some tests, gives you
> > > a
> > > stable URL, tracks the review (and the various iteration recently
> > > iirc)...
> > > 
> > 
> > How difficult is to setup and maintain patchew?
> > Can be setup on a cloud?
> > Different people here are quite good with Gitlab and using PR Gitlab I
> > think would be easier.
> 
> If the primary mean to do patch review is through ML (which I prefer
> personally), then patchew is a worth complementary tool.
> 
> I don't know how hard it is to set up, but there is a link to request adding
> new projects on http://patchew.org/
> 

Looks interesting but looks more like an improved version of patchwork
adding test support and some additional notification.
Looking at the feature list there's nothing that seems to help with
old series tracking. For instance looking at Qemu patches
(http://patchew.org/QEMU/) there's no status of the merge. Maybe
there are more option if you log in but I think the status should
be visible (and editable by maintainers).

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Fix build without egl

[Marc-André Lureau]

- Original Message -
> spice_display_widget_gl_scanout is defined only when building with egl
>  ./.libs/libspice-client-gtk-3.0.so: undefined reference to
>  `spice_display_widget_gl_scanout'
> ---
>  src/spice-widget-priv.h | 2 ++
>  src/spice-widget.c  | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> index ea7ed8e..1189cbb 100644
> --- a/src/spice-widget-priv.h
> +++ b/src/spice-widget-priv.h
> @@ -169,7 +169,9 @@ gboolean spice_egl_update_scanout
> (SpiceDisplay *display,
>GError **err);
>  void spice_egl_cursor_set(SpiceDisplay *display);
>  
> +#ifdef HAVE_EGL
>  void spice_display_widget_gl_scanout (SpiceDisplay *display);
> +#endif
>  void spice_display_widget_update_monitor_area(SpiceDisplay *display);
>  
>  G_END_DECLS
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 6f4abc0..d5ebd9d 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -616,7 +616,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS
>  static void
>  drawing_area_realize(GtkWidget *area, gpointer user_data)
>  {
> -#ifdef GDK_WINDOWING_X11
> +#if defined(GDK_WINDOWING_X11) && defined(HAVE_EGL)
>  SpiceDisplay *display = SPICE_DISPLAY(user_data);
>  
>  if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> --

ack (fixes for 977db3bb)

> 2.13.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk] Fix build without egl

[Pavel Grunt]

spice_display_widget_gl_scanout is defined only when building with egl
 ./.libs/libspice-client-gtk-3.0.so: undefined reference to 
`spice_display_widget_gl_scanout'
---
 src/spice-widget-priv.h | 2 ++
 src/spice-widget.c  | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
index ea7ed8e..1189cbb 100644
--- a/src/spice-widget-priv.h
+++ b/src/spice-widget-priv.h
@@ -169,7 +169,9 @@ gboolean spice_egl_update_scanout(SpiceDisplay 
*display,
   GError **err);
 void spice_egl_cursor_set(SpiceDisplay *display);
 
+#ifdef HAVE_EGL
 void spice_display_widget_gl_scanout (SpiceDisplay *display);
+#endif
 void spice_display_widget_update_monitor_area(SpiceDisplay *display);
 
 G_END_DECLS
diff --git a/src/spice-widget.c b/src/spice-widget.c
index 6f4abc0..d5ebd9d 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -616,7 +616,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS
 static void
 drawing_area_realize(GtkWidget *area, gpointer user_data)
 {
-#ifdef GDK_WINDOWING_X11
+#if defined(GDK_WINDOWING_X11) && defined(HAVE_EGL)
 SpiceDisplay *display = SPICE_DISPLAY(user_data);
 
 if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
-- 
2.13.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1] display-gst: Improve h264 elements filtering

[Victor Toso]

Hi,

On Wed, Jul 19, 2017 at 11:34:14AM +0200, Victor Toso wrote:
> Hi,
>
> On Thu, Jul 13, 2017 at 02:33:48PM +0200, Victor Toso wrote:
> > From: Victor Toso 
> >
> > This patch fixes the avdec_h264 element not being present on
> > gstvideo_has_codec() which get all decoder elements from GstRegistry
> > and filter them on our GstCaps in order to get the ones for given
> > codec.
> >
> > The issue is around the filtering. The current GstCaps for h264 is not
> > consider a subset of avdec_h264's capabilites and that will fiter this
> > element out of the list.
> >
> > The proposed solution for that is to set `subsetonly` parameter from
> > gst_element_factory_list_filter() to false.
> >
> > While at it, the cap "stream-format=byte-stream" is less useful now
> > because it isn't needed to play the stream - see 6fe88871240c53b8
> >
> > In my system, our debug shows:
> > .. gstvideo_debug_available_decoders: From 228 video decoder elements,
> > - 4 can handle caps   image/jpeg: jpegdec, nvdec, avdec_mjpeg, vaapijpegdec
> > - 3 can handle caps  video/x-vp8: vaapidecodebin, vp8dec, avdec_vp8
> > - 4 can handle caps video/x-h264: vaapidecodebin, avdec_h264, nvdec, 
> > vaapih264dec
> > - 3 can handle caps  video/x-vp9: vaapidecodebin, vp9dec, avdec_vp9
> >
> > Signed-off-by: Victor Toso 
>
> I consider this one a blocker to the release as a client that had only
> the avdec_h264 decoder for h264, would be able to decode h264 streams
> but without this patch it can't as we will not find any available
> decoder due wrong filtering which will disable the capability for h264
> decoding.
>
> Any comments?

The related discussion below...

(2017-07-13 at #gstreamer - freenode)

toso  Hi, I'm wondering if I'm getting the documentation wrong about
  the meaning of gst_caps_is_subset()... isn't it "video/x-h264,
  stream-format=(string)byte-stream" a subset of "video/x-h264,
  alignment=(string)au, stream-format=(string){ avc, byte-stream }" ?
__tim toso, no, because it's missing the alignment field
__tim which means alignment could be anything
__tim which means it  might not be au
toso  that's a bit confusing, no?
__tim it's not a strict subset :)
toso  I really thought it would be a subset in this case.. meaning, we
  don't care about the aligment on _is_subset
__tim you can do can_intersect() if you don't care
slomo it's an actual subset if you define the abscence of a field as
  "the field can have every possible value"
toso  __tim: problem is that it isn't a subset on calling
  gst_element_factory_list_filter()
toso  slomo: yeah, that was my assumption on the meaning of subset
toso  so, just to confirm, this is the expected behavior and not a
  bug, right? :)
slomo yes
__tim slomo, foo, bar={1,2} is not a subset of  foo, bar=1  is it?
slomo but if something does not work as expect for you, that might be
  a bug :)
slomo __tim: correct
slomo __tim: but foo, bar=1 is a subset of foo, bar={1,2}
__tim yes
slomo __tim: and foo, bar=1 is a subset of foo
slomo __tim: and foo is not a subset of foo, bar=1
__tim yes. yes.
__tim andn the case we're talking about is basically  "foo" + "foo,
  bar=1"
slomo yes, the first is a superset of the second there
slomo toso: if something does not work for you that might also be a
  bug of course ;)
__tim ok, so not a subset.
__tim then we're on the same page I think :)
toso  slomo: right, my point was that I would like to list the h264
  decoders that are available and I was using `video/x-h264,
  stream-format=byte-stream` as input for
  gst_element_factory_list_filter() .. and avdec_h264 is the one
  not showing because it needs the aligment
toso  but as I got it wrong, I'll rethink on how to fix it
slomo toso: pass FALSE as the last argument
slomo and you can just use 'video/x-h264'
slomo and with gst_element_factory_list_get_elements() you can make
  sure to get only a list of video decoders to begin with
toso  just gst_element_factory_list_get_elements() gives me like 220
  elements
toso  using _type_decoder | _type_media_video | _type_media_image
slomo is that a problem?
toso  trying to reduce that to actual decoding elements
slomo they should be all actual video decoders
slomo and with gst_element_factory_list_filter(list, 'video/x-h264',
  GST_PAD_SINK, FALSE) you can then filter out all non-h264 ones
  dv_   is it generally ok if an audio decoder messes with the
  PTS/durations?
toso  right, let me try that then
toso  slomo, __tim many thanks :)

>
> > ---
> >  src/channel-display-gst.c  | 2 +-
> >  src/channel-display-priv.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 20d236a..1bd7df1 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -647,7 +647,7 @@ gboolean gstvideo_has_codec(int codec_type)
> >  }
> >  
> >