Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Tsoy <alexander@tsoy.me>
To: Pavel Hofman <pavel.hofman@ivitera.com>, alsa-devel@alsa-project.org
Cc: Takashi Iwai <tiwai@suse.de>, Gregor Pintar <grpintar@gmail.com>,
	Roope Salmi <rpsalmi@gmail.com>
Subject: Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation
Date: Fri, 24 Apr 2020 12:29:53 +0300
Message-ID: <a876d867496c2c58866a626430dd2f174f16c107.camel@tsoy.me> (raw)
In-Reply-To: <11dca14b-39a7-635a-a62f-ea10f21aa697@ivitera.com>

В Пт, 24/04/2020 в 11:19 +0200, Pavel Hofman пишет:
> Dne 24. 04. 20 v 4:24 Alexander Tsoy napsal(a):
> > For computation of the the next frame size current value of fs/fps
> > and
> > accumulated fractional parts of fs/fps are used, where values are
> > stored
> > in Q16.16 format. This is quite natural for computing frame size
> > for
> > asynchronous endpoints driven by explicit feedback, since in this
> > case
> > fs/fps is a value provided by the feedback endpoint and it's
> > already in
> > the Q format. If an error is accumulated over time, the device can
> > adjust fs/fps value to prevent buffer overruns/underruns.
> > 
> > But for synchronous endpoints the accuracy provided by these
> > computations
> > is not enough. Due to accumulated error the driver periodically
> > produces
> > frames with incorrect size (+/- 1 audio sample).
> > 
> > This patch fixes this issue by implementing a different algorithm
> > for
> > frame size computation. It is based on accumulating of the
> > remainders
> > from division fs/fps and it doesn't accumulate errors over time.
> > This
> > new method is enabled for synchronous and adaptive playback
> > endpoints.
> > 
> > Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
> > ---
> >  sound/usb/card.h     |  4 ++++
> >  sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++
> > -----
> >  sound/usb/endpoint.h |  1 +
> >  sound/usb/pcm.c      |  2 ++
> >  4 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/usb/card.h b/sound/usb/card.h
> > index 395403a2d33f..820e564656ed 100644
> > --- a/sound/usb/card.h
> > +++ b/sound/usb/card.h
> > @@ -84,6 +84,10 @@ struct snd_usb_endpoint {
> >  	dma_addr_t sync_dma;		/* DMA address of syncbuf
> > */
> >  
> >  	unsigned int pipe;		/* the data i/o pipe */
> > +	unsigned int framesize[2];	/* small/large frame sizes in
> > samples */
> > +	unsigned int sample_rem;	/* remainder from division fs/fps
> > */
> > +	unsigned int sample_accum;	/* sample accumulator */
> > +	unsigned int fps;		/* frames per second */
> >  	unsigned int freqn;		/* nominal sampling rate in fs/fps
> > in Q16.16 format */
> >  	unsigned int freqm;		/* momentary sampling rate in
> > fs/fps in Q16.16 format */
> >  	int	   freqshift;		/* how much to shift the feedback
> > value to get Q16.16 */
> > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> > index 4a9a2f6ef5a4..d8dc7cb56d43 100644
> > --- a/sound/usb/endpoint.c
> > +++ b/sound/usb/endpoint.c
> > @@ -124,12 +124,12 @@ int
> > snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint
> > *ep)
> >  
> >  /*
> >   * For streaming based on information derived from sync endpoints,
> > - * prepare_outbound_urb_sizes() will call next_packet_size() to
> > + * prepare_outbound_urb_sizes() will call slave_next_packet_size()
> > to
> >   * determine the number of samples to be sent in the next packet.
> 
> Please should not this read
> 
> "For streaming based on information derived from async endpoints,"
> 
> or
> 
> "For streaming based on information derived from sync-master
> endpoints,"?

"sync endpoints" actually means "feedback endpoints" here. This is the
terminology used by the driver. So it is not the type of
synchronization of the endpoint for which this function is being
called. :)

Probably comment I made for snd_usb_endpoint_next_packet_size() is
slightly inaccurate, because this function will be also used for
asynchronous endpoints in the case feedback endpoint is not configured
for some reason.

> 
> Because the next method says:
> 
> For adaptive and synchronous endpoints,
> prepare_outbound_urb_sizes()...
> 
> Thanks for the great patch,
> 
> Pavel.


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24  2:24 Alexander Tsoy
2020-04-24  2:24 ` [PATCH 2/2] ALSA: usb-audio: Remove async workaround for Scarlett 2nd gen Alexander Tsoy
2020-04-24  6:28   ` Takashi Iwai
2020-04-24 15:21   ` Gregor Pintar
2020-04-24  6:28 ` [PATCH 1/2] ALSA: usb-audio: Improve frames size computation Takashi Iwai
2020-04-24  9:19 ` Pavel Hofman
2020-04-24  9:29   ` Alexander Tsoy [this message]
2020-04-24  9:39     ` Pavel Hofman
2020-04-24 15:02 ` Gregor Pintar
2020-04-24 16:43   ` Alexander Tsoy
2020-04-25 16:50     ` Gregor Pintar
2020-04-25 18:09       ` Gregor Pintar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a876d867496c2c58866a626430dd2f174f16c107.camel@tsoy.me \
    --to=alexander@tsoy.me \
    --cc=alsa-devel@alsa-project.org \
    --cc=grpintar@gmail.com \
    --cc=pavel.hofman@ivitera.com \
    --cc=rpsalmi@gmail.com \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git