* [PATCH v2] ALSA: compress: allow setting codec params after next track @ 2023-06-09 15:04 Srinivas Kandagatla 2023-06-14 7:04 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Srinivas Kandagatla @ 2023-06-09 15:04 UTC (permalink / raw) To: vkoul, broonie, tiwai Cc: corbet, alsa-devel, linux-doc, linux-kernel, quic_plai, quic_mohs, Srinivas Kandagatla For gapless playback it is possible that each track can have different codec profile with same decoder, for example we have WMA album, we may have different tracks as WMA v9, WMA v10 and so on Or if DSP's like QDSP have abililty to switch decoders on single stream for each track, then this call could be used to set new codec parameters. Existing code does not allow to change this profile while doing gapless playback. Reuse existing SNDRV_COMPRESS_SET_PARAMS to set this new track params along some additional checks to enforce proper state machine. With this new changes now the user can call SNDRV_COMPRESS_SET_PARAMS anytime after setting next track and additional check in write should also ensure that params are set before writing new data. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- Changes since v1/RFC: - removed introduction of new IOCTL, as suggested. - update the state-machine doc. .../sound/designs/compress-offload.rst | 52 +++++++++---------- sound/core/compress_offload.c | 10 ++-- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst index 935f325dbc77..205cadcabe70 100644 --- a/Documentation/sound/designs/compress-offload.rst +++ b/Documentation/sound/designs/compress-offload.rst @@ -256,32 +256,32 @@ Gapless Playback SM For Gapless, we move from running state to partial drain and back, along with setting of meta_data and signalling for next track :: - - +----------+ - compr_drain_notify() | | - +------------------------>| RUNNING | - | | | - | +----------+ - | | - | | - | | compr_next_track() - | | - | V - | +----------+ - | | | - | |NEXT_TRACK| - | | | - | +----------+ - | | - | | - | | compr_partial_drain() - | | - | V - | +----------+ - | | | - +------------------------ | PARTIAL_ | - | DRAIN | - +----------+ + +----------+ + compr_drain_notify() | | compr_set_params() iff next-track set. + +------------------------>| RUNNING |----------------------+ + | | | | + | +----------+ | + | | | + | | | + | | compr_next_track() | + | | V + | V | + | +----------+ | + | | | | + | |NEXT_TRACK| | + | | | | + | +----------+ | + | | | + | +----------------------------+ + | | + | | compr_partial_drain() + | | + | V + | +----------+ + | | | + +------------------------ | PARTIAL_ | + | DRAIN | + +----------+ Not supported ============= diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 243acad89fd3..fe67228e74b3 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -294,6 +294,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, case SNDRV_PCM_STATE_SETUP: case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_RUNNING: + /* Make sure next track params are set before writing new data */ + if (stream->next_track) + return -EPERM; break; default: mutex_unlock(&stream->device->lock); @@ -589,7 +592,8 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) struct snd_compr_params *params; int retval; - if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { + if (stream->runtime->state == SNDRV_PCM_STATE_OPEN || + (stream->runtime->state == SNDRV_PCM_STATE_RUNNING && stream->next_track)) { /* * we should allow parameter change only when stream has been * opened not in other cases @@ -615,7 +619,8 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) stream->metadata_set = false; stream->next_track = false; - stream->runtime->state = SNDRV_PCM_STATE_SETUP; + if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) + stream->runtime->state = SNDRV_PCM_STATE_SETUP; } else { return -EPERM; } @@ -956,7 +961,6 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) return retval; } - stream->next_track = false; return snd_compress_wait_for_drain(stream); } -- 2.21.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: compress: allow setting codec params after next track 2023-06-09 15:04 [PATCH v2] ALSA: compress: allow setting codec params after next track Srinivas Kandagatla @ 2023-06-14 7:04 ` Takashi Iwai 2023-06-14 11:55 ` Srinivas Kandagatla 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2023-06-14 7:04 UTC (permalink / raw) To: Srinivas Kandagatla Cc: vkoul, broonie, tiwai, corbet, alsa-devel, linux-doc, linux-kernel, quic_plai, quic_mohs On Fri, 09 Jun 2023 17:04:16 +0200, Srinivas Kandagatla wrote: > > For gapless playback it is possible that each track can have different > codec profile with same decoder, for example we have WMA album, > we may have different tracks as WMA v9, WMA v10 and so on > > Or if DSP's like QDSP have abililty to switch decoders on single stream > for each track, then this call could be used to set new codec parameters. > > Existing code does not allow to change this profile while doing gapless > playback. > > Reuse existing SNDRV_COMPRESS_SET_PARAMS to set this new track params along > some additional checks to enforce proper state machine. > > With this new changes now the user can call SNDRV_COMPRESS_SET_PARAMS > anytime after setting next track and additional check in write should > also ensure that params are set before writing new data. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > Changes since v1/RFC: > - removed introduction of new IOCTL, as suggested. > - update the state-machine doc. > > .../sound/designs/compress-offload.rst | 52 +++++++++---------- > sound/core/compress_offload.c | 10 ++-- > 2 files changed, 33 insertions(+), 29 deletions(-) > > diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst > index 935f325dbc77..205cadcabe70 100644 > --- a/Documentation/sound/designs/compress-offload.rst > +++ b/Documentation/sound/designs/compress-offload.rst > @@ -256,32 +256,32 @@ Gapless Playback SM > For Gapless, we move from running state to partial drain and back, along > with setting of meta_data and signalling for next track :: > > - > - +----------+ > - compr_drain_notify() | | > - +------------------------>| RUNNING | > - | | | > - | +----------+ > - | | > - | | > - | | compr_next_track() > - | | > - | V > - | +----------+ > - | | | > - | |NEXT_TRACK| > - | | | > - | +----------+ > - | | > - | | > - | | compr_partial_drain() > - | | > - | V > - | +----------+ > - | | | > - +------------------------ | PARTIAL_ | > - | DRAIN | > - +----------+ > + +----------+ > + compr_drain_notify() | | compr_set_params() iff next-track set. > + +------------------------>| RUNNING |----------------------+ > + | | | | > + | +----------+ | > + | | | > + | | | > + | | compr_next_track() | > + | | V > + | V | > + | +----------+ | > + | | | | > + | |NEXT_TRACK| | > + | | | | > + | +----------+ | > + | | | > + | +----------------------------+ > + | | > + | | compr_partial_drain() > + | | > + | V > + | +----------+ > + | | | > + +------------------------ | PARTIAL_ | > + | DRAIN | > + +----------+ > > Not supported > ============= > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > index 243acad89fd3..fe67228e74b3 100644 > --- a/sound/core/compress_offload.c > +++ b/sound/core/compress_offload.c > @@ -294,6 +294,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, > case SNDRV_PCM_STATE_SETUP: > case SNDRV_PCM_STATE_PREPARED: > case SNDRV_PCM_STATE_RUNNING: > + /* Make sure next track params are set before writing new data */ > + if (stream->next_track) > + return -EPERM; Hm, does this logic correctly match with the comment above? Also, this misses the mutex unlock. thanks, Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: compress: allow setting codec params after next track 2023-06-14 7:04 ` Takashi Iwai @ 2023-06-14 11:55 ` Srinivas Kandagatla 2023-06-14 12:29 ` Vinod Koul 0 siblings, 1 reply; 4+ messages in thread From: Srinivas Kandagatla @ 2023-06-14 11:55 UTC (permalink / raw) To: Takashi Iwai Cc: vkoul, broonie, tiwai, corbet, alsa-devel, linux-doc, linux-kernel, quic_plai, quic_mohs Thanks Takashi for review, On 14/06/2023 08:04, Takashi Iwai wrote: > On Fri, 09 Jun 2023 17:04:16 +0200, > Srinivas Kandagatla wrote: >> >> For gapless playback it is possible that each track can have different >> codec profile with same decoder, for example we have WMA album, >> we may have different tracks as WMA v9, WMA v10 and so on >> >> Or if DSP's like QDSP have abililty to switch decoders on single stream >> for each track, then this call could be used to set new codec parameters. >> >> Existing code does not allow to change this profile while doing gapless >> playback. >> >> Reuse existing SNDRV_COMPRESS_SET_PARAMS to set this new track params along >> some additional checks to enforce proper state machine. >> >> With this new changes now the user can call SNDRV_COMPRESS_SET_PARAMS >> anytime after setting next track and additional check in write should >> also ensure that params are set before writing new data. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> Changes since v1/RFC: >> - removed introduction of new IOCTL, as suggested. >> - update the state-machine doc. >> >> .../sound/designs/compress-offload.rst | 52 +++++++++---------- >> sound/core/compress_offload.c | 10 ++-- >> 2 files changed, 33 insertions(+), 29 deletions(-) >> >> diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst >> index 935f325dbc77..205cadcabe70 100644 >> --- a/Documentation/sound/designs/compress-offload.rst >> +++ b/Documentation/sound/designs/compress-offload.rst >> @@ -256,32 +256,32 @@ Gapless Playback SM >> For Gapless, we move from running state to partial drain and back, along >> with setting of meta_data and signalling for next track :: >> >> - >> - +----------+ >> - compr_drain_notify() | | >> - +------------------------>| RUNNING | >> - | | | >> - | +----------+ >> - | | >> - | | >> - | | compr_next_track() >> - | | >> - | V >> - | +----------+ >> - | | | >> - | |NEXT_TRACK| >> - | | | >> - | +----------+ >> - | | >> - | | >> - | | compr_partial_drain() >> - | | >> - | V >> - | +----------+ >> - | | | >> - +------------------------ | PARTIAL_ | >> - | DRAIN | >> - +----------+ >> + +----------+ >> + compr_drain_notify() | | compr_set_params() iff next-track set. >> + +------------------------>| RUNNING |----------------------+ >> + | | | | >> + | +----------+ | >> + | | | >> + | | | >> + | | compr_next_track() | >> + | | V >> + | V | >> + | +----------+ | >> + | | | | >> + | |NEXT_TRACK| | >> + | | | | >> + | +----------+ | >> + | | | >> + | +----------------------------+ >> + | | >> + | | compr_partial_drain() >> + | | >> + | V >> + | +----------+ >> + | | | >> + +------------------------ | PARTIAL_ | >> + | DRAIN | >> + +----------+ >> >> Not supported >> ============= >> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c >> index 243acad89fd3..fe67228e74b3 100644 >> --- a/sound/core/compress_offload.c >> +++ b/sound/core/compress_offload.c >> @@ -294,6 +294,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, >> case SNDRV_PCM_STATE_SETUP: >> case SNDRV_PCM_STATE_PREPARED: >> case SNDRV_PCM_STATE_RUNNING: >> + /* Make sure next track params are set before writing new data */ >> + if (stream->next_track) >> + return -EPERM; > > Hm, does this logic correctly match with the comment above? Yes I agree, it bit confusing and also going to break the partial drain path. > Also, this misses the mutex unlock. Thinking about this again, Its not required have this check here to start with. Something like below works, I can send this as v3, if this looks fine. ------------------------->cut<---------------------------------- diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst index 935f325dbc77..655624f77092 100644 --- a/Documentation/sound/designs/compress-offload.rst +++ b/Documentation/sound/designs/compress-offload.rst @@ -268,11 +268,12 @@ with setting of meta_data and signalling for next track :: | | | V | +----------+ - | | | - | |NEXT_TRACK| - | | | - | +----------+ - | | + | compr_set_params() | | + | +-----------|NEXT_TRACK| + | | | | + | | +--+-------+ + | | | | + | +--------------+ | | | | | compr_partial_drain() | | diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 243acad89fd3..30f73097447b 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -589,7 +589,7 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) struct snd_compr_params *params; int retval; - if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { + if (stream->runtime->state == SNDRV_PCM_STATE_OPEN || stream->next_track) { /* * we should allow parameter change only when stream has been * opened not in other cases @@ -612,6 +612,9 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) if (retval) goto out; + if (stream->next_track) + goto out; + stream->metadata_set = false; stream->next_track = false; ------------------------->cut<---------------------------------- > > > thanks, > > Takashi ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: compress: allow setting codec params after next track 2023-06-14 11:55 ` Srinivas Kandagatla @ 2023-06-14 12:29 ` Vinod Koul 0 siblings, 0 replies; 4+ messages in thread From: Vinod Koul @ 2023-06-14 12:29 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Takashi Iwai, broonie, tiwai, corbet, alsa-devel, linux-doc, linux-kernel, quic_plai, quic_mohs On 14-06-23, 12:55, Srinivas Kandagatla wrote: > Thanks Takashi for review, > > On 14/06/2023 08:04, Takashi Iwai wrote: > > On Fri, 09 Jun 2023 17:04:16 +0200, > > Srinivas Kandagatla wrote: > > > > > > For gapless playback it is possible that each track can have different > > > codec profile with same decoder, for example we have WMA album, > > > we may have different tracks as WMA v9, WMA v10 and so on > > > > > > Or if DSP's like QDSP have abililty to switch decoders on single stream > > > for each track, then this call could be used to set new codec parameters. > > > > > > Existing code does not allow to change this profile while doing gapless > > > playback. > > > > > > Reuse existing SNDRV_COMPRESS_SET_PARAMS to set this new track params along > > > some additional checks to enforce proper state machine. > > > > > > With this new changes now the user can call SNDRV_COMPRESS_SET_PARAMS > > > anytime after setting next track and additional check in write should > > > also ensure that params are set before writing new data. > > > > > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > --- > > > Changes since v1/RFC: > > > - removed introduction of new IOCTL, as suggested. > > > - update the state-machine doc. > > > > > > .../sound/designs/compress-offload.rst | 52 +++++++++---------- > > > sound/core/compress_offload.c | 10 ++-- > > > 2 files changed, 33 insertions(+), 29 deletions(-) > > > > > > diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst > > > index 935f325dbc77..205cadcabe70 100644 > > > --- a/Documentation/sound/designs/compress-offload.rst > > > +++ b/Documentation/sound/designs/compress-offload.rst > > > @@ -256,32 +256,32 @@ Gapless Playback SM > > > For Gapless, we move from running state to partial drain and back, along > > > with setting of meta_data and signalling for next track :: > > > - > > > - +----------+ > > > - compr_drain_notify() | | > > > - +------------------------>| RUNNING | > > > - | | | > > > - | +----------+ > > > - | | > > > - | | > > > - | | compr_next_track() > > > - | | > > > - | V > > > - | +----------+ > > > - | | | > > > - | |NEXT_TRACK| > > > - | | | > > > - | +----------+ > > > - | | > > > - | | > > > - | | compr_partial_drain() > > > - | | > > > - | V > > > - | +----------+ > > > - | | | > > > - +------------------------ | PARTIAL_ | > > > - | DRAIN | > > > - +----------+ > > > + +----------+ > > > + compr_drain_notify() | | compr_set_params() iff next-track set. > > > + +------------------------>| RUNNING |----------------------+ > > > + | | | | > > > + | +----------+ | > > > + | | | > > > + | | | > > > + | | compr_next_track() | > > > + | | V > > > + | V | > > > + | +----------+ | > > > + | | | | > > > + | |NEXT_TRACK| | > > > + | | | | > > > + | +----------+ | > > > + | | | > > > + | +----------------------------+ > > > + | | > > > + | | compr_partial_drain() > > > + | | > > > + | V > > > + | +----------+ > > > + | | | > > > + +------------------------ | PARTIAL_ | > > > + | DRAIN | > > > + +----------+ > > > Not supported > > > ============= > > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > > > index 243acad89fd3..fe67228e74b3 100644 > > > --- a/sound/core/compress_offload.c > > > +++ b/sound/core/compress_offload.c > > > @@ -294,6 +294,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, > > > case SNDRV_PCM_STATE_SETUP: > > > case SNDRV_PCM_STATE_PREPARED: > > > case SNDRV_PCM_STATE_RUNNING: > > > + /* Make sure next track params are set before writing new data */ > > > + if (stream->next_track) > > > + return -EPERM; > > > > Hm, does this logic correctly match with the comment above? > Yes I agree, it bit confusing and also going to break the partial drain > path. > > Also, this misses the mutex unlock. > > Thinking about this again, Its not required have this check here to start > with. > > Something like below works, I can send this as v3, if this looks fine. > > ------------------------->cut<---------------------------------- > > diff --git a/Documentation/sound/designs/compress-offload.rst > b/Documentation/sound/designs/compress-offload.rst > index 935f325dbc77..655624f77092 100644 > --- a/Documentation/sound/designs/compress-offload.rst > +++ b/Documentation/sound/designs/compress-offload.rst > @@ -268,11 +268,12 @@ with setting of meta_data and signalling for next > track :: > | | > | V > | +----------+ > - | | | > - | |NEXT_TRACK| > - | | | > - | +----------+ > - | | > + | compr_set_params() | | > + | +-----------|NEXT_TRACK| > + | | | | > + | | +--+-------+ > + | | | | > + | +--------------+ | > | | > | | compr_partial_drain() > | | > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > index 243acad89fd3..30f73097447b 100644 > --- a/sound/core/compress_offload.c > +++ b/sound/core/compress_offload.c > @@ -589,7 +589,7 @@ snd_compr_set_params(struct snd_compr_stream *stream, > unsigned long arg) > struct snd_compr_params *params; > int retval; > > - if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { > + if (stream->runtime->state == SNDRV_PCM_STATE_OPEN || > stream->next_track) { Yes this would be my idea for this as well, allow params to be set when we have signalled partial drain > /* > * we should allow parameter change only when stream has > been > * opened not in other cases > @@ -612,6 +612,9 @@ snd_compr_set_params(struct snd_compr_stream *stream, > unsigned long arg) > if (retval) > goto out; > > + if (stream->next_track) > + goto out; > + > stream->metadata_set = false; > stream->next_track = false; > ------------------------->cut<---------------------------------- > > > > > > > thanks, > > > > Takashi -- ~Vinod ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-14 12:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-09 15:04 [PATCH v2] ALSA: compress: allow setting codec params after next track Srinivas Kandagatla 2023-06-14 7:04 ` Takashi Iwai 2023-06-14 11:55 ` Srinivas Kandagatla 2023-06-14 12:29 ` Vinod Koul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).