Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 20.05.19 13:32, Alexander E. Patrakov wrote:

пн, 20 мая 2019 г. в 15:35, Georg Chini :

On 20.05.19 10:47, Alexander E. Patrakov wrote:

пн, 20 мая 2019 г. в 13:17, Georg Chini :

Hi Alexander, Tanu,

would it make sense to give filters some old data for preconditioning when
the filter should be rewound? Then the filter could simply be reset and the
preconditioning data run through the filter instead of doing a "real"
rewind.
The amount of data needed for preconditioning could be made configurable.

The problem is that for IIR filter the needed amount of data is
infinite. So - no.

That's only true in theory. In practice, you never have an IIR filter
because you do not have infinite resolution. And in practice, it will
probably be sufficient to precondition the filter in many cases.
Surely the result will not be perfect, but may be good enough to
avoid audible artifacts.

But if you think it does not make sense, I'll leave it as it is.

In theory, you are right, except for a slowly-recovering automatic
digital gain control filter (similar to the analog filters found in
tape recorders from late 80s - they take about 30-60 seconds to adapt
to unusually quiet sounds, and that still was a useful filter, not
"chewing" the sounds exactly because of such slow recovery).

In practice, I am also strongly against supporting any form of
rewinds, for the following reasons (I admit that I am overly harsh
here).

1. There is already some code, in the form of the LFE filter, that
implements some filtering logic. Making it rewind-aware took 4 review
iterations (which is too many), and there was a "how do I test"
question.
2. Look at alsa-lib code. ALSA API supports rewinds. There is a lot of
code that is supposed to handle rewinds. However, nothing works even
in the "dmix" plugin which is not doing any history-related
processing.
3. There was a submission of xrdp sink. It also had some (meaningless,
because you can't unsend a packet) code written for rewind handling,
and it was obviously not tested.

To be fair, in the case (3) there was no good documentation what a
sink should do, but case (2) comes from ALSA developers, so "appeal to
authority" argument does apply here.

In other words, I don't trust random people to write the rewind
related code correctly (in fact, at this moment, I only trust David
Henningsson), and I don't trust them even more to test it. In some
sense, rewind-related code is similar to error handling: hard to get
right and hard to test. And hard to fix (many people looked at the
current situation with monitor sources, but they are still broken if
there are rewinds). My personal opinion - we should not add such code
to PulseAudio, and maybe even remove support except for the very easy
cases.


Well, regarding the difficulty to debug/write rewinding stuff,
I agree with you. Current pulseaudio master still has at least
one bug which makes it impossible to properly disable rewinding
and another which can lead to too much rewinding during moves.
Also I really had a hard time limiting rewinding for virtual sinks
properly and at least the speex resampler doesn't support
rewinding.

But I do not see why this should prevent us from using rewinding.
I think the statement that it should be avoided because it is hard
to get right is the wrong kind of argument. And current PA code
relies on it, for example you can lose up to one latency of audio
during a sink input move if rewinding is disabled, which is very
audible, even if the latency is limited to 50ms.


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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пн, 20 мая 2019 г. в 15:35, Georg Chini :
>
> On 20.05.19 10:47, Alexander E. Patrakov wrote:
> > пн, 20 мая 2019 г. в 13:17, Georg Chini :
> >> Hi Alexander, Tanu,
> >>
> >> would it make sense to give filters some old data for preconditioning when
> >> the filter should be rewound? Then the filter could simply be reset and the
> >> preconditioning data run through the filter instead of doing a "real"
> >> rewind.
> >> The amount of data needed for preconditioning could be made configurable.
> > The problem is that for IIR filter the needed amount of data is
> > infinite. So - no.
>
> That's only true in theory. In practice, you never have an IIR filter
> because you do not have infinite resolution. And in practice, it will
> probably be sufficient to precondition the filter in many cases.
> Surely the result will not be perfect, but may be good enough to
> avoid audible artifacts.
>
> But if you think it does not make sense, I'll leave it as it is.

In theory, you are right, except for a slowly-recovering automatic
digital gain control filter (similar to the analog filters found in
tape recorders from late 80s - they take about 30-60 seconds to adapt
to unusually quiet sounds, and that still was a useful filter, not
"chewing" the sounds exactly because of such slow recovery).

In practice, I am also strongly against supporting any form of
rewinds, for the following reasons (I admit that I am overly harsh
here).

1. There is already some code, in the form of the LFE filter, that
implements some filtering logic. Making it rewind-aware took 4 review
iterations (which is too many), and there was a "how do I test"
question.
2. Look at alsa-lib code. ALSA API supports rewinds. There is a lot of
code that is supposed to handle rewinds. However, nothing works even
in the "dmix" plugin which is not doing any history-related
processing.
3. There was a submission of xrdp sink. It also had some (meaningless,
because you can't unsend a packet) code written for rewind handling,
and it was obviously not tested.

To be fair, in the case (3) there was no good documentation what a
sink should do, but case (2) comes from ALSA developers, so "appeal to
authority" argument does apply here.

In other words, I don't trust random people to write the rewind
related code correctly (in fact, at this moment, I only trust David
Henningsson), and I don't trust them even more to test it. In some
sense, rewind-related code is similar to error handling: hard to get
right and hard to test. And hard to fix (many people looked at the
current situation with monitor sources, but they are still broken if
there are rewinds). My personal opinion - we should not add such code
to PulseAudio, and maybe even remove support except for the very easy
cases.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 20.05.19 10:47, Alexander E. Patrakov wrote:

пн, 20 мая 2019 г. в 13:17, Georg Chini :

Hi Alexander, Tanu,

would it make sense to give filters some old data for preconditioning when
the filter should be rewound? Then the filter could simply be reset and the
preconditioning data run through the filter instead of doing a "real"
rewind.
The amount of data needed for preconditioning could be made configurable.

The problem is that for IIR filter the needed amount of data is
infinite. So - no.


That's only true in theory. In practice, you never have an IIR filter
because you do not have infinite resolution. And in practice, it will
probably be sufficient to precondition the filter in many cases.
Surely the result will not be perfect, but may be good enough to
avoid audible artifacts.

But if you think it does not make sense, I'll leave it as it is.


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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пн, 20 мая 2019 г. в 13:17, Georg Chini :
>
> Hi Alexander, Tanu,
>
> would it make sense to give filters some old data for preconditioning when
> the filter should be rewound? Then the filter could simply be reset and the
> preconditioning data run through the filter instead of doing a "real"
> rewind.
> The amount of data needed for preconditioning could be made configurable.

The problem is that for IIR filter the needed amount of data is
infinite. So - no.

An IIR-based replacement of module-virtual-surround will be sent to
Georg privately to demonstrate the problem. Please note that it is not
suitable for inclusion of PulseAudio.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

Hi Alexander, Tanu,

would it make sense to give filters some old data for preconditioning when
the filter should be rewound? Then the filter could simply be reset and the
preconditioning data run through the filter instead of doing a "real" 
rewind.

The amount of data needed for preconditioning could be made configurable.

I was able to make module-virtual-surround-sink rewindable with a similar
approach. In that case it is enough to set the input buffer to the chunk of
data before the rewound data.

Regards
    Georg

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 10.05.19 19:27, Tanu Kaskinen wrote:

On Sat, 2019-05-04 at 22:41 +0200, Georg Chini wrote:
I don't see the need for copying the channel position and error enums.
We can #include the relevant headers.


I thought the point was to create a header file that allows development
of filters completely independent from pulseaudio, that is you can compile
the code without having PA installed. If that was not the intention, we can
include the headers. Can we then also link with libpulse?





/* Function called when the filter detects a condition to unload the filter. */
typedef void (*Kill_Filter_Function)(void *module);

What's the use case for for filter self-destruction?


A filter might detect that it is no longer appropriate based on the
sink/port information it receives. Then it can either disable itself
internally or just kill itself, so that PA will take over processing.




I think we should use the normal PA naming conventions, so this would
be named pa_filter_plugin_kill_cb_t.


Why should we follow PA naming conventions? Again, I thought
it should be rather independent from PA and authors of filters
do not need to know PA internals. That's one of the reasons I
dropped the PA_ prefix for channel map and error values.
If you prefer, I can nevertheless change back to PA conventions.





/* Filter plugin structure */
typedef struct filter_plugin {
 const char *name; /* Name of the filter. Used to 
identify the filter. */

Is this expected to be globally unique? Are there limitations on length
or accepted characters? Would there be problems with using the .so file
name as the name? I expect the .so file name to be needed for
identification anyway, and it's not clear to me what benefit another
layer of naming brings.


It would be nice to have this globally unique, but I think that is difficult
to ensure. The concept of loading filters follows the LADSPA scheme,
so one file can contain multiple filters. See comment to the
get_Filter_Info() function.





 const char *desc_head;/* Leading part of description 
string used for the
* sink and sink input 
description in PA. */

Can you give an example how this is used? What about sources, maybe
we'll one day have module-plugin-source too?

The sources we currently have are quite different and not easy
to consolidate. So I decided (for the moment) to leave it at that.


Is this the same thing that is returned as the filter description in
the parameter-get-description message? If so, I think "description"
would be a better name for this field than "desc_head".


No, this has nothing to do with the parameter description. This
is just a bit of text that is used in the description property of
sink and sink input. For the virtual sink it would just be
"Virtual Sink" or "LADSPA Sink" for the ladspa sink. There is
surely a better name for it ...





 const char *filter_type;  /* Name for the type of filter, 
used as suffix for
* the sink name if the name is 
derived from the
* master sink. */

I believe using a prefix is better than suffix. A tunnel sink has
prefix "tunnel.", so if there's a sink named "tunnel.foo.eq", it's
ambiguous whether that's a tunnel sink connected to remote "foo.eq"
sink or a local eq sink using "tunnel.foo" as master. "eq.tunnel.foo"
doesn't have this problem.


The current virtual sinks all use a suffix. Are you OK with changing
that for all virtual sinks?





 unsigned input_channels;  /* Number of audio input 
channels, 0 if variable */
 unsigned output_channels; /* Number of audio output 
channels, 0 if variable */

How are these used? You wanted to avoid deinterleaving, so I guess mono
filters are expected to support variable channels. What kind of filters
are going to set fixed channels? Is a filter expected to support all
possible channel counts if it supports more than one channel count?


Either the filter provides a fixed number of channels (for example
2 output and 6 input channels for a virtual surround filter) or the
number of channels can be set freely (based on master sink and/or
provided channel maps). There may be filter specific conditions like
number of in/out channels being equal, but this must be checked
by the filter at initialization time.





 size_t max_chunk_size;/* Maximum chunk size in bytes 
that the filter will
* accept, 0 for default */

Shouldn't this be in frames rather than in bytes, since the other
fields use frames? And what's the difference between "chunk" and
"block"? If there's none, then let's call it a "block" always.

What's the default chunk size? Does 0 mean that the chunk size is
allowed to change between process_chunk() calls?

Is this even necessary? LADSPA doesn't 

Re: [pulseaudio-discuss] new module module-plugin-sink

[Tanu Kaskinen]

On Sat, 2019-05-04 at 22:41 +0200, Georg Chini wrote:
> /***
>   This file is part of PulseAudio.
> 
>   PulseAudio is free software; you can redistribute it and/or modify
>   it under the terms of the GNU Lesser General Public License as published
>   by the Free Software Foundation; either version 2.1 of the License,
>   or (at your option) any later version.
> 
>   PulseAudio 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 Lesser General Public License
>   along with PulseAudio; if not, see ;.
> ***/
> 
> /* Channel positions copied from pulseaudio channelmap.h */
> enum channel_position {
> CHANNEL_POSITION_INVALID = -1,
> CHANNEL_POSITION_MONO = 0,
> 
> CHANNEL_POSITION_FRONT_LEFT,   /**< Apple, Dolby call this 
> 'Left' */
> CHANNEL_POSITION_FRONT_RIGHT,  /**< Apple, Dolby call this 
> 'Right' */
> CHANNEL_POSITION_FRONT_CENTER, /**< Apple, Dolby call this 
> 'Center' */
> 
> CHANNEL_POSITION_LEFT = CHANNEL_POSITION_FRONT_LEFT,
> CHANNEL_POSITION_RIGHT = CHANNEL_POSITION_FRONT_RIGHT,
> CHANNEL_POSITION_CENTER = CHANNEL_POSITION_FRONT_CENTER,
> 
> CHANNEL_POSITION_REAR_CENTER,  /**< Microsoft calls this 
> 'Back Center', Apple calls this 'Center Surround', Dolby calls this 'Surround 
> Rear Center' */
> CHANNEL_POSITION_REAR_LEFT,/**< Microsoft calls this 
> 'Back Left', Apple calls this 'Left Surround' (!), Dolby calls this 'Surround 
> Rear Left'  */
> CHANNEL_POSITION_REAR_RIGHT,   /**< Microsoft calls this 
> 'Back Right', Apple calls this 'Right Surround' (!), Dolby calls this 
> 'Surround Rear Right'  */
> 
> CHANNEL_POSITION_LFE,  /**< Microsoft calls this 'Low 
> Frequency', Apple calls this 'LFEScreen' */
> CHANNEL_POSITION_SUBWOOFER = CHANNEL_POSITION_LFE,
> 
> CHANNEL_POSITION_FRONT_LEFT_OF_CENTER, /**< Apple, Dolby call this 
> 'Left Center' */
> CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER,/**< Apple, Dolby call this 
> 'Right Center */
> 
> CHANNEL_POSITION_SIDE_LEFT,/**< Apple calls this 'Left 
> Surround Direct', Dolby calls this 'Surround Left' (!) */
> CHANNEL_POSITION_SIDE_RIGHT,   /**< Apple calls this 'Right 
> Surround Direct', Dolby calls this 'Surround Right' (!) */
> 
> CHANNEL_POSITION_AUX0,
> CHANNEL_POSITION_AUX1,
> CHANNEL_POSITION_AUX2,
> CHANNEL_POSITION_AUX3,
> CHANNEL_POSITION_AUX4,
> CHANNEL_POSITION_AUX5,
> CHANNEL_POSITION_AUX6,
> CHANNEL_POSITION_AUX7,
> CHANNEL_POSITION_AUX8,
> CHANNEL_POSITION_AUX9,
> CHANNEL_POSITION_AUX10,
> CHANNEL_POSITION_AUX11,
> CHANNEL_POSITION_AUX12,
> CHANNEL_POSITION_AUX13,
> CHANNEL_POSITION_AUX14,
> CHANNEL_POSITION_AUX15,
> CHANNEL_POSITION_AUX16,
> CHANNEL_POSITION_AUX17,
> CHANNEL_POSITION_AUX18,
> CHANNEL_POSITION_AUX19,
> CHANNEL_POSITION_AUX20,
> CHANNEL_POSITION_AUX21,
> CHANNEL_POSITION_AUX22,
> CHANNEL_POSITION_AUX23,
> CHANNEL_POSITION_AUX24,
> CHANNEL_POSITION_AUX25,
> CHANNEL_POSITION_AUX26,
> CHANNEL_POSITION_AUX27,
> CHANNEL_POSITION_AUX28,
> CHANNEL_POSITION_AUX29,
> CHANNEL_POSITION_AUX30,
> CHANNEL_POSITION_AUX31,
> 
> CHANNEL_POSITION_TOP_CENTER,   /**< Apple calls this 'Top 
> Center Surround' */
> 
> CHANNEL_POSITION_TOP_FRONT_LEFT,   /**< Apple calls this 
> 'Vertical Height Left' */
> CHANNEL_POSITION_TOP_FRONT_RIGHT,  /**< Apple calls this 
> 'Vertical Height Right' */
> CHANNEL_POSITION_TOP_FRONT_CENTER, /**< Apple calls this 
> 'Vertical Height Center' */
> 
> CHANNEL_POSITION_TOP_REAR_LEFT,/**< Microsoft and Apple call 
> this 'Top Back Left' */
> CHANNEL_POSITION_TOP_REAR_RIGHT,   /**< Microsoft and Apple call 
> this 'Top Back Right' */
> CHANNEL_POSITION_TOP_REAR_CENTER,  /**< Microsoft and Apple call 
> this 'Top Back Center' */
> 
> CHANNEL_POSITION_MAX
> };
> 
> /* Error codes copied from pulseaudio def.h */
> enum {
> OK = 0, /**< No error */
> ERR_ACCESS, /**< Access failure */
> ERR_COMMAND,/**< Unknown command */
> ERR_INVALID,/**< Invalid argument */
> ERR_EXIST,  /**< Entity exists */
> ERR_NOENTITY,   /**< No such entity */
> ERR_CONNECTIONREFUSED,  /**< Connection refused */
> ERR_PROTOCOL,   /**< Protocol error */
> ERR_TIMEOUT,/**< Timeout */
> ERR_AUTHKEY,/**< No authentication key */
> ERR_INTERNAL,   /**< Internal error */

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

вс, 5 мая 2019 г. в 22:58, Georg Chini :
>
> On 05.05.19 18:41, Alexander E. Patrakov wrote:
> > вс, 5 мая 2019 г. в 01:41, Georg Chini :
> >> On 04.05.19 20:54, Alexander E. Patrakov wrote:
> >>> сб, 4 мая 2019 г. в 20:25, Georg Chini :
>  On 04.05.19 16:42, Alexander E. Patrakov wrote:
> > сб, 4 мая 2019 г. в 16:17, Georg Chini :
> >> Here is the new version of the header file, based on your feedback.
> >> The main changes are:
> >>
> >> - The create_filter() function now receives the channel maps for input
> >> and output.
> >> - The create_filter() function receives a kill_filter() function and a
> >> module pointer
> >>which makes it possible for the filter to initiate unloading of 
> >> the
> >> module if it
> >>detects that it is no longer applicable.
> >> - An output_changed() function was added which communicates current 
> >> sink
> >>   and port name to the filter, so that it can detect if the output 
> >> has
> >> changed.
> >>
> >> Also I did a bit of cleanup and added a few more comments. Hope it 
> >> looks
> >> better now.
> > It definitely looks better.
> >
> > I am still confused about disable_rewind and max_latency. Let's
> > suppose that someone wants to implement a rewindable filter. In this
> > case, they need to keep history, because PulseAudio can ask the filter
> > to rewind some samples. And, as it is not allowed to say "no", they
> > must keep enough history to satisfy any possible rewind request. But
> > some upper bound must exist. Do I understand correctly that
> > max_latency serves as such upper bound?
> >
> > Regarding the non-rewindable filters, we do need to limit the latency,
> > but I believe it is wrong for each individual filter to specify its
> > own value for such limit. It should be a global policy (the same value
> > for all non-rewindable sinks), and I don't see any reason for the
> > filter to be able to influence it.
> >
> > Therefore, I believe these two fields can be replaced by one,
> > max_rewind, which is the size of history, in samples, that the filter
> > is willing to keep. Zero means a non-rewindable filter.
> >
>  That sounds like a good suggestion. I would however think
>  that it is better if 0 means that the filter will rewind as far as
>  PA wants it to. There may be filters that are stateless (like the
>  trivial amplifier example). We could use -1 to disable rewinding.
> >>> OK.
> >>>
>  That would also mean to limit the latency to whatever the filter
>  can rewind, correct? I would use the maximum of max_rewind
>  and the default latency for non-rewindable filters as the
>  max_latency value then, because I don't think it makes sense
>  to set the maximum latency even smaller than for non-rewindable
>  filters.
> >>> Makes sense.
> >>>
>  What do you think is reasonable for non-rewindable filters?
>  50 ms?
> >>> There were different opinions on that matter. 50 ms is indeed in a
> >>> range that I would agree to.
> >>>
> >> Just finished the next version. Does this look OK to you now?
> > I think that the only significant difference is the addition of error
> > codes. I cannot comment on them with any authority, but the majority
> > do not seem applicable to filters. Probably we need some mechanism
> > that allows arbitrary error strings to be logged?
> >
> The significant difference was the removal of max_latency and
> the change of bool disable_rewind to int max_rewind. I need the
> error codes for handling message replies, that's why I added
> them. We should not need any other error reporting I think.

Then OK on the "significant" difference, and no review on anything else.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 05.05.19 18:41, Alexander E. Patrakov wrote:

вс, 5 мая 2019 г. в 01:41, Georg Chini :

On 04.05.19 20:54, Alexander E. Patrakov wrote:

сб, 4 мая 2019 г. в 20:25, Georg Chini :

On 04.05.19 16:42, Alexander E. Patrakov wrote:

сб, 4 мая 2019 г. в 16:17, Georg Chini :

Here is the new version of the header file, based on your feedback.
The main changes are:

- The create_filter() function now receives the channel maps for input
and output.
- The create_filter() function receives a kill_filter() function and a
module pointer
   which makes it possible for the filter to initiate unloading of the
module if it
   detects that it is no longer applicable.
- An output_changed() function was added which communicates current sink
  and port name to the filter, so that it can detect if the output has
changed.

Also I did a bit of cleanup and added a few more comments. Hope it looks
better now.

It definitely looks better.

I am still confused about disable_rewind and max_latency. Let's
suppose that someone wants to implement a rewindable filter. In this
case, they need to keep history, because PulseAudio can ask the filter
to rewind some samples. And, as it is not allowed to say "no", they
must keep enough history to satisfy any possible rewind request. But
some upper bound must exist. Do I understand correctly that
max_latency serves as such upper bound?

Regarding the non-rewindable filters, we do need to limit the latency,
but I believe it is wrong for each individual filter to specify its
own value for such limit. It should be a global policy (the same value
for all non-rewindable sinks), and I don't see any reason for the
filter to be able to influence it.

Therefore, I believe these two fields can be replaced by one,
max_rewind, which is the size of history, in samples, that the filter
is willing to keep. Zero means a non-rewindable filter.


That sounds like a good suggestion. I would however think
that it is better if 0 means that the filter will rewind as far as
PA wants it to. There may be filters that are stateless (like the
trivial amplifier example). We could use -1 to disable rewinding.

OK.


That would also mean to limit the latency to whatever the filter
can rewind, correct? I would use the maximum of max_rewind
and the default latency for non-rewindable filters as the
max_latency value then, because I don't think it makes sense
to set the maximum latency even smaller than for non-rewindable
filters.

Makes sense.


What do you think is reasonable for non-rewindable filters?
50 ms?

There were different opinions on that matter. 50 ms is indeed in a
range that I would agree to.


Just finished the next version. Does this look OK to you now?

I think that the only significant difference is the addition of error
codes. I cannot comment on them with any authority, but the majority
do not seem applicable to filters. Probably we need some mechanism
that allows arbitrary error strings to be logged?


The significant difference was the removal of max_latency and
the change of bool disable_rewind to int max_rewind. I need the
error codes for handling message replies, that's why I added
them. We should not need any other error reporting I think.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

вс, 5 мая 2019 г. в 01:41, Georg Chini :
>
> On 04.05.19 20:54, Alexander E. Patrakov wrote:
> > сб, 4 мая 2019 г. в 20:25, Georg Chini :
> >> On 04.05.19 16:42, Alexander E. Patrakov wrote:
> >>> сб, 4 мая 2019 г. в 16:17, Georg Chini :
>  Here is the new version of the header file, based on your feedback.
>  The main changes are:
> 
>  - The create_filter() function now receives the channel maps for input
>  and output.
>  - The create_filter() function receives a kill_filter() function and a
>  module pointer
>    which makes it possible for the filter to initiate unloading of the
>  module if it
>    detects that it is no longer applicable.
>  - An output_changed() function was added which communicates current sink
>   and port name to the filter, so that it can detect if the output has
>  changed.
> 
>  Also I did a bit of cleanup and added a few more comments. Hope it looks
>  better now.
> >>> It definitely looks better.
> >>>
> >>> I am still confused about disable_rewind and max_latency. Let's
> >>> suppose that someone wants to implement a rewindable filter. In this
> >>> case, they need to keep history, because PulseAudio can ask the filter
> >>> to rewind some samples. And, as it is not allowed to say "no", they
> >>> must keep enough history to satisfy any possible rewind request. But
> >>> some upper bound must exist. Do I understand correctly that
> >>> max_latency serves as such upper bound?
> >>>
> >>> Regarding the non-rewindable filters, we do need to limit the latency,
> >>> but I believe it is wrong for each individual filter to specify its
> >>> own value for such limit. It should be a global policy (the same value
> >>> for all non-rewindable sinks), and I don't see any reason for the
> >>> filter to be able to influence it.
> >>>
> >>> Therefore, I believe these two fields can be replaced by one,
> >>> max_rewind, which is the size of history, in samples, that the filter
> >>> is willing to keep. Zero means a non-rewindable filter.
> >>>
> >> That sounds like a good suggestion. I would however think
> >> that it is better if 0 means that the filter will rewind as far as
> >> PA wants it to. There may be filters that are stateless (like the
> >> trivial amplifier example). We could use -1 to disable rewinding.
> > OK.
> >
> >> That would also mean to limit the latency to whatever the filter
> >> can rewind, correct? I would use the maximum of max_rewind
> >> and the default latency for non-rewindable filters as the
> >> max_latency value then, because I don't think it makes sense
> >> to set the maximum latency even smaller than for non-rewindable
> >> filters.
> > Makes sense.
> >
> >> What do you think is reasonable for non-rewindable filters?
> >> 50 ms?
> > There were different opinions on that matter. 50 ms is indeed in a
> > range that I would agree to.
> >
> Just finished the next version. Does this look OK to you now?

I think that the only significant difference is the addition of error
codes. I cannot comment on them with any authority, but the majority
do not seem applicable to filters. Probably we need some mechanism
that allows arbitrary error strings to be logged?

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 04.05.19 20:54, Alexander E. Patrakov wrote:

сб, 4 мая 2019 г. в 20:25, Georg Chini :

On 04.05.19 16:42, Alexander E. Patrakov wrote:

сб, 4 мая 2019 г. в 16:17, Georg Chini :

Here is the new version of the header file, based on your feedback.
The main changes are:

- The create_filter() function now receives the channel maps for input
and output.
- The create_filter() function receives a kill_filter() function and a
module pointer
  which makes it possible for the filter to initiate unloading of the
module if it
  detects that it is no longer applicable.
- An output_changed() function was added which communicates current sink
 and port name to the filter, so that it can detect if the output has
changed.

Also I did a bit of cleanup and added a few more comments. Hope it looks
better now.

It definitely looks better.

I am still confused about disable_rewind and max_latency. Let's
suppose that someone wants to implement a rewindable filter. In this
case, they need to keep history, because PulseAudio can ask the filter
to rewind some samples. And, as it is not allowed to say "no", they
must keep enough history to satisfy any possible rewind request. But
some upper bound must exist. Do I understand correctly that
max_latency serves as such upper bound?

Regarding the non-rewindable filters, we do need to limit the latency,
but I believe it is wrong for each individual filter to specify its
own value for such limit. It should be a global policy (the same value
for all non-rewindable sinks), and I don't see any reason for the
filter to be able to influence it.

Therefore, I believe these two fields can be replaced by one,
max_rewind, which is the size of history, in samples, that the filter
is willing to keep. Zero means a non-rewindable filter.


That sounds like a good suggestion. I would however think
that it is better if 0 means that the filter will rewind as far as
PA wants it to. There may be filters that are stateless (like the
trivial amplifier example). We could use -1 to disable rewinding.

OK.


That would also mean to limit the latency to whatever the filter
can rewind, correct? I would use the maximum of max_rewind
and the default latency for non-rewindable filters as the
max_latency value then, because I don't think it makes sense
to set the maximum latency even smaller than for non-rewindable
filters.

Makes sense.


What do you think is reasonable for non-rewindable filters?
50 ms?

There were different opinions on that matter. 50 ms is indeed in a
range that I would agree to.


Just finished the next version. Does this look OK to you now?

/***
  This file is part of PulseAudio.

  PulseAudio is free software; you can redistribute it and/or modify
  it under the terms of the GNU Lesser General Public License as published
  by the Free Software Foundation; either version 2.1 of the License,
  or (at your option) any later version.

  PulseAudio 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 Lesser General Public License
  along with PulseAudio; if not, see .
***/

/* Channel positions copied from pulseaudio channelmap.h */
enum channel_position {
CHANNEL_POSITION_INVALID = -1,
CHANNEL_POSITION_MONO = 0,

CHANNEL_POSITION_FRONT_LEFT,   /**< Apple, Dolby call this 'Left' */
CHANNEL_POSITION_FRONT_RIGHT,  /**< Apple, Dolby call this 'Right' */
CHANNEL_POSITION_FRONT_CENTER, /**< Apple, Dolby call this 'Center' */

CHANNEL_POSITION_LEFT = CHANNEL_POSITION_FRONT_LEFT,
CHANNEL_POSITION_RIGHT = CHANNEL_POSITION_FRONT_RIGHT,
CHANNEL_POSITION_CENTER = CHANNEL_POSITION_FRONT_CENTER,

CHANNEL_POSITION_REAR_CENTER,  /**< Microsoft calls this 'Back Center', Apple calls this 'Center Surround', Dolby calls this 'Surround Rear Center' */
CHANNEL_POSITION_REAR_LEFT,/**< Microsoft calls this 'Back Left', Apple calls this 'Left Surround' (!), Dolby calls this 'Surround Rear Left'  */
CHANNEL_POSITION_REAR_RIGHT,   /**< Microsoft calls this 'Back Right', Apple calls this 'Right Surround' (!), Dolby calls this 'Surround Rear Right'  */

CHANNEL_POSITION_LFE,  /**< Microsoft calls this 'Low Frequency', Apple calls this 'LFEScreen' */
CHANNEL_POSITION_SUBWOOFER = CHANNEL_POSITION_LFE,

CHANNEL_POSITION_FRONT_LEFT_OF_CENTER, /**< Apple, Dolby call this 'Left Center' */
CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER,/**< Apple, Dolby call this 'Right Center */

CHANNEL_POSITION_SIDE_LEFT,/**< Apple calls this 'Left Surround Direct', Dolby calls this 'Surround Left' (!) */
CHANNEL_POSITION_SIDE_RIGHT,   /**< Apple calls this 'Right Surround Direct', Dolby calls 

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

сб, 4 мая 2019 г. в 20:25, Georg Chini :
>
> On 04.05.19 16:42, Alexander E. Patrakov wrote:
> > сб, 4 мая 2019 г. в 16:17, Georg Chini :
> >> Here is the new version of the header file, based on your feedback.
> >> The main changes are:
> >>
> >> - The create_filter() function now receives the channel maps for input
> >> and output.
> >> - The create_filter() function receives a kill_filter() function and a
> >> module pointer
> >>  which makes it possible for the filter to initiate unloading of the
> >> module if it
> >>  detects that it is no longer applicable.
> >> - An output_changed() function was added which communicates current sink
> >> and port name to the filter, so that it can detect if the output has
> >> changed.
> >>
> >> Also I did a bit of cleanup and added a few more comments. Hope it looks
> >> better now.
> > It definitely looks better.
> >
> > I am still confused about disable_rewind and max_latency. Let's
> > suppose that someone wants to implement a rewindable filter. In this
> > case, they need to keep history, because PulseAudio can ask the filter
> > to rewind some samples. And, as it is not allowed to say "no", they
> > must keep enough history to satisfy any possible rewind request. But
> > some upper bound must exist. Do I understand correctly that
> > max_latency serves as such upper bound?
> >
> > Regarding the non-rewindable filters, we do need to limit the latency,
> > but I believe it is wrong for each individual filter to specify its
> > own value for such limit. It should be a global policy (the same value
> > for all non-rewindable sinks), and I don't see any reason for the
> > filter to be able to influence it.
> >
> > Therefore, I believe these two fields can be replaced by one,
> > max_rewind, which is the size of history, in samples, that the filter
> > is willing to keep. Zero means a non-rewindable filter.
> >
> That sounds like a good suggestion. I would however think
> that it is better if 0 means that the filter will rewind as far as
> PA wants it to. There may be filters that are stateless (like the
> trivial amplifier example). We could use -1 to disable rewinding.

OK.

> That would also mean to limit the latency to whatever the filter
> can rewind, correct? I would use the maximum of max_rewind
> and the default latency for non-rewindable filters as the
> max_latency value then, because I don't think it makes sense
> to set the maximum latency even smaller than for non-rewindable
> filters.

Makes sense.

> What do you think is reasonable for non-rewindable filters?
> 50 ms?

There were different opinions on that matter. 50 ms is indeed in a
range that I would agree to.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 04.05.19 16:42, Alexander E. Patrakov wrote:

сб, 4 мая 2019 г. в 16:17, Georg Chini :

Here is the new version of the header file, based on your feedback.
The main changes are:

- The create_filter() function now receives the channel maps for input
and output.
- The create_filter() function receives a kill_filter() function and a
module pointer
 which makes it possible for the filter to initiate unloading of the
module if it
 detects that it is no longer applicable.
- An output_changed() function was added which communicates current sink
and port name to the filter, so that it can detect if the output has
changed.

Also I did a bit of cleanup and added a few more comments. Hope it looks
better now.

It definitely looks better.

I am still confused about disable_rewind and max_latency. Let's
suppose that someone wants to implement a rewindable filter. In this
case, they need to keep history, because PulseAudio can ask the filter
to rewind some samples. And, as it is not allowed to say "no", they
must keep enough history to satisfy any possible rewind request. But
some upper bound must exist. Do I understand correctly that
max_latency serves as such upper bound?

Regarding the non-rewindable filters, we do need to limit the latency,
but I believe it is wrong for each individual filter to specify its
own value for such limit. It should be a global policy (the same value
for all non-rewindable sinks), and I don't see any reason for the
filter to be able to influence it.

Therefore, I believe these two fields can be replaced by one,
max_rewind, which is the size of history, in samples, that the filter
is willing to keep. Zero means a non-rewindable filter.


That sounds like a good suggestion. I would however think
that it is better if 0 means that the filter will rewind as far as
PA wants it to. There may be filters that are stateless (like the
trivial amplifier example). We could use -1 to disable rewinding.

That would also mean to limit the latency to whatever the filter
can rewind, correct? I would use the maximum of max_rewind
and the default latency for non-rewindable filters as the
max_latency value then, because I don't think it makes sense
to set the maximum latency even smaller than for non-rewindable
filters.

What do you think is reasonable for non-rewindable filters?
50 ms?

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

сб, 4 мая 2019 г. в 16:17, Georg Chini :
>
> Here is the new version of the header file, based on your feedback.
> The main changes are:
>
> - The create_filter() function now receives the channel maps for input
> and output.
> - The create_filter() function receives a kill_filter() function and a
> module pointer
> which makes it possible for the filter to initiate unloading of the
> module if it
> detects that it is no longer applicable.
> - An output_changed() function was added which communicates current sink
>and port name to the filter, so that it can detect if the output has
> changed.
>
> Also I did a bit of cleanup and added a few more comments. Hope it looks
> better now.

It definitely looks better.

I am still confused about disable_rewind and max_latency. Let's
suppose that someone wants to implement a rewindable filter. In this
case, they need to keep history, because PulseAudio can ask the filter
to rewind some samples. And, as it is not allowed to say "no", they
must keep enough history to satisfy any possible rewind request. But
some upper bound must exist. Do I understand correctly that
max_latency serves as such upper bound?

Regarding the non-rewindable filters, we do need to limit the latency,
but I believe it is wrong for each individual filter to specify its
own value for such limit. It should be a global policy (the same value
for all non-rewindable sinks), and I don't see any reason for the
filter to be able to influence it.

Therefore, I believe these two fields can be replaced by one,
max_rewind, which is the size of history, in samples, that the filter
is willing to keep. Zero means a non-rewindable filter.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

Here is the new version of the header file, based on your feedback.
The main changes are:

- The create_filter() function now receives the channel maps for input 
and output.
- The create_filter() function receives a kill_filter() function and a 
module pointer
   which makes it possible for the filter to initiate unloading of the 
module if it

   detects that it is no longer applicable.
- An output_changed() function was added which communicates current sink
  and port name to the filter, so that it can detect if the output has 
changed.


Also I did a bit of cleanup and added a few more comments. Hope it looks
better now.

Regards
    Georg
/***
  This file is part of PulseAudio.

  PulseAudio is free software; you can redistribute it and/or modify
  it under the terms of the GNU Lesser General Public License as published
  by the Free Software Foundation; either version 2.1 of the License,
  or (at your option) any later version.

  PulseAudio 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 Lesser General Public License
  along with PulseAudio; if not, see .
***/

/* Channel positions copied from pulseaudio channelmap.h */
enum channel_position {
CHANNEL_POSITION_INVALID = -1,
CHANNEL_POSITION_MONO = 0,

CHANNEL_POSITION_FRONT_LEFT,   /**< Apple, Dolby call this 'Left' */
CHANNEL_POSITION_FRONT_RIGHT,  /**< Apple, Dolby call this 'Right' */
CHANNEL_POSITION_FRONT_CENTER, /**< Apple, Dolby call this 'Center' */

CHANNEL_POSITION_LEFT = CHANNEL_POSITION_FRONT_LEFT,
CHANNEL_POSITION_RIGHT = CHANNEL_POSITION_FRONT_RIGHT,
CHANNEL_POSITION_CENTER = CHANNEL_POSITION_FRONT_CENTER,

CHANNEL_POSITION_REAR_CENTER,  /**< Microsoft calls this 'Back Center', Apple calls this 'Center Surround', Dolby calls this 'Surround Rear Center' */
CHANNEL_POSITION_REAR_LEFT,/**< Microsoft calls this 'Back Left', Apple calls this 'Left Surround' (!), Dolby calls this 'Surround Rear Left'  */
CHANNEL_POSITION_REAR_RIGHT,   /**< Microsoft calls this 'Back Right', Apple calls this 'Right Surround' (!), Dolby calls this 'Surround Rear Right'  */

CHANNEL_POSITION_LFE,  /**< Microsoft calls this 'Low Frequency', Apple calls this 'LFEScreen' */
CHANNEL_POSITION_SUBWOOFER = CHANNEL_POSITION_LFE,

CHANNEL_POSITION_FRONT_LEFT_OF_CENTER, /**< Apple, Dolby call this 'Left Center' */
CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER,/**< Apple, Dolby call this 'Right Center */

CHANNEL_POSITION_SIDE_LEFT,/**< Apple calls this 'Left Surround Direct', Dolby calls this 'Surround Left' (!) */
CHANNEL_POSITION_SIDE_RIGHT,   /**< Apple calls this 'Right Surround Direct', Dolby calls this 'Surround Right' (!) */

CHANNEL_POSITION_AUX0,
CHANNEL_POSITION_AUX1,
CHANNEL_POSITION_AUX2,
CHANNEL_POSITION_AUX3,
CHANNEL_POSITION_AUX4,
CHANNEL_POSITION_AUX5,
CHANNEL_POSITION_AUX6,
CHANNEL_POSITION_AUX7,
CHANNEL_POSITION_AUX8,
CHANNEL_POSITION_AUX9,
CHANNEL_POSITION_AUX10,
CHANNEL_POSITION_AUX11,
CHANNEL_POSITION_AUX12,
CHANNEL_POSITION_AUX13,
CHANNEL_POSITION_AUX14,
CHANNEL_POSITION_AUX15,
CHANNEL_POSITION_AUX16,
CHANNEL_POSITION_AUX17,
CHANNEL_POSITION_AUX18,
CHANNEL_POSITION_AUX19,
CHANNEL_POSITION_AUX20,
CHANNEL_POSITION_AUX21,
CHANNEL_POSITION_AUX22,
CHANNEL_POSITION_AUX23,
CHANNEL_POSITION_AUX24,
CHANNEL_POSITION_AUX25,
CHANNEL_POSITION_AUX26,
CHANNEL_POSITION_AUX27,
CHANNEL_POSITION_AUX28,
CHANNEL_POSITION_AUX29,
CHANNEL_POSITION_AUX30,
CHANNEL_POSITION_AUX31,

CHANNEL_POSITION_TOP_CENTER,   /**< Apple calls this 'Top Center Surround' */

CHANNEL_POSITION_TOP_FRONT_LEFT,   /**< Apple calls this 'Vertical Height Left' */
CHANNEL_POSITION_TOP_FRONT_RIGHT,  /**< Apple calls this 'Vertical Height Right' */
CHANNEL_POSITION_TOP_FRONT_CENTER, /**< Apple calls this 'Vertical Height Center' */

CHANNEL_POSITION_TOP_REAR_LEFT,/**< Microsoft and Apple call this 'Top Back Left' */
CHANNEL_POSITION_TOP_REAR_RIGHT,   /**< Microsoft and Apple call this 'Top Back Right' */
CHANNEL_POSITION_TOP_REAR_CENTER,  /**< Microsoft and Apple call this 'Top Back Center' */

CHANNEL_POSITION_MAX
};

/* Function called when the filter detects a condition to unload the filter. */
typedef void (*Kill_Filter_Function)(void *module);

/* Filter plugin structure */
typedef struct filter_plugin {
const char *name; /* Name of the filter. Used to identify the filter. */
const char 

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 04.05.19 01:34, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 23:04, Georg Chini :

On 03.05.19 16:32, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 11:57, Georg Chini :

The channel layout of input/output
and the playback device is known to module-plugin-sink, so if
a filter needs it, it can be passed as parameter. No need to
have it in the interface.

(I have also received your next email, ACK on the thought that the
channel maps don't change on the fly).

Having it (also with other properties like sink or port name) as a
parameter is indeed a neat idea that solves a lot of problems.

Why would the filter code need the sink name? I understand
that it can be useful to know what kind of output you have,
but the sink name will not mean anything to the filter. The
plugins are compiled outside PA, similar to LADSPA plugins.

Because filter sinks can, in theory, be moved, e.g. due to device
hotplug or port status change, to a different master sink, to which
they somehow know they are not applicable.


The filter cannot have a knowledge which sink is "good" or "bad",
as a user you can always rename a sink. This makes it impossible
for a filter to decide if it is applicable based on the sink name.
This is different if the filter is implemented within PA, like the
virtual surround sink. Here you could check after a move if the
number of channels or other conditions do still match.




However, we should still think about the boolean bypass parameter, how
it is supposed to work. Is my understanding below correct?

1. The virtual surround filter gets created by PulseAudio for 6 input
and 2 output channels and gets the input channel layout (5.1), output
channel layout (stereo), and playback sink and port as parameters.
2. Some audio plays through it.
3. The user unplugs headphones, so that the output now goes through
stereo speakers
4. Before sending the next chunk of audio, PulseAudio updates the
filter parameters related to the sink port (a), and/or calls the
set_port callback function (b).
5a. The filter notices the parameter change, processes the audio
anyway, and sets the self-disable parameter to true.
6a. PulseAudio reads the audio and the self-disable parameter, throws
away the processed audio and downmixes 5.1 to stereo by itself.
5b. set_port says "no" or updates the self-disable parameter,
PulseAudio notices and downmixes 5.1 to stereo by itself.

When the filter is in the chain, audio is processed by the filter.
Therefore, down mixing would have to be implemented in the
filter code. The next chunk after the parameter change would
need to be down mixed.
It is impossible to pass the original signal on to PA without
destroying and re-creating the sink input. (See also below).

And that's exactly the problem with the filter sink model when the
filter is applicable only in some cases that depend e.g. on what's
connected.

I have just tried to do this:

0. Make sure that both analog outputs and HDMI outputs are available
as two sinks
1. Add a module-virtual-sink on top of analog stereo output, play some
file in vlc through it.
2. Switch the analog sound card profile to 5.1 profile. AFAIK this
could happen automatically on headphone unplugging on systems that
have a separate headphone jack (mine doesn't, it only has line
outputs, spdif, line input and microphone input on the back panel).

Result: the virtual sink is still on top of the onboard analog sound
card, and vlc is still playing through it.

Yes, but it is playing on a new sink. So if you really want to be
sure that the filter does not move, you can use the autoloaded
flag. This will kill the filter, if the master sink moves (that is
changes number of channels), but not if you switch from
headphones to speaker.


I don't mind at all if you use the existing virtual surround sink code
as an example to validate your API, but due to the above scenario, I
think that it was a mistake to implement the virtual surround effect
as a virtual sink. It should have been a special case in the remixer,
just like the current LFE remixing code is now.


I don't use the virtual surround sink as an example for the external
API. I converted it to the internal version of the API though. To test
the external API I have written a small demo amplifier plugin. As
said above, the virtual surround sink module could check which
kind of sink it is playing on and react accordingly.


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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пт, 3 мая 2019 г. в 23:04, Georg Chini :
>
> On 03.05.19 16:32, Alexander E. Patrakov wrote:
> > пт, 3 мая 2019 г. в 11:57, Georg Chini :
> >> The channel layout of input/output
> >> and the playback device is known to module-plugin-sink, so if
> >> a filter needs it, it can be passed as parameter. No need to
> >> have it in the interface.
> > (I have also received your next email, ACK on the thought that the
> > channel maps don't change on the fly).
> >
> > Having it (also with other properties like sink or port name) as a
> > parameter is indeed a neat idea that solves a lot of problems.
> Why would the filter code need the sink name? I understand
> that it can be useful to know what kind of output you have,
> but the sink name will not mean anything to the filter. The
> plugins are compiled outside PA, similar to LADSPA plugins.

Because filter sinks can, in theory, be moved, e.g. due to device
hotplug or port status change, to a different master sink, to which
they somehow know they are not applicable.

> > However, we should still think about the boolean bypass parameter, how
> > it is supposed to work. Is my understanding below correct?
> >
> > 1. The virtual surround filter gets created by PulseAudio for 6 input
> > and 2 output channels and gets the input channel layout (5.1), output
> > channel layout (stereo), and playback sink and port as parameters.
> > 2. Some audio plays through it.
> > 3. The user unplugs headphones, so that the output now goes through
> > stereo speakers
> > 4. Before sending the next chunk of audio, PulseAudio updates the
> > filter parameters related to the sink port (a), and/or calls the
> > set_port callback function (b).
> > 5a. The filter notices the parameter change, processes the audio
> > anyway, and sets the self-disable parameter to true.
> > 6a. PulseAudio reads the audio and the self-disable parameter, throws
> > away the processed audio and downmixes 5.1 to stereo by itself.
> > 5b. set_port says "no" or updates the self-disable parameter,
> > PulseAudio notices and downmixes 5.1 to stereo by itself.
>
> When the filter is in the chain, audio is processed by the filter.
> Therefore, down mixing would have to be implemented in the
> filter code. The next chunk after the parameter change would
> need to be down mixed.
> It is impossible to pass the original signal on to PA without
> destroying and re-creating the sink input. (See also below).

And that's exactly the problem with the filter sink model when the
filter is applicable only in some cases that depend e.g. on what's
connected.

I have just tried to do this:

0. Make sure that both analog outputs and HDMI outputs are available
as two sinks
1. Add a module-virtual-sink on top of analog stereo output, play some
file in vlc through it.
2. Switch the analog sound card profile to 5.1 profile. AFAIK this
could happen automatically on headphone unplugging on systems that
have a separate headphone jack (mine doesn't, it only has line
outputs, spdif, line input and microphone input on the back panel).

Result: the virtual sink is still on top of the onboard analog sound
card, and vlc is still playing through it.

If this happens with a module-virtual-surround-sink, then it would
convert the 5.1 sound from vlc to stereo according to an algorithm
that is only valid for headphones (or, according to any other
algorithm that it falls back to), and then PulseAudio would upmix this
back to 5.1. Which is bad. The original 5.1 sound should have been
passed through. Or, if it was not 5.1, the remixing should have
happened from the original, not from the sound corrupted to stereo by
the virtual-surround effect.

I don't mind at all if you use the existing virtual surround sink code
as an example to validate your API, but due to the above scenario, I
think that it was a mistake to implement the virtual surround effect
as a virtual sink. It should have been a special case in the remixer,
just like the current LFE remixing code is now.

I do understand that there are cases where the "external filter plugin
sink" model is applicable. Just saying that virtual surround, if done
properly, is not one of them.

> > It would also be interesting to see what happens if unplugging the
> > headphones causes the number of channels (of the channel layout in
> > general) to change. Note that I don't have such setup, not even sure
> > how it is handled currently.
> >
> The sink input that the filter uses will not change, regardless
> what kind of sink the input is connected to. From the filter
> perspective the channel counts and layouts are static. What does
> PA usually do if the number of channels for a stream does not
> match the number of sink channels, for example if you play
> stereo content to a 5.1 sink?

It remixes the content according to the settings in daemon.conf such
as enable-remixing, remixing-use-all-sink-channels and
enable-lfe-remixing.

-- 
Alexander E. Patrakov
___

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 16:32, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 11:57, Georg Chini :

The channel layout of input/output
and the playback device is known to module-plugin-sink, so if
a filter needs it, it can be passed as parameter. No need to
have it in the interface.

(I have also received your next email, ACK on the thought that the
channel maps don't change on the fly).

Having it (also with other properties like sink or port name) as a
parameter is indeed a neat idea that solves a lot of problems.

Why would the filter code need the sink name? I understand
that it can be useful to know what kind of output you have,
but the sink name will not mean anything to the filter. The
plugins are compiled outside PA, similar to LADSPA plugins.


However, we should still think about the boolean bypass parameter, how
it is supposed to work. Is my understanding below correct?

1. The virtual surround filter gets created by PulseAudio for 6 input
and 2 output channels and gets the input channel layout (5.1), output
channel layout (stereo), and playback sink and port as parameters.
2. Some audio plays through it.
3. The user unplugs headphones, so that the output now goes through
stereo speakers
4. Before sending the next chunk of audio, PulseAudio updates the
filter parameters related to the sink port (a), and/or calls the
set_port callback function (b).
5a. The filter notices the parameter change, processes the audio
anyway, and sets the self-disable parameter to true.
6a. PulseAudio reads the audio and the self-disable parameter, throws
away the processed audio and downmixes 5.1 to stereo by itself.
5b. set_port says "no" or updates the self-disable parameter,
PulseAudio notices and downmixes 5.1 to stereo by itself.


When the filter is in the chain, audio is processed by the filter.
Therefore, down mixing would have to be implemented in the
filter code. The next chunk after the parameter change would
need to be down mixed.
It is impossible to pass the original signal on to PA without
destroying and re-creating the sink input. (See also below).



It would also be interesting to see what happens if unplugging the
headphones causes the number of channels (of the channel layout in
general) to change. Note that I don't have such setup, not even sure
how it is handled currently.


The sink input that the filter uses will not change, regardless
what kind of sink the input is connected to. From the filter
perspective the channel counts and layouts are static. What does
PA usually do if the number of channels for a stream does not
match the number of sink channels, for example if you play
stereo content to a 5.1 sink?

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 08:57, Georg Chini wrote:

On 03.05.19 06:56, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 09:31, Georg Chini :

On 03.05.19 02:22, Alexander E. Patrakov wrote:

чт, 2 мая 2019 г. в 23:45, Georg Chini :

Hi,

I created a new module-plugin-sink and a small amplifier demo plugin
based on the attached header file. I did not (yet) drop max_latency,
disable_rewind and rewind_filter() because I am still waiting for 
more

feedback on the specification. I made it however more clear (using
Alexander's wording) that this should only be used if "real" 
rewinding

is possible.




If you think channel layout and playback device should be
in the interface, I can integrate it, but using parameters looks
easier to me because this way we need no additional notification
functions.


The channel maps do not change on the fly, so we can pass them in at
filter creation time as arguments to the create_filter() function.
Then we can have a set_port() function, which - if provided - will be called
by module-plugin-sink when the port of the master sink changes. I will 
integrate

that in the next version.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пт, 3 мая 2019 г. в 11:57, Georg Chini :
> The channel layout of input/output
> and the playback device is known to module-plugin-sink, so if
> a filter needs it, it can be passed as parameter. No need to
> have it in the interface.

(I have also received your next email, ACK on the thought that the
channel maps don't change on the fly).

Having it (also with other properties like sink or port name) as a
parameter is indeed a neat idea that solves a lot of problems.
However, we should still think about the boolean bypass parameter, how
it is supposed to work. Is my understanding below correct?

1. The virtual surround filter gets created by PulseAudio for 6 input
and 2 output channels and gets the input channel layout (5.1), output
channel layout (stereo), and playback sink and port as parameters.
2. Some audio plays through it.
3. The user unplugs headphones, so that the output now goes through
stereo speakers
4. Before sending the next chunk of audio, PulseAudio updates the
filter parameters related to the sink port (a), and/or calls the
set_port callback function (b).
5a. The filter notices the parameter change, processes the audio
anyway, and sets the self-disable parameter to true.
6a. PulseAudio reads the audio and the self-disable parameter, throws
away the processed audio and downmixes 5.1 to stereo by itself.
5b. set_port says "no" or updates the self-disable parameter,
PulseAudio notices and downmixes 5.1 to stereo by itself.

It would also be interesting to see what happens if unplugging the
headphones causes the number of channels (of the channel layout in
general) to change. Note that I don't have such setup, not even sure
how it is handled currently.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 06:56, Alexander E. Patrakov wrote:

пт, 3 мая 2019 г. в 09:31, Georg Chini :

On 03.05.19 02:22, Alexander E. Patrakov wrote:

чт, 2 мая 2019 г. в 23:45, Georg Chini :

Hi,

I created a new module-plugin-sink and a small amplifier demo plugin
based on the attached header file. I did not (yet) drop max_latency,
disable_rewind and rewind_filter() because I am still waiting for more
feedback on the specification. I made it however more clear (using
Alexander's wording) that this should only be used if "real" rewinding
is possible.

Thanks for that.

However, the interface still thinks in terms of "number of channels",
which is, in the general case, wrong. E.g., a "proper"
module-virtual-surround-sink (not what we currently have) would sound
differently if given the same samples in "5.1" and "5.1 (Side)"
channel layouts.

I do not understand what you mean. How would you do it without
number of channels? You have to know how many input and
output channels are there, otherwise you can't process audio.

I am saying that, if we are going to support filters with non-trivial
interaction between channels, we would need to have a pa_channel_map
somewhere. In your header, this structure is not mentioned at all.

"5.1" and "5.1 (Side)" both have 6 channels. The hypothetical proper
virtual surround plugin would need to know if its input is "5.1" or
"5.1 (Side)", but, based on your interface, it can't.


I still don't see your point. The channel layout of input/output
and the playback device is known to module-plugin-sink, so if
a filter needs it, it can be passed as parameter. No need to
have it in the interface.




I wouldn't mind the number of channels, if we limit the interface to
"true" filters that have the same number of input and output channels
and don't care about the channel map. I.e., explicitly declare the
virtual surround sink as out-of-scope. But then, what useful things
are left in scope except the equalizer and maybe a dynamic range
compressor?

Why would there be any reason to limit it to filters with
equal number of channels?

Because that follows from the premise that the filter does not mix
channels. And without channel layout information (i.e. given only the
total number of inputs, but not even their order), it is impossible to
mix channels meaningfully.

See above.



Also, please make sure that, in the callbacks, the filter handle
always comes as the first parameter.

No problem, but usually in pulseaudio the userdata pointer
comes last in callbacks.

Ok, ok, the real idea was "be consistent" :)


Well, things are still at a very early stage and surely I will do
some cleanup once the general approach is accepted.




Regarding the virtual surround plugin, I actually have one more
business argument why, if implemented properly (and not how it is done
currently) it should be out of scope. The reason is that this plugin
with publicly available HRIRs only applies to headphones. From a
usability perspective, it should be switched off (or to a different
filter, specifically crafted for each set of speakers and the
listening room) when the audio is playing to something else than
headphones. The proposed interface does not have (and likely should
not have, because we want to keep it simple and stable) any place to
express such policy.

Why would that be a problem? You can have a boolean bypass
parameter that switches off the filter when needed. Or even a
choice that switches between different HRIR files.

Who would switch it (the virtual surround effect, I am not talking
about other effects) off? The idea is that it is not the user (because
with this particular effect it should really be automatic), and is not
the filter itself (because it cannot know, based on your interface,
that it is playing to speakers). So it has to be some third entity,
with enough knowledge both about the filter and the PulseAudio notion
of ports (to distinguish between speakers, headphones, and something
unknown). And, given that entity, it does not really make sense to
abstract the virtual surround implementation behind some common plugin
interface, it has already leaked.

As an example of prior art, please also consider the LFE channel
remixer - it is not implemented as a virtual sink precisely for this
reason.

As said above, there are parameters that can be passed to
the filter if needed and module-plugin-sink could propagate
changes in the playback device to the filter that way.
We could reserve special parameter names for this information,
because these controls would not be exposed to the user.

If you think channel layout and playback device should be
in the interface, I can integrate it, but using parameters looks
easier to me because this way we need no additional notification
functions.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

пт, 3 мая 2019 г. в 09:31, Georg Chini :
>
> On 03.05.19 02:22, Alexander E. Patrakov wrote:
> > чт, 2 мая 2019 г. в 23:45, Georg Chini :
> >> Hi,
> >>
> >> I created a new module-plugin-sink and a small amplifier demo plugin
> >> based on the attached header file. I did not (yet) drop max_latency,
> >> disable_rewind and rewind_filter() because I am still waiting for more
> >> feedback on the specification. I made it however more clear (using
> >> Alexander's wording) that this should only be used if "real" rewinding
> >> is possible.
> > Thanks for that.
> >
> > However, the interface still thinks in terms of "number of channels",
> > which is, in the general case, wrong. E.g., a "proper"
> > module-virtual-surround-sink (not what we currently have) would sound
> > differently if given the same samples in "5.1" and "5.1 (Side)"
> > channel layouts.
> I do not understand what you mean. How would you do it without
> number of channels? You have to know how many input and
> output channels are there, otherwise you can't process audio.

I am saying that, if we are going to support filters with non-trivial
interaction between channels, we would need to have a pa_channel_map
somewhere. In your header, this structure is not mentioned at all.

"5.1" and "5.1 (Side)" both have 6 channels. The hypothetical proper
virtual surround plugin would need to know if its input is "5.1" or
"5.1 (Side)", but, based on your interface, it can't.

> > I wouldn't mind the number of channels, if we limit the interface to
> > "true" filters that have the same number of input and output channels
> > and don't care about the channel map. I.e., explicitly declare the
> > virtual surround sink as out-of-scope. But then, what useful things
> > are left in scope except the equalizer and maybe a dynamic range
> > compressor?
> Why would there be any reason to limit it to filters with
> equal number of channels?

Because that follows from the premise that the filter does not mix
channels. And without channel layout information (i.e. given only the
total number of inputs, but not even their order), it is impossible to
mix channels meaningfully.

> > Also, please make sure that, in the callbacks, the filter handle
> > always comes as the first parameter.
> No problem, but usually in pulseaudio the userdata pointer
> comes last in callbacks.

Ok, ok, the real idea was "be consistent" :)

> > Regarding the virtual surround plugin, I actually have one more
> > business argument why, if implemented properly (and not how it is done
> > currently) it should be out of scope. The reason is that this plugin
> > with publicly available HRIRs only applies to headphones. From a
> > usability perspective, it should be switched off (or to a different
> > filter, specifically crafted for each set of speakers and the
> > listening room) when the audio is playing to something else than
> > headphones. The proposed interface does not have (and likely should
> > not have, because we want to keep it simple and stable) any place to
> > express such policy.

> Why would that be a problem? You can have a boolean bypass
> parameter that switches off the filter when needed. Or even a
> choice that switches between different HRIR files.

Who would switch it (the virtual surround effect, I am not talking
about other effects) off? The idea is that it is not the user (because
with this particular effect it should really be automatic), and is not
the filter itself (because it cannot know, based on your interface,
that it is playing to speakers). So it has to be some third entity,
with enough knowledge both about the filter and the PulseAudio notion
of ports (to distinguish between speakers, headphones, and something
unknown). And, given that entity, it does not really make sense to
abstract the virtual surround implementation behind some common plugin
interface, it has already leaked.

As an example of prior art, please also consider the LFE channel
remixer - it is not implemented as a virtual sink precisely for this
reason.

> > In other words, I am still very skeptical about the proposal, simply
> > because we don't have enough filters to test the validity of the
> > proposed interface.
> >
> I guess we need to start somewhere.

Fair enough.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Georg Chini]

On 03.05.19 02:22, Alexander E. Patrakov wrote:

чт, 2 мая 2019 г. в 23:45, Georg Chini :

Hi,

I created a new module-plugin-sink and a small amplifier demo plugin
based on the attached header file. I did not (yet) drop max_latency,
disable_rewind and rewind_filter() because I am still waiting for more
feedback on the specification. I made it however more clear (using
Alexander's wording) that this should only be used if "real" rewinding
is possible.

Thanks for that.

However, the interface still thinks in terms of "number of channels",
which is, in the general case, wrong. E.g., a "proper"
module-virtual-surround-sink (not what we currently have) would sound
differently if given the same samples in "5.1" and "5.1 (Side)"
channel layouts.

I do not understand what you mean. How would you do it without
number of channels? You have to know how many input and
output channels are there, otherwise you can't process audio.


I wouldn't mind the number of channels, if we limit the interface to
"true" filters that have the same number of input and output channels
and don't care about the channel map. I.e., explicitly declare the
virtual surround sink as out-of-scope. But then, what useful things
are left in scope except the equalizer and maybe a dynamic range
compressor?

Why would there be any reason to limit it to filters with
equal number of channels?


Also, please make sure that, in the callbacks, the filter handle
always comes as the first parameter.

No problem, but usually in pulseaudio the userdata pointer
comes last in callbacks.


Regarding the virtual surround plugin, I actually have one more
business argument why, if implemented properly (and not how it is done
currently) it should be out of scope. The reason is that this plugin
with publicly available HRIRs only applies to headphones. From a
usability perspective, it should be switched off (or to a different
filter, specifically crafted for each set of speakers and the
listening room) when the audio is playing to something else than
headphones. The proposed interface does not have (and likely should
not have, because we want to keep it simple and stable) any place to
express such policy.

Why would that be a problem? You can have a boolean bypass
parameter that switches off the filter when needed. Or even a
choice that switches between different HRIR files.


In other words, I am still very skeptical about the proposal, simply
because we don't have enough filters to test the validity of the
proposed interface.


I guess we need to start somewhere.

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

Re: [pulseaudio-discuss] new module module-plugin-sink

[Alexander E. Patrakov]

чт, 2 мая 2019 г. в 23:45, Georg Chini :
>
> Hi,
>
> I created a new module-plugin-sink and a small amplifier demo plugin
> based on the attached header file. I did not (yet) drop max_latency,
> disable_rewind and rewind_filter() because I am still waiting for more
> feedback on the specification. I made it however more clear (using
> Alexander's wording) that this should only be used if "real" rewinding
> is possible.

Thanks for that.

However, the interface still thinks in terms of "number of channels",
which is, in the general case, wrong. E.g., a "proper"
module-virtual-surround-sink (not what we currently have) would sound
differently if given the same samples in "5.1" and "5.1 (Side)"
channel layouts.

I wouldn't mind the number of channels, if we limit the interface to
"true" filters that have the same number of input and output channels
and don't care about the channel map. I.e., explicitly declare the
virtual surround sink as out-of-scope. But then, what useful things
are left in scope except the equalizer and maybe a dynamic range
compressor?

Also, please make sure that, in the callbacks, the filter handle
always comes as the first parameter.

Regarding the virtual surround plugin, I actually have one more
business argument why, if implemented properly (and not how it is done
currently) it should be out of scope. The reason is that this plugin
with publicly available HRIRs only applies to headphones. From a
usability perspective, it should be switched off (or to a different
filter, specifically crafted for each set of speakers and the
listening room) when the audio is playing to something else than
headphones. The proposed interface does not have (and likely should
not have, because we want to keep it simple and stable) any place to
express such policy.

In other words, I am still very skeptical about the proposal, simply
because we don't have enough filters to test the validity of the
proposed interface.

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