by Alain Michaud

[permalink] [raw]

Subject: Re: [Bluez PATCH v1 1/2] doc:adding a WidebandSpeechEnabled Api

Hi Marcel,

Indeed, we do have the packet_msg patch to carry the erroneous data
flag. We will send results once validated.

However, the profile implementation will still need to know if
erroneous data reporting is enabled/supported and that the USB driver
is correctly using the alt setting on the USB isoch endpoint. As a
result, we still need to carry some form of notification that all of
these things are supported from kernel below. Again, this is a
controller attribute, not a connection attribute so it may best be
served outside of the sco socket.

Thanks,
Alain

On Wed, May 6, 2020 at 12:43 PM Marcel Holtmann <[email protected]> wrote:
>
> Hi Luiz,
>
> >>>>>>>>>>>>> This change adds an adapter Api to report the controller's
> >>>>>>>>>>>>> widebandspeech enabled state.
> >>>>>>>>>>>
> >>>>>>>>>>> I wonder if this shouldn't be queried over SCO socket, or simple fail
> >>>>>>>>>>> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> >>>>>>>>>>> normally used when using mSBC.
> >>>>>>>>>>
> >>>>>>>>>> I think there is value in both.
> >>>>>>>>>
> >>>>>>>>> Can you expand on that? I think this might generate confusion if the
> >>>>>>>>> property indicates support for it but HFP implementation don't support
> >>>>>>>>> it, since the later is usually implemented as a external profile so we
> >>>>>>>>> don't have the features it may support, or perhaps the intention here
> >>>>>>>>> is to actually indicate when it is in use?
> >>>>>>>>
> >>>>>>>> This is a signal that the adapter supports it and has everything
> >>>>>>>> enabled to support it. Driver indicated it supports it and erroneous
> >>>>>>>> data reporting was enabled. The profile has it's own state which may
> >>>>>>>> indicate if msbc will be used, but this will be on a per connection
> >>>>>>>> basis and is independent from this adapter property.
> >>>>>>>>
> >>>>>>>> The value in this property is to support diagnostic UX about
> >>>>>>>> controller capabilities/state and also allow profiles that are
> >>>>>>>> implemented outside of bluetoothd to see which codec it can attempt to
> >>>>>>>> negotiate with the device.
> >>>>>>>
> >>>>>>> For diagnosic I think we would be better off with some dedicated
> >>>>>>> interface to query this, as for the later the information we are
> >>>>>>> giving does not actually tell anything about the codec support, which
> >>>>>>> was part of my original argument that for the likes of HFP and other
> >>>>>>> profiles using it it might not be enough and they still need to use
> >>>>>>> BT_VOICE in order to enable the use of custom codecs, if you take
> >>>>>>> ofono for example it does implement support for wideband speech
> >>>>>>> already and it would completely disregard this property which can give
> >>>>>>> the false impression that wideband speech cannot be activated when in
> >>>>>>> fact it can, it just don't have erroneous data reporting enable, so
> >>>>>>> perhaps we should indicate the actual adapter feature (e.g.
> >>>>>>> ErrnoneousDataReporting) not the profile feature here, so the profile
> >>>>>>> implementation can check weather this would disable use of wideband
> >>>>>>> speech or not, futhermore we should probably report the errors back to
> >>>>>>> the SCO socket or is that just for diagnostic and cannot be used to
> >>>>>>> adjust the streaming?
> >>>>>>
> >>>>>> My original patch actually had this MGMT feature called erroneous data
> >>>>>> reporting and Marcel argued against it. If you both agree, then I'm
> >>>>>> happy to rename all of this to erroneous data reporting. We'd still
> >>>>>> need some way for the driver support to be messaged some other way
> >>>>>> though. see:
> >>>>>>
> >>>>>> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> >>>>>> set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
> >>>
> >>> So this will get a little bit confusing if we don't have a HFP
> >>> implementation that actually does implement the profile parts to
> >>> enable wideband speech so I wonder if we should rename it to the
> >>> underline feature it enables, that said this sort of feature is
> >>> probably easier to be queried over the socket itself rather than over
> >>> D-Bus so it can be used in conjunction with the likes of BT_VOICE,
> >>> also regarding the erroneous data should that be also enabled by the
> >>> HFP profile, because depending on the platform it may not support
> >>> wideband speech so enabling erroneous data automatically may result in
> >>> artifacs, specially in cases where the codecs don't have error
> >>> correction or data loss concealment to handle data with possible error
> >>> and lost data respectively, btw we don't seem to be parsing the
> >>> SCO/ESCO flags and even if we do this properly we need to find a way
> >>> to notify them over the socket so things like plc works properly.
> >>
> >> This indeed makes sense, but we'll need to decide how erroneous data
> >> reporting gets enabled. The SCO socket is a per connection scope
> >> thing while erroneous data reporting is a controller level thing.
> >
> > The way I would have done this is to make use of recvmsg and then add
> > the packet flags as msg_flags:
> >
> > recvmsg()
> > The recvmsg() call uses a msghdr structure to minimize the
> > number of directly supplied arguments. This structure is defined as
> > follows in <sys/socket.h>:
> >
> > struct iovec { /* Scatter/gather array items */
> > void *iov_base; /* Starting address */
> > size_t iov_len; /* Number of bytes to transfer */
> > };
> >
> > struct msghdr {
> > void *msg_name; /* Optional address */
> > socklen_t msg_namelen; /* Size of address */
> > struct iovec *msg_iov; /* Scatter/gather array */
> > size_t msg_iovlen; /* # elements in msg_iov */
> > void *msg_control; /* Ancillary data, see below */
> > size_t msg_controllen; /* Ancillary data buffer len */
> > int msg_flags; /* Flags on received message */
> > };
> >
> > But to make this backward compatible I would recommend adding a
> > socketopt that enables this new behavior and in case there is more
> > than one SCO socket those that did not use the sockopt should probably
> > drop packets with error or lost data so they work as without erroneous
> > data report since in that case the application will likely be using
> > regular reads it won't be able to detect the use of msg_flags.
>
> I think that I already send an example on how to do that a while ago. Don’t remember if that was a private email or addressed to the mailing list, but I remember showing how this can be done. Or I confused this with something similar :(
>
> Regards
>
> Marcel
>