mbox series

[BlueZ,0/3] Add transport.select method

Message ID 20240710081338.17262-1-vlad.pruteanu@nxp.com
Headers show
Series Add transport.select method | expand

Message

Vlad Pruteanu July 10, 2024, 8:13 a.m. UTC
This series adds a new "select" method that can be called by
the user on the broadcast sink side, to select the stream for
which synchronization is desired. This is achieved by changing
the states flow so that transport are not set to pending automatically
anymore. Instead, the transport must first be selected by the user,
which will update it's state from idle to pending. Any pending
transport will be acquired by PipeWire.

Furthermore, this method will be used for setting the broadcast code
of a stream on the sink side. If the encryption flag is set, the
transport.select call in bluetoothctl will prompt the user to enter
the code

Vlad Pruteanu (3):
  transport: Add "select" method
  client/player: Expose transport "select" method to the user
  transport: Broadcast sink: wait for user to select transport

 client/player.c            | 52 ++++++++++++++++++++++++++++++++++++++
 profiles/audio/transport.c | 29 ++++++++++++++++++---
 2 files changed, 77 insertions(+), 4 deletions(-)

Comments

Vlad Pruteanu July 19, 2024, 7:34 a.m. UTC | #1
Hello Luiz,

> Hi Vlad,
> 
> On Wed, Jul 10, 2024 at 4:13 AM Vlad Pruteanu <vlad.pruteanu@nxp.com>
> wrote:
> >
> > This series adds a new "select" method that can be called by
> > the user on the broadcast sink side, to select the stream for
> > which synchronization is desired. This is achieved by changing
> > the states flow so that transport are not set to pending automatically
> > anymore. Instead, the transport must first be selected by the user,
> > which will update it's state from idle to pending. Any pending
> > transport will be acquired by PipeWire.
> 
> Hmm, perhaps it would have been better that PipeWire don't auto
> pick-up transport on pending state if there are broadcasters, or we
> could perhaps perhaps use a different state e.g. "broadcasting", since
> pending doesn't really apply for broadcasters as far as streaming is
> concerned.
> 
What I propose with this patch is to slightly change how the PENDING
state is triggered on the Broadcast Sink side.
 
Currently:
A Sink scans a Source and automatically moves the associated transport
to PENDING. PipeWire sees this and acquires.
 
My implementation:
A Sink scans a Source,  the associated transport remains in IDLE. User
does transport.select, moving it to PENDING. PipeWire sees this and
acquires.
 
So I wouldn't be changing how the PENDING state is used, just how the
transport gets there. In any case, I think that everything is in line with
the comment for this state:
TRANSPORT_STATE_PENDING,	/* Playing but not acquired */
 
Nonetheless, if you think that the use of this state in the Broadcast context
can cause confusion I will add a comment clarifying it's meaning. Perhaps it
could be placed in the select_transport function.

> > Furthermore, this method will be used for setting the broadcast code
> > of a stream on the sink side. If the encryption flag is set, the
> > transport.select call in bluetoothctl will prompt the user to enter
> > the code
> 
> The roles of bluetoothctl is mostly to demonstrate how client can
> implement the handling of D-Bus APIs, so we have to be careful not to
> assume it will be used to replace things like Pipewire/audio settings,
> so for instance the broadcast code shall be provided by the same
> entity attempting to do Transport.Acquire otherwise things may get a
> little too convoluted if we need different entities to set transport
> up before it is ready to be acquired.
> 
> > Vlad Pruteanu (3):
> >   transport: Add "select" method
> >   client/player: Expose transport "select" method to the user
> >   transport: Broadcast sink: wait for user to select transport
> >
> >  client/player.c            | 52 ++++++++++++++++++++++++++++++++++++++
> >  profiles/audio/transport.c | 29 ++++++++++++++++++---
> >  2 files changed, 77 insertions(+), 4 deletions(-)
> >
> > --
> > 2.40.1
> >
> 
> 
> --
> Luiz Augusto von Dentz
Luiz Augusto von Dentz July 19, 2024, 2:42 p.m. UTC | #2
Hi Vlad,

On Fri, Jul 19, 2024 at 3:34 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
>
> Hello Luiz,
>
> > Hi Vlad,
> >
> > On Wed, Jul 10, 2024 at 4:13 AM Vlad Pruteanu <vlad.pruteanu@nxp.com>
> > wrote:
> > >
> > > This series adds a new "select" method that can be called by
> > > the user on the broadcast sink side, to select the stream for
> > > which synchronization is desired. This is achieved by changing
> > > the states flow so that transport are not set to pending automatically
> > > anymore. Instead, the transport must first be selected by the user,
> > > which will update it's state from idle to pending. Any pending
> > > transport will be acquired by PipeWire.
> >
> > Hmm, perhaps it would have been better that PipeWire don't auto
> > pick-up transport on pending state if there are broadcasters, or we
> > could perhaps perhaps use a different state e.g. "broadcasting", since
> > pending doesn't really apply for broadcasters as far as streaming is
> > concerned.
> >
> What I propose with this patch is to slightly change how the PENDING
> state is triggered on the Broadcast Sink side.
>
> Currently:
> A Sink scans a Source and automatically moves the associated transport
> to PENDING. PipeWire sees this and acquires.
>
> My implementation:
> A Sink scans a Source,  the associated transport remains in IDLE. User
> does transport.select, moving it to PENDING. PipeWire sees this and
> acquires.
>
> So I wouldn't be changing how the PENDING state is used, just how the
> transport gets there. In any case, I think that everything is in line with
> the comment for this state:
> TRANSPORT_STATE_PENDING,        /* Playing but not acquired */
>
> Nonetheless, if you think that the use of this state in the Broadcast context
> can cause confusion I will add a comment clarifying it's meaning. Perhaps it
> could be placed in the select_transport function.

Got it, while this could work I think having a dedicated state for
broadcasters is better since we can document exactly the behavior
since these transports are not attached to any endpoint it need to
manually be acquired by the user rather than automatically like
unicast.

> > > Furthermore, this method will be used for setting the broadcast code
> > > of a stream on the sink side. If the encryption flag is set, the
> > > transport.select call in bluetoothctl will prompt the user to enter
> > > the code
> >
> > The roles of bluetoothctl is mostly to demonstrate how client can
> > implement the handling of D-Bus APIs, so we have to be careful not to
> > assume it will be used to replace things like Pipewire/audio settings,
> > so for instance the broadcast code shall be provided by the same
> > entity attempting to do Transport.Acquire otherwise things may get a
> > little too convoluted if we need different entities to set transport
> > up before it is ready to be acquired.
> >
> > > Vlad Pruteanu (3):
> > >   transport: Add "select" method
> > >   client/player: Expose transport "select" method to the user
> > >   transport: Broadcast sink: wait for user to select transport
> > >
> > >  client/player.c            | 52 ++++++++++++++++++++++++++++++++++++++
> > >  profiles/audio/transport.c | 29 ++++++++++++++++++---
> > >  2 files changed, 77 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.40.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
patchwork-bot+bluetooth@kernel.org Aug. 1, 2024, 9:20 a.m. UTC | #3
Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed, 10 Jul 2024 11:13:35 +0300 you wrote:
> This series adds a new "select" method that can be called by
> the user on the broadcast sink side, to select the stream for
> which synchronization is desired. This is achieved by changing
> the states flow so that transport are not set to pending automatically
> anymore. Instead, the transport must first be selected by the user,
> which will update it's state from idle to pending. Any pending
> transport will be acquired by PipeWire.
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/3] transport: Add "select" method
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=083d1a7b66b5
  - [BlueZ,2/3] client/player: Expose transport "select" method to the user
    (no matching commit)
  - [BlueZ,3/3] transport: Broadcast sink: wait for user to select transport
    (no matching commit)

You are awesome, thank you!