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

2017-08-09 Thread Frediano Ziglio 
> 
> Hi
> 
> - Original Message -
> > Hi,
> > 
> > On Wed, Aug 09, 2017 at 04:40:05AM -0400, Frediano Ziglio wrote:
> > > > 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
> > >
> > > If there's a problem on the server and this patch was just a
> > > workaround let's try to solve the server issue.
> > >
> > > Frediano
> > 
> > Sync video on audio is a feature, IMHO. That's why I proposed this as
> > API in the client.
> 
> Which client would reasonably want to disable sync? Imho that doesn't deserve
> to be exposed in API.
> 
> > Server mmtime generation/update has issues that needs investigation and
> 
> It would be worth to find the root cause of the sync issue you have, instead
> of just disabling sync because it seems to be better.
> 

Agree.

> > I'm puzzled on how to sync video stream where source is the guest with
> > audio that comes from QEMU, at least I don't have a full picture today
> > on how that should be done properly - disabling the `sync video on
> > audio` with this patch, improves the situation but a proper sync is
> > hard.
> 
> agree, now that the video encoding design is changing, it will be more
> difficult to do sync.
> 
> I don't have a good picture of where we are going with video encoding on
> guest side, but the video stream should probably have timestamps matching
> the audio time (I don't know what clock it's using today). I would also
> consider what gnome will have to offer with pipewire, afaik it will be able
> to record or stream the desktop (using gstreamer, and hopefully hw
> accelerated & audio).
> 
>  
> 
> > 
> > Suggestions?
> > 
> 

I'll try to sum up as far as my knowledge goes.

From the protocol prospective there are:
- message to send the current time (init and multi_media_time);
- audio start;
- audio mode change;
- audio/video data have a time.
Now, the time is defined as milliseconds with not defined 0 point
(the init/multi_media_time are used to sync the times between
client and server).
One note on setting the time on data. When they should occur?
For instance audio data have a duration. There's no hard protocol
specification but the time is taken when the sound is captured
that is at the end (see below).

From server prospective. Here got a bit more complicated.
Time on data is set as soon as data are read from guest.
The time is MOSTLY on sync beside the current time messages.
In this case the time is skewed by a mm_time_latency which is
not constant (the minumum is MM_TIME_DELTA which is defines
as 400). This skew is always increased, never decreased and
is basically used to tell client to delay the display.
Note that audio start and audio mode are not affected by this
skew so they should not be used like init (to try to sync
times between client and server).
The skew computation is basically affected by the video
streaming code (see update_client_playback_delay). Affected
by network latency (which is weird to me) and frame size
(make sense as the frame has to travel through network so
it introduces a not fixed delay).

As far as I can see the problem can raise from the skew
management. What are the behaviour (beside a big latency)
that happens on the client?
How should these stuff be handled? Maybe there should be
no skew at all and all should be handled by the client?
Honestly it's quite hard to understand all the tuning made
but client and server together (I personally don't have a
much clear client picture). For instance the skew at the
end end up in the client and is used to count the "dropped"
frames on the client side (which at the end trigger a
possible future skew change on the server side).

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

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

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

2017-08-03 Thread Marc-André Lureau 
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.

> 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.

> Cheers,
> toso
> >
> > 
> > > Cheers,
> > > toso
> > > 
> > > Victor Toso (2):
> > >   session: introduce "video-sync-on-audio-latency" property
> > >   spicy: toggle SpiceSession::video-sync-on-audio-latency
> > > 
> > >  src/channel-playback.c   |  7 ++-
> > >  src/spice-session-priv.h |  1 +
> > >  src/spice-session.c  | 33 +
> > >  tools/spicy.c| 43
> > >  ++-
> > >  4 files changed, 82 insertions(+), 2 deletions(-)
> > > 
> > > --
> > > 2.13.0
> > > 
> > > ___
> > > 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

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

2017-08-03 Thread Victor Toso 
Hi,

On Thu, Aug 03, 2017 at 09:29:43AM -0400, Marc-André Lureau wrote:
> Hi
>
> - Original Message -
> > From: Victor Toso 
> >
> > There should be some effort to make spice-server adjust mmtime correctly
> > instead
> > of relying on audio backend output directly.
> >
> > The latency from audio backend should be forwarded to spice-server but 
> > that's
> > yet to be implemented.
> >
> > This two patches allow us to simply turn-off adjusting mmtime in the client
> > directly from audio's backend.
> >
> > A property sounds reasoanble as this might be something nice to test even if
> > mmtime from spice-server looks great, for instance, considering a mocked 
> > data
> > from server we can compare if mmtime from server is better then spice-gtk
> > adjustments based on pulseaudio input.
>
> This really looks like test code to me.
>
> Is this really something we want to have checked in? and in the API?

Relying on audio backend works well in general but the correct mmtime
should come from server. We *use* the tweak since 2010 :)

> If it's generally useful tweak, I would rather use an environment
> variable, but I doubt this is the case.

What should be done is to have mmtime sent correctly, which is not the
case.

> 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.

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

Cheers,
toso
>
> 
> > Cheers,
> > toso
> > 
> > Victor Toso (2):
> >   session: introduce "video-sync-on-audio-latency" property
> >   spicy: toggle SpiceSession::video-sync-on-audio-latency
> > 
> >  src/channel-playback.c   |  7 ++-
> >  src/spice-session-priv.h |  1 +
> >  src/spice-session.c  | 33 +
> >  tools/spicy.c| 43 ++-
> >  4 files changed, 82 insertions(+), 2 deletions(-)
> > 
> > --
> > 2.13.0
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 


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 0/2] Disabling mmtime adjustment in the client (from audio backend)

2017-08-03 Thread Marc-André Lureau 
Hi

- Original Message -
> From: Victor Toso 
> 
> There should be some effort to make spice-server adjust mmtime correctly
> instead
> of relying on audio backend output directly.
> 
> The latency from audio backend should be forwarded to spice-server but that's
> yet to be implemented.
> 
> This two patches allow us to simply turn-off adjusting mmtime in the client
> directly from audio's backend.
> 
> A property sounds reasoanble as this might be something nice to test even if
> mmtime from spice-server looks great, for instance, considering a mocked data
> from server we can compare if mmtime from server is better then spice-gtk
> adjustments based on pulseaudio input.

This really looks like test code to me.

Is this really something we want to have checked in? and in the API?

If it's generally useful tweak, I would rather use an environment variable, but 
I doubt this is the case.

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.


> Cheers,
> toso
> 
> Victor Toso (2):
>   session: introduce "video-sync-on-audio-latency" property
>   spicy: toggle SpiceSession::video-sync-on-audio-latency
> 
>  src/channel-playback.c   |  7 ++-
>  src/spice-session-priv.h |  1 +
>  src/spice-session.c  | 33 +
>  tools/spicy.c| 43 ++-
>  4 files changed, 82 insertions(+), 2 deletions(-)
> 
> --
> 2.13.0
> 
> ___
> 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 0/2] Disabling mmtime adjustment in the client (from audio backend)

2017-08-03 Thread Victor Toso 
From: Victor Toso 

There should be some effort to make spice-server adjust mmtime correctly instead
of relying on audio backend output directly.

The latency from audio backend should be forwarded to spice-server but that's
yet to be implemented.

This two patches allow us to simply turn-off adjusting mmtime in the client
directly from audio's backend.

A property sounds reasoanble as this might be something nice to test even if
mmtime from spice-server looks great, for instance, considering a mocked data
from server we can compare if mmtime from server is better then spice-gtk
adjustments based on pulseaudio input.

Cheers,
toso

Victor Toso (2):
  session: introduce "video-sync-on-audio-latency" property
  spicy: toggle SpiceSession::video-sync-on-audio-latency

 src/channel-playback.c   |  7 ++-
 src/spice-session-priv.h |  1 +
 src/spice-session.c  | 33 +
 tools/spicy.c| 43 ++-
 4 files changed, 82 insertions(+), 2 deletions(-)

-- 
2.13.0

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