Re: [Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Christophe de Dinechin 

> On 4 Aug 2017, at 14:49, Victor Toso  wrote:
> 
> On Fri, Aug 04, 2017 at 02:34:44PM +0200, Christophe de Dinechin wrote:
>> 
>>> On 4 Aug 2017, at 14:03, Frediano Ziglio  wrote:
>>> 
 
 Hi,
 
 On Fri, Aug 04, 2017 at 01:01:05PM +0200, Christophe de Dinechin wrote:
>> It does not check if given patch was pushed, no. :(
> 
> If so, that’s a lot less useful than PRs.
 
 All comes down to workflow in the end... Some people love it :)
 https://patchwork.linuxtv.org/
 
>>> 
>>> The missing synchronization with git was discussed already on
>>> our ML thread, both patchew and patchwork share this missing
>>> feature.
>> 
>> I love the phrasing “share this missing feature” ;-)
>> 
>>> 
>>> Not saying that patchwork have few interesting feature, indeed.
>>> Just don't have this. Mostly patchew add CI feature (as far as
>>> I saw). Just today I was wondering why this CI feature was
>>> not implemented on top of patchwork instead of having another
>>> tool.
>> 
>> I wonder if patchwork is not supposed to have at least some testing
>> capabilities. There is a “Test” menu on 
>> https://patchwork.freedesktop.org/project/Spice/series.
> 
> For..  Tested-by: Victor Toso 
> :)

Oh, I see. Manual CI it is then ;-) 

How does it know about the “pass/fail/info/warning”?

> 
>> Anyway, let me answer the question about the “Done” state.
>> Apparently, you can only change the state for a patch *you* submitted.
>> So the “state” button for the patch I referred to earlier does not show up,
>> presumably because only Pavel can change it.
> 
> I think anyone from Spice group in freedesktop can, manually, change it.

Ah, I may have made the mistake of creating another account on patchwork.
So I guess I’m supposed to reuse my freedesktop account?

> 
>> If I look at a patch I sent, then I have a button and also a “delegate to” 
>> pop up.
>> One of the choices is pw-git-hook. What does that mean?
>> 
>> Thanks
>> Christophe
> 
> pw is a command line tool, you could apply a series with something like
> git pw apply-series 3837 (series-id)
> 
> Not sure about pw-git-hook.

If you don’t know, who knows?…


Thanks
Christophe

> 

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

Re: [Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Victor Toso 
On Fri, Aug 04, 2017 at 02:34:44PM +0200, Christophe de Dinechin wrote:
> 
> > On 4 Aug 2017, at 14:03, Frediano Ziglio  wrote:
> > 
> >> 
> >> Hi,
> >> 
> >> On Fri, Aug 04, 2017 at 01:01:05PM +0200, Christophe de Dinechin wrote:
>  It does not check if given patch was pushed, no. :(
> >>> 
> >>> If so, that’s a lot less useful than PRs.
> >> 
> >> All comes down to workflow in the end... Some people love it :)
> >> https://patchwork.linuxtv.org/
> >> 
> > 
> > The missing synchronization with git was discussed already on
> > our ML thread, both patchew and patchwork share this missing
> > feature.
> 
> I love the phrasing “share this missing feature” ;-)
> 
> > 
> > Not saying that patchwork have few interesting feature, indeed.
> > Just don't have this. Mostly patchew add CI feature (as far as
> > I saw). Just today I was wondering why this CI feature was
> > not implemented on top of patchwork instead of having another
> > tool.
> 
> I wonder if patchwork is not supposed to have at least some testing
> capabilities. There is a “Test” menu on 
> https://patchwork.freedesktop.org/project/Spice/series.

For..  Tested-by: Victor Toso 
:)

> Anyway, let me answer the question about the “Done” state.
> Apparently, you can only change the state for a patch *you* submitted.
> So the “state” button for the patch I referred to earlier does not show up,
> presumably because only Pavel can change it.

I think anyone from Spice group in freedesktop can, manually, change it.

> If I look at a patch I sent, then I have a button and also a “delegate to” 
> pop up.
> One of the choices is pw-git-hook. What does that mean?
>
> Thanks
> Christophe

pw is a command line tool, you could apply a series with something like
git pw apply-series 3837 (series-id)

Not sure about pw-git-hook.



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] [PATCH spice-gtk v2] spice-gtk: Use time comparisons that still work after wraparound

2017-08-04 Thread Christophe de Dinechin 

> On 4 Aug 2017, at 13:44, Christophe Fergeau  wrote:
> 
> On Fri, Aug 04, 2017 at 12:11:44PM +0200, Christophe de Dinechin wrote:
>> 
>>> On 18 Jul 2017, at 16:48, Christophe Fergeau  wrote:
>>> 
>>> Hey,
>>> 
>>> On Tue, Jul 18, 2017 at 04:37:29PM +0200, Christophe de Dinechin wrote:
 OK. Since you seem to feel more strongly about this than me, I changed it.
 Pushed to freedesktop.org. Since this is the first time I push myself to 
 this repo,
 would you please be kind enough to check that what I pushed looks sane?
>>> 
>>> This looks all good, only minor comment I have is with the 'spice-gtk'
>>> prefix, you prepended it to the commit log, so it appears in git log, we
>>> usually use nothing, or a more specific prefix.
>>> The messages to the mailing list are prefixed with "spice-gtk", but
>>> through git send-email --subject-prefix spice-gtk.
>> 
>> Does that count as an “ack”? Or do you want me to resend the series
>> with just the subject line change?
> 
> You've already pushed the patch as  0bedca, that comment was meant for
> future commits.

Indeed. As a result, I’m starting to hate patchwork with intense dedication ;-)

Christophe

> 
> Christophe

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

Re: [Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Christophe de Dinechin 

> On 4 Aug 2017, at 14:03, Frediano Ziglio  wrote:
> 
>> 
>> Hi,
>> 
>> On Fri, Aug 04, 2017 at 01:01:05PM +0200, Christophe de Dinechin wrote:
 It does not check if given patch was pushed, no. :(
>>> 
>>> If so, that’s a lot less useful than PRs.
>> 
>> All comes down to workflow in the end... Some people love it :)
>> https://patchwork.linuxtv.org/
>> 
> 
> The missing synchronization with git was discussed already on
> our ML thread, both patchew and patchwork share this missing
> feature.

I love the phrasing “share this missing feature” ;-)

> 
> Not saying that patchwork have few interesting feature, indeed.
> Just don't have this. Mostly patchew add CI feature (as far as
> I saw). Just today I was wondering why this CI feature was
> not implemented on top of patchwork instead of having another
> tool.

I wonder if patchwork is not supposed to have at least some testing
capabilities. There is a “Test” menu on 
https://patchwork.freedesktop.org/project/Spice/series.

Anyway, let me answer the question about the “Done” state.
Apparently, you can only change the state for a patch *you* submitted.
So the “state” button for the patch I referred to earlier does not show up,
presumably because only Pavel can change it.

If I look at a patch I sent, then I have a button and also a “delegate to” pop 
up.
One of the choices is pw-git-hook. What does that mean?


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

Re: [Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Frediano Ziglio 
> 
> Hi,
> 
> On Fri, Aug 04, 2017 at 01:01:05PM +0200, Christophe de Dinechin wrote:
> > > It does not check if given patch was pushed, no. :(
> >
> > If so, that’s a lot less useful than PRs.
> 
> All comes down to workflow in the end... Some people love it :)
> https://patchwork.linuxtv.org/
> 

The missing synchronization with git was discussed already on
our ML thread, both patchew and patchwork share this missing
feature.

Not saying that patchwork have few interesting feature, indeed.
Just don't have this. Mostly patchew add CI feature (as far as
I saw). Just today I was wondering why this CI feature was
not implemented on top of patchwork instead of having another
tool.

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

Re: [Spice-devel] [PATCH spice-gtk v2] spice-gtk: Use time comparisons that still work after wraparound

2017-08-04 Thread Christophe Fergeau 
On Fri, Aug 04, 2017 at 12:11:44PM +0200, Christophe de Dinechin wrote:
> 
> > On 18 Jul 2017, at 16:48, Christophe Fergeau  wrote:
> > 
> > Hey,
> > 
> > On Tue, Jul 18, 2017 at 04:37:29PM +0200, Christophe de Dinechin wrote:
> >> OK. Since you seem to feel more strongly about this than me, I changed it.
> >> Pushed to freedesktop.org. Since this is the first time I push myself to 
> >> this repo,
> >> would you please be kind enough to check that what I pushed looks sane?
> > 
> > This looks all good, only minor comment I have is with the 'spice-gtk'
> > prefix, you prepended it to the commit log, so it appears in git log, we
> > usually use nothing, or a more specific prefix.
> > The messages to the mailing list are prefixed with "spice-gtk", but
> > through git send-email --subject-prefix spice-gtk.
> 
> Does that count as an “ack”? Or do you want me to resend the series
> with just the subject line change?

You've already pushed the patch as  0bedca, that comment was meant for
future commits.

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] Getting patchwork to acknowledge acks?

2017-08-04 Thread Victor Toso 
Hi,

On Fri, Aug 04, 2017 at 01:01:05PM +0200, Christophe de Dinechin wrote:
> > It does not check if given patch was pushed, no. :(
>
> If so, that’s a lot less useful than PRs.

All comes down to workflow in the end... Some people love it :)
https://patchwork.linuxtv.org/



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] Getting patchwork to acknowledge acks?

2017-08-04 Thread Christophe de Dinechin 

> On 4 Aug 2017, at 12:48, Victor Toso  wrote:
> 
> Hi,
> 
> On Fri, Aug 04, 2017 at 12:42:39PM +0200, Christophe de Dinechin wrote:
>> During the discussion on PRs and stuff, several people pointed to
>> patchwork.
>> 
>> Does anyone know why this tool does not acknowledge acks? For example
>> https://patchwork.freedesktop.org/series/27298/ has a "Acked-by:
>> Christophe de Dinechin ” in the fifth comment,
>> and has been merged as 4cdd6e07d3f7ec07dccfa11c12099cb45ac60d3d. Yet
>> it’s still marked as “New” by patchwork.
> 
> It parses the acked-by and does +1 per-patch
> https://patchwork.freedesktop.org/project/Spice/patches/

It does not seem too smart about it though. I wrote “can’t ack right now”, and 
it apparently counted that as a second “ack” :-D

> 
> That`s the only automated feature that it does, afaik.
> 
>> There are a few series marked as “Done”:
>> https://patchwork.freedesktop.org/project/Spice/series/?ordering=-last_updated.
>> For example, this one https://patchwork.freedesktop.org/series/25980/
>> was marked as “Done”. It’s recent (June 19). Christophe, did you
>> explicitly mark it as “Done”, or is some fuzzy parser in patchwork
>> smart enough to parse your "I've now pushed this upstream” comment?
>> 
>> Christophe
> 
> It does not check if given patch was pushed, no. :(

If so, that’s a lot less useful than PRs.


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

Re: [Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Victor Toso 
Hi,

On Fri, Aug 04, 2017 at 12:42:39PM +0200, Christophe de Dinechin wrote:
> During the discussion on PRs and stuff, several people pointed to
> patchwork.
>
> Does anyone know why this tool does not acknowledge acks? For example
> https://patchwork.freedesktop.org/series/27298/ has a "Acked-by:
> Christophe de Dinechin ” in the fifth comment,
> and has been merged as 4cdd6e07d3f7ec07dccfa11c12099cb45ac60d3d. Yet
> it’s still marked as “New” by patchwork.

It parses the acked-by and does +1 per-patch
https://patchwork.freedesktop.org/project/Spice/patches/

That`s the only automated feature that it does, afaik.

> There are a few series marked as “Done”:
> https://patchwork.freedesktop.org/project/Spice/series/?ordering=-last_updated.
> For example, this one https://patchwork.freedesktop.org/series/25980/
> was marked as “Done”. It’s recent (June 19). Christophe, did you
> explicitly mark it as “Done”, or is some fuzzy parser in patchwork
> smart enough to parse your "I've now pushed this upstream” comment?
>
> Christophe

It does not check if given patch was pushed, no. :(


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

[Spice-devel] Getting patchwork to acknowledge acks?

2017-08-04 Thread Christophe de Dinechin 
During the discussion on PRs and stuff, several people pointed to patchwork.

Does anyone know why this tool does not acknowledge acks? For example 
https://patchwork.freedesktop.org/series/27298/ has a "Acked-by: Christophe de 
Dinechin ” in the fifth comment, and has been merged as 
4cdd6e07d3f7ec07dccfa11c12099cb45ac60d3d. Yet it’s still marked as “New” by 
patchwork.

There are a few series marked as “Done”: 
https://patchwork.freedesktop.org/project/Spice/series/?ordering=-last_updated. 
For example, this one https://patchwork.freedesktop.org/series/25980/ was 
marked as “Done”. It’s recent (June 19). Christophe, did you explicitly mark it 
as “Done”, or is some fuzzy parser in patchwork smart enough to parse your 
"I've now pushed this upstream” comment?


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

Re: [Spice-devel] [PATCH spice-gtk v2] spice-gtk: Use time comparisons that still work after wraparound

2017-08-04 Thread Christophe de Dinechin 

> On 18 Jul 2017, at 16:48, Christophe Fergeau  wrote:
> 
> Hey,
> 
> On Tue, Jul 18, 2017 at 04:37:29PM +0200, Christophe de Dinechin wrote:
>> OK. Since you seem to feel more strongly about this than me, I changed it.
>> Pushed to freedesktop.org. Since this is the first time I push myself to 
>> this repo,
>> would you please be kind enough to check that what I pushed looks sane?
> 
> This looks all good, only minor comment I have is with the 'spice-gtk'
> prefix, you prepended it to the commit log, so it appears in git log, we
> usually use nothing, or a more specific prefix.
> The messages to the mailing list are prefixed with "spice-gtk", but
> through git send-email --subject-prefix spice-gtk.

Does that count as an “ack”? Or do you want me to resend the series with just 
the subject line change?

> 
> Christophe

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

[Spice-devel] [PATCH spice-common 3/5] quic: remove Channel::encoder

2017-08-04 Thread Frediano Ziglio 
This field is easily accessible from Encoder structure.

Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 42670ad..a4c46d3 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -133,8 +133,6 @@ typedef struct FamilyStat {
 } FamilyStat;
 
 typedef struct Channel {
-Encoder *encoder;
-
 int correlate_row_width;
 BYTE *correlate_row;
 
@@ -994,7 +992,6 @@ static int init_channel(Encoder *encoder, Channel *channel)
 unsigned int n_buckets;
 unsigned int n_buckets_ptrs;
 
-channel->encoder = encoder;
 channel->correlate_row_width = 0;
 channel->correlate_row = NULL;
 
@@ -1020,9 +1017,9 @@ static int init_channel(Encoder *encoder, Channel 
*channel)
 return TRUE;
 }
 
-static void destroy_channel(Channel *channel)
+static void destroy_channel(Encoder *encoder, Channel *channel)
 {
-QuicUsrContext *usr = channel->encoder->usr;
+QuicUsrContext *usr = encoder->usr;
 if (channel->correlate_row) {
 usr->free(usr, channel->correlate_row - 1);
 }
@@ -1039,7 +1036,7 @@ static int init_encoder(Encoder *encoder, QuicUsrContext 
*usr)
 for (i = 0; i < MAX_CHANNELS; i++) {
 if (!init_channel(encoder, >channels[i])) {
 for (--i; i >= 0; i--) {
-destroy_channel(>channels[i]);
+destroy_channel(encoder, >channels[i]);
 }
 return FALSE;
 }
@@ -1638,7 +1635,7 @@ void quic_destroy(QuicContext *quic)
 }
 
 for (i = 0; i < MAX_CHANNELS; i++) {
-destroy_channel(>channels[i]);
+destroy_channel(encoder, >channels[i]);
 }
 encoder->usr->free(encoder->usr, encoder);
 }
-- 
2.13.3

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

[Spice-devel] [PATCH spice-common 5/5] quic: turn back some commented out checks as compile time

2017-08-04 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 7c069e4..1be28c6 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -223,8 +223,8 @@ static const unsigned int tabrand_chaos[TABRAND_TABSIZE] = {
 
 static unsigned int stabrand(void)
 {
-//spice_assert( !(TABRAND_SEEDMASK & TABRAND_TABSIZE));
-//spice_assert( TABRAND_SEEDMASK + 1 == TABRAND_TABSIZE );
+SPICE_VERIFY( !(TABRAND_SEEDMASK & TABRAND_TABSIZE));
+SPICE_VERIFY( TABRAND_SEEDMASK + 1 == TABRAND_TABSIZE );
 
 return TABRAND_SEEDMASK;
 }
-- 
2.13.3

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

[Spice-devel] [PATCH spice-common 1/5] quic: remove only assigned num_channels field

2017-08-04 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index b753d07..511e733 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -153,7 +153,6 @@ struct Encoder {
 QuicImageType type;
 unsigned int width;
 unsigned int height;
-unsigned int num_channels;
 
 unsigned int n_buckets_8bpc;
 unsigned int n_buckets_5bpc;
@@ -1079,8 +1078,6 @@ static int encoder_reset_channels(Encoder *encoder, int 
channels, int width, int
 {
 int i;
 
-encoder->num_channels = channels;
-
 for (i = 0; i < channels; i++) {
 s_bucket *bucket;
 s_bucket *end_bucket;
-- 
2.13.3

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

[Spice-devel] [PATCH spice-common 2/5] quic: remove only assigned CommonState::encoder

2017-08-04 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 511e733..42670ad 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -103,8 +103,6 @@ typedef struct s_bucket {
 typedef struct Encoder Encoder;
 
 typedef struct CommonState {
-Encoder *encoder;
-
 unsigned int waitcnt;
 unsigned int tabrand_seed;
 unsigned int wm_trigger;
@@ -997,7 +995,6 @@ static int init_channel(Encoder *encoder, Channel *channel)
 unsigned int n_buckets_ptrs;
 
 channel->encoder = encoder;
-channel->state.encoder = encoder;
 channel->correlate_row_width = 0;
 channel->correlate_row = NULL;
 
@@ -1038,7 +1035,6 @@ static int init_encoder(Encoder *encoder, QuicUsrContext 
*usr)
 int i;
 
 encoder->usr = usr;
-encoder->rgb_state.encoder = encoder;
 
 for (i = 0; i < MAX_CHANNELS; i++) {
 if (!init_channel(encoder, >channels[i])) {
-- 
2.13.3

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

[Spice-devel] [PATCH spice-common 4/5] quic: use 32 bit for bppmask

2017-08-04 Thread Frediano Ziglio 
In most occurrences bppmask is converted to 32 bit anyway.
In the left one a possible more bigger precision is not needed.

Signed-off-by: Frediano Ziglio 
---
 common/quic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index a4c46d3..7c069e4 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -168,7 +168,7 @@ struct Encoder {
 };
 
 /* bppmask[i] contains i ones as lsb-s */
-static const unsigned long int bppmask[33] = {
+static const unsigned int bppmask[33] = {
 0x, /* [0] */
 0x0001, 0x0003, 0x0007, 0x000f,
 0x001f, 0x003f, 0x007f, 0x00ff,
@@ -329,7 +329,7 @@ static void decorrelate_init(QuicFamily *family, int bpc)
 
 static void correlate_init(QuicFamily *family, int bpc)
 {
-const unsigned long int pixelbitmask = bppmask[bpc];
+const unsigned int pixelbitmask = bppmask[bpc];
 unsigned long int s;
 
 //spice_assert(bpc <= 8);
-- 
2.13.3

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

Re: [Spice-devel] [spice-common v3 06/12] quic: Factor common code

2017-08-04 Thread Frediano Ziglio 
> 
> We don't need 2 different implementations when the only difference is
> the CommonState which is being used.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c | 194
>  ++
>  1 file changed, 73 insertions(+), 121 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index 3778bdc..1251bbd 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -519,145 +519,97 @@ static void encoder_init_rle(CommonState *state)
>  }
>  
>  
> +static void encode_state_run(Encoder *encoder, CommonState *state, unsigned
> int runlen)
> +{
> +int hits = 0;
> +
> +while (runlen >= state->melcorder) {
> +hits++;
> +runlen -= state->melcorder;
> +if (state->melcstate < MELCSTATES - 1) {
> +state->melclen = J[++state->melcstate];
> +state->melcorder = (1L << state->melclen);
> +}
> +}
> +
> +/* send the required number of "hit" bits (one per occurrence
> +   of a run of length melcorder). This number is never too big:
> +   after 31 such "hit" bits, each "hit" would represent a run of 32K
> +   pixels.
> +*/
> +encode_ones(encoder, hits);
> +
> +encode(encoder, runlen, state->melclen + 1);
> +
> +/* adjust melcoder parameters */
> +if (state->melcstate) {
> +state->melclen = J[--state->melcstate];
> +state->melcorder = (1L << state->melclen);
> +}
> +}
> +
>  static void encode_run(Encoder *encoder, unsigned int runlen) //todo: try
>  use end of line
>  {
> -int hits = 0;
> -
> -while (runlen >= encoder->rgb_state.melcorder) {
> -hits++;
> -runlen -= encoder->rgb_state.melcorder;
> -if (encoder->rgb_state.melcstate < MELCSTATES - 1) {
> -encoder->rgb_state.melclen = J[++encoder->rgb_state.melcstate];
> -encoder->rgb_state.melcorder = (1L <<
> encoder->rgb_state.melclen);
> -}
> -}
> -
> -/* send the required number of "hit" bits (one per occurrence
> -   of a run of length melcorder). This number is never too big:
> -   after 31 such "hit" bits, each "hit" would represent a run of 32K
> -   pixels.
> -*/
> -encode_ones(encoder, hits);
> -
> -encode(encoder, runlen, encoder->rgb_state.melclen + 1);
> -
> -/* adjust melcoder parameters */
> -if (encoder->rgb_state.melcstate) {
> -encoder->rgb_state.melclen = J[--encoder->rgb_state.melcstate];
> -encoder->rgb_state.melcorder = (1L << encoder->rgb_state.melclen);
> -}
> +encode_state_run(encoder, >rgb_state, runlen);
>  }
>  
>  static void encode_channel_run(Encoder *encoder, Channel *channel, unsigned
>  int runlen)
>  {
> -//todo: try use end of line
> -int hits = 0;
> -
> -while (runlen >= channel->state.melcorder) {
> -hits++;
> -runlen -= channel->state.melcorder;
> -if (channel->state.melcstate < MELCSTATES - 1) {
> -channel->state.melclen = J[++channel->state.melcstate];
> -channel->state.melcorder = (1L << channel->state.melclen);
> -}
> -}
> -
> -/* send the required number of "hit" bits (one per occurrence
> -   of a run of length melcorder). This number is never too big:
> -   after 31 such "hit" bits, each "hit" would represent a run of 32K
> -   pixels.
> -*/
> -encode_ones(encoder, hits);
> -
> -encode(encoder, runlen, channel->state.melclen + 1);
> -
> -/* adjust melcoder parameters */
> -if (channel->state.melcstate) {
> -channel->state.melclen = J[--channel->state.melcstate];
> -channel->state.melcorder = (1L << channel->state.melclen);
> -}
> +encode_state_run(encoder, >state, runlen);
>  }
>  
> +
>  /* decoding routine: reads bits from the input and returns a run length. */
>  /* argument is the number of pixels left to end-of-line (bound on run
>  length) */
>  
> +static int decode_state_run(Encoder *encoder, CommonState *state)
> +{
> +int runlen = 0;
> +
> +do {
> +register int temp, hits;
> +temp = lzeroes[(BYTE)(~(encoder->io_word >> 24))];/* number of
> leading ones in the
> +  input
> stream, up to 8 */
> +for (hits = 1; hits <= temp; hits++) {
> +runlen += state->melcorder;
> +
> +if (state->melcstate < MELCSTATES - 1) {
> +state->melclen = J[++state->melcstate];
> +state->melcorder = (1U << state->melclen);
> +}
> +}
> +if (temp != 8) {
> +decode_eatbits(encoder, temp + 1);  /* consume the leading
> +0 of the
> remainder encoding */
> +break;
> +}
> +decode_eatbits(encoder, 8);
> +} while (1);
> +
> +/* read the length of the remainder */
> +if (state->melclen) {
> +runlen +=

Re: [Spice-devel] [spice-common v3 05/12] quic: Get rid of RLE #define

2017-08-04 Thread Frediano Ziglio 
> 
> It's always set, no need to have conditional compilation based on it.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c  |  9 -
>  common/quic_rgb_tmpl.c | 12 
>  common/quic_tmpl.c | 12 
>  3 files changed, 33 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index 0403db9..3778bdc 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -29,8 +29,6 @@
>  #include "spice_common.h"
>  #include "bitops.h"
>  
> -#define RLE
> -
>  /* ASCII "QUIC" */
>  #define QUIC_MAGIC 0x43495551
>  #define QUIC_VERSION_MAJOR 0U
> @@ -493,8 +491,6 @@ static inline void decode_eat32bits(Encoder *encoder)
>  decode_eatbits(encoder, 16);
>  }
>  
> -#ifdef RLE
> -
>  static inline void encode_ones(Encoder *encoder, unsigned int n)
>  {
>  unsigned int count;
> @@ -663,7 +659,6 @@ static int decode_channel_run(Encoder *encoder, Channel
> *channel)
>  
>  return runlen;
>  }
> -#endif
>  
>  static inline void init_decode_io(Encoder *encoder)
>  {
> @@ -993,9 +988,7 @@ static int encoder_reset(Encoder *encoder, uint32_t
> *io_ptr, uint32_t *io_ptr_en
>  encoder->rgb_state.wmileft = DEFwminext;
>  set_wm_trigger(>rgb_state);
>  
> -#if defined(RLE)
>  encoder_init_rle(>rgb_state);
> -#endif
>  
>  encoder->io_words_count = io_ptr_end - io_ptr;
>  encoder->io_now = io_ptr;
> @@ -1057,9 +1050,7 @@ static int encoder_reset_channels(Encoder *encoder, int
> channels, int width, int
>  encoder->channels[i].state.wmileft = DEFwminext;
>  set_wm_trigger(>channels[i].state);
>  
> -#if defined(RLE)
>  encoder_init_rle(>channels[i].state);
> -#endif
>  }
>  return TRUE;
>  }
> diff --git a/common/quic_rgb_tmpl.c b/common/quic_rgb_tmpl.c
> index 3cacb33..96d3a94 100644
> --- a/common/quic_rgb_tmpl.c
> +++ b/common/quic_rgb_tmpl.c
> @@ -154,16 +154,12 @@
>  correlate_row_b[index]);
>  
>  
> -#ifdef RLE
>  #define RLE_PRED_IMP
>  \
>  if (SAME_PIXEL(_row[i - 1], _row[i])) {
>  \
>  if (run_index != i && i > 2 && SAME_PIXEL(_row[i - 1], _row[i -
>  2])) {  \
>  goto do_run;
>  \
>  }
>  \
>  }
> -#else
> -#define RLE_PRED_IMP
> -#endif
>  
>  #ifdef COMPRESS_IMP
>  
> @@ -290,10 +286,8 @@ static void FNAME(compress_row_seg)(Encoder *encoder,
> int i,
>  BYTE * const correlate_row_g = channel_g->correlate_row;
>  BYTE * const correlate_row_b = channel_b->correlate_row;
>  int stopidx;
> -#ifdef RLE
>  int run_index = 0;
>  int run_size;
> -#endif
>  
>  spice_assert(end - i > 0);
>  
> @@ -339,7 +333,6 @@ static void FNAME(compress_row_seg)(Encoder *encoder, int
> i,
>  
>  return;
>  
> -#ifdef RLE
>  do_run:
>  run_index = i;
>  encoder->rgb_state.waitcnt = stopidx - i;
> @@ -354,7 +347,6 @@ do_run:
>  }
>  encode_run(encoder, run_size);
>  stopidx = i + encoder->rgb_state.waitcnt;
> -#endif
>  }
>  }
>  
> @@ -546,10 +538,8 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder,
>  BYTE * const correlate_row_b = channel_b->correlate_row;
>  const unsigned int waitmask = bppmask[encoder->rgb_state.wmidx];
>  int stopidx;
> -#ifdef RLE
>  int run_index = 0;
>  int run_end;
> -#endif
>  
>  spice_assert(end - i > 0);
>  
> @@ -600,7 +590,6 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder,
>  
>  return;
>  
> -#ifdef RLE
>  do_run:
>  encoder->rgb_state.waitcnt = stopidx - i;
>  run_index = i;
> @@ -618,7 +607,6 @@ do_run:
>  }
>  
>  stopidx = i + encoder->rgb_state.waitcnt;
> -#endif
>  }
>  }
>  
> diff --git a/common/quic_tmpl.c b/common/quic_tmpl.c
> index 315973d..f86addf 100644
> --- a/common/quic_tmpl.c
> +++ b/common/quic_tmpl.c
> @@ -44,16 +44,12 @@
>  #define _PIXEL_B ((unsigned int)prev[0].a)
>  #define _PIXEL_C ((unsigned int)prev[-1].a)
>  
> -#ifdef RLE
>  #define RLE_PRED_IMP   \
>  if (prev_row[i - 1].a == prev_row[i].a) {  \
>  if (run_index != i && i > 2 && cur_row[i - 1].a == cur_row[i - 2].a) { \
>  goto do_run;   \
>  }  \
>  }
> -#else
> -#define RLE_PRED_IMP
> -#endif
>  
>  /*  a  */
>  static inline BYTE FNAME(decorrelate_0)(const PIXEL * const curr, const
>  unsigned int bpc_mask)
> @@ -183,10 +179,8 @@ static void FNAME(compress_row_seg)(Encoder *encoder,
> Channel *channel, int i,
>  {
>  BYTE * const decorrelate_drow = channel->correlate_row;
>  int stopidx;
> -#ifdef RLE
>  int run_index = 0;
>  int run_size;
> -#endif
>  
>  spice_assert(end - i > 0);
>  
> @@ -243,7 +237,6 @@ static void FNAME(compress_row_seg)(Encoder *encoder,
> Channel *channel, int i,
>  
>  return;
>  
> -#ifdef RLE
>  do_run:
>

Re: [Spice-devel] [spice-common v3 04/12] quic: Get rid of RLE_STAT #define

2017-08-04 Thread Frediano Ziglio 
> 
> It's always set, no need to have conditional compilation based on it.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c  | 53
>  ++---
>  common/quic_tmpl.c | 12 
>  2 files changed, 2 insertions(+), 63 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index db9f2f7..0403db9 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -30,7 +30,6 @@
>  #include "bitops.h"
>  
>  #define RLE
> -#define RLE_STAT
>  
>  /* ASCII "QUIC" */
>  #define QUIC_MAGIC 0x43495551
> @@ -106,7 +105,6 @@ typedef struct CommonState {
>  unsigned int wmidx;
>  unsigned int wmileft;
>  
> -#ifdef RLE_STAT
>  int melcstate; /* index to the state array */
>  
>  int melclen;  /* contents of the state array location
> @@ -117,7 +115,6 @@ typedef struct CommonState {
>   of melclen bits */
>  
>  unsigned long melcorder;  /* 2^ melclen */
> -#endif
>  } CommonState;
>  
>  
> @@ -498,8 +495,6 @@ static inline void decode_eat32bits(Encoder *encoder)
>  
>  #ifdef RLE
>  
> -#ifdef RLE_STAT
> -
>  static inline void encode_ones(Encoder *encoder, unsigned int n)
>  {
>  unsigned int count;
> @@ -668,50 +663,6 @@ static int decode_channel_run(Encoder *encoder, Channel
> *channel)
>  
>  return runlen;
>  }
> -
> -#else
> -
> -static inline void encode_run(Encoder *encoder, unsigned int len)
> -{
> -int odd = len & 1U;
> -int msb;
> -
> -len &= ~1U;
> -
> -while ((msb = spice_bit_find_msb(len))) {
> -len &= ~(1 << (msb - 1));
> -spice_assert(msb < 32);
> -encode(encoder, (1 << (msb)) - 1, msb);
> -encode(encoder, 0, 1);
> -}
> -
> -if (odd) {
> -encode(encoder, 2, 2);
> -} else {
> -encode(encoder, 0, 1);
> -}
> -}
> -
> -static inline unsigned int decode_run(Encoder *encoder)
> -{
> -unsigned int len = 0;
> -int count;
> -
> -do {
> -count = 0;
> -while (encoder->io_word & (1U << 31)) {
> -decode_eatbits(encoder, 1);
> -count++;
> -spice_assert(count < 32);
> -}
> -decode_eatbits(encoder, 1);
> -len += (1U << count) >> 1;
> -} while (count > 1);
> -
> -return len;
> -}
> -
> -#endif
>  #endif
>  
>  static inline void init_decode_io(Encoder *encoder)
> @@ -1042,7 +993,7 @@ static int encoder_reset(Encoder *encoder, uint32_t
> *io_ptr, uint32_t *io_ptr_en
>  encoder->rgb_state.wmileft = DEFwminext;
>  set_wm_trigger(>rgb_state);
>  
> -#if defined(RLE) && defined(RLE_STAT)
> +#if defined(RLE)
>  encoder_init_rle(>rgb_state);
>  #endif
>  
> @@ -1106,7 +1057,7 @@ static int encoder_reset_channels(Encoder *encoder, int
> channels, int width, int
>  encoder->channels[i].state.wmileft = DEFwminext;
>  set_wm_trigger(>channels[i].state);
>  
> -#if defined(RLE) && defined(RLE_STAT)
> +#if defined(RLE)
>  encoder_init_rle(>channels[i].state);
>  #endif
>  }
> diff --git a/common/quic_tmpl.c b/common/quic_tmpl.c
> index f620cc9..315973d 100644
> --- a/common/quic_tmpl.c
> +++ b/common/quic_tmpl.c
> @@ -252,19 +252,11 @@ do_run:
>  while (cur_row[i].a == cur_row[i - 1].a) {
>  run_size++;
>  if (++i == end) {
> -#ifdef RLE_STAT
>  encode_channel_run(encoder, channel, run_size);
> -#else
> -encode_run(encoder, run_size);
> -#endif
>  return;
>  }
>  }
> -#ifdef RLE_STAT
>  encode_channel_run(encoder, channel, run_size);
> -#else
> -encode_run(encoder, run_size);
> -#endif
>  stopidx = i + channel->state.waitcnt;
>  #endif
>  }
> @@ -481,11 +473,7 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder,
> Channel *channel,
>  do_run:
>  channel->state.waitcnt = stopidx - i;
>  run_index = i;
> -#ifdef RLE_STAT
>  run_end = i + decode_channel_run(encoder, channel);
> -#else
> -run_end = i + decode_run(encoder);
> -#endif
>  
>  for (; i < run_end; i++) {
>  cur_row[i].a = cur_row[i - 1].a;

I would also extend the rationale. The name of this macro is quite bad but
it changes the image encoding so for compatibility we cannot change it now
that easily.

Acked-by: Frediano Ziglio 

Code compile to the same binary.

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

Re: [Spice-devel] [spice-common v3 03/12] quic: Get rid of QUIC_RGB #define

2017-08-04 Thread Frediano Ziglio 
> 
> It's always set, no need to have conditional compilation based on it.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c  | 151
>  -
>  common/quic_tmpl.c |   6 ---
>  2 files changed, 157 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index c8c9547..db9f2f7 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -31,7 +31,6 @@
>  
>  #define RLE
>  #define RLE_STAT
> -#define QUIC_RGB
>  
>  /* ASCII "QUIC" */
>  #define QUIC_MAGIC 0x43495551
> @@ -306,10 +305,8 @@ static unsigned int cnt_l_zeroes(const unsigned int
> bits)
>  #define QUIC_FAMILY_8BPC
>  #include "quic_family_tmpl.c"
>  
> -#ifdef QUIC_RGB
>  #define QUIC_FAMILY_5BPC
>  #include "quic_family_tmpl.c"
> -#endif
>  
>  static void decorrelate_init(QuicFamily *family, int bpc)
>  {
> @@ -530,7 +527,6 @@ static void encoder_init_rle(CommonState *state)
>  state->melcorder = 1 << state->melclen;
>  }
>  
> -#ifdef QUIC_RGB
>  
>  static void encode_run(Encoder *encoder, unsigned int runlen) //todo: try
>  use end of line
>  {
> @@ -561,8 +557,6 @@ static void encode_run(Encoder *encoder, unsigned int
> runlen) //todo: try use en
>  }
>  }
>  
> -#endif
> -
>  static void encode_channel_run(Encoder *encoder, Channel *channel, unsigned
>  int runlen)
>  {
>  //todo: try use end of line
> @@ -596,7 +590,6 @@ static void encode_channel_run(Encoder *encoder, Channel
> *channel, unsigned int
>  /* decoding routine: reads bits from the input and returns a run length. */
>  /* argument is the number of pixels left to end-of-line (bound on run
>  length) */
>  
> -#ifdef QUIC_RGB
>  static int decode_run(Encoder *encoder)
>  {
>  int runlen = 0;
> @@ -636,7 +629,6 @@ static int decode_run(Encoder *encoder)
>  return runlen;
>  }
>  
> -#endif
>  
>  static int decode_channel_run(Encoder *encoder, Channel *channel)
>  {
> @@ -770,8 +762,6 @@ typedef uint16_t rgb16_pixel_t;
>  #define FOUR_BYTE
>  #include "quic_tmpl.c"
>  
> -#ifdef QUIC_RGB
> -
>  #define QUIC_RGB32
>  #include "quic_rgb_tmpl.c"
>  
> @@ -784,13 +774,6 @@ typedef uint16_t rgb16_pixel_t;
>  #define QUIC_RGB16_TO_32
>  #include "quic_rgb_tmpl.c"
>  
> -#else
> -
> -#define THREE_BYTE
> -#include "quic_tmpl.c"
> -
> -#endif
> -
>  static void fill_model_structures(SPICE_GNUC_UNUSED Encoder *encoder,
>  FamilyStat *family_stat,
>unsigned int rep_first, unsigned int
>first_size,
>unsigned int rep_next, unsigned int
>mul_size,
> @@ -1141,9 +1124,6 @@ static void quic_image_params(Encoder *encoder,
> QuicImageType type, int *channel
>  case QUIC_IMAGE_TYPE_RGB16:
>  *channels = 3;
>  *bpc = 5;
> -#ifndef QUIC_RGB
> -encoder->usr->error(encoder->usr, "not implemented\n");
> -#endif
>  break;
>  case QUIC_IMAGE_TYPE_RGB24:
>  *channels = 3;
> @@ -1208,9 +1188,6 @@ int quic_encode(QuicContext *quic, QuicImageType type,
> int width, int height,
>  uint8_t *prev;
>  int channels;
>  int bpc;
> -#ifndef QUIC_RGB
> -int i;
> -#endif
>  
>  lines_end = line + num_lines * stride;
>  if (line == NULL && lines_end != line) {
> @@ -1237,7 +1214,6 @@ int quic_encode(QuicContext *quic, QuicImageType type,
> int width, int height,
>  FILL_LINES();
>  
>  switch (type) {
> -#ifdef QUIC_RGB
>  case QUIC_IMAGE_TYPE_RGB32:
>  spice_assert(ABS(stride) >= width * 4);
>  QUIC_COMPRESS_RGB(32);
> @@ -1277,47 +1253,6 @@ int quic_encode(QuicContext *quic, QuicImageType type,
> int width, int height,
>  encoder->rows_completed++;
>  }
>  break;
> -#else
> -case QUIC_IMAGE_TYPE_RGB24:
> -spice_assert(ABS(stride) >= width * 3);
> -for (i = 0; i < 3; i++) {
> -encoder->channels[i].correlate_row[-1] = 0;
> -quic_three_compress_row0(encoder, >channels[i],
> (three_bytes_t *)(line + i),
> - width);
> -}
> -encoder->rows_completed++;
> -for (row = 1; row < height; row++) {
> -prev = line;
> -NEXT_LINE();
> -for (i = 0; i < 3; i++) {
> -encoder->channels[i].correlate_row[-1] =
> encoder->channels[i].correlate_row[0];
> -quic_three_compress_row(encoder, >channels[i],
> (three_bytes_t *)(prev + i),
> -(three_bytes_t *)(line + i), width);
> -}
> -encoder->rows_completed++;
> -}
> -break;
> -case QUIC_IMAGE_TYPE_RGB32:
> -case QUIC_IMAGE_TYPE_RGBA:
> -spice_assert(ABS(stride) >= width * 4);
> -for (i = 0; i < channels; i++) {
> -encoder->channels[i].correlate_row[-1] = 0;
> -quic_four_compress_row0(encoder, >channels[i],
> (four_bytes_t

Re: [Spice-devel] [spice-common v3 02/12] quic: Remove configurable PRED

2017-08-04 Thread Frediano Ziglio 
> 
> It's hardcoded at compile-time, and I don't think it was changed in
> years...
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c  |  1 -
>  common/quic_rgb_tmpl.c | 31 ---
>  common/quic_tmpl.c | 38 --
>  3 files changed, 70 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index 309ee75..c8c9547 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -31,7 +31,6 @@
>  
>  #define RLE
>  #define RLE_STAT
> -#define PRED_1
>  #define QUIC_RGB
>  
>  /* ASCII "QUIC" */
> diff --git a/common/quic_rgb_tmpl.c b/common/quic_rgb_tmpl.c
> index cc3c045..3cacb33 100644
> --- a/common/quic_rgb_tmpl.c
> +++ b/common/quic_rgb_tmpl.c
> @@ -120,7 +120,6 @@
>  #define CORRELATE_0(channel, curr, correlate, bpc_mask)\
>  ((family.xlatL2U[correlate] + _PIXEL_A(channel, curr)) & bpc_mask)
>  
> -#ifdef PRED_1
>  
>  /*  (a+b)/2  */
>  #define DECORRELATE(channel, prev, curr, bpc_mask, r)
>  \
> @@ -130,36 +129,6 @@
>  #define CORRELATE(channel, prev, curr, correlate, bpc_mask, r)
>  \
>  SET_##channel(r, ((family.xlatL2U[correlate] +
>  \
>(int)((_PIXEL_A(channel, curr) + _PIXEL_B(channel, prev)) >> 1)) &
>bpc_mask))
> -#endif
> -
> -#ifdef PRED_2
> -
> -/*  .75a+.75b-.5c  */
> -#define DECORRELATE(channel, prev, curr, bpc_mask, r) {
> \
> -int p = ((int)(3 * (_PIXEL_A(channel, curr) + _PIXEL_B(channel, prev)))
> -   \
> -(int)(_PIXEL_C(channel, prev) << 1)) >> 2;
> \
> -if (p < 0) {
> \
> -p = 0;
> \
> -} else if ((unsigned)p > bpc_mask) {
> \
> -p = bpc_mask;
> \
> -}
> \
> -r = family.xlatU2L[(unsigned)((int)GET_##channel(curr) - p) & bpc_mask];
> \
> -}
> -
> -#define CORRELATE(channel, prev, curr, correlate, bpc_mask, r) {
> \
> -const int p = ((int)(3 * (_PIXEL_A(channel, curr) + _PIXEL_B(channel,
> prev))) - \
> -(int)(_PIXEL_C(channel, prev) << 1) ) >> 2;
> \
> -const unsigned int s = family.xlatL2U[correlate];
> \
> -if (!(p & ~bpc_mask)) {
> \
> -SET_##channel(r, (s + (unsigned)p) & bpc_mask);
> \
> -} else if (p < 0) {
> \
> -SET_##channel(r, s);
> \
> -} else {
> \
> -SET_##channel(r, (s + bpc_mask) & bpc_mask);
> \
> -}
> \
> -}
> -
> -#endif
>  
>  
>  #define COMPRESS_ONE_ROW0_0(channel)
>  \
> diff --git a/common/quic_tmpl.c b/common/quic_tmpl.c
> index d49a408..6c639c7 100644
> --- a/common/quic_tmpl.c
> +++ b/common/quic_tmpl.c
> @@ -73,7 +73,6 @@ static inline void FNAME(correlate_0)(PIXEL *curr, const
> BYTE correlate,
>  curr->a = (family.xlatL2U[correlate] + _PIXEL_A) & bpc_mask;
>  }
>  
> -#ifdef PRED_1
>  
>  /*  (a+b)/2  */
>  static inline BYTE FNAME(decorrelate)(const PIXEL *const prev, const PIXEL *
>  const curr,
> @@ -89,43 +88,6 @@ static inline void FNAME(correlate)(const PIXEL *prev,
> PIXEL *curr, const BYTE c
>  curr->a = (family.xlatL2U[correlate] + (int)((_PIXEL_A + _PIXEL_B) >>
>  1)) & bpc_mask;
>  }
>  
> -#endif
> -
> -#ifdef PRED_2
> -
> -/*  .75a+.75b-.5c  */
> -static inline BYTE FNAME(decorrelate)(const PIXEL *const prev, const PIXEL *
> const curr,
> -  const unsigned int bpc_mask)
> -{
> -int p = ((int)(3 * (_PIXEL_A + _PIXEL_B)) - (int)(_PIXEL_C << 1)) >> 2;
> -
> -if (p < 0) {
> -p = 0;
> -} else if ((unsigned)p > bpc_mask) {
> -p = bpc_mask;
> -}
> -
> -{
> -return family.xlatU2L[(unsigned)((int)curr->a - p) & bpc_mask];
> -}
> -}
> -
> -static inline void FNAME(correlate)(const PIXEL *prev, PIXEL *curr, const
> BYTE correlate,
> -const unsigned int bpc_mask)
> -{
> -const int p = ((int)(3 * (_PIXEL_A + _PIXEL_B)) - (int)(_PIXEL_C << 1))
> >> 2;
> -const unsigned int s = family.xlatL2U[correlate];
> -
> -if (!(p & ~bpc_mask)) {
> -curr->a = (s + (unsigned)p) & bpc_mask;
> -} else if (p < 0) {
> -curr->a = s;
> -} else {
> -curr->a = (s + bpc_mask) & bpc_mask;
> -}
> -}
> -
> -#endif
>  
>  static void FNAME(compress_row0_seg)(Encoder *encoder, Channel *channel, int
>  i,
>   const PIXEL * const cur_row,

Acked-by: Frediano Ziglio 

Code compile to the same binary.

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

Re: [Spice-devel] [spice-common v3 01/12] quic: Remove configurable RLE_PRED

2017-08-04 Thread Frediano Ziglio 
> 
> It's hardcoded at compile-time, and I don't think it was changed in
> years...
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/quic.c  |  3 ---
>  common/quic_rgb_tmpl.c | 55
>  --
>  common/quic_tmpl.c | 55
>  --
>  3 files changed, 16 insertions(+), 97 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index b753d07..309ee75 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -32,9 +32,6 @@
>  #define RLE
>  #define RLE_STAT
>  #define PRED_1
> -//#define RLE_PRED_1
> -#define RLE_PRED_2
> -//#define RLE_PRED_3
>  #define QUIC_RGB
>  
>  /* ASCII "QUIC" */
> diff --git a/common/quic_rgb_tmpl.c b/common/quic_rgb_tmpl.c
> index 23dea39..cc3c045 100644
> --- a/common/quic_rgb_tmpl.c
> +++ b/common/quic_rgb_tmpl.c
> @@ -185,36 +185,15 @@
>  correlate_row_b[index]);
>  
>  
> -#ifdef RLE_PRED_1
> -#define RLE_PRED_1_IMP
> \
> -if (SAME_PIXEL(_row[i - 1], _row[i])) {
> \
> -if (run_index != i && SAME_PIXEL(_row[i - 1], _row[i]) &&
> \
> -i + 1 < end && SAME_PIXEL(_row[i],
> _row[i + 1])) {\
> -goto do_run;
> \
> -}
> \
> -}
> -#else
> -#define RLE_PRED_1_IMP
> -#endif
> -
> -#ifdef RLE_PRED_2
> -#define RLE_PRED_2_IMP
> \
> +#ifdef RLE
> +#define RLE_PRED_IMP
> \
>  if (SAME_PIXEL(_row[i - 1], _row[i])) {
>  \
>  if (run_index != i && i > 2 && SAME_PIXEL(_row[i - 1], _row[i -
>  2])) {  \
>  goto do_run;
>  \
>  }
>  \
>  }
>  #else
> -#define RLE_PRED_2_IMP
> -#endif
> -
> -#ifdef RLE_PRED_3
> -#define RLE_PRED_3_IMP
> \
> -if (i > 1 &&  SAME_PIXEL(_row[i - 1], _row[i - 2]) && i !=
> run_index) { \
> -goto do_run;
> \
> -}
> -#else
> -#define RLE_PRED_3_IMP
> +#define RLE_PRED_IMP
>  #endif
>  
>  #ifdef COMPRESS_IMP
> @@ -370,11 +349,7 @@ static void FNAME(compress_row_seg)(Encoder *encoder,
> int i,
>  while (stopidx < end) {
>  for (; i <= stopidx; i++) {
>  unsigned int codeword, codewordlen;
> -#ifdef RLE
> -RLE_PRED_1_IMP;
> -RLE_PRED_2_IMP;
> -RLE_PRED_3_IMP;
> -#endif
> +RLE_PRED_IMP;
>  COMPRESS_ONE(r, i);
>  COMPRESS_ONE(g, i);
>  COMPRESS_ONE(b, i);
> @@ -386,11 +361,7 @@ static void FNAME(compress_row_seg)(Encoder *encoder,
> int i,
>  
>  for (; i < end; i++) {
>  unsigned int codeword, codewordlen;
> -#ifdef RLE
> -RLE_PRED_1_IMP;
> -RLE_PRED_2_IMP;
> -RLE_PRED_3_IMP;
> -#endif
> +RLE_PRED_IMP;
>  COMPRESS_ONE(r, i);
>  COMPRESS_ONE(g, i);
>  COMPRESS_ONE(b, i);
> @@ -635,11 +606,7 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder,
>  while (stopidx < end) {
>  for (; i <= stopidx; i++) {
>  unsigned int codewordlen;
> -#ifdef RLE
> -RLE_PRED_1_IMP;
> -RLE_PRED_2_IMP;
> -RLE_PRED_3_IMP;
> -#endif
> +RLE_PRED_IMP;
>  UNCOMPRESS_PIX_START(_row[i]);
>  UNCOMPRESS_ONE(r);
>  UNCOMPRESS_ONE(g);
> @@ -653,11 +620,7 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder,
>  
>  for (; i < end; i++) {
>  unsigned int codewordlen;
> -#ifdef RLE
> -RLE_PRED_1_IMP;
> -RLE_PRED_2_IMP;
> -RLE_PRED_3_IMP;
> -#endif
> +RLE_PRED_IMP;
>  UNCOMPRESS_PIX_START(_row[i]);
>  UNCOMPRESS_ONE(r);
>  UNCOMPRESS_ONE(g);
> @@ -732,9 +695,7 @@ static void FNAME(uncompress_row)(Encoder *encoder,
>  #undef _PIXEL_B
>  #undef _PIXEL_C
>  #undef SAME_PIXEL
> -#undef RLE_PRED_1_IMP
> -#undef RLE_PRED_2_IMP
> -#undef RLE_PRED_3_IMP
> +#undef RLE_PRED_IMP
>  #undef UPDATE_MODEL
>  #undef DECORRELATE_0
>  #undef DECORRELATE
> diff --git a/common/quic_tmpl.c b/common/quic_tmpl.c
> index 3c84791..d49a408 100644
> --- a/common/quic_tmpl.c
> +++ b/common/quic_tmpl.c
> @@ -50,36 +50,15 @@
>  #define _PIXEL_B ((unsigned int)prev[0].a)
>  #define _PIXEL_C ((unsigned int)prev[-1].a)
>  
> -#ifdef RLE_PRED_1
> -#define RLE_PRED_1_IMP
> \
> -if (cur_row[i - 1].a == prev_row[i].a) {
> \
> -if (run_index != i && prev_row[i - 1].a == prev_row[i].a &&
> \
> -i + 1 < end && prev_row[i].a == prev_row[i + 1].a) {
> \
> -goto do_run;
> \
> -}
> \
> -}
> -#else
> -#define RLE_PRED_1_IMP
> -#endif
> -
> -#ifdef RLE_PRED_2
> -#define RLE_PRED_2_IMP \
> +#ifdef RLE
> +#define RLE_PRED_IMP   \
>  if (prev_row[i - 1].a == prev_row[i].a) {  \
>  if (run_index != i && i > 2 && cur_row[i - 1].a == cur_row[i -

Re: [Spice-devel] [PATCH spice-gtk 0/2] Disabling mmtime adjustment in the client (from audio backend)

2017-08-04 Thread Victor Toso 
Hi,

On Thu, Aug 03, 2017 at 10:13:10AM -0400, Marc-André Lureau wrote:
> Hi
>
> - Original Message -
> > > I start to be worried about all of our streaming tweaks and issues. Is
> > > there any effort to use RTP/SRTP instead? I think this would be a big
> > > opportunity to improve the situation going forward.
> >
> > There is not effort on that AFAIK.
> >
>
> I would put my effort there.

Okay, I'll check it out.

> > But I'm interested on improving the mmtime in server side but it'll be
> > hard if I can't disable this long-term workaround.
> >
> > If it isn't a workaround, I would like to understand why.
> >
> >   commit fbe3b5ec32e3d93f0a0f41239b85be723d8d91c5
> >   Author: Marc-André Lureau 
> >   Date:   Wed Dec 22 15:32:45 2010 +0100
> >
> >   gtk: update mm time based on playback time+delay
> >
>
> This is not a workaround, iirc.
>
> The way I remember it, video frames have ts, based on audio time (or
> supposed to I think server implementation is lacking there).
>
> When we receive audio packets, we are supposed to set the current
> mmtime. We asked the audio backend what is the current audio delay,
> and update mmtime = last_packet.time - audio_delay, so we can sync the
> video frames on audio.
>
> Wrong? Yeah probably, video streaming is not an easy subject, that's
> why effort should be put on using the real thing that gstreamer can
> provide us (almost) for free.

Not easy, yeah. But I doubt that changing to different protocol will
solve our synchronization issues without extra effort because the video
could be encoded in different ways in the host or it could be encoded in
the guest and spice-server only needs to wrap it under its protocol.

Too many possibilities which makes fixing mmtime on the client
error-prone.

I'll investigate a bit how to improve mmtime in the server and look into
rtp/srtp and how can we use it with GStreamer.

If I can improve the mmtime generation, we could change the default of
this proposed property (sync-video-on-audio) to FALSE instead.

Thanks again for the discussion,
toso


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