Re: [PATCH v2] Fix forward/rewind support in iec958 plugin

[Takashi Iwai <tiwai@suse.de>]

At Thu, 24 Apr 2014 20:40:06 +0600,
Alexander E. Patrakov wrote:
> 
> When forwarding or rewinding, the frame counter was not updated. This
> could result in corrupted channel status words or misplaced Z-type
> preamble.
> 
> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
> ---
> v2: As suggested by David Henningsson, patch a copy of
> snd_pcm_plugin_fast_ops at runtime instead of making all plugin finctions
> non-static. But, contrary to his proposal, I made fast_ops just a part of
> struct snd_pcm_iec958, because the needed lifetime is exactly the same.
> 
> I still have no hardware to test this, and I strongly suspect that the
> patch is wrong, because nobody else uses dynamic allocation of fast_ops.
> A potentially suspicious operation would be to place a plug on top of this,
> because plug copies fast_ops around. However, each time it closes a slave,
> it also resets fast_ops to something sane, or at least I think so.

The embedded fast_ops should work fine, so I applied the patch as is
now.  Let's see whether it really fixes the issues properly...


thanks,

Takashi

> 
>  src/pcm/pcm_iec958.c | 37 ++++++++++++++++++++++++++++++++++++-
>  src/pcm/pcm_plugin.c |  4 ++--
>  src/pcm/pcm_plugin.h |  6 ++++++
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/src/pcm/pcm_iec958.c b/src/pcm/pcm_iec958.c
> index d81b0a1..0c61fc1 100644
> --- a/src/pcm/pcm_iec958.c
> +++ b/src/pcm/pcm_iec958.c
> @@ -62,6 +62,7 @@ struct snd_pcm_iec958 {
>  	unsigned char status[24];
>  	unsigned int byteswap;
>  	unsigned char preamble[3];	/* B/M/W or Z/X/Y */
> +	snd_pcm_fast_ops_t fops;
>  };
>  
>  enum { PREAMBLE_Z, PREAMBLE_X, PREAMBLE_Y };
> @@ -416,6 +417,35 @@ static void snd_pcm_iec958_dump(snd_pcm_t *pcm, snd_output_t *out)
>  	snd_pcm_dump(iec->plug.gen.slave, out);
>  }
>  
> +static snd_pcm_sframes_t snd_pcm_iec958_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
> +{
> +	unsigned int counter_decrement;
> +	snd_pcm_iec958_t *iec = pcm->private_data;
> +	snd_pcm_sframes_t result = snd_pcm_plugin_rewind(pcm, frames);
> +	if (result <= 0)
> +		return result;
> +
> +	counter_decrement = result % 192;
> +	iec->counter += 192 - counter_decrement;
> +	iec->counter %= 192;
> +	return result;
> +}
> +
> +static snd_pcm_sframes_t snd_pcm_iec958_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
> +
> +{
> +	unsigned int counter_increment;
> +	snd_pcm_iec958_t *iec = pcm->private_data;
> +	snd_pcm_sframes_t result = snd_pcm_plugin_rewind(pcm, frames);
> +	if (result <= 0)
> +		return result;
> +
> +	counter_increment = result % 192;
> +	iec->counter += counter_increment;
> +	iec->counter %= 192;
> +	return result;
> +}
> +
>  static const snd_pcm_ops_t snd_pcm_iec958_ops = {
>  	.close = snd_pcm_generic_close,
>  	.info = snd_pcm_generic_info,
> @@ -495,7 +525,12 @@ int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sfo
>  		return err;
>  	}
>  	pcm->ops = &snd_pcm_iec958_ops;
> -	pcm->fast_ops = &snd_pcm_plugin_fast_ops;
> +
> +	iec->fops = snd_pcm_plugin_fast_ops;
> +	iec->fops.rewind = snd_pcm_iec958_rewind;
> +	iec->fops.forward = snd_pcm_iec958_forward;
> +	pcm->fast_ops = &iec->fops;
> +
>  	pcm->private_data = iec;
>  	pcm->poll_fd = slave->poll_fd;
>  	pcm->poll_events = slave->poll_events;
> diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
> index 17157e8..4ddf10c 100644
> --- a/src/pcm/pcm_plugin.c
> +++ b/src/pcm/pcm_plugin.c
> @@ -201,7 +201,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm)
>  	return snd_pcm_mmap_hw_avail(pcm);
>  }
>  
> -static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
> +snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
>  {
>  	snd_pcm_plugin_t *plugin = pcm->private_data;
>  	snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm);
> @@ -229,7 +229,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm)
>  	return snd_pcm_mmap_avail(pcm);
>  }
>  
> -static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
> +snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
>  {
>  	snd_pcm_plugin_t *plugin = pcm->private_data;
>  	snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm);
> diff --git a/src/pcm/pcm_plugin.h b/src/pcm/pcm_plugin.h
> index 7ee7c7f..19e82c3 100644
> --- a/src/pcm/pcm_plugin.h
> +++ b/src/pcm/pcm_plugin.h
> @@ -58,8 +58,14 @@ typedef struct {
>  	snd1_pcm_plugin_undo_read_generic
>  #define snd_pcm_plugin_undo_write_generic \
>  	snd1_pcm_plugin_undo_write_generic
> +#define snd_pcm_plugin_rewind \
> +	snd1_pcm_plugin_rewind
> +#define snd_pcm_plugin_forward \
> +	snd1_pcm_plugin_forward
>  
>  void snd_pcm_plugin_init(snd_pcm_plugin_t *plugin);
> +snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
> +snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
>  
>  extern const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops;
>  
> -- 
> 1.9.2
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>