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

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

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

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

typedef struct filter_plugin {
const char *name; /* Name of the filter */
unsigned input_channels;  /* Number of audio input channels, 0 if variable */
unsigned output_channels; /* Number of audio output channels, 0 if variable */

const char *desc_head;/* Leading part of description string */
const char *filter_type;  /* Name for the type of filter */

size_t max_chunk_size;/* Maximum chunk size in bytes that the filter will
   * accept, 0 for default */
size_t fixed_block_size;  /* Block size in frames for fixed block size filters,
   * 0 if block size is variable */
uint64_t max_latency; /* Maximum latency allowed for the sink, 0 if unused */
bool disable_rewind;  /* True when rewinding is disabled */

/* Callback to rewind the filter when pulseaudio requests it. May be NULL.
 * Amount indicates the number of bytes to roll back. The filter state must
 * not be reset, but seamlessly restored to the specified point in the past
 * (which is the filter's responsibility to keep).
 * If it is not possible to do so, the best option is to disable rewinding
 * completely and limit the latency. */
void (*rewind_filter)(void *filter_handle, size_t amount);

/* Callback to process a chunk of data by the filter. May be NULL */
void (*process_chunk)(float *src, float *dst, unsigned count, void *filter_handle);

/* Callback to retrieve additional latency caused by the filter. May be NULL */
uint64_t (*get_extra_latency)(void *filter_handle);

/* Initializes a new filter instance and returns a handle to it. */
void *(*create_filter)(unsigned input_channels, unsigned output_channels, unsigned sample_rate);

/* Deletes filter instance. */
void (*delete_filter)(void *filter_handle);

/* Activates filter. Returns 0 on success and a negative errror code on failure.
 * May be NULL. */
int (*activate_filter)(void *filter_handle);

/* Deactivates filter. May be NULL. */
void (*deactivate_filter)(void *filter_handle);

/* If set, this function is called in thread context when an update of the
 * filter parameters is requested, may be NULL. The function must replace
 * the currently used parameter structure by the new structure and return
 * a pointer to the old structure so that it can be freed in the main thread
 * using parameter_free(). If the old structure can be re-used, the function
 * may return NULL. */
void *(*update_filter_parameters)(void *parameters, void *filter_handle);

/* Frees a parameter structure. May only be NULL, if update_filter_parameters()
 * is also NULL or if update_filter_parameters() always returns NULL. */
void (*parameter_free)(void *parameters);

/* The following functions can be provided to set and get parameters. The parameter
 * structure is defined by the filter and should contain all parameters that are
 * configurable by the user. The library code makes no assumption on the contents
 * of the structure but only passes references. The library implements a message
 * handler which supports the following messages that use the functions below:
 * parameter-get - Retrieve a single parameter.
 * parameter-set - Set a single parameter.
 * parameter-get-all - Retrieve all parameters as a list in message format.
 * parameter-set-all - Set all parameters simultaneously.
 * parameter-get-description - Returns a filter description and a list that describes
 * all parameters. Example of the list element format:
 * 

Re: [pulseaudio-discuss] [PATCH v9 0/8] Bluetooth A2DP codecs

[Pali Rohár]

On Tuesday 30 April 2019 12:42:28 Luiz Augusto von Dentz wrote:
> Btw, if you are to send a v10 also include the following:
> 
> @@ -1838,6 +1847,9 @@ static DBusMessage
> *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>  t->release = bluez5_transport_release_cb;
>  pa_bluetooth_transport_put(t);
> 
> +if (!d->change_a2dp_profile_in_progress)
> +pa_bluetooth_transport_set_state(t,
> PA_BLUETOOTH_TRANSPORT_STATE_PLAYING);
> +
>  pa_log_debug("Transport %s available for profile %s", t->path,
> pa_bluetooth_profile_to_string(t->profile));
> 
>  return NULL;
> 
> Otherwise last used logic don't work as intended.

I'm going to rework patches which choose initial profile. It would be
based on transport state (and not on availability). So all those "hacks"
are not needed.

-- 
Pali Rohár
pali.ro...@gmail.com


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

Re: [pulseaudio-discuss] [PATCH v9 0/8] Bluetooth A2DP codecs

[Pali Rohár]

On Friday 26 April 2019 12:01:15 Tanu Kaskinen wrote:
> On Thu, 2019-04-25 at 18:54 +0200, Pali Rohár wrote:
> > On Thursday 25 April 2019 18:51:49 Pali Rohár wrote:
> > > On Thursday 25 April 2019 19:43:28 Luiz Augusto von Dentz wrote:
> > > > Hi Pali,
> > > > 
> > > > On Thu, Apr 25, 2019 at 2:42 PM Luiz Augusto von Dentz
> > > >  wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On Thu, Apr 25, 2019 at 2:35 PM Pali Rohár  
> > > > > wrote:
> > > > > > On Thursday 25 April 2019 13:28:16 Pali Rohár wrote:
> > > > > > > On Thursday 25 April 2019 14:19:15 Luiz Augusto von Dentz wrote:
> > > > > > > > These seems to work great, I can even switch on the fly the 
> > > > > > > > profiles
> > > > > > > > and after a short delay it switches without a problem, there is 
> > > > > > > > one
> > > > > > > > issue related to reconnecting though:
> > > > > > > > 
> > > > > > > > https://gist.github.com/Vudentz/40f10e85fb860083958edae67093f016
> > > > > > > > 
> > > > > > > > With BlueZ remembering the last used endpoint (aptX) it seems 
> > > > > > > > the
> > > > > > > > policy ignores that and reverts to highest priority (SBC UHQ),
> > > > > > 
> > > > > > Relevant lines:
> > > > > > 
> > > > > > D: [lt-pulseaudio] bluez5-util.c: Transport 
> > > > > > /org/bluez/hci0/dev_94_20_53_2E_08_CE/sep5/fd26 available for 
> > > > > > profile a2dp_sink_aptx
> > > > > > ...
> > > > > > D: [lt-pulseaudio] card.c: a2dp_sink_aptx availability unknown
> > > > > > ...
> > > > > > D: [lt-pulseaudio] card.c: off availability yes
> > > > > > I: [lt-pulseaudio] card.c: bluez_card.94_20_53_2E_08_CE: 
> > > > > > active_profile: a2dp_sink_sbc_uhq2
> > > > > > D: [lt-pulseaudio] module-bluetooth-policy.c: Looking for A2DP 
> > > > > > profile activated by bluez for card bluez_card.94_20_53_2E_08_CE
> > > > > > I: [lt-pulseaudio] card.c: Created 5 "bluez_card.94_20_53_2E_08_CE"
> > > > > > 
> > > > > > We got information that sep5 is activated with fd26 and it 
> > > > > > corespondent
> > > > > > to profile a2dp_sink_aptx. And on next lines we see that profile has
> > > > > > unknown availability -- which means that it is possible to switch to
> > > > > > that codec/profile, but it is not activated yet. On next lines we 
> > > > > > see
> > > > > > that module-bluetooth-policy is trying to find "a2dp_*" which has
> > > > > > availability "on", but there is no one. So initial profile stay
> > > > > > a2dp_sink_sbc_uhq2 which was chosen as default by card.c.
> > > > > > 
> > > > > > So problem is why a2dp_sink_aptx profile has unknown availability 
> > > > > > even
> > > > > > it is activated? It should have "on" availability. And then policy
> > > > > > choose it as initial.
> > > > > 
> > > > > Right, looks like the state is not correct since it has a fd already
> > > > > it should have been marked available.
> > > > 
> > > > Problem seems to be that we need to set the transport state to playing
> > > > since we introduce the following code:
> > > > 
> > > > if (cp->available == PA_AVAILABLE_NO &&
> > > > u->support_a2dp_codec_switch && pa_bluetooth_profile_is_a2dp(profile))
> > > > cp->available = PA_AVAILABLE_UNKNOWN;
> > > > 
> > > > That means every A2DP profile will be set to unknown including even if
> > > > they have no transport yet
> > > 
> > > That is truth. But if there is a transport then availability is YES.
> > > 
> > > Availability NO is used when it is not possible to activate transport
> > > because it is unsupported (e.g. A2DP not connected or when bluez does
> > > not support profile switching).
> > > 
> > > > so now we have to set the initial transport to playing
> > > 
> > > How it should help? I do not see reason now...
> > > 
> > > Anyway, I was not able to reproduce your problem, basically I always had
> > > availability for activated profile set to YES.
> > 
> > Now I see it. PA_AVAILABLE_YES is returned only for
> > PA_BLUETOOTH_TRANSPORT_STATE_PLAYING.
> > 
> > So for PA_BLUETOOTH_TRANSPORT_STATE_IDLE we also get
> > PA_AVAILABLE_UNKNOWN and therefore module-bluetooth-policy does not know
> > which transport is activated.
> > 
> > Cannot we get transport state in module-bluetooth-policy? Seems that
> > this is the right direction.
> 
> Yes, I think module-bluetooth-policy should look at the transport
> states directly, if that provides better information than the profile
> availability.

Yes, code which choose initial profile should really look at transport
state. But this is not possible in module-blueooth-policy as transport
state is private member of module-bluez5-device.

I will move that code which choose initial profile into
module-bluez5-device. Really it should be there as it is up to the
module itself to decide which is the best initial profile. Other modules
(based on user settings) may override it. But initial setting should be
done by bluez which knows which transport is already activated.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature

Re: [pulseaudio-discuss] [PATCH v9 0/8] Bluetooth A2DP codecs

[Pali Rohár]

On Friday 26 April 2019 12:06:08 Tanu Kaskinen wrote:
> On Thu, 2019-04-25 at 19:03 +0200, Pali Rohár wrote:
> > On Thursday 25 April 2019 18:54:05 Pali Rohár wrote:
> > > On Thursday 25 April 2019 18:51:49 Pali Rohár wrote:
> > > > On Thursday 25 April 2019 19:43:28 Luiz Augusto von Dentz wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On Thu, Apr 25, 2019 at 2:42 PM Luiz Augusto von Dentz
> > > > >  wrote:
> > > > > > Hi Pali,
> > > > > > 
> > > > > > On Thu, Apr 25, 2019 at 2:35 PM Pali Rohár  
> > > > > > wrote:
> > > > > > > On Thursday 25 April 2019 13:28:16 Pali Rohár wrote:
> > > > > > > > On Thursday 25 April 2019 14:19:15 Luiz Augusto von Dentz wrote:
> > > > > > > > > These seems to work great, I can even switch on the fly the 
> > > > > > > > > profiles
> > > > > > > > > and after a short delay it switches without a problem, there 
> > > > > > > > > is one
> > > > > > > > > issue related to reconnecting though:
> > > > > > > > > 
> > > > > > > > > https://gist.github.com/Vudentz/40f10e85fb860083958edae67093f016
> > > > > > > > > 
> > > > > > > > > With BlueZ remembering the last used endpoint (aptX) it seems 
> > > > > > > > > the
> > > > > > > > > policy ignores that and reverts to highest priority (SBC UHQ),
> > > > > > > 
> > > > > > > Relevant lines:
> > > > > > > 
> > > > > > > D: [lt-pulseaudio] bluez5-util.c: Transport 
> > > > > > > /org/bluez/hci0/dev_94_20_53_2E_08_CE/sep5/fd26 available for 
> > > > > > > profile a2dp_sink_aptx
> > > > > > > ...
> > > > > > > D: [lt-pulseaudio] card.c: a2dp_sink_aptx availability unknown
> > > > > > > ...
> > > > > > > D: [lt-pulseaudio] card.c: off availability yes
> > > > > > > I: [lt-pulseaudio] card.c: bluez_card.94_20_53_2E_08_CE: 
> > > > > > > active_profile: a2dp_sink_sbc_uhq2
> > > > > > > D: [lt-pulseaudio] module-bluetooth-policy.c: Looking for A2DP 
> > > > > > > profile activated by bluez for card bluez_card.94_20_53_2E_08_CE
> > > > > > > I: [lt-pulseaudio] card.c: Created 5 
> > > > > > > "bluez_card.94_20_53_2E_08_CE"
> > > > > > > 
> > > > > > > We got information that sep5 is activated with fd26 and it 
> > > > > > > corespondent
> > > > > > > to profile a2dp_sink_aptx. And on next lines we see that profile 
> > > > > > > has
> > > > > > > unknown availability -- which means that it is possible to switch 
> > > > > > > to
> > > > > > > that codec/profile, but it is not activated yet. On next lines we 
> > > > > > > see
> > > > > > > that module-bluetooth-policy is trying to find "a2dp_*" which has
> > > > > > > availability "on", but there is no one. So initial profile stay
> > > > > > > a2dp_sink_sbc_uhq2 which was chosen as default by card.c.
> > > > > > > 
> > > > > > > So problem is why a2dp_sink_aptx profile has unknown availability 
> > > > > > > even
> > > > > > > it is activated? It should have "on" availability. And then policy
> > > > > > > choose it as initial.
> > > > > > 
> > > > > > Right, looks like the state is not correct since it has a fd already
> > > > > > it should have been marked available.
> > > > > 
> > > > > Problem seems to be that we need to set the transport state to playing
> > > > > since we introduce the following code:
> > > > > 
> > > > > if (cp->available == PA_AVAILABLE_NO &&
> > > > > u->support_a2dp_codec_switch && pa_bluetooth_profile_is_a2dp(profile))
> > > > > cp->available = PA_AVAILABLE_UNKNOWN;
> > > > > 
> > > > > That means every A2DP profile will be set to unknown including even if
> > > > > they have no transport yet
> > > > 
> > > > That is truth. But if there is a transport then availability is YES.
> > > > 
> > > > Availability NO is used when it is not possible to activate transport
> > > > because it is unsupported (e.g. A2DP not connected or when bluez does
> > > > not support profile switching).
> > > > 
> > > > > so now we have to set the initial transport to playing
> > > > 
> > > > How it should help? I do not see reason now...
> > > > 
> > > > Anyway, I was not able to reproduce your problem, basically I always had
> > > > availability for activated profile set to YES.
> > > 
> > > Now I see it. PA_AVAILABLE_YES is returned only for
> > > PA_BLUETOOTH_TRANSPORT_STATE_PLAYING.
> > > 
> > > So for PA_BLUETOOTH_TRANSPORT_STATE_IDLE we also get
> > > PA_AVAILABLE_UNKNOWN and therefore module-bluetooth-policy does not know
> > > which transport is activated.
> > > 
> > > Cannot we get transport state in module-bluetooth-policy? Seems that
> > > this is the right direction.
> > 
> > Currently we have following mapping:
> > 
> > PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED --> PA_AVAILABLE_NO
> > PA_BLUETOOTH_TRANSPORT_STATE_PLAYING --> PA_AVAILABLE_YES
> > PA_BLUETOOTH_TRANSPORT_STATE_IDLE --> PA_AVAILABLE_UNKNOWN
> > 
> > Plus PA_AVAILABLE_NO is changed to PA_AVAILABLE_UNKNOWN when pulseaudio
> > can activate that transport via bluez codec switch API as described
> > above.
> > 
> > PA_PORT_AVAILABLE_YES is defined as:
> > This port is available, likely because the jack