Re: [pulseaudio-discuss] [PATCH 0/2 pavucontrol] A couple of small cleanups

[Colin Leroy]

On Wed,  1 Nov 2017 13:47:04 +0200, Tanu Kaskinen  wrote:

Hi,

>   remove unnecessary RefPtr wrapping of PavuApplication
>   remove unnecessary Window -> MainWindow casting

Seems all well to me!
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

[Colin Leroy]

On Wed, 01 Nov 2017 13:50:02 +0200, Tanu Kaskinen  wrote:

> Thanks! I applied your patch now.

Hi Tanu,

Thanks ! and thanks for your patches on top of it :)

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

[Colin Leroy]

On Mon, 30 Oct 2017 16:15:52 +0200, Tanu Kaskinen <ta...@iki.fi> wrote:

Hi Tanu,

Here's an updated patch and more precise replies to your comments.

> The division of responsibilities isn't always very clean, but I don't
> have ideas for simple fixes for that. I think PavuApplication should
> create the UI and an object for managing the libpulse stuff, and the
> libpulse stuff shouldn't directly depend on the UI parts. Currently
> the libpulse stuff is mostly handled in pavucontrol.cc, but that is
> very much dependent on the UI stuff, so fixing this would be a big
> project.

I agree on this, and didn't change anything in this regard.

> This is a weird function. The name implies that it will select the
> best tab on some heuristics, but it only does that if default_tab is
> 0. If default_tab is -1, then the function does absolutely nothing.
> The "default_tab = -1" assignment at the end doesn't make any sense.
> 
> I think it would be better if this function was only called when the
> caller really wants to apply the heuristics.

The new patch separates "select best tab" and "select what you're told
to" in two functions for clarity.

> Why does the PavuApplication need to be wrapped in a RefPtr?

To build :)
This is apparently inherent to overloading Gtk::Application as I get
compilation errors without it. (I find cpp errors very hard to read but
this is rather clear that it's a template thing).


> > +add_window(*pavucontrol_window);
> > +[...]
> > +auto windows = get_windows();
> > +
> > +if (windows.size() > 0) {
> > +pavucontrol_window =
> > dynamic_cast<Gtk::Window*>(windows[0]);
> > +}  
> 
> This seems a bit ugly. Couldn't the main window be a member variable
> of PavuApplication?

I've made it a member.
I still have to call Gtk::Application->add_window() (or the GtkApp
doesn't correctly initialize - GtkApps need at least one window
registered). I don't use get_windows() anymore though.

Sidenote: I still have to make an ugly (MainWindow*) cast, because I
can't use MainWindow in pavucontrol.h as I would have to include
mainwindow.h which already includes pavucontrol.h. (Which proves
there's a responsabilities separation issue).

> This is probably part of the "switch tab if already running" logic,
> but I don't understand how that works in practice. If there's already
> one instance running, how does the tab index get passed from the new
> process to the old one? I'm sure I could figure it out from the GLib
> documentation, but there's apparently a lot of magic happening behind
> the scenes, and it would be good to have a comment that explains how
> the magic works.

I've added comments on every Gtk::Application aspect of the thing. I
hope they make things more clear.

Thanks,
-- 
Colin
>From 6326135813c0ec95739db94f0d81a0cd696dd106 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Thu, 26 Oct 2017 22:20:36 +0200
Subject: [PATCH] Implement single-launch with Gtk::Application

This introduces a new file for clarity. Options
handling changes so that --tab changes the tab
if the window is already opened. Other options
are only used at start time.
---
 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/mainwindow.cc  |  16 +
 src/mainwindow.h   |   3 +
 src/pavuapplication.cc | 181 +
 src/pavuapplication.h  |  55 +++
 src/pavucontrol.cc | 102 
 src/pavucontrol.h  |   2 +
 8 files changed, 274 insertions(+), 87 deletions(-)
 create mode 100644 src/pavuapplication.cc
 create mode 100644 src/pavuapplication.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c952a83..e9d0f55 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -12,3 +12,4 @@ src/sinkwidget.cc
 src/sourceoutputwidget.cc
 src/sourcewidget.cc
 src/streamwidget.cc
+src/pavuapplication.cc
diff --git a/src/Makefile.am b/src/Makefile.am
index 7257260..b5c0314 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
   rolewidget.h rolewidget.cc \
   mainwindow.h mainwindow.cc \
   pavucontrol.h pavucontrol.cc \
+  pavuapplication.cc pavuapplication.h \
   i18n.h
 
 pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
diff --git a/src/mainwindow.cc b/src/mainwindow.cc
index 31d5695..da598fd 100644
--- a/src/mainwindow.cc
+++ b/src/mainwindow.cc
@@ -340,6 +340,22 @@ static void updatePorts(DeviceWidget *w, std::map<Glib::ustring, PortInfo> 
 }
 }
 
+void MainWindow::selectBestTab() {
+if (sinkInputWidgets.size() > 0)
+notebook->set_current_page(0);
+else if (sourceOutputWidgets.size() > 0)
+notebook->set_current_page(1);
+else if (sourceWidgets.size() > 0 && sinkWidge

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

[Colin Leroy]

On 30 October 2017 at 16h15, Tanu Kaskinen wrote:

Hi, 

> This is a weird function. [...]
> I think it would be better if this function was only called when the
> caller really wants to apply the heuristics.

I'll update the patch to do that.
Below are answers to the rest of your questions:

> > +Glib::RefPtr PavuApplication::create() {
> > +return Glib::RefPtr(new PavuApplication());
> > +
> > +}  
> 
> Why does the PavuApplication need to be wrapped in a RefPtr?

I'm not sure it needs. I used the same general architecture as the
GtkMM Gtk::Application example.

> > +if (windows.size() > 0) {
> > +pavucontrol_window =
> > dynamic_cast(windows[0]);
> > +}  
> 
> This seems a bit ugly. Couldn't the main window be a member variable
> of PavuApplication?

I'll try that. 

> > +
> > +if (pavucontrol_window == NULL) {
> > +pavucontrol_window = create_window();
> > +} else if (tab != -1) {
> > +((MainWindow *)pavucontrol_window)->selectBestTab(tab);
> > +}
> > +
> > +pavucontrol_window->present();
> > +}  
> 
> This is probably part of the "switch tab if already running" logic,
> but I don't understand how that works in practice. If there's already
> one instance running, how does the tab index get passed from the new
> process to the old one?

We're already in the first existing process here. I think the second
process sends a message to the first one with argc and argv[]
somewhere. It then gets handled by the first process's on_command_line
callback. I'll have to check gtkApplication's doc to know how it's
communicating. I'm guessing Unix socket, at least on Unix systems.

> I'm sure I could figure it out from the GLib documentation, but
> there's apparently a lot of magic happening behind the scenes, and it
> would be good to have a comment that explains how the magic works.

I'll add that :)

Thanks!
-- 
Colin


pgpCPDCowD1au.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

[Colin Leroy]

Aand, I sent the wrong patch. Here's the correct one.

-- 
Colin
From 2120613fa5bd68ebbdd0e9428357030736fce013 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Thu, 26 Oct 2017 22:20:36 +0200
Subject: [PATCH] Implement single-launch with Gtk::Application

This introduces a new file for clarity. Options
handling changes so that --tab changes the tab
if the window is already opened. Other options
are only used at start time.
---
 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/mainwindow.cc  |  18 ++
 src/mainwindow.h   |   2 +
 src/pavuapplication.cc | 151 +
 src/pavuapplication.h  |  51 +
 src/pavucontrol.cc |  98 
 src/pavucontrol.h  |   2 +
 8 files changed, 237 insertions(+), 87 deletions(-)
 create mode 100644 src/pavuapplication.cc
 create mode 100644 src/pavuapplication.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c952a83..e9d0f55 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -12,3 +12,4 @@ src/sinkwidget.cc
 src/sourceoutputwidget.cc
 src/sourcewidget.cc
 src/streamwidget.cc
+src/pavuapplication.cc
diff --git a/src/Makefile.am b/src/Makefile.am
index 7257260..b5c0314 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
   rolewidget.h rolewidget.cc \
   mainwindow.h mainwindow.cc \
   pavucontrol.h pavucontrol.cc \
+  pavuapplication.cc pavuapplication.h \
   i18n.h
 
 pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
diff --git a/src/mainwindow.cc b/src/mainwindow.cc
index 31d5695..b73108f 100644
--- a/src/mainwindow.cc
+++ b/src/mainwindow.cc
@@ -340,6 +340,24 @@ static void updatePorts(DeviceWidget *w, std::map<Glib::ustring, PortInfo> 
 }
 }
 
+void MainWindow::selectBestTab(int default_tab) {
+if (default_tab != -1) {
+if (default_tab < 1 || default_tab > notebook->get_n_pages()) {
+if (sinkInputWidgets.size() > 0)
+notebook->set_current_page(0);
+else if (sourceOutputWidgets.size() > 0)
+notebook->set_current_page(1);
+else if (sourceWidgets.size() > 0 && sinkWidgets.size() == 0)
+notebook->set_current_page(3);
+else
+notebook->set_current_page(2);
+} else {
+notebook->set_current_page(default_tab - 1);
+}
+default_tab = -1;
+}
+}
+
 void MainWindow::updateCard(const pa_card_info ) {
 CardWidget *w;
 bool is_new = false;
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 30e1ad0..a163069 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -60,6 +60,8 @@ public:
 void removeSourceOutput(uint32_t index);
 void removeClient(uint32_t index);
 
+void selectBestTab(int default_tab);
+
 void removeAllWidgets();
 
 void setConnectingMessage(const char *string = NULL);
diff --git a/src/pavuapplication.cc b/src/pavuapplication.cc
new file mode 100644
index 000..18f8d0b
--- /dev/null
+++ b/src/pavuapplication.cc
@@ -0,0 +1,151 @@
+/***
+  This file is part of pavucontrol.
+
+  Copyright 2006-2008 Lennart Poettering
+  Copyright 2008 Sjoerd Simons <sjo...@luon.net>
+
+  pavucontrol 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.
+
+  pavucontrol 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 pavucontrol. If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include "i18n.h"
+
+#include 
+
+#include "pavuapplication.h"
+#include "pavucontrol.h"
+#include "mainwindow.h"
+
+PavuApplication::PavuApplication() : Gtk::Application("org.pulseaudio.pavucontrol", Gio::ApplicationFlags::APPLICATION_HANDLES_COMMAND_LINE) {
+}
+
+Glib::RefPtr PavuApplication::create() {
+return Glib::RefPtr(new PavuApplication());
+
+}
+
+Gtk::Window* PavuApplication::create_window()
+{
+m = pa_glib_mainloop_new(g_main_context_default());
+g_assert(m);
+
+Gtk::Window* pavucontrol_window = pavucontrol_get_window(m, maximize, retry, tab);
+add_window(*pavucontrol_window);
+
+pavucontrol_window->signal_hide().connect(
+ sigc::bind<Gtk::Window*>(sigc::mem_fun(*this,
+ ::on_hide_window), pavucontrol_window));
+
+return pavucontrol_window;
+}
+
+void PavuApplication::on_activate()
+{
+Gtk::Window* pavucontrol_window = NULL;
+
+aut

[pulseaudio-discuss] [PATCH] pavucontrol: implement single-launch

[Colin Leroy]

Hi,

Following my last mail on the subject and having received only positive
reaction, I've implemented single-launch with Gtk::Application.

This introduces a new file for clarity. Options handling changes so
that --tab changes the tab if the window is already opened. Other
options are only used at start time.

I hope you'll consider it for inclusion :)
-- 
Colin
From: Colin Leroy <co...@colino.net>
Date: Thu, 26 Oct 2017 22:20:36 +0200
Subject: [PATCH 2/2] Implement single-launch with Gtk::Application

This introduces a new file for clarity. Options
handling changes so that --tab changes the tab
if the window is already opened. Other options
are only used at start time.
---
 po/POTFILES.in |   1 +
 src/Makefile.am|   1 +
 src/mainwindow.cc  |  18 ++
 src/mainwindow.h   |   2 +
 src/pavuapplication.cc | 151 +
 src/pavuapplication.h  |  51 +
 src/pavucontrol.cc |  87 +++-
 src/pavucontrol.h  |   2 +
 8 files changed, 234 insertions(+), 79 deletions(-)
 create mode 100644 src/pavuapplication.cc
 create mode 100644 src/pavuapplication.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c952a83..e9d0f55 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -12,3 +12,4 @@ src/sinkwidget.cc
 src/sourceoutputwidget.cc
 src/sourcewidget.cc
 src/streamwidget.cc
+src/pavuapplication.cc
diff --git a/src/Makefile.am b/src/Makefile.am
index 7257260..b5c0314 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,6 +37,7 @@ pavucontrol_SOURCES= \
   rolewidget.h rolewidget.cc \
   mainwindow.h mainwindow.cc \
   pavucontrol.h pavucontrol.cc \
+  pavuapplication.cc pavuapplication.h \
   i18n.h
 
 pavucontrol_LDADD=$(AM_LDADD) $(GUILIBS_LIBS) $(PULSE_LIBS)
diff --git a/src/mainwindow.cc b/src/mainwindow.cc
index 31d5695..b73108f 100644
--- a/src/mainwindow.cc
+++ b/src/mainwindow.cc
@@ -340,6 +340,24 @@ static void updatePorts(DeviceWidget *w, std::map<Glib::ustring, PortInfo> 
 }
 }
 
+void MainWindow::selectBestTab(int default_tab) {
+if (default_tab != -1) {
+if (default_tab < 1 || default_tab > notebook->get_n_pages()) {
+if (sinkInputWidgets.size() > 0)
+notebook->set_current_page(0);
+else if (sourceOutputWidgets.size() > 0)
+notebook->set_current_page(1);
+else if (sourceWidgets.size() > 0 && sinkWidgets.size() == 0)
+notebook->set_current_page(3);
+else
+notebook->set_current_page(2);
+} else {
+notebook->set_current_page(default_tab - 1);
+}
+default_tab = -1;
+}
+}
+
 void MainWindow::updateCard(const pa_card_info ) {
 CardWidget *w;
 bool is_new = false;
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 30e1ad0..a163069 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -60,6 +60,8 @@ public:
 void removeSourceOutput(uint32_t index);
 void removeClient(uint32_t index);
 
+void selectBestTab(int default_tab);
+
 void removeAllWidgets();
 
 void setConnectingMessage(const char *string = NULL);
diff --git a/src/pavuapplication.cc b/src/pavuapplication.cc
new file mode 100644
index 000..18f8d0b
--- /dev/null
+++ b/src/pavuapplication.cc
@@ -0,0 +1,151 @@
+/***
+  This file is part of pavucontrol.
+
+  Copyright 2006-2008 Lennart Poettering
+  Copyright 2008 Sjoerd Simons <sjo...@luon.net>
+
+  pavucontrol 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.
+
+  pavucontrol 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 pavucontrol. If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include "i18n.h"
+
+#include 
+
+#include "pavuapplication.h"
+#include "pavucontrol.h"
+#include "mainwindow.h"
+
+PavuApplication::PavuApplication() : Gtk::Application("org.pulseaudio.pavucontrol", Gio::ApplicationFlags::APPLICATION_HANDLES_COMMAND_LINE) {
+}
+
+Glib::RefPtr PavuApplication::create() {
+return Glib::RefPtr(new PavuApplication());
+
+}
+
+Gtk::Window* PavuApplication::create_window()
+{
+m = pa_glib_mainloop_new(g_main_context_default());
+g_assert(m);
+
+Gtk::Window* pavucontrol_window = pavucontrol_get_window(m, maximize, retry, tab);
+add_window(*pavucontrol_window);
+
+pavucontrol_window->signal_hide().connect(
+ sigc::bind<Gtk::Wind

[pulseaudio-discuss] pavucontrol and single launch

[Colin Leroy]

Hello,

I very often end up with six or more instances of pavucontrol running,
because I click on its launcher to set things up, and then go back to
whatever I was doing without closing the pavucontrol window.

Would a single-instance launch patch be welcome ? If so, I could cook
up something based on GtkApplication.

Thanks,
-- 
Colin


pgpG36u7xLd99.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] RAOP latency by hardware: call for testers

[Colin Leroy]

On 12 October 2017 at 22h04, Colin Leroy wrote:

Hi, 

> I would really appreciate if owners of different AirPlay hardware
> (like Apple's hardware) could test with PulseAudio's git master

I forgot to say: Alternatively, you can also test with PulseAudio 11.0
or 11.1, but you'll have to delay video using software that supports
it, like mplayer (+/- keys by default).
-- 
Colin


pgp1mLR1eYdIE.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] RAOP latency by hardware: call for testers

[Colin Leroy]

Hello,

In the last monthes I've worked a bit on Pulseaudio's RAOP module, to
make it work better with various AirPlay servers. My latest work has
been to announce the correct latency needed to play synchronized
audio/video system-wide. 

Unfortunately I could only test with my own hardware (a Pioneer N-30)
and the ShairplaySync software server.

I would really appreciate if owners of different AirPlay hardware (like
Apple's hardware) could test with PulseAudio's git master and report if
a latency offset is needed to synchronize things well.

If you want to help, you can install PulseAudio's git master, then try
and play a video like this one :
https://www.youtube.com/watch?v=rzF6bNSXZac

(Check it first with an internal sound card to see which notes play in
sync with which color/waveform).

Then send the output to your Airplay hardware using pavucontrol's
Playback tab. If the sound and video are out of sync, head to
pavucontrol's Output Devices tab, expand your Airplay server's
"Advanced" knob and set the latency offset to a value making audio and
video synchronised. If you need to set it, there's a good chance 352ms
will be the correct value.

You can then verify everything is OK with a normal (non-looping) video
that has easy to check synchronization (like Pulp Fiction's Ezekiel
scene :D) 

Thanks in advance !
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] PATCH 1/2: RAOP: Show device codename in sink properties

[Colin Leroy]

This will make it easier for testers to report latency offset for
their specific hardware.

-- 
Colin
>From af1470e2c46189a7a735e0d50a8fd6deaecc0f4b Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Mon, 9 Oct 2017 09:26:42 +0200
Subject: [PATCH 1/2] RAOP: Show device codename in sink  properties

This will make it easier for testers to report latency offset for their
specific hardware.
---
 src/modules/raop/module-raop-discover.c | 39 +++--
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index ac4a025..b0c3eaa 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -168,6 +168,7 @@ static void resolver_cb(
 char *device = NULL, *nicename, *dname, *vname, *args;
 char *tp = NULL, *et = NULL, *cn = NULL;
 char *ch = NULL, *ss = NULL, *sr = NULL;
+char *dm = NULL;
 char *t = NULL;
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
@@ -256,7 +257,8 @@ static void resolver_cb(
 sr = pa_xstrdup(value);
 } else if (pa_streq(key, "am")) {
 /* Device model */
-latency = guess_latency_from_device(value);
+pa_xfree(dm);
+dm = pa_xstrdup(value);
 }
 
 avahi_free(key);
@@ -278,6 +280,7 @@ static void resolver_cb(
 pa_xfree(ch);
 pa_xfree(ss);
 pa_xfree(sr);
+pa_xfree(dm);
 goto finish;
 }
 
@@ -285,22 +288,24 @@ static void resolver_cb(
 pa_xfree(dname);
 
 avahi_address_snprint(at, sizeof(at), a);
-if (nicename) {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s "
- "sink_properties='device.description=\"%s (%s:%u)\"'",
- at, port,
- vname,
- nicename, at, port);
-pa_xfree(nicename);
-} else {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s"
- "sink_properties='device.description=\"%s:%u\"'",
- at, port,
- vname,
- at, port);
-}
+
+if (nicename == NULL)
+nicename = pa_xstrdup("RAOP");
+
+if (dm == NULL)
+dm = pa_xstrdup(_("Unknown device model"));
+
+latency = guess_latency_from_device(dm);
+
+args = pa_sprintf_malloc("server=[%s]:%u "
+ "sink_name=%s "
+ "sink_properties='device.description=\"%s (%s:%u)\" device.model=\"%s\"'",
+ at, port,
+ vname,
+ nicename, at, port,
+ dm);
+pa_xfree(nicename);
+pa_xfree(dm);
 
 if (tp != NULL) {
 t = args;
-- 
2.1.4

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] PATCH 2/2: RAOP: Init dummy port and card for latency change

[Colin Leroy]

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.

Also move the IP:port part of the sink name to the port name.

-- 
Colin
>From 900faa7190875bb83318d76961085b55386f7f3c Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Mon, 9 Oct 2017 09:42:27 +0200
Subject: [PATCH 2/2] RAOP: Init dummy port and card for latency change

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.

Also move the IP:port part of the sink name to the port name.
---
 src/modules/raop/module-raop-discover.c |   4 +-
 src/modules/raop/raop-sink.c| 103 
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index b0c3eaa..52919e6 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -299,10 +299,10 @@ static void resolver_cb(
 
 args = pa_sprintf_malloc("server=[%s]:%u "
  "sink_name=%s "
- "sink_properties='device.description=\"%s (%s:%u)\" device.model=\"%s\"'",
+ "sink_properties='device.description=\"%s\" device.model=\"%s\"'",
  at, port,
  vname,
- nicename, at, port,
+ nicename,
  dm);
 pa_xfree(nicename);
 pa_xfree(dm);
diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
index cd90e3d..5fbb9f9 100644
--- a/src/modules/raop/raop-sink.c
+++ b/src/modules/raop/raop-sink.c
@@ -69,6 +69,7 @@ struct userdata {
 pa_core *core;
 pa_module *module;
 pa_sink *sink;
+pa_card *card;
 
 pa_thread *thread;
 pa_thread_mq thread_mq;
@@ -125,6 +126,8 @@ static int64_t sink_get_latency(const struct userdata *u) {
 /* RAOP default latency */
 latency += u->latency * PA_USEC_PER_MSEC;
 
+latency += u->sink->port_latency_offset;
+
 return latency;
 }
 
@@ -465,6 +468,77 @@ finish:
 pa_log_debug("Thread shutting down");
 }
 
+static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
+return 0;
+}
+
+static pa_device_port *raop_create_port(struct userdata *u, const char *server) {
+pa_device_port_new_data data;
+pa_device_port *port;
+
+pa_device_port_new_data_init();
+
+pa_device_port_new_data_set_name(, "network-output");
+pa_device_port_new_data_set_description(, server);
+pa_device_port_new_data_set_direction(, PA_DIRECTION_OUTPUT);
+
+port = pa_device_port_new(u->core, , 0);
+
+pa_device_port_new_data_done();
+
+if (port == NULL)
+return NULL;
+
+pa_device_port_ref(port);
+
+return port;
+}
+
+static pa_card_profile *raop_create_profile() {
+pa_card_profile *profile;
+
+profile = pa_card_profile_new("RAOP", _("RAOP standard profile"), 0);
+profile->priority = 10;
+profile->n_sinks = 1;
+profile->n_sources = 0;
+profile->max_sink_channels = 2;
+profile->max_source_channels = 0;
+
+return profile;
+}
+
+static pa_card *raop_create_card(pa_module *m, pa_device_port *port, pa_card_profile *profile, const char *server, const char *nicename) {
+pa_card_new_data data;
+pa_card *card;
+char *card_name;
+
+pa_card_new_data_init();
+
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, server);
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, nicename);
+data.driver = __FILE__;
+
+card_name = pa_sprintf_malloc("raop_client.%s", server);
+pa_card_new_data_set_name(, card_name);
+pa_xfree(card_name);
+
+pa_hashmap_put(data.ports, port->name, port);
+pa_hashmap_put(data.profiles, profile->name, profile);
+
+card = pa_card_new(m->core, );
+
+pa_card_new_data_done();
+
+if (card == NULL)
+return NULL;
+
+pa_card_choose_initial_profile(card);
+
+pa_card_put(card);
+
+return card;
+}
+
 pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 struct userdata *u = NULL;
 pa_sample_spec ss;
@@ -473,6 +547,9 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 const char /* *username, */ *password;
 pa_sink_new_data data;
 const char *name = NULL;
+const char *description = NULL;
+pa_device_port *port;
+pa_card_profile *profile;
 
 pa_assert(m);
 pa_assert(ma);
@@ -590,6 +667,28 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 g

[pulseaudio-discuss] RAOP latency patches v3

[Colin Leroy]

Hi folks, hi Tanu,

Here's the v3 of the patches that allow user-setting of RAOP's latency
offset.

Hope this helps!
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] PATCH 3/3: RAOP: Add latency value for Shairport RAOP server

[Colin Leroy]

On 08 October 2017 at 21h21, Tanu Kaskinen wrote:

Hi, 

> > +} else if (pa_streq(model, "ShairportSync")) {
> > +/* Shairport - software AirPort server */
> > +default_latency = 2352;
> >  }
> >  
> >  pa_log_debug("Default latency is %u ms for device model %s.",
> > default_latency, model);  
> 
> Thanks! Applied.

Thanks! I'll rework the two other ones and resend them.
 
Concerning the default latency for unknown hardware, which we chose to
set to 2000ms, I hope to have some news from John Clare to see which
value should apply to his Apple hardware.
-- 
Colin


pgpSA__kIxIFa.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] RAOP patches again

[Colin Leroy]

Hi,

Here's a new version of my last patchset after Tanu's partial review.
The first one adds a device.model property to the sink; the second one
adds a port to the sink so that its latency can be controlled at
runtime; and the last one adds the correct latency value for
the Shairport server.

HTH,
-- 
Colin


pgp9feMhmEOSp.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] PATCH 1/3: RAOP: Show device codename in sink properties

[Colin Leroy]

This will make it easier for testers to report latency offset for their
specific hardware.

Also, remove the IP:port part from the device description; it will go
in the pa_device_port description in the next commit.
-- 
Colin
From a93c8e7a66f10d586f44001ab8c321bb105312ab Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 7 Oct 2017 20:04:42 +0200
Subject: [PATCH 1/3] RAOP: Show device codename in sink properties

This will make it easier for testers to report latency offset for their
specific hardware.

Also, remove the IP:port part from the device description; it will go
in the pa_device_port description in the next commit.
---
 src/modules/raop/module-raop-discover.c | 39 +++--
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index 8b7ba49b..7fe17dfe 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -165,6 +165,7 @@ static void resolver_cb(
 char *device = NULL, *nicename, *dname, *vname, *args;
 char *tp = NULL, *et = NULL, *cn = NULL;
 char *ch = NULL, *ss = NULL, *sr = NULL;
+char *dm = NULL;
 char *t = NULL;
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
@@ -253,7 +254,8 @@ static void resolver_cb(
 sr = pa_xstrdup(value);
 } else if (pa_streq(key, "am")) {
 /* Device model */
-latency = guess_latency_from_device(value);
+pa_xfree(dm);
+dm = pa_xstrdup(value);
 }
 
 avahi_free(key);
@@ -275,6 +277,7 @@ static void resolver_cb(
 pa_xfree(ch);
 pa_xfree(ss);
 pa_xfree(sr);
+pa_xfree(dm);
 goto finish;
 }
 
@@ -282,22 +285,24 @@ static void resolver_cb(
 pa_xfree(dname);
 
 avahi_address_snprint(at, sizeof(at), a);
-if (nicename) {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s "
- "sink_properties='device.description=\"%s (%s:%u)\"'",
- at, port,
- vname,
- nicename, at, port);
-pa_xfree(nicename);
-} else {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s"
- "sink_properties='device.description=\"%s:%u\"'",
- at, port,
- vname,
- at, port);
-}
+
+if (nicename == NULL)
+nicename = pa_xstrdup("RAOP");
+
+if (dm == NULL)
+dm = pa_xstrdup(_("Unknown device model"));
+
+latency = guess_latency_from_device(dm);
+
+args = pa_sprintf_malloc("server=[%s]:%u "
+ "sink_name=%s "
+ "sink_properties='device.description=\"%s\" device.model=\"%s\"'",
+ at, port,
+ vname,
+ nicename,
+ dm);
+pa_xfree(nicename);
+pa_xfree(dm);
 
 if (tp != NULL) {
 t = args;
-- 
2.11.0



pgpjSUcp8AeK_.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] PATCH 2/3: RAOP: Init dummy port and card for latency change

[Colin Leroy]

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.
-- 
Colin
From bd815a400ad280627083cbce3ef9b927f669faa0 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 7 Oct 2017 20:10:21 +0200
Subject: [PATCH 2/3] RAOP: Init dummy port and card for  latency change

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.
---
 src/modules/raop/raop-sink.c | 110 +--
 1 file changed, 107 insertions(+), 3 deletions(-)

diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
index cd90e3d9..244f6951 100644
--- a/src/modules/raop/raop-sink.c
+++ b/src/modules/raop/raop-sink.c
@@ -69,6 +69,7 @@ struct userdata {
 pa_core *core;
 pa_module *module;
 pa_sink *sink;
+pa_card *card;
 
 pa_thread *thread;
 pa_thread_mq thread_mq;
@@ -122,9 +123,6 @@ static int64_t sink_get_latency(const struct userdata *u) {
 
 latency = pa_bytes_to_usec(u->write_count, >sink->sample_spec) - (int64_t) now;
 
-/* RAOP default latency */
-latency += u->latency * PA_USEC_PER_MSEC;
-
 return latency;
 }
 
@@ -465,6 +463,83 @@ finish:
 pa_log_debug("Thread shutting down");
 }
 
+static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
+return 0;
+}
+
+static pa_device_port *raop_create_port(struct userdata *u, const char *server) {
+pa_device_port_new_data data;
+pa_device_port *port;
+
+pa_device_port_new_data_init();
+
+pa_device_port_new_data_set_name(, "network-output");
+pa_device_port_new_data_set_description(, server);
+pa_device_port_new_data_set_direction(, PA_DIRECTION_OUTPUT);
+
+port = pa_device_port_new(u->core, , 0);
+
+pa_device_port_new_data_done();
+
+if (port == NULL)
+return NULL;
+
+pa_device_port_ref(port);
+/* init the latency_offset directly instead of using
+ * pa_device_port_set_latency_offset(), so that mode module-card-restore
+ * can do its job when PulseAudio restarts and the user-set latency
+ * is not overwritten.
+ */
+port->latency_offset = u->latency * PA_USEC_PER_MSEC;
+
+return port;
+}
+
+static pa_card_profile *raop_create_profile() {
+pa_card_profile *profile;
+
+profile = pa_card_profile_new("RAOP", _("RAOP standard profile"), 0);
+profile->priority = 10;
+profile->n_sinks = 1;
+profile->n_sources = 0;
+profile->max_sink_channels = 2;
+profile->max_source_channels = 0;
+
+return profile;
+}
+
+static pa_card *raop_create_card(pa_module *m, pa_device_port *port, pa_card_profile *profile, const char *server, const char *nicename) {
+pa_card_new_data data;
+pa_card *card;
+char *card_name;
+
+pa_card_new_data_init();
+
+card_name = pa_sprintf_malloc("raop_client.%s", server);
+
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, card_name);
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, nicename);
+data.driver = __FILE__;
+
+pa_card_new_data_set_name(, card_name);
+pa_xfree(card_name);
+
+pa_hashmap_put(data.ports, port->name, port);
+pa_hashmap_put(data.profiles, profile->name, profile);
+
+card = pa_card_new(m->core, );
+
+pa_card_new_data_done();
+
+if (card == NULL)
+return NULL;
+
+card->active_profile = profile;
+pa_card_put(card);
+
+return card;
+}
+
 pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 struct userdata *u = NULL;
 pa_sample_spec ss;
@@ -473,6 +548,9 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 const char /* *username, */ *password;
 pa_sink_new_data data;
 const char *name = NULL;
+const char *description = NULL;
+pa_device_port *port;
+pa_card_profile *profile;
 
 pa_assert(m);
 pa_assert(ma);
@@ -590,6 +668,28 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 goto fail;
 }
 
+port = raop_create_port(u, server);
+if (port == NULL) {
+pa_log("Failed to create port object");
+goto fail;
+}
+
+profile = raop_create_profile();
+pa_hashmap_put(port->profiles, profile->name, profile);
+
+description = pa_proplist_gets(data.proplist, PA_PROP_DEVICE_DESCRIPTION);
+if (description == NULL)
+description = server;
+
+u->card = raop_create_card(m, port, profile, server, description);
+if (u->card == NULL) {
+pa_log("Failed to create card object");
+goto fail;
+}
+
+data.card = u->card;
+pa_hashmap_put(data.ports, port->name, port);
+
 u->sink = pa_s

[pulseaudio-discuss] PATCH 3/3: RAOP: Add latency value for Shairport RAOP server

[Colin Leroy]

After testing, Shairport server's latency is the same as Pioneer N30.
-- 
Colin
From 5882bd15c528a0e9987a02f9ddcdd093087ae548 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 7 Oct 2017 20:11:49 +0200
Subject: [PATCH 3/3] RAOP: Add latency value for Shairport RAOP server

After testing, Shairport server's latency is the same as Pioneer N30.
---
 src/modules/raop/module-raop-discover.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index 7fe17dfe..5b0c67aa 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -145,6 +145,9 @@ static uint32_t guess_latency_from_device(const char *model) {
 if (pa_streq(model, "PIONEER,1")) {
 /* Pioneer N-30 */
 default_latency = 2352;
+} else if (pa_streq(model, "ShairportSync")) {
+/* Shairport - software AirPort server */
+default_latency = 2352;
 }
 
 pa_log_debug("Default latency is %u ms for device model %s.", default_latency, model);
-- 
2.11.0



pgpQMpSfyxnxO.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] PulseAudio 11.1 - Airplay to Airport Express doesn't work

[Colin Leroy]

Hi John,

If you manage to get it to work, can you keep me informed? Because I'd
love if you could test something I can't test with your Apple
hardware :)

Thanks!

Colin

> Hi Colin,
> 
> Thanks for this. I’m far from being a networking expert, but my setup
> is as follows:
> 
> 1) Sky ADSL cable modem - wireless turned off but DCHP on. I wanted
> to use my Airport Express for wireless as it has 5GHz. 2) Airport
> Express connected to this using cable. Router mode off, and is in
> Bridge mode 3) Connecting to Airport Express wirelessly, all internet
> access and Airplay using iTunes on my Mac is fine
> 
> I assumed my Airport would be on an 192.168.0.* address, so I don’t
> know where 169.254.13.13 is coming from. Do you think this could be
> the problem then?
> 
> Regards,
> 
> John
> 
> > On 6 Oct 2017, at 13:47, Colin Leroy <co...@colino.net> wrote:
> > 
> > On Fri, 06 Oct 2017 12:37:52 +0100, John Clare <johncl...@me.com>
> > wrote:
> > 
> > Hi John,
> > 
> > The log doesn't seem to indicate much wrong, apart from the
> > obvious  
> >> E: [pulseaudio] rtsp_client.c: Connection failed: Connection timed
> >> out  
> > 
> > It seems your Airport Express's IP address is the "failed DHCP
> > request" address:
> >   
> >> D: [pulseaudio] rtsp_client.c: Attempting to connect to server
> >> '169.254.13.13:5000'  
> > 
> > I guess your network is not on 169.254.0.0.
> > 
> > Hope this helps,
> > -- 
> > Colin
> > ___
> > pulseaudio-discuss mailing list
> > pulseaudio-discuss@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss  
> 
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss



-- 
Colin


pgpp3fZ6T1Ra8.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] RAOP, pavucontrol and general "Latency offset"

[Colin Leroy]

On 06 October 2017 at 09h14, Colin Leroy wrote:

Hi folks, 

> I'm not sure I understand correctly, but it looks like to me
> setLatencyOffset() is only called if the sink has more than one port?
> 
> I would love if someone could enlighten me: did I understand the
> pavucontrol code correctly? Is there a reason for that or does that
> look like a bug? 

Don't spend time replying to those questions, I figured them out - what
was needed was a port and a profile, and a card because profiles need
cards.
-- 
Colin


pgpZi7ypFYxrJ.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pavucontrol: Set latency offset's maximum to 5000ms

[Colin Leroy]

Hi,

Airplay hardware has a inherent latency of at least 2 seconds, with
2350ms being a common value.

-- 
Colin
From a08262dcf1b4d0c5251070b1b78e0ab6cbc63a58 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Fri, 6 Oct 2017 20:20:16 +0200
Subject: [PATCH] Set latency offset's maximum to 5000ms

Airplay hardware has a inherent latency of at least 2 seconds, with
2350ms being a common value.
---
 src/devicewidget.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/devicewidget.cc b/src/devicewidget.cc
index 5831f21..18d6714 100644
--- a/src/devicewidget.cc
+++ b/src/devicewidget.cc
@@ -65,10 +65,10 @@ DeviceWidget::DeviceWidget(BaseObjectType* cobject, const Glib::RefPtrconfigure(offsetAdjustment, 0, 2);
 #else
-offsetAdjustment = new Gtk::Adjustment(0.0, -2000.0, 2000.0, 10.0, 50.0, 0.0);
+offsetAdjustment = new Gtk::Adjustment(0.0, -2000.0, 5000.0, 10.0, 50.0, 0.0);
 offsetButton->configure(*offsetAdjustment, 0.0, 2);
 #endif  /* HAVE_GTK3 */
 }
-- 
2.11.0



pgptRJiqNK50m.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] RAOP: Add latency value for Shairport RAOP server

[Colin Leroy]

Hi,

After testing, Shairport server's latency is the same as Pioneer N30.

I have a feeling we should make 2352ms the default for unknown hardware
too. Not a hard feeling though, we can think about it after we get some
results from a call to tests.
-- 
Colin
From aa6c6de3304a0834d3c1ebcf1a5b3780498216fe Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Fri, 6 Oct 2017 20:11:32 +0200
Subject: [PATCH 2/2] RAOP: Add latency value for Shairport RAOP server

After testing, Shairport server's latency is the same as Pioneer N30.
---
 src/modules/raop/module-raop-discover.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index 0a258785..aca5fa0c 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -145,6 +145,9 @@ static uint32_t guess_latency_from_device(const char *model) {
 if (pa_streq(model, "PIONEER,1")) {
 /* Pioneer N-30 */
 default_latency = 2352;
+} else if (pa_streq(model, "ShairportSync")) {
+/* Shairport - software AirPort server */
+default_latency = 2352;
 }
 
 pa_log_debug("Default latency is %u ms for device model %s.", default_latency, model);
-- 
2.11.0



pgpLlrt3jGLfT.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] RAOP: Init dummy port and card for latency change

[Colin Leroy]

Hi,

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.

-- 
Colin
From d4549b517930975d0c637fb685f6ae147018f8a4 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Fri, 6 Oct 2017 20:03:42 +0200
Subject: [PATCH 1/2] RAOP: Init dummy port and card for latency change

This adds a port, card and profile to RAOP sinks to make it
possible to change the latency at runtime (and have it persist)
using pavucontrol or pactl set-port-latency-offset.
---
 src/modules/raop/raop-sink.c | 110 +--
 1 file changed, 107 insertions(+), 3 deletions(-)

diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
index cd90e3d9..bb9c4556 100644
--- a/src/modules/raop/raop-sink.c
+++ b/src/modules/raop/raop-sink.c
@@ -69,6 +69,7 @@ struct userdata {
 pa_core *core;
 pa_module *module;
 pa_sink *sink;
+pa_card *card;
 
 pa_thread *thread;
 pa_thread_mq thread_mq;
@@ -122,9 +123,6 @@ static int64_t sink_get_latency(const struct userdata *u) {
 
 latency = pa_bytes_to_usec(u->write_count, >sink->sample_spec) - (int64_t) now;
 
-/* RAOP default latency */
-latency += u->latency * PA_USEC_PER_MSEC;
-
 return latency;
 }
 
@@ -465,6 +463,83 @@ finish:
 pa_log_debug("Thread shutting down");
 }
 
+static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
+return 0;
+}
+
+static pa_device_port *raop_create_port(struct userdata *u) {
+pa_device_port_new_data data;
+pa_device_port *port;
+
+pa_device_port_new_data_init();
+
+pa_device_port_new_data_set_name(, "network-output");
+pa_device_port_new_data_set_description(, _("Network output"));
+pa_device_port_new_data_set_direction(, PA_DIRECTION_OUTPUT);
+
+port = pa_device_port_new(u->core, , 0);
+
+pa_device_port_new_data_done();
+
+if (port == NULL)
+return NULL;
+
+pa_device_port_ref(port);
+/* init the latency_offset directly instead of using
+ * pa_device_port_set_latency_offset(), so that mode module-card-restore
+ * can do its job when PulseAudio restarts and the user-set latency
+ * is not overwritten.
+ */
+port->latency_offset = u->latency * PA_USEC_PER_MSEC;
+
+return port;
+}
+
+static pa_card_profile *raop_create_profile() {
+pa_card_profile *profile;
+
+profile = pa_card_profile_new("RAOP", _("RAOP standard profile"), 0);
+profile->priority = 10;
+profile->n_sinks = 1;
+profile->n_sources = 0;
+profile->max_sink_channels = 2;
+profile->max_source_channels = 0;
+
+return profile;
+}
+
+static pa_card *raop_create_card(pa_module *m, pa_device_port *port, pa_card_profile *profile, const char *server, const char *nicename) {
+pa_card_new_data data;
+pa_card *card;
+char *card_name;
+
+pa_card_new_data_init();
+
+card_name = pa_sprintf_malloc("raop_client.%s", server);
+
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, card_name);
+pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, nicename);
+data.driver = __FILE__;
+
+pa_card_new_data_set_name(, card_name);
+pa_xfree(card_name);
+
+pa_hashmap_put(data.ports, port->name, port);
+pa_hashmap_put(data.profiles, profile->name, profile);
+
+card = pa_card_new(m->core, );
+
+if (card == NULL)
+return NULL;
+
+pa_card_new_data_done();
+
+card->active_profile = profile;
+pa_card_put(card);
+
+return card;
+}
+
 pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 struct userdata *u = NULL;
 pa_sample_spec ss;
@@ -473,6 +548,9 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 const char /* *username, */ *password;
 pa_sink_new_data data;
 const char *name = NULL;
+const char *description = NULL;
+pa_device_port *port;
+pa_card_profile *profile;
 
 pa_assert(m);
 pa_assert(ma);
@@ -590,6 +668,28 @@ pa_sink* pa_raop_sink_new(pa_module *m, pa_modargs *ma, const char *driver) {
 goto fail;
 }
 
+port = raop_create_port(u);
+if (port == NULL) {
+pa_log("Failed to create port object");
+goto fail;
+}
+
+profile = raop_create_profile();
+pa_hashmap_put(port->profiles, profile->name, profile);
+
+description = pa_proplist_gets(data.proplist, PA_PROP_DEVICE_DESCRIPTION);
+if (description == NULL)
+description = server;
+
+u->card = raop_create_card(m, port, profile, server, description);
+if (u->card == NULL) {
+pa_log("Failed to create card object");
+goto fail;
+}
+
+data.card = u->card;
+pa_hashmap_put(data.ports, port->name, port);
+
 u->sink = pa_sin

Re: [pulseaudio-discuss] PulseAudio 11.1 - Airplay to Airport Express doesn't work

[Colin Leroy]

On Fri, 06 Oct 2017 14:00:55 +0100, John Clare  wrote:

> I assumed my Airport would be on an 192.168.0.* address, so I don’t
> know where 169.254.13.13 is coming from. Do you think this could be
> the problem then?

Yes, very probably. Check what your IP is on your Linux computer, and
try to set the Airport Express to a static unused IP on the same
network.

(I wonder how OS X manages to connect to it !)

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] PulseAudio 11.1 - Airplay to Airport Express doesn't work

[Colin Leroy]

On Fri, 06 Oct 2017 12:37:52 +0100, John Clare  wrote:

Hi John,

The log doesn't seem to indicate much wrong, apart from the obvious
> E: [pulseaudio] rtsp_client.c: Connection failed: Connection timed out

It seems your Airport Express's IP address is the "failed DHCP request"
address:

> D: [pulseaudio] rtsp_client.c: Attempting to connect to server
> '169.254.13.13:5000'

I guess your network is not on 169.254.0.0.

Hope this helps,
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] RAOP, pavucontrol and general "Latency offset"

[Colin Leroy]

Hello,

While working on the previously posted patch, I've noticed (it was
about time!) a little "Advanced" section in my internal sound card's
Output Device entry in pavucontrol. Clicking it I discovered a
latency offset setter.

I thought that would be ideal to have that in RAOP sinks too, and
started investigating why it's grayed out on the RAOP sink Output
device entry.

I discovered that the sink needs to have one or more "ports" for that,
and added one to raop-sink, which enabled the Advanced section in
the RAOP sink in pavucontrol.

But changing the setting did nothing, and after lots of gdb breakpoint
setting, I noticed that in the method DeviceWidget::onOffsetChange() of
pavucontrol, we return early because offsetButtonEnabled == false.

This is because offsetButtonEnabled is set to false when the
deviceWidget is constructed, and then set to true later when
DeviceWidget::setLatencyOffset() is called. It's called from 
mainwindow.cc::updatePorts() only if ports.find(w->activePort) !=
ports.end().

I'm not sure I understand correctly, but it looks like to me
setLatencyOffset() is only called if the sink has more than one port?

I would love if someone could enlighten me: did I understand the
pavucontrol code correctly? Is there a reason for that or does that
look like a bug? 

Ideally, if I manage to finish that patch, I'd plug the RAOP latency
offset into sink->port_latency_offset, and the value would show in
pavucontrol, and it would be changeable at runtime.

Thanks a lot in advance!
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] RAOP: Show device codename in sink description

[Colin Leroy]

Hi,

Tanu, here's the patch you were suggesting earlier.

This will make it easier for testers to report latency offset for their
specific hardware.

-- 
Colin
From 4ee8ecffad804f48ba38984299fc0a3289d56941 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Thu, 5 Oct 2017 21:21:48 +0200
Subject: [PATCH] RAOP: Show device codename in sink description

This will make it easier for testers to report latency offset for their
specific hardware.
---
 src/modules/raop/module-raop-discover.c | 38 +++--
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index 8b7ba49b..0a258785 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -253,13 +253,28 @@ static void resolver_cb(
 sr = pa_xstrdup(value);
 } else if (pa_streq(key, "am")) {
 /* Device model */
+char *internal_name;
 latency = guess_latency_from_device(value);
+
+internal_name = pa_escape(value, "\"'");
+
+if (nicename == NULL) {
+nicename = pa_xstrdup(internal_name);
+} else {
+t = nicename;
+nicename = pa_sprintf_malloc("%s (%s)", nicename, internal_name);
+pa_xfree(t);
+}
+pa_xfree(internal_name);
 }
 
 avahi_free(key);
 avahi_free(value);
 }
 
+if (nicename == NULL)
+nicename = pa_xstrdup("RAOP");
+
 if (device)
 dname = pa_sprintf_malloc("raop_output.%s.%s", host_name, device);
 else
@@ -282,22 +297,13 @@ static void resolver_cb(
 pa_xfree(dname);
 
 avahi_address_snprint(at, sizeof(at), a);
-if (nicename) {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s "
- "sink_properties='device.description=\"%s (%s:%u)\"'",
- at, port,
- vname,
- nicename, at, port);
-pa_xfree(nicename);
-} else {
-args = pa_sprintf_malloc("server=[%s]:%u "
- "sink_name=%s"
- "sink_properties='device.description=\"%s:%u\"'",
- at, port,
- vname,
- at, port);
-}
+args = pa_sprintf_malloc("server=[%s]:%u "
+ "sink_name=%s "
+ "sink_properties='device.description=\"%s [%s:%u]\"'",
+ at, port,
+ vname,
+ nicename, at, port);
+pa_xfree(nicename);
 
 if (tp != NULL) {
 t = args;
-- 
2.11.0



pgpg61KhSPR7y.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 05 October 2017 at 21h19, Tanu Kaskinen wrote:

Hi, 

> Yes, that seems like a fine idea. I'd like to first make the device
> model string easily available for testers. Digging it up from the
> verbose log isn't really user-friendly. It could be simply added to
> the raop sink proplist, so "pactl list sinks" would show it. Would you
> possibly have the time and motivation to implement that?

Sure, good idea! I'll keep you informed :)

-- 
Colin


pgpg85gTVFZIh.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On Thu, 21 Sep 2017 16:00:28 +0200, Colin Leroy <co...@colino.net>
wrote:

> > Thanks! Applied. I took the liberty to remove the
> > pa_modargs_get_value() from raop-sink.c, because it wasn't necessary
> > there.  
>
> Yay! Thank you for your guidance :)

Do you think it would be good to make a call for testers before 12.0,
so we can fill in a bit the guess_latency_from_device() function with
more hardware?

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On Thu, 21 Sep 2017 16:54:14 +0300, Tanu Kaskinen  wrote:

Hi,

>Thanks! Applied. I took the liberty to remove the
>pa_modargs_get_value() from raop-sink.c, because it wasn't necessary
>there.

Yay! Thank you for your guidance :)

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 18 September 2017 at 22h47, Tanu Kaskinen wrote:

Hi, 

> No, the function doesn't set the parameter when the argument is not
> present.

Oh, right, I misread it. I could have initialized the argument to an
arbitrary value and compare that after parsing, but that would be
uglier.

> Since the function returns zero both when a valid value is provided
> and when the argument is not set, you need to call
> pa_modargs_get_value() in order to know whether to set the
> latency_set flag or not, so it's not as useless as I first thought.

Indeed. Reworked that part a bit as I didn't need a temporary variable
in fact.

> If the argument is set, but parsing it fails, you should log an error
> message and fail. (Somehow it didn't occur to me to complain about
> this earlier.)

Thanks, done. 

-- 
Colin
From 6712ac7078659626516bf6f56fdfb64a35d8fcf5 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sun, 17 Sep 2017 20:46:49 +0200
Subject: [PATCH] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
Switch from pausing/resuming the smoother to resetting it because the
smoother got stuck returning the same value after an idle/running cycle,
making latency calculation wrong.
---
 src/modules/raop/module-raop-discover.c | 46 +++--
 src/modules/raop/module-raop-sink.c |  4 ++-
 src/modules/raop/raop-sink.c| 17 ++--
 src/modules/raop/raop-util.h|  2 ++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index d65615b2..8b7ba49b 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -44,11 +44,14 @@
 #include 
 
 #include "module-raop-discover-symdef.h"
+#include "raop-util.h"
 
 PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"latency_msec= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +64,13 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+
+bool latency_set;
+uint32_t latency;
 };
 
 static const char* const valid_modargs[] = {
+"latency_msec",
 NULL
 };
 
@@ -127,6 +134,23 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = RAOP_DEFAULT_LATENCY;
+
+if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352;
+}
+
+pa_log_debug("Default latency is %u ms for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +169,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = RAOP_DEFAULT_LATENCY;
 
 pa_assert(u);
 
@@ -226,6 +251,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +336,13 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->latency_set)
+latency = u->latency;
+
+t = args;
+args = pa_sprintf_malloc("%s latency_msec=%u", args, latency);
+pa_xfree(t);
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if (pa_module_load(, u->core, "module-raop-sink", args) >= 0) {
@@ -432,10 +467,17 @@ int pa__init(pa_module *m) {
 goto fail;
 }
 
-m->userdata = u = pa_xnew(struct userdata, 1);
+m->userdata = u = pa_xnew0(struct userdata, 1);
 u->core = m->core;
 u->module = m;
-u->sink_browser = NULL;
+
+if (pa_modargs_get_value(ma, "latency_msec", NULL) != NULL) {
+u->latency_set = true;
+if (pa_modargs_get_value_u32(ma, "latency_msec", >latency) < 0) {
+pa_log("Failed to parse latency_msec argument.");
+goto fail;
+}
+}
 
 u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
diff --git a/src/modules/raop/module-ra

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 17 September 2017 at 19h51, Tanu Kaskinen wrote:

Hi, 

> "git am" fails to apply the patch on top of the master branch (as it
> failed to apply the previous versions too, but I forgot to mention
> that earlier).

Sorry, I forgot to pull :) 

> The commit message could explain also why pa_smoother_reset() is used
> instead of pause()/resume().

Done.

> Why do you check the return value of pa_modargs_get_value() before
> calling pa_modargs_get_value_u32()? As far as I can tell, that makes
> no difference.

pa_modargs_get_value_u32() (and its friends) set the parameter to 0 if
it's not present, returning -1 only in case of integer conversion error.

> Nitpicking: the convention is to use the PA_USEC_PER_MSEC constant
> instead of bare "1000".

Fixed too :)

I attached an updated patch.

-- 
Colin
From ed8d46e28280dbb435d6c1f5a854e532b6e33dd1 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sun, 17 Sep 2017 20:46:49 +0200
Subject: [PATCH] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
Switch from pausing/resuming the smoother to resetting it because the
smoother got stuck returning the same value after an idle/running cycle,
making latency calculation wrong.
---
 src/modules/raop/module-raop-discover.c | 46 +++--
 src/modules/raop/module-raop-sink.c |  4 ++-
 src/modules/raop/raop-sink.c| 17 ++--
 src/modules/raop/raop-util.h|  2 ++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index d65615b2..37c2c6cf 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -44,11 +44,14 @@
 #include 
 
 #include "module-raop-discover-symdef.h"
+#include "raop-util.h"
 
 PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"latency_msec= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +64,13 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+
+bool latency_set;
+uint32_t latency;
 };
 
 static const char* const valid_modargs[] = {
+"latency_msec",
 NULL
 };
 
@@ -127,6 +134,23 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = RAOP_DEFAULT_LATENCY;
+
+if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352;
+}
+
+pa_log_debug("Default latency is %u ms for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +169,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = RAOP_DEFAULT_LATENCY;
 
 pa_assert(u);
 
@@ -226,6 +251,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +336,13 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->latency_set)
+	latency = u->latency;
+
+t = args;
+args = pa_sprintf_malloc("%s latency_msec=%u", args, latency);
+pa_xfree(t);
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if (pa_module_load(, u->core, "module-raop-sink", args) >= 0) {
@@ -426,16 +461,23 @@ int pa__init(pa_module *m) {
 struct userdata *u;
 pa_modargs *ma = NULL;
 int error;
+uint32_t l;
 
 if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
 pa_log("Failed to parse module arguments.");
 goto fail;
 }
 
-m->userdata = u = pa_xnew(struct userdata, 1);
+m->userdata = u = pa_xnew0(struct userdata, 1);
 u->core = m->core;
 u->module = m;
-u->sink_browser = NULL;
+
+if (pa_modargs_get_value(ma, "latency_msec", NULL) != NULL) {
+if (pa_modargs_get_value_u32(ma, "latency_msec", ) == 0) {
+u->latency_set = true;
+  

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 15 September 2017 at 15h44, Tanu Kaskinen wrote:

Hi, 

> [...]

Thanks for your detailed review. I reworked the patch to follow your
advices. In particular, I've made all latency parameter handling and
passing to use milliseconds, only use microseconds at the only place
needed.
For initializing the optional parameters to the default (2s) value if
not passed, I had to do a double if() to check for the parameter's
existence first. Nevertheless, it's cleaner than before.

Attached is an updated patch.

Thanks!
-- 
Colin
From ffda854fab9641a66eacdf7bdd812f41f2af65ce Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 22 Jul 2017 14:50:40 +0200
Subject: [PATCH] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
---
 src/modules/raop/module-raop-discover.c | 46 +++--
 src/modules/raop/module-raop-sink.c |  4 ++-
 src/modules/raop/raop-sink.c| 17 ++--
 src/modules/raop/raop-util.h|  2 ++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index c1becb17..a2083932 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -44,11 +44,14 @@
 #include 
 
 #include "module-raop-discover-symdef.h"
+#include "raop-util.h"
 
 PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"latency_msec= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +64,13 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+
+bool latency_set;
+uint32_t latency;
 };
 
 static const char* const valid_modargs[] = {
+"latency_msec",
 NULL
 };
 
@@ -127,6 +134,23 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = RAOP_DEFAULT_LATENCY;
+
+if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352;
+}
+
+pa_log_debug("Default latency is %u ms for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +169,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = RAOP_DEFAULT_LATENCY;
 
 pa_assert(u);
 
@@ -226,6 +251,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +336,13 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->latency_set)
+	latency = u->latency;
+
+t = args;
+args = pa_sprintf_malloc("%s latency_msec=%u", args, latency);
+pa_xfree(t);
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if (pa_module_load(, u->core, "module-raop-sink", args) == PA_OK) {
@@ -426,16 +461,23 @@ int pa__init(pa_module *m) {
 struct userdata *u;
 pa_modargs *ma = NULL;
 int error;
+uint32_t l;
 
 if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
 pa_log("Failed to parse module arguments.");
 goto fail;
 }
 
-m->userdata = u = pa_xnew(struct userdata, 1);
+m->userdata = u = pa_xnew0(struct userdata, 1);
 u->core = m->core;
 u->module = m;
-u->sink_browser = NULL;
+
+if (pa_modargs_get_value(ma, "latency_msec", NULL) != NULL) {
+if (pa_modargs_get_value_u32(ma, "latency_msec", ) == 0) {
+u->latency_set = true;
+u->latency = l;
+	}
+}
 
 u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
index 82fa48d9..d6826572 100644
--- a/src/modules/raop/module-raop-sink.c
+++ b/src/modules/raop/module-raop-sink.c
@@ -46,7 +46,8 @@ PA_MODULE_USAGE(
 "rate= "
 "channels= "
 "username= "
-"password=");
+"password= "
+

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 09 September 2017 at 20h44, Georg Chini wrote:

Hi, 

> pa_smoother_pause() and pa_smoother_resume() do not seem to
> work as expected from my experience. After a smoother_resume()
> the initial error is high again, so using pause()/resume() is no big
> advantage over reset().

Thanks a bunch to you too, Georg!

I figured out the problem (but not its root cause): after an
idle/running cycle, the smoother got stuck and returned always the same
value. I didn't figure out why. But resetting it before resuming it
fixed the problem :)

So here's the last iteration of the patch, which I now consider to work
well and would like to submit for inclusion (after addressing the
team's remarks of course).

Thanks again to both of you for your help :)
-- 
Colin
From e9ac9ae71f9ef134f69757bbe481983374c08b04 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 22 Jul 2017 14:50:40 +0200
Subject: [PATCH] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
---
 src/modules/raop/module-raop-discover.c | 42 +
 src/modules/raop/module-raop-sink.c |  4 +++-
 src/modules/raop/raop-sink.c| 12 ++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index c1becb17..7ec72dfd 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -49,6 +49,8 @@ PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"force_latency= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +63,11 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+uint32_t force_latency;
 };
 
 static const char* const valid_modargs[] = {
+"force_latency",
 NULL
 };
 
@@ -127,6 +131,25 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = 200;
+
+if (pa_streq(model, "AppleTV2,1")) {
+default_latency = 200;
+} else if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352000;
+}
+
+pa_log_debug("Default latency is %u for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +168,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = 0;
 
 pa_assert(u);
 
@@ -226,6 +250,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +335,16 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->force_latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, u->force_latency);
+pa_xfree(t);
+} else if (latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, latency);
+pa_xfree(t);
+}
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if (pa_module_load(, u->core, "module-raop-sink", args) == PA_OK) {
@@ -426,6 +463,7 @@ int pa__init(pa_module *m) {
 struct userdata *u;
 pa_modargs *ma = NULL;
 int error;
+uint32_t l;
 
 if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
 pa_log("Failed to parse module arguments.");
@@ -437,6 +475,10 @@ int pa__init(pa_module *m) {
 u->module = m;
 u->sink_browser = NULL;
 
+if (pa_modargs_get_value_u32(ma, "force_latency", ) == 0) {
+u->force_latency = l;
+}
+
 u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
 u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
index 82fa48d9..bc1d0d67 100644
--- a/src/modules/raop/module-raop-sink.c
+++ b/src/modules/raop/module-raop-sink.c
@@ -46,7 +46,8 @@ PA_MODULE_USAGE(
 "rate= "
 &q

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 09 September 2017 at 20h16, Tanu Kaskinen wrote:

Hi, 

> Well I definitely wrote things, but Evolution did act strange while
> composing the message, and my "sent" folder confirms your
> observation... I supposed I'll have to write it all again :(

Oh damn :( Bugs with user data loss suck. (OT: you should try Claws
Mail ;) )

> > I don't understand why, and if you have some pointers on how
> > pa_rtpoll and pa_smoother work, I guess that'd help.  
> 
> pa_rtpoll is just a mechanism for sleeping until the set timeout or
> [...]

Thanks a lot for taking the time to write this very detailed reply, I
think it'll help me :)

-- 
Colin


pgp3c1QXHi6h1.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 09 September 2017 at 18h07, Colin Leroy wrote:

Hi, 

> I don't understand why, and if you have some pointers on how pa_rtpoll
> and pa_smoother work, I guess that'd help.

So, instead of understanding what's wrong after a pause, for now I use
this instead.


-- 
Colin
From 09031f5e91e66502d379b41e3884bc26c331dfd1 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 22 Jul 2017 14:50:40 +0200
Subject: [PATCH] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
---
 src/modules/raop/module-raop-discover.c | 42 +
 src/modules/raop/module-raop-sink.c |  4 +++-
 src/modules/raop/raop-sink.c| 14 +++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index c1becb17..7ec72dfd 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -49,6 +49,8 @@ PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"force_latency= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +63,11 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+uint32_t force_latency;
 };
 
 static const char* const valid_modargs[] = {
+"force_latency",
 NULL
 };
 
@@ -127,6 +131,25 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = 200;
+
+if (pa_streq(model, "AppleTV2,1")) {
+default_latency = 200;
+} else if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352000;
+}
+
+pa_log_debug("Default latency is %u for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +168,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = 0;
 
 pa_assert(u);
 
@@ -226,6 +250,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +335,16 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->force_latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, u->force_latency);
+pa_xfree(t);
+} else if (latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, latency);
+pa_xfree(t);
+}
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if (pa_module_load(, u->core, "module-raop-sink", args) == PA_OK) {
@@ -426,6 +463,7 @@ int pa__init(pa_module *m) {
 struct userdata *u;
 pa_modargs *ma = NULL;
 int error;
+uint32_t l;
 
 if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
 pa_log("Failed to parse module arguments.");
@@ -437,6 +475,10 @@ int pa__init(pa_module *m) {
 u->module = m;
 u->sink_browser = NULL;
 
+if (pa_modargs_get_value_u32(ma, "force_latency", ) == 0) {
+u->force_latency = l;
+}
+
 u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
 u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
index 82fa48d9..bc1d0d67 100644
--- a/src/modules/raop/module-raop-sink.c
+++ b/src/modules/raop/module-raop-sink.c
@@ -46,7 +46,8 @@ PA_MODULE_USAGE(
 "rate= "
 "channels= "
 "username= "
-"password=");
+"password= "
+"latency=");
 
 static const char* const valid_modargs[] = {
 "name",
@@ -61,6 +62,7 @@ static const char* const valid_modargs[] = {
 "channels",
 "username",
 "password",
+"latency",
 NULL
 };
 
diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
index e5d219e8

Re: [pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

On 09 September 2017 at 18h00, Tanu Kaskinen wrote:

Hi Tanu,

Your mail's empty :)

Anyway, after this patch, there still is a bug left (which was not
visible with the previous patch where I did 

if (latency < 0) {
latency = 0;
}

in sink_get_latency().

The bug is that latency's all wrong (like, -10 to -5 seconds instead of
+2.3) after a pause (that is, after switching to PA_SINK_PA_SINK_IDLE
and then back to PA_SINK_RUNNING). At the first playback, it's fine.

I don't understand why, and if you have some pointers on how pa_rtpoll
and pa_smoother work, I guess that'd help.
-- 
Colin


pgpFKsu4I6qHm.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [Patch] RAOP: fix audio synchronisation, take two

[Colin Leroy]

Hi,

Following the discussion with Tanu, here's a second patch to announce
the correct latency in raop-sink.

This patch makes latency configurable in module-raop-sink, with a
default of 0 as it is right now.

It also makes module-raop-discover pass a latency= parameter to
module-raop-sink, and I made it so the latency is set depending on the
device model. Like USB VIDs/PIDs in the kernel, the best would be to
complete the list of device models/latencies when people try it on
other hardware.
I've set the latency to the correct value for my Pioneer N-30, and made
it so it returns 2s for Apple Airport Express. (I couldn't test Apple
hardware, though.)

Finally, I added a force_latency= parameter to module-raop-discover,
which, if set, is passed down to the module-raop-sinks it spawns when
it discovers AirPlay hardware, taking precedence over the
guess_latency_from_device() function.

Advice is welcome :)
-- 
Colin
From 84aff2424129a18b14d96b6fc4645428ea2739e0 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 22 Jul 2017 14:50:40 +0200
Subject: [PATCH 1/2] RAOP: Announce real latency

Use predefined values depending on the server, and make it configurable.
AirPlay is supposed to have 2s of latency. With my hardware, this is
more 2.352 seconds after numerous tests.
---
 src/modules/raop/module-raop-discover.c | 42 +
 src/modules/raop/module-raop-sink.c |  4 +++-
 src/modules/raop/raop-sink.c| 11 +
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/src/modules/raop/module-raop-discover.c b/src/modules/raop/module-raop-discover.c
index 9c7ac3cd..1a47f727 100644
--- a/src/modules/raop/module-raop-discover.c
+++ b/src/modules/raop/module-raop-discover.c
@@ -49,6 +49,8 @@ PA_MODULE_AUTHOR("Colin Guthrie");
 PA_MODULE_DESCRIPTION("mDNS/DNS-SD Service Discovery of RAOP devices");
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_USAGE(
+"force_latency= ");
 
 #define SERVICE_TYPE_SINK "_raop._tcp"
 
@@ -61,9 +63,11 @@ struct userdata {
 AvahiServiceBrowser *sink_browser;
 
 pa_hashmap *tunnels;
+uint32_t force_latency;
 };
 
 static const char* const valid_modargs[] = {
+"force_latency",
 NULL
 };
 
@@ -127,6 +131,25 @@ static void tunnel_free(struct tunnel *t) {
 pa_xfree(t);
 }
 
+/* This functions returns RAOP audio latency as guessed by the
+ * device model header.
+ * Feel free to complete the possible values after testing with
+ * your hardware.
+ */
+static uint32_t guess_latency_from_device(const char *model) {
+uint32_t default_latency = 200;
+
+if (pa_streq(model, "AppleTV2,1")) {
+default_latency = 200;
+} else if (pa_streq(model, "PIONEER,1")) {
+/* Pioneer N-30 */
+default_latency = 2352000;
+}
+
+pa_log_debug("Default latency is %u for device model %s.", default_latency, model);
+return default_latency;
+}
+
 static void resolver_cb(
 AvahiServiceResolver *r,
 AvahiIfIndex interface, AvahiProtocol protocol,
@@ -145,6 +168,7 @@ static void resolver_cb(
 char at[AVAHI_ADDRESS_STR_MAX];
 AvahiStringList *l;
 pa_module *m;
+uint32_t latency = 0;
 
 pa_assert(u);
 
@@ -226,6 +250,9 @@ static void resolver_cb(
 /* Sample rate */
 pa_xfree(sr);
 sr = pa_xstrdup(value);
+} else if (pa_streq(key, "am")) {
+/* Device model */
+latency = guess_latency_from_device(value);
 }
 
 avahi_free(key);
@@ -308,6 +335,16 @@ static void resolver_cb(
 pa_xfree(t);
 }
 
+if (u->force_latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, u->force_latency);
+pa_xfree(t);
+} else if (latency > 0) {
+t = args;
+args = pa_sprintf_malloc("%s latency=%u", args, latency);
+pa_xfree(t);
+}
+
 pa_log_debug("Loading module-raop-sink with arguments '%s'", args);
 
 if ((m = pa_module_load(u->core, "module-raop-sink", args))) {
@@ -426,6 +463,7 @@ int pa__init(pa_module *m) {
 struct userdata *u;
 pa_modargs *ma = NULL;
 int error;
+uint32_t l;
 
 if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
 pa_log("Failed to parse module arguments.");
@@ -437,6 +475,10 @@ int pa__init(pa_module *m) {
 u->module = m;
 u->sink_browser = NULL;
 
+if (pa_modargs_get_value_u32(ma, "force_latency", ) == 0) {
+u->force_latency = l;
+}
+
 u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
 u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
index 82fa48d9..bc1d0d67 100644
--- 

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 07 September 2017 at 14h54, Tanu Kaskinen wrote:

Hi, 

> A parameter for the latency constant would be nice indeed.

There's also an header, Audio-Latency, in Airplay protocol, which is
not documented in the unofficial doc.

Some servers send 2205, mine sends 4096, it seems (from the internet)
that other values are possible, but I can't make sense of them (and
apparently, no one did yet on internet.)

That annoys me because I think it's key to a perfect patch instead of
hardcoding :/

If anyone has ideas about it...
-- 
Colin


pgp1riWrlT6B7.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 06 September 2017 at 14h59, Tanu Kaskinen wrote:

Hi Tanu, 

> > +    latency += 200; /* RAOP default latency */
> > +    latency += pa_raop_client_get_rtp_diff(u->raop) * 1000; /*
> > plus last packet length in usec */  
> 
> pa_raop_client_get_rtp_diff() returns the size of the last audio chunk
> in bytes divided by four, but you treat that as a time value. That's a
> clear bug.

You may well be right! I'm not quite used to the RTP protocol, but it
seemed to me that rtptime is fed into the 'timestamp' part of the
RTP header and that it represented a time - from what I gathered from
the spec.

I derived rtpdiff from the previous way of incrementing rtptime:

> -c->rtptime += length / 4;
> +c->rtpdiff = length / 4;
> +c->rtptime += c->rtpdiff;

it is indeed incremented in a way that I don't understand, though, so
I'm not sure about that.

> Adding the size of the last packet doesn't make sense to me anyway,
> though. The size of the last packet is already taken into account in
> u->write_count. Now you're counting it twice.  

My reasoning was that the "last" (I should have commented "current",
maybe) packet doesn't get played before the next one is received, in
which case it makes sense that it adds an "packet duration" latency.

I did find the audio to be better synchronised to the video with this
patch, than with stock PulseAudio and shifting audio -2.3s in mplayer, 
which is what made me think that calculation made sense.

> If the 2 second constant results in too low latency, I think the
> constant should be made bigger.

If this seems too much like guesswork to you, I can understand that and
change the patch to set the latency empirically to a bigger value
(around 2.3 seconds). I can also, if we go this route, set the default
latency empirically and add a parameter to the module to allow changing
it.

One way or another is great anyway, compared to having to shift audio
in the media player (and being left with unsynced audio on Youtube :) ).

Thanks for your help :)
-- 
Colin


pgpYROi_y_R0P.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 06 September 2017 at 08h35, Alexander E. Patrakov wrote:

Hi, 

> That's with the default settings. Please read about the
> --video-sync=... option of mpv.

All methods work great, apart 'desync' of course where the sound and
the video are out of sync, but audio still plays.

-- 
Colin


pgpXfO4GdYTTM.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On Wed, 6 Sep 2017 08:35:37 +0500, "Alexander E. Patrakov"
 wrote:

Hi,

>> Basically it seems only VLC tries to sync audio to video instead of
>> the contrary :)  
>
>That's with the default settings. Please read about the
>--video-sync=... option of mpv.

Oh, I wasn't aware of this option. I'll test it this evening.
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 05 September 2017 at 16h50, Tanu Kaskinen wrote:

Hi, 

> Sorry for not responding earlier. I intended to reply to this mail,
> but I accidentally marked it as read and forgot about it.
> 
> It's VLC's problem if it can't handle big latencies. VLC will be quite
> useless anyway regardless of what pulseaudio does, because if we
> report too small latencies, then audio and video will be badly out of
> sync.
> 
> Does VLC work with big latencies when playing audio-only content?

No, it doesn't either - the same codepath seems to be used and it
complains that "playback is late" and flushing buffers to try and fix
it.

I made some more tests using all software I have installed, though
(some of them probably sharing libraries) :

- Audacity works
- mPlayer and mpv work
- Parole works
- Totem works
- Rhythmbox works

Basically it seems only VLC tries to sync audio to video instead of the
contrary :)
-- 
Colin


pgp72RisvjDOx.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Don't abort on double module load

[Colin Leroy]

On Wed, 30 Aug 2017 18:30:25 +0300, Tanu Kaskinen  wrote:

Hi,

>Thanks! I applied the patch to the "next" branch with some
>modifications: even though the pa_error_code_t type exists, errors are
>still reported as unsigned ints. I also added some more explanation to
>the commit message.

Thanks Tanu :)
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Don't abort on double module load

[Colin Leroy]

On 27 August 2017 at 19h07, Colin Leroy wrote:

Hi, 

> > The solution seems good to me. Just a couple of minor issues:  
> 
> Here's a patch addressing your remarks. I squashed the two commits in
> one for clarity.

He, sorry, I changed the return code of two pa__init() functions by
mistake. Please add 0002-... to the previous one - or use 0001-...v2
instead of the patch in the previous mail.

Thanks!
-- 
Colin
From 58e25796220c317acc412516d44d320bc19e9931 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sun, 27 Aug 2017 22:44:07 +0200
Subject: [PATCH] Simple: Fix return code on pa__init() calls

---
 src/modules/module-combine.c| 2 +-
 src/modules/module-volume-restore.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/modules/module-combine.c b/src/modules/module-combine.c
index ad1adab8..df002d3a 100644
--- a/src/modules/module-combine.c
+++ b/src/modules/module-combine.c
@@ -52,7 +52,7 @@ int pa__init(pa_module*m) {
 err = pa_module_load(, m->core, "module-combine-sink", m->argument);
 u->module_index = module ? module->index : PA_INVALID_INDEX;
 
-return err;
+return (err == PA_OK ? 0 : -1);
 }
 
 void pa__done(pa_module*m) {
diff --git a/src/modules/module-volume-restore.c b/src/modules/module-volume-restore.c
index cf271b1b..9ae229cd 100644
--- a/src/modules/module-volume-restore.c
+++ b/src/modules/module-volume-restore.c
@@ -74,7 +74,7 @@ int pa__init(pa_module*m) {
 
 pa_modargs_free(ma);
 
-return err;
+return (err == PA_OK ? 0 : -1);
 
 fail:
 if (ma)
-- 
2.11.0

From c8a241fcb7e1e5a1b1a5a9935e1805a0348ba994 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 26 Aug 2017 11:21:15 +0200
Subject: [PATCH] CLI: Don't exit on "module already loaded" errors.

Some modules may only be loaded once, and trying to load them
twice currently aborts PulseAudio.
This patch adds a way to differentiate module load errors, and
make cli-command ignore the error when the module is already
loaded.
---
 src/daemon/main.c |  8 +---
 src/modules/bluetooth/module-bluetooth-discover.c |  4 ++--
 src/modules/bluetooth/module-bluetooth-policy.c   |  6 --
 src/modules/bluetooth/module-bluez4-discover.c|  2 +-
 src/modules/bluetooth/module-bluez5-discover.c|  2 +-
 src/modules/dbus/iface-core.c |  2 +-
 src/modules/gconf/module-gconf.c  |  2 +-
 src/modules/jack/module-jackdbus-detect.c |  2 +-
 src/modules/macosx/module-coreaudio-detect.c  |  2 +-
 src/modules/module-allow-passthrough.c|  2 +-
 src/modules/module-always-sink.c  |  2 +-
 src/modules/module-combine.c  |  5 +++--
 src/modules/module-detect.c   | 14 +-
 src/modules/module-filter-apply.c |  2 +-
 src/modules/module-hal-detect-compat.c|  2 +-
 src/modules/module-udev-detect.c  |  2 +-
 src/modules/module-volume-restore.c   |  5 +++--
 src/modules/module-zeroconf-discover.c|  2 +-
 src/modules/raop/module-raop-discover.c   |  2 +-
 src/pulsecore/cli-command.c   | 12 +---
 src/pulsecore/module.c| 20 
 src/pulsecore/module.h|  2 +-
 src/pulsecore/protocol-native.c   |  2 +-
 23 files changed, 66 insertions(+), 38 deletions(-)

diff --git a/src/daemon/main.c b/src/daemon/main.c
index f35252d0..c109c668 100644
--- a/src/daemon/main.c
+++ b/src/daemon/main.c
@@ -111,19 +111,21 @@ int deny_severity = LOG_WARNING;
 int __padsp_disabled__ = 7;
 #endif
 
-static void signal_callback(pa_mainloop_api*m, pa_signal_event *e, int sig, void *userdata) {
+static void signal_callback(pa_mainloop_api* m, pa_signal_event *e, int sig, void *userdata) {
+pa_module *module = NULL;
+
 pa_log_info("Got signal %s.", pa_sig2str(sig));
 
 switch (sig) {
 #ifdef SIGUSR1
 case SIGUSR1:
-pa_module_load(userdata, "module-cli", NULL);
+pa_module_load(, userdata, "module-cli", NULL);
 break;
 #endif
 
 #ifdef SIGUSR2
 case SIGUSR2:
-pa_module_load(userdata, "module-cli-protocol-unix", NULL);
+pa_module_load(, userdata, "module-cli-protocol-unix", NULL);
 break;
 #endif
 
diff --git a/src/modules/bluetooth/module-bluetooth-discover.c b/src/modules/bluetooth/module-bluetooth-discover.c
index d69a77f1..509db443 100644
--- a/src/modules/bluetooth/module-bluetooth-discover.c
+++ b/src/modules/bluetooth/module-bluetooth-discover.c
@@ -52,13 +52,13 @@ int pa__init(pa_module* m) {
 u->bluez4_module_idx = PA_INVALID_INDEX;
 
 if (pa_module_exists("module-bluez5-discover")) {
-mm = pa_module_load

Re: [pulseaudio-discuss] [PATCH] Don't abort on double module load

[Colin Leroy]

On 27 August 2017 at 12h56, Tanu Kaskinen wrote:

Hi Tanu, 

> The solution seems good to me. Just a couple of minor issues:

Here's a patch addressing your remarks. I squashed the two commits in
one for clarity.

Hope it helps :)
-- 
Colin
From 9037b44ebd3bf626185cb50e2391212cf1bb3f50 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 26 Aug 2017 11:21:15 +0200
Subject: [PATCH] CLI: Don't exit on "module already loaded" errors.

Some modules may only be loaded once, and trying to load them
twice currently aborts PulseAudio.
This patch adds a way to differentiate module load errors, and
make cli-command ignore the error when the module is already
loaded.
---
 src/daemon/main.c |  8 +---
 src/modules/bluetooth/module-bluetooth-discover.c |  4 ++--
 src/modules/bluetooth/module-bluetooth-policy.c   |  6 --
 src/modules/bluetooth/module-bluez4-discover.c|  2 +-
 src/modules/bluetooth/module-bluez5-discover.c|  2 +-
 src/modules/dbus/iface-core.c |  2 +-
 src/modules/gconf/module-gconf.c  |  2 +-
 src/modules/jack/module-jackdbus-detect.c |  2 +-
 src/modules/macosx/module-coreaudio-detect.c  |  2 +-
 src/modules/module-allow-passthrough.c|  2 +-
 src/modules/module-always-sink.c  |  2 +-
 src/modules/module-combine.c  |  5 +++--
 src/modules/module-detect.c   | 14 +-
 src/modules/module-filter-apply.c |  2 +-
 src/modules/module-hal-detect-compat.c|  2 +-
 src/modules/module-udev-detect.c  |  2 +-
 src/modules/module-volume-restore.c   |  5 +++--
 src/modules/module-zeroconf-discover.c|  2 +-
 src/modules/raop/module-raop-discover.c   |  2 +-
 src/pulsecore/cli-command.c   | 12 +---
 src/pulsecore/module.c| 20 
 src/pulsecore/module.h|  2 +-
 src/pulsecore/protocol-native.c   |  2 +-
 23 files changed, 66 insertions(+), 38 deletions(-)

diff --git a/src/daemon/main.c b/src/daemon/main.c
index f35252d0..c109c668 100644
--- a/src/daemon/main.c
+++ b/src/daemon/main.c
@@ -111,19 +111,21 @@ int deny_severity = LOG_WARNING;
 int __padsp_disabled__ = 7;
 #endif
 
-static void signal_callback(pa_mainloop_api*m, pa_signal_event *e, int sig, void *userdata) {
+static void signal_callback(pa_mainloop_api* m, pa_signal_event *e, int sig, void *userdata) {
+pa_module *module = NULL;
+
 pa_log_info("Got signal %s.", pa_sig2str(sig));
 
 switch (sig) {
 #ifdef SIGUSR1
 case SIGUSR1:
-pa_module_load(userdata, "module-cli", NULL);
+pa_module_load(, userdata, "module-cli", NULL);
 break;
 #endif
 
 #ifdef SIGUSR2
 case SIGUSR2:
-pa_module_load(userdata, "module-cli-protocol-unix", NULL);
+pa_module_load(, userdata, "module-cli-protocol-unix", NULL);
 break;
 #endif
 
diff --git a/src/modules/bluetooth/module-bluetooth-discover.c b/src/modules/bluetooth/module-bluetooth-discover.c
index d69a77f1..509db443 100644
--- a/src/modules/bluetooth/module-bluetooth-discover.c
+++ b/src/modules/bluetooth/module-bluetooth-discover.c
@@ -52,13 +52,13 @@ int pa__init(pa_module* m) {
 u->bluez4_module_idx = PA_INVALID_INDEX;
 
 if (pa_module_exists("module-bluez5-discover")) {
-mm = pa_module_load(m->core, "module-bluez5-discover", m->argument);
+pa_module_load(, m->core, "module-bluez5-discover", m->argument);
 if (mm)
 u->bluez5_module_idx = mm->index;
 }
 
 if (pa_module_exists("module-bluez4-discover")) {
-mm = pa_module_load(m->core, "module-bluez4-discover",  NULL);
+pa_module_load(, m->core, "module-bluez4-discover",  NULL);
 if (mm)
 u->bluez4_module_idx = mm->index;
 }
diff --git a/src/modules/bluetooth/module-bluetooth-policy.c b/src/modules/bluetooth/module-bluetooth-policy.c
index 316b9a82..24930450 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -71,6 +71,7 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
 const char *s;
 const char *role;
 char *args;
+pa_module *m = NULL;
 
 pa_assert(c);
 pa_assert(source);
@@ -100,7 +101,7 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
 /* Load module-loopback */
 args = pa_sprintf_malloc("source=\"%s\" source_dont_move=\"true\" sink_input_properties=\"media.role=%s\"", source->name,
  role);
-(void) pa_module_

Re: [pulseaudio-discuss] [PATCH] Don't abort on double module load

[Colin Leroy]

On 27 August 2017 at 12h56, Tanu Kaskinen wrote:

Hi Tanu, 

Thank you for your review :)

> We don't have a whole lot of functions that need to both allocate a
> new object and report the reason on failure, but there is some
> precedent. At least pa_sink_input_new() and pa_source_output_new()
> are this kind of functions, and they return the two things
> differently than pa_module_load() in your patch. I think we should be
> consistent, so pa_module_load() should return the error code as the
> function return value, and the first argument of pa_module_load()
> should be a pointer to a pa_module pointer.

I hesitated between both methods, should have chosen the other one :)

> The error codes should be from the pa_error_code_t enumeration.

OK, great !

> pa_module_load() starts with this:
> 
> errcode = -EIO; /* default error */
> 
> However, you always set errcode explicitly in every error case (which
> is good), so this line is not needed, and it will also prevent the
> compiler from issuing a warning if setting errcode is forgotten when
> adding a new error case.

Indeed.

I'll cook up a new patch and send it back quite soon!
-- 
Colin


pgpJszKv03ta4.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] Don't abort on double module load

[Colin Leroy]

Hello,

I recently had to investigate quite a bit why PulseAudio started to
abort on my computer, and finally figured out the reason :

- I had "module-load module-raop-discover" set in /etc/pulse/default.pa
- I fiddled with `paprefs` while PA was running and checked "Make
  discoverable Apple AirTunes sound devices available locally"
- This triggered a load of module-raop-discover via module-gconf, which
  failed silently because the module was already loaded:

D: [pulseaudio] module-gconf.c: Loading module 'module-raop-discover'
  with args '' due to GConf configuration.
E: [pulseaudio] module.c: Module "module-raop-discover" should be
  loaded once at most. Refusing to load.
E: [pulseaudio] module-gconf.c: pa_module_load() failed

- At the next reboot, module-raop-discover was loaded via module-gconf,
  then cli-command tried to load it again, failed and PulseAudio exited.

The attached patch fixes it by adding an optional error code pointer to
pa_module_load(), and checking for the error code in cli-command.

I don't know if this is an acceptable solution for the PulseAudio team,
I'm not even sure that's a case you want to handle, but I thought it
would be worth it to mention this problem to you, gather your input
on the subject and come up with an idea to handle it.

Thanks,
-- 
Colin
From 7e82e6e4367238b211c932c5baf98a69a268 Mon Sep 17 00:00:00 2001
From: Colin Leroy <co...@colino.net>
Date: Sat, 26 Aug 2017 11:21:15 +0200
Subject: [PATCH] CLI: Don't exit on "module already loaded" errors.

Some modules may only be loaded once, and trying to load them
twice currently aborts PulseAudio.
This patch adds a way to differentiate module load errors, and
make cli-command ignore the error when the module is already
loaded.
---
 src/daemon/main.c |  4 ++--
 src/modules/bluetooth/module-bluetooth-discover.c |  4 ++--
 src/modules/bluetooth/module-bluetooth-policy.c   |  4 ++--
 src/modules/bluetooth/module-bluez4-discover.c|  2 +-
 src/modules/bluetooth/module-bluez5-discover.c|  2 +-
 src/modules/dbus/iface-core.c |  2 +-
 src/modules/gconf/module-gconf.c  |  2 +-
 src/modules/jack/module-jackdbus-detect.c |  2 +-
 src/modules/macosx/module-coreaudio-detect.c  |  2 +-
 src/modules/module-allow-passthrough.c|  2 +-
 src/modules/module-always-sink.c  |  2 +-
 src/modules/module-combine.c  |  2 +-
 src/modules/module-detect.c   | 10 +-
 src/modules/module-filter-apply.c |  2 +-
 src/modules/module-hal-detect-compat.c|  2 +-
 src/modules/module-udev-detect.c  |  2 +-
 src/modules/module-volume-restore.c   |  2 +-
 src/modules/module-zeroconf-discover.c|  2 +-
 src/modules/raop/module-raop-discover.c   |  2 +-
 src/pulsecore/cli-command.c   | 11 ---
 src/pulsecore/module.c| 19 +--
 src/pulsecore/module.h|  2 +-
 src/pulsecore/protocol-native.c   |  2 +-
 23 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/src/daemon/main.c b/src/daemon/main.c
index f35252d0..f81dffb9 100644
--- a/src/daemon/main.c
+++ b/src/daemon/main.c
@@ -117,13 +117,13 @@ static void signal_callback(pa_mainloop_api*m, pa_signal_event *e, int sig, void
 switch (sig) {
 #ifdef SIGUSR1
 case SIGUSR1:
-pa_module_load(userdata, "module-cli", NULL);
+pa_module_load(userdata, "module-cli", NULL, NULL);
 break;
 #endif
 
 #ifdef SIGUSR2
 case SIGUSR2:
-pa_module_load(userdata, "module-cli-protocol-unix", NULL);
+pa_module_load(userdata, "module-cli-protocol-unix", NULL, NULL);
 break;
 #endif
 
diff --git a/src/modules/bluetooth/module-bluetooth-discover.c b/src/modules/bluetooth/module-bluetooth-discover.c
index d69a77f1..f99d6af5 100644
--- a/src/modules/bluetooth/module-bluetooth-discover.c
+++ b/src/modules/bluetooth/module-bluetooth-discover.c
@@ -52,13 +52,13 @@ int pa__init(pa_module* m) {
 u->bluez4_module_idx = PA_INVALID_INDEX;
 
 if (pa_module_exists("module-bluez5-discover")) {
-mm = pa_module_load(m->core, "module-bluez5-discover", m->argument);
+mm = pa_module_load(m->core, "module-bluez5-discover", m->argument, NULL);
 if (mm)
 u->bluez5_module_idx = mm->index;
 }
 
 if (pa_module_exists("module-bluez4-discover")) {
-mm = pa_module_load(m->core, "module-bluez4-discover",  NULL);
+mm = pa_module_load(m->core, "module-bluez4-discover",  NULL, NULL);
 if (mm)
 u->bluez4_module_idx = mm->index;
 }
diff --git a/src/

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 05 August 2017 at 10h24, Colin Leroy wrote:

Hi, 

> > I think I've figured out how to cleanly synchronise audio and video
> > with RAOP with relying on my empirical "shifting audio 2300ms in
> > mplayer works".  
> 
> Hmmm, weirdly enough, that patch breaks VLC as I've just discovered,

After investigating VLC's problem, it seems to be due to the fact they
sync audio to video and not the other way around like most apps do.
It's probably not an easy fix for me to do in VLC.

What do you think we should do with that raop latency patch ? Should we
consider it a problem that it breaks some apps, or that it's the app's
problem that they can't handle a big latency ?

-- 
Colin


pgp1Ubnp8osz3.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 22 July 2017 at 14h53, Colin Leroy wrote:

Hi, 

> I think I've figured out how to cleanly synchronise audio and video
> with RAOP with relying on my empirical "shifting audio 2300ms in
> mplayer works".

Hmmm, weirdly enough, that patch breaks VLC as I've just discovered,
with PA logs full of those. I'd love if anybody has any idea why this
happens and how to fix it, because now I'm addicted to not having to
the comfort of having synced audio on mplayer and my browser !

D: [pulseaudio] rtsp_client.c: Full response received. Dispatching
D: [pulseaudio] raop-client.c: RAOP: FLUSHED
D: [raop-sink-[192.168.1.10]:1032] protocol-native.c: Implicit underrun
of 'audio stream' D: [raop-sink-[192.168.1.10]:1032] protocol-native.c:
Requesting rewind due to rewrite. D: [raop-sink-[192.168.1.10]:1032]
sink-input.c: Have to rewind 16 bytes on implementor. D:
[raop-sink-[192.168.1.10]:1032] sink-input.c: Requesting rewind due to
corking D: [raop-sink-[192.168.1.10]:1032] raop-sink.c: RAOP: IDLE D:
[raop-sink-[192.168.1.10]:1032] rtsp_client.c: Sending command: FLUSH
D: [raop-sink-[192.168.1.10]:1032] sink-input.c: Requesting rewind due
to uncorking D: [pulseaudio] module-suspend-on-idle.c: Sink
raop_output.N-30.local becomes busy, resuming. D:
[raop-sink-[192.168.1.10]:1032] raop-sink.c: RAOP: RUNNING D:
[raop-sink-[192.168.1.10]:1032] sink-input.c: Requesting rewind due to
corking D: [raop-sink-[192.168.1.10]:1032] raop-sink.c: RAOP: IDLE D:
[raop-sink-[192.168.1.10]:1032] rtsp_client.c: Sending command: FLUSH
D: [raop-sink-[192.168.1.10]:1032] sink-input.c: Requesting rewind due
to uncorking D: [pulseaudio] module-suspend-on-idle.c: Sink
raop_output.N-30.local becomes busy, resuming.

-- 
Colin


pgp0UgF5DZcRm.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 25 July 2017 at 22h49, Hajime Fujita wrote:

Hi Hajime, 

> I haven’t had a chance to actually test the patch, but just skimmed
> it. The idea seems reasonable. Will try to see if I can test it this
> weekend.

Have you been able to test ? :)

-- 
Colin


pgp2sxBRY5JCD.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Colin Leroy]

On 22 July 2017 at 14h53, Colin Leroy wrote:

Hi, 

> I think I've figured out how to cleanly synchronise audio and video
> with RAOP with relying on my empirical "shifting audio 2300ms in
> mplayer works".

*without*relying on, obviously :)

-- 
Colin


pgpXjMhc_qoTC.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v7 00/33] raop2 support for module-raop-sink

[Colin Leroy]

On Fri, 13 Jan 2017 04:00:11 +, Toby Haynes 
wrote:

> So glad to hear this is landing in the main branch, having run
> Hajime's code for at least a couple of years.
> 
> Thanks to everyone involved in getting this landed.

Oh that's great ! I'll be happy to see it shipped in the distros !
:)

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v7 00/33] raop2 support for module-raop-sink

[Colin Leroy]

On Thu, 5 Jan 2017 22:34:29 +0100, Anton Lundin 
wrote:

> I've tested and glanced at the raop-code and it ain't perfect but its
> way better than the current code for anything modern, so I'd suggest
> merging this.

Yay :) That's good news !

-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Colin Leroy]

On 24 October 2016 at 20h58, Tanu Kaskinen wrote:

Hi, 

> It would be great to have more people reviewing patches, but I don't
> know how to acquire those people. I don't think the reviews have to
> necessarily be done by someone with a title of "maintainer", but on
> the other hand, giving much trust to drive-by contributors seems risky
> too...

Reviewing is hard when you don't have an extensive knowledge of the
codebase... I'd propose to do it, but I'd suck at it - "yeah, seems
fine to me" :D

-- 
Colin


pgp7e38WNLNgR.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Colin Leroy]

Hello everybody, Hajime,

> I truly understand that there's a huge shortage in manpower to review
> the patches, and that raop is not a high priority thing in the
> project.

Thanks a lot, again, for your persistence with this patchset :) I still
use it daily and with no adverse effect rebased against PA 9.0 if I
remember correctly.

I would love to help getting this upstreamed but I don't really know
what more I could do?

Thanks,
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] only ALAC! [Was: raop on gentoo+systemd]

[Colin Leroy]

On 12 November 2014 at 21h17, Martin Blanchard wrote:

Hi, 

 RAOP has a ~2sec latency by design. Anyway, the current pulseaudio
 sink does not correctly report this latency. This results in
 synchronization problem between player (mpv here) and output stream.
 This is a known issue; mpv is not to be blamed here, and your network
 stack is probably not either...

I'm using +2300ms A-V delay on mplayer to counteract that latency and
it's very close to perfect sync to be very enjoyable.

-- 
Colin


pgp_N1Co8PqLr.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Winter is Coming...

[Colin Leroy]

On Mon, 10 Nov 2014 15:32:15 +0530, Arun Raghavan a...@accosted.net
wrote:

 I'd like to freeze master for 6.0 in a couple of days (tentatively
 morning of Nov 13th). If you have any patches pending that you'd like
 included for the next release, please scream and shout now.

RAOP2!! That would be great wouldn't it?

 Ideally, I'd like to avoid adding anything non-obvious/trivial at this
 point 

OK I guess that doesn't fit this description then. 

I'd love to have this patchset reviewed sometime though :)
-- 
Colin


pgpBIu0VjW8bX.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

[re-sending to the list]

On 28 September 2014 at 13h47, Tanu Kaskinen wrote:

Hi, 

   Subject: [PATCH] raop: Remove unimplemented code (PCM and AAC)
  
  Thanks, applied  pushed to my branch.
 
 Note that the patch review status page[1] refers to a fixed set of
 commits to make it clear what has been submitted for review.  

Ah, thanks, so indeed, here's the complete set of patches that I'd like
reviewed:

https://github.com/colinleroy/pulseaudio/compare/e521d387...1a410ad

I know it's rather a lot :)

Thanks!
-- 
Colin


pgp1PcznNber8.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

On Thu, 25 Sep 2014 17:25:58 +0200, Martin Blanchard tch...@gmx.com
wrote:

Hi Martin,

 Subject: [PATCH] raop: Remove unimplemented code (PCM and AAC)

Thanks, applied  pushed to my branch.
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Pulseaudio's RAOP2 support

[Colin Leroy]

On Thu, 18 Sep 2014 14:28:12 +0530, Arun Raghavan a...@accosted.net
wrote:

Hi,

 So what's needed to start looking at pushing this upstream? I don't
 have any ROAP devices to test with, but can try to help with reviews
 (and maybe hunt for something that can do ROAP ... XBMC?).

I just have to brace myself and send the patchset for review!

I'll probably do it today!
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Pulseaudio's RAOP2 support

[Colin Leroy]

On Thu, 18 Sep 2014 11:10:30 +0200, Colin Leroy co...@colino.net
wrote:

  So what's needed to start looking at pushing this upstream? I don't
  have any ROAP devices to test with, but can try to help with reviews
  (and maybe hunt for something that can do ROAP ... XBMC?).  
 
 I just have to brace myself and send the patchset for review!

There are 55 of them, is it OK to send them here?

Thanks in advance,
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

On Thu, 18 Sep 2014 16:43:05 +0530, Arun Raghavan a...@accosted.net
wrote:

 On 18 Sep 2014 15:35, Colin Leroy co...@colino.net wrote:
 
  On Thu, 18 Sep 2014 11:10:30 +0200, Colin Leroy co...@colino.net
  wrote:
 
So what's needed to start looking at pushing this upstream? I
don't have any ROAP devices to test with, but can try to help
with reviews (and maybe hunt for something that can do ROAP ...
XBMC?).
  
   I just have to brace myself and send the patchset for review!
 
  There are 55 of them, is it OK to send them here?
 
 That would get messy quickly. A link to a branch would work better.
 If it's already on GitHub, we could just use the review mechanism
 there for simplicity.

Thanks. I hope I've done that well; my first repository wasn't ideally
planned (with everything in master), I've made a new one.

The branch is raop2 at https://github.com/colinleroy/pulseaudio

You can see all commits there:

https://github.com/colinleroy/pulseaudio/compare/pulseaudio:master...raop2

Thanks in advance.
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

On Thu, 18 Sep 2014 19:17:53 +0600, Alexander E. Patrakov
patra...@gmail.com wrote:

 I had a look at the diff between the master and the end result. Too
 much whitespace changes and other non-functional changes (e.g. I
 don't understand why the definition of struct userdata has to be
 moved up in odule-raop-discover.c). I have not looked at the
 individual commits.

Most of the whitespace changes are actually extra whitespace removal. Do
you think I should revert those? I agree that they make the diff
longer, but that seems like wasting the time of the people who did
these fixes...

 Also, you have some unimplemented stuff like AAC encoding. Could
 you please remove it? We can always use ALAC, and already have a 
 presumably-working standalone encoder (that just slaps a header
 with an is-not-compressed bit) implementation in write_ALAC_data().

Sure.

-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

On Thu, 18 Sep 2014 19:37:49 +0600, Alexander E. Patrakov
patra...@gmail.com wrote:

 Well, if you prefer to have them, then OK, let's have them (and only 
 them) at the beginning and only functional changes at the end. If
 some patch adds badly-formatted code only to be fixed up later,
 please squash the formatting fixup into the original patch.
 
   
  Also, you have some unimplemented stuff like AAC encoding. Could
  you please remove it? We can always use ALAC, and already have a
  presumably-working standalone encoder (that just slaps a header
  with an is-not-compressed bit) implementation in
  write_ALAC_data().  
 
  Sure.  
 
 Also forgot to say: some patches in the series look like fixups for 
 others. E.g. raop: Add a MD5 hashing fuction and raop: Correct the 
 MD5 hashing helper function. I think it is a good idea to squash
 such fixups.

OK, I see what you mean. That means I probably have to start from
pulseaudio/master, apply existing patches manually and commit them in a
new form, without referencing the original raop2 branch. Is this
OK ?

Thanks,
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Please review RAOP2 support :)

[Colin Leroy]

On Thu, 18 Sep 2014 19:52:15 +0600, Alexander E. Patrakov
patra...@gmail.com wrote:

 That would be perfect.
 [...]
 Then git rebase -i master is your friend.

Thanks, that helped :)

Here's the updated result :

https://github.com/colinleroy/pulseaudio/compare/raop2-v2

HTH,
-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Pulseaudio's RAOP2 support

[Colin Leroy]

On Tue, 16 Sep 2014 00:30:21 -0500, Hajime Fujita
crisp.fuj...@nifty.com wrote:

 Hello Colin and Martin,

Hi,

(Changing the recipient to the mailing-list so everyone interested can
chime in if necessary)

 I'm really sorry to say that I've been blocking all the progress on
 this project. If I remember correctly Martin sent me a series of
 patches to me and I promised to review and merge them into my tree.
 However my life gets so busier and busier that I could not find a
 time for this project so far.

Life happens! :) 

 I think the big items right now are:
 - merge Martin's patch into the tree

OK, could you send it to me, Martin ?

 - rebase the current tree onto PA 5.0 (perhaps you did it already)

Yes, I've done so at https://github.com/colinleroy/pulseaudio

I hope I've done it the easiest way for upstream integration, I
pulled from PA's master and then integrated your raop2-for-merge2
patchset.

 - go through PA's review process to get the tree into master

Which is still TBD.

 So what I propose for Martin is to publish the pending patches to
 somewhere so that not only me but also other people can take a look.
 (or he can just merge them as he propose)

That seems good :)

I've started looking at the code and I'm not yet very comfortable with
it, but that should come sometime...

 I do apologize again and am happy to welcome anyone who wants to
 contribute to this project.

Thanks!

-- 
Colin

 I still have an interest for this project and do want to contribute a
 lot as before, but unfortunately I can't make a tangible commitment at
 this moment.
 
 
 Thanks,
 Hajime
 
 Martin Blanchard wrote:
  Hello Colin,
  
  You'll probably find a few answers here:
  
  http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-September/021644.html
  
  You and Tanu asking for some news about this code makes me feel that
  there is still some interest in the RAOP backend. I still got some
  time to spend on this, and of course you're more than welcome if
  you want to help !
  
  Martin
  
  On Mon, 2014-09-15 at 11:08 +0200, Colin Leroy wrote:
  Hello Hajime and Martin,
 
  I've recently bought a Pioneer N30 network player and it works with
  RAOP2. It made me investigate its support in PulseAudio and I had
  success with your implementation at
   
https://github.com/hfujita/pulseaudio-raop2
 
  However, I've also seen that there have been no commits to the
  repository since the end of 2013, and your last patchset on PA's
  mailing list did not get integrated:
 

  http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/18157/focus=21313
 
  I am wondering whether life got too busy for you to continue the
  effort of upstreaming that? and I am pondering whether I'd be
  welcome to try and continue this effort?
 
  Thanks,
  
 



-- 
Colin


signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Pulseaudio's RAOP2 support

[Colin Leroy]

On 16 September 2014 at 12h09, Martin Blanchard wrote:

Hi, 

   I think the big items right now are:
   - merge Martin's patch into the tree  
  
  OK, could you send it to me, Martin ?

 
 Done. Please let me know if you have difficulties rebasing this on top
 of 5.0.

I've rebased everything to my branch, it seems to work fine for me.

I have added a commit that fixes the sink getting destroyed after
being set idle: first a PA_SINK_MESSAGE_SET_STATE/PA_SINK_IDLE message
is received, which disables the rtpoll; then, seconds later, a
PA_SINK_MESSAGE_SET_RAOP_STATE/PA_RAOP_DISCONNECTED is received after
the connection with the device is torn down, and as the thread state is
IDLE a module unload request was fired. 

I'm unsure if my patch is good. I've attached it to have your opinion...

The result of your patchset rebase is in
https://github.com/colinleroy/pulseaudio

-- 
Colin
From 19e99a550cda012d2c1654a246c0381c0db8b9fc Mon Sep 17 00:00:00 2001
From: Colin Leroy co...@colino.net
Date: Tue, 16 Sep 2014 18:42:16 +0200
Subject: [PATCH] raop: Fix sink getting destroyed after one use

It is expected to get disconnected after switching back to a
different sink.
---
 src/modules/raop/raop-sink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/raop/raop-sink.c b/src/modules/raop/raop-sink.c
index a1ec9cf..18b9e16 100644
--- a/src/modules/raop/raop-sink.c
+++ b/src/modules/raop/raop-sink.c
@@ -252,7 +252,7 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse
 
 if (u-sink-thread_info.state == PA_SINK_SUSPENDED)
 pa_rtpoll_set_timer_disabled(u-rtpoll);
-else
+else if (u-sink-thread_info.state != PA_SINK_IDLE)
 pa_module_unload_request(u-module, true);
 
 return 0;
-- 
1.9.1



pgpNMZKiOVOQi.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Pulseaudio's RAOP2 support

[Colin Leroy]

On 16 September 2014 at 18h53, Colin Leroy wrote:

Hi, 

  Done. Please let me know if you have difficulties rebasing this on
  top of 5.0.  
 
 I've rebased everything to my branch, it seems to work fine for me.

Almost fine - the client's seq number, being uint16_t, wraps around
0x in less than five minutes of playback. It makes
raop-packet-buffer assert.

I've implemented that attached patch to wrap around to the initial
seqnum, and I have continuous playback since more than one hour :)

-- 
Colin
From 54ef2c8a91bb24332fa49a30b582b143cc0c5de7 Mon Sep 17 00:00:00 2001
From: Colin Leroy co...@colino.net
Date: Tue, 16 Sep 2014 21:39:21 +0200
Subject: [PATCH] raop: fix sequence number overflow

Shift back to the initial sequence number when we reach uint16_t's max
0x. This avoids asserting in pa_raop_packet_buffer_get().
I've chosen to shift back to the initial sequence number instead of 1
because 1 somehow triggered a flood of UDP timing packets.
---
 src/modules/raop/raop-client.c| 16 ++--
 src/modules/raop/raop-packet-buffer.c | 13 +
 src/modules/raop/raop-packet-buffer.h |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/modules/raop/raop-client.c b/src/modules/raop/raop-client.c
index 2d2f9e3..b9b29be 100644
--- a/src/modules/raop/raop-client.c
+++ b/src/modules/raop/raop-client.c
@@ -109,6 +109,8 @@ struct pa_raop_client {
 pa_raop_packet_buffer *pbuf;
 
 uint16_t seq;
+uint16_t initial_seq;
+
 uint32_t rtptime;
 bool is_recording;
 uint32_t ssrc;
@@ -336,7 +338,12 @@ static size_t build_tcp_audio_packet(pa_raop_client *c, pa_memchunk *block, pa_m
 buffer += packet-index / sizeof(uint32_t);
 raw += block-index;
 
-c-seq++;
+if (c-seq == 0x) {
+	pa_log_debug(resetting sequence number);
+	c-seq = pa_raop_packet_buffer_shift_seq(c-pbuf, c-seq, c-initial_seq);
+} else
+	c-seq++;
+
 memcpy(buffer, tcp_audio_header, sizeof(tcp_audio_header));
 buffer[1] |= htonl((uint32_t) c-seq);
 buffer[2] = htonl(c-rtptime);
@@ -437,7 +444,11 @@ static size_t build_udp_audio_packet(pa_raop_client *c, pa_memchunk *block, pa_m
 else
 size += write_AAC_data(((uint8_t *) buffer + head), packet-length - head, raw, length);
 c-rtptime += length / 4;
-c-seq++;
+if (c-seq == 0x) {
+	pa_log_debug(resetting sequence number);
+	c-seq = pa_raop_packet_buffer_shift_seq(c-pbuf, c-seq, c-initial_seq);
+} else
+	c-seq++;
 
 pa_memblock_release(block-memblock);
 
@@ -1050,6 +1061,7 @@ static void rtsp_stream_cb(pa_rtsp_client *rtsp, pa_rtsp_state_t state, pa_rtsp_
 }
 
 pa_rtsp_record(c-rtsp, c-seq, c-rtptime);
+	c-initial_seq = c-seq;
 
 pa_xfree(trs);
 pa_xfree(ajs);
diff --git a/src/modules/raop/raop-packet-buffer.c b/src/modules/raop/raop-packet-buffer.c
index 06e3125..dda5ad4 100644
--- a/src/modules/raop/raop-packet-buffer.c
+++ b/src/modules/raop/raop-packet-buffer.c
@@ -88,6 +88,19 @@ void pa_raop_packet_buffer_reset(pa_raop_packet_buffer *pb, uint16_t seq) {
 }
 }
 
+uint16_t pa_raop_packet_buffer_shift_seq(pa_raop_packet_buffer *pb, uint16_t seq, uint16_t initial_seq) {
+pa_assert(pb);
+int seq_shift = seq - initial_seq;
+
+if (seq_shift  pb-seq)
+	seq_shift = pb-seq - 1;
+
+pb-seq -= seq_shift;
+
+return seq - seq_shift;
+
+}
+
 pa_memchunk *pa_raop_packet_buffer_get(pa_raop_packet_buffer *pb, uint16_t seq, const size_t size) {
 pa_memchunk *packet = NULL;
 size_t delta, i;
diff --git a/src/modules/raop/raop-packet-buffer.h b/src/modules/raop/raop-packet-buffer.h
index b8c7617..72951f7 100644
--- a/src/modules/raop/raop-packet-buffer.h
+++ b/src/modules/raop/raop-packet-buffer.h
@@ -35,4 +35,5 @@ void pa_raop_packet_buffer_free(pa_raop_packet_buffer *pb);
 void pa_raop_packet_buffer_reset(pa_raop_packet_buffer *pb, uint16_t seq);
 pa_memchunk *pa_raop_packet_buffer_get(pa_raop_packet_buffer *pb, uint16_t seq, const size_t size);
 
+uint16_t pa_raop_packet_buffer_shift_seq(pa_raop_packet_buffer *pb, uint16_t seq, uint16_t initial_seq);
 #endif
-- 
1.9.1



pgp1Y0EK8ZEH4.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss