Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make it possible to use gdppay and gdpdepay instead of ogg #1075

Closed
totaam opened this issue Jan 5, 2016 · 29 comments
Closed

make it possible to use gdppay and gdpdepay instead of ogg #1075

totaam opened this issue Jan 5, 2016 · 29 comments
Labels

Comments

@totaam
Copy link
Collaborator

totaam commented Jan 5, 2016

Issue migrated from trac ticket # 1075

component: sound | priority: critical | resolution: wontfix

2016-01-05 09:07:19: antoine created the issue


Currently, the muxers are hard-coded for any given codec combination.

It would be nice if the client and server could negotiate which muxers they want to use so that we can use gdppay / gdpdepay instead of ogg.
Ogg is heavier and adds latency.
Especially for opus (#1074). This builds on the work done in #849.

@totaam
Copy link
Collaborator Author

totaam commented Jan 5, 2016

2016-01-05 16:30:16: antoine changed owner from antoine to afarr

@totaam
Copy link
Collaborator Author

totaam commented Jan 5, 2016

2016-01-05 16:30:16: antoine edited the issue description

@totaam
Copy link
Collaborator Author

totaam commented Jan 5, 2016

2016-01-05 16:30:16: antoine commented


Negotiating the muxers would have made the code very very hard to maintain, and there is a much easier solution implemented in r11594 + r11596: we add some new virtual codecs named "flac+gdp", "opus+gdp" and "speex+gdp".

I now get on Fedora 23:

./xpra/sound/gstreamer_util.py
(...)

GStreamer version: 1.6.2.0
PyGStreamer version: 3.18.2

encoders supported: vorbis, opus+gdp, opus, flac+gdp, mp3, wav, wavpack, speex+gdp, speex
decoders supported: vorbis, opus+gdp, opus, flac+gdp, mp3, wav, wavpack, speex+gdp, speex
source plugins: pulsesrc, autoaudiosrc, alsasrc, osssrc, oss4src, audiotestsrc
sink plugins: autoaudiosink, pulsesink, alsasink, osssink, oss4sink
default sink: pulsesink

With this in place, we now prefer the "gdp" muxer variant for "opus", "flac" and "speex" if both ends support it. And if we end up making opus the default (#1074), we will just have "opus+gdp" first in the list.


Having some hard data to verify that gdp is really better than ogg would be useful too, though that's going to be hard to measure (everything else would need to be turned off: no screen updates, just sound), I would expect:

  • the bandwidth usage to be marginally lower (maybe more so when the sound compresses very well, ie: silence rather than music)
  • the CPU usage to also be lower (by a very small margin..)
  • latency should be lower too (though ogg's 20ms is pretty low already - not sure how to measure that)

If possible, it is better to completely remove xpra from the equation for making measurements. For example we can run the gstreamer pipelines standalone.
After running the server with -d sound we can see in the log output that the pipeline used for "opus+gdp" is:

sound source pipeline=pulsesrc device=alsa_output.pci-0000_00_14.2.analog-stereo.monitor ! \
    queue name=queue min-threshold-time=0 max-size-buffers=0 max-size-bytes=0 max-size-time=50000000 leaky=2 silent=1 ! \
    volume name=volume volume=1.0 ! opusenc cbr=0 complexity=0 ! \
    gdppay crc-payload=0 crc-header=0 ! \
    appsink name=sink emit-signals=true max-buffers=10 drop=true sync=false async=false qos=false

Then we can replace the source with a test "audiotestsrc" and the pulsesink with a "fakesink". Script the whole thing to run for 10 seconds and measure CPU usage with time:

time gst-launch-1.0 audiotestsrc ! \
    queue name=queue min-threshold-time=0 max-size-buffers=0 max-size-bytes=0 max-size-time=50000000 leaky=2 silent=1 ! \
    volume name=volume volume=1.0 ! opusenc cbr=0 complexity=0 ! \
    gdppay crc-payload=0 crc-header=0 ! fakesink &
sleep 10;killall gst-launch-1.0

Then we can repeat this process for all the different codecs... both client and server.
(and if we wanted to, we could even test gstreamer 0.10 vs 1.x by replacing gst-launch-1.0 by gst-launch-0.10).

If we wanted to be really thorough, we could also change the audiotestsrc to produce different types of test sounds, ie: to get silence: audiotestsrc wave=4 (more info here: [http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-plugins/html/gst-plugins-base-plugins-audiotestsrc.html]). Or even feeding a specific music sampled as input using filesrc.

For more information on the output of time, see Real, User and Sys process time statistics.
Ideally we would get more detailed statistics using a tool like perf, but this will do for now.

So far I got:
||# Client or Server||# Codec||# Real CPU||# User CPU||# Sys CPU||
||Server || opus+gdp || 10.0 || 19.9 || ~0.04 +- 0.02 ||
||Server || opus || 10.0 || 19.9 || 0.09 +- 0.02 ||
So that's a very small win for gdp, which is spending less time in kernel mode, probably because it does not rely on timers for chunking the data.


@afarr: please test that you can select the new "+gdp" codec options, unless you have lots of spare time and feel like compiling lots of interesting numbers as I started doing above...

@totaam
Copy link
Collaborator Author

totaam commented Jan 6, 2016

2016-01-06 01:29:41: afarr changed owner from afarr to antoine

@totaam
Copy link
Collaborator Author

totaam commented Jan 6, 2016

2016-01-06 01:29:41: afarr commented


Testing with fedora 23 0.17.0 r11580, I'm getting the same ./xpra/sound/gstreamer_util.py output as you posted, and the server runs with any of the "+gdp" codec options.

I don't have any clients handy that will also support those speaker-codecs, however, so I haven't tested that those codecs actually work (at least not yet).

I'll assign this back to you for now... but I'll test with a fedora client soon (which will presumably also support the codecs) to see if they actually output sound. I'll see what I can do about making the time to run some of those benchmark tests too... hopefully also in the near future. Not sure if you want to close or hold out for my making time (I could also fill out chart in closed ticket... if you'd prefer, until it's wiki-ready).

@totaam
Copy link
Collaborator Author

totaam commented Jan 6, 2016

2016-01-06 03:09:39: antoine changed owner from antoine to afarr

@totaam
Copy link
Collaborator Author

totaam commented Jan 6, 2016

2016-01-06 03:09:39: antoine commented


I think we want to make sure that each new combination gets tested before the 0.17.0 release. No rush.

I've also added "vorbis+ogg" in r11599, which may help with the html5 client. r11628 adjusts the UI to make it fit on session info.

Please forward this to David.

@totaam
Copy link
Collaborator Author

totaam commented Jan 12, 2016

2016-01-12 00:50:27: afarr commented


Testing the "vorbis+ogg" with 0.17.0 r11640 osx client against 0.17.0 r11649 fedora 23 server... I'm stumbling across a client side error that's triggering a speaker stop.

2016-01-11 16:39:38,482 sound output pipeline error: gst-stream-error-quark: Could not decode stream. (7)
2016-01-11 16:39:38,482 sound output  gstvorbisdec.c(519)
2016-01-11 16:39:38,482 sound output  vorbis_handle_data_packet ()
2016-01-11 16:39:38,482 sound output  /GstPipeline:pipeline0/GstVorbisDec:vorbisdec0:
no header sent yet
2016-01-11 16:39:38,482 stopping speaker because of error: gst-stream-error-quark: Could not decode stream. (7)

I'll also attach full client and server side -d sound connection and error logs, in case there's something there of interest.

(Now that I have clients that will support codecs, I'll test the rest as well... and post any further issues.)

@totaam
Copy link
Collaborator Author

totaam commented Jan 12, 2016

2016-01-12 00:51:08: afarr uploaded file ticket1075_0-17-r11640_client-d-sound-log.txt (51.9 KiB)

r11640 0.17 client output with -d sound

@totaam
Copy link
Collaborator Author

totaam commented Jan 12, 2016

2016-01-12 00:51:43: afarr uploaded file ticket1075_0-17-r11649_fedora23-server-log.txt (85.2 KiB)

r11649 0.17 fedora 23 server outpur with -d sound

@totaam
Copy link
Collaborator Author

totaam commented Jan 12, 2016

2016-01-12 04:21:38: antoine commented


r11654 removes vorbis+ogg and adds a note in the code, I had it working before, no idea why this doesn't work anymore (gstreamer 1.x?) and not going to care.

If all the other encodings work, please close.

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2016

2016-01-13 03:03:58: afarr changed owner from afarr to antoine

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2016

2016-01-13 03:03:58: afarr commented


Hmmm... quick summation (I'll throw in some tracebacks and such below... good news?).

-Windows 0.17.0 r11653* (so listed from xpra.org/beta .. but indicating itself to be r11649 (1 change) )

  • Working as expected - vorbis, opus, opus+gdp, mp3, wav, wavpack

  • Causing client system warnings as the sound crashes (no good client or server output) — speex (sometimes works), speex+gdp (sometimes works)

2016-01-12 18:29:23,867 sound output Warning: bencode import failed: No module named 'xpra.net.bencode'
2016-01-12 18:31:10,473 the speex sound sink has stopped
2016-01-12 17:15:34,515 sound output Warning: bencode import failed: No module named 'xpra.net.bencode'
2016-01-12 17:15:49,492 the speex+gdp sound sink has stopped
  • Throwing same error as vorbis+ogg — flac+gdp:
2016-01-12 17:11:27,784 sound output pipeline error: gst-stream-error-quark: Internal data flow error. (1)
2016-01-12 17:11:27,784 sound output  gstbasesrc.c(2943)
2016-01-12 17:11:27,784 sound output  gst_base_src_loop ()
2016-01-12 17:11:27,784 sound output  /GstPipeline:pipeline0/GstAppSrc:src:
streaming task paused, reason not-negotiated (-4)
2016-01-12 17:11:27,784 sound output push-buffer error: <enum GST_FLOW_FLUSHING of type GstFlowReturn>
2016-01-12 17:11:27,784 sound output push-buffer error: <enum GST_FLOW_FLUSHING of type GstFlowReturn>
2016-01-12 17:11:27,783 stopping speaker because of error: push-buffer error: <enum GST_FLOW_FLUSHING of type GstFlowReturn>
2016-01-12 17:11:27,784 sound output push-buffer error: <enum GST_FLOW_FLUSHING of type GstFlowReturn>
2016-01-12 17:11:27,784 sound output stopping
2016-01-12 17:11:28,269 sound output using audio codec: flac+gdp

with the irregular bonus of:

2016-01-12 18:39:47,490 received console event CTRL_C
2016-01-12 18:39:47,490 error closing None
Traceback (most recent call last):
  File "xpra\net\protocol.pyc", line 942, in close
AttributeError: 'NoneType' object has no attribute 'input_bytecount'

-* OSX 0.17.0 r11653 **

  • Working as expected — vorbis, speex, speex+gdp, mp3, wav, wavpack

  • Throwing same error as vorbis+ogg — flac+gdp

  • Failing "elegantly" — flac, opus, opus+gdp

2016-01-12 18:48:27,330 Warning: invalid value for speaker-codec: opus+gdp
2016-01-12 18:48:27,330  valid options: vorbis, vorbis+ogg, flac+gdp, mp3, wav, wavpack, speex+gdp, speex

(And the same message for opus and flac, with appropriate change to first line.)


I suppose I can try to test with gstreamer 0.10 as best I can... but it looks like vorbis+ogg isn't the only codec misbehaving.

I'll also see about taking a stab at that automated testing command you provided, see what I can get out of it...

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2016

2016-01-13 06:29:13: antoine changed owner from antoine to afarr

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2016

2016-01-13 06:29:13: antoine commented


Windows 0.17.0 r11653 (so listed from xpra.org/beta .. but indicating itself to be r11649 (1 change) )
[[BR]]
Well spotted, I'll look into it.


Causing client system warnings as the sound crashes
[[BR]]
Those are caused by the switch to gstreamer 1.x and will be shown (until I remove them) whether the sound works or not, I've mentioned them in #1041#comment:3. Just ignore them for now.
[[BR]]
Doesn't speex / speex+gdp work? Works fine here.


Throwing same error as vorbis+ogg — flac+gdp:
[[BR]]

  • as per comment:5, vorbis+ogg has already been removed - but we now have vorbis+webm (add new sound container formats: webm / matroska #1090)
  • flac+gdp is exactly the same problem that makes us disable flac with gstreamer 1.x, running the gstreamer util tool with "-v" shows: skipping flac with GStreamer 1.x to avoid obscure 'not-neogtiated' errors I do not have time for, and now I've made it skip flac+gdp in r11678 (this explains why it had been enabled: I believe this works with gstreamer 0.10)

Failing "elegantly" — flac, opus, opus+gdp
[[BR]]
They're not really "failing", just not available.
In the case of flac, that's expected (see just above), but opus should be available.
It is on my systems and we even want to make it the default codec. Problems related to opus belong in #1074.

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2016

2016-01-13 11:42:41: antoine commented


I refused to give up and came up with a way to get flac to work with gstreamer 1.x, done in r11686. (an ugly fix..)

I also attempted (and failed) with those combinations: vorbis+ogg, flac (which is flac+ogg), opus+rtp, opus+webm..

For the record: some codec names are short (ie: "flac") because they were introduced before we had a choice of muxers to use... we probably should support both short and long names (ie: "flac" == "flac+ogg").

also:

  • r11685 fixes the "bencode" warning
  • r11687 should make it much easier to debug gstreamer sound bits without having to use the big hammer -d sound, we now have a -d gstreamer for this

@totaam
Copy link
Collaborator Author

totaam commented Apr 15, 2016

2016-04-15 15:47:35: antoine changed priority from major to critical

@totaam
Copy link
Collaborator Author

totaam commented Apr 15, 2016

2016-04-15 15:47:35: antoine commented


Raising for the imminent 0.17 release.

@totaam
Copy link
Collaborator Author

totaam commented May 18, 2016

2016-05-18 00:16:42: afarr changed owner from afarr to antoine

@totaam
Copy link
Collaborator Author

totaam commented May 18, 2016

2016-05-18 00:16:42: afarr commented


Well... not so sure this late is better than never... but here goes.

Re-tested with 0.17.0 12465 osx client against 0.17.2 r12453 (unknown changes) fedora 23 server (I think I'll have to add something to those build scripts to be sure it deletes old revision numbers).

Checking the list of available codecs from /usr/lib64/python2.7/site-packages/xpra/sound/gstreamer_util.py, it looks like all the listed codec are working rather nicely except VORBIS_MKA, which I'm presuming is largely intended for the html5 client (?).

As I was going through the codecs though, I started noticing that, with a 1440p display and using the BBC HD sync test page from youtube, I was occasionally getting some significantly different av-sync results.

  • Vorbis, opus, flac+gdp, speex, and speex+gdp all seemed to be between 2 and 4 frames audio-late (+/- 100ms).

  • Opus+gdp was about 0-2 frames audio-late (a theoretical 33 ms, but not detectable to my eye/ear).

  • Wav was about 1-3 frames audio late (some theoretical 66 ms?, also undetectable to my ear/eye except in a sync test).

  • Wavpack was 0-1 frame audio-late - but the video was so consistently jumpy that my evaluation may well have been just extrapolation (when there was video at the beep, it was virtually perfectly in sync).

  • Mp3, on the other hand, seemed to be about 6-8 frames audio late.

All work though. I suspect closing this and addressing av-sync fine tuning in another ticket is the best plan... but let me know if you'd feel better to have me testing with a variety of clients before closing.

@totaam
Copy link
Collaborator Author

totaam commented Aug 10, 2016

2016-08-10 03:32:47: antoine changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Aug 10, 2016

2016-08-10 03:32:47: antoine set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Aug 10, 2016

2016-08-10 03:32:47: antoine commented


all the listed codec are working rather nicely except VORBIS_MKA
[[BR]]
This should be recorded in #1090

[[BR]]

I was occasionally getting some significantly different av-sync results
[[BR]]
This should go in #1164.

Closing, even though some codecs don't seem to work on osx: #1276.

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:01: antoine changed status from closed to reopened

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:01: antoine removed resolution (was fixed)

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:01: antoine commented


gdp muxers were broken, see #1276.

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:53: antoine changed status from reopened to closed

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:53: antoine set resolution to wontfix

@totaam
Copy link
Collaborator Author

totaam commented Aug 24, 2016

2016-08-24 06:37:53: antoine commented


It would have been good to catch this during the 0.17 release cycle :(

See #1276#comment:13. gdp is no more.

@totaam totaam closed this as completed Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant