Re: [alsa-devel] [PATCH] Fix forward/rewind support in iec958 plugin

24 Apr 2014

On 2014-04-23 20:40, 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.

I found the issue by inspecting the code. However, I have no hardware
to test this fix on.
src/pcm/pcm_iec958.c | 64 +++++++++++++++++++++++++++++++++++++++++++++-
  src/pcm/pcm_plugin.c | 28 ++++++++++----------
  src/pcm/pcm_plugin.h | 72 +++++++++++++++++++++++++++++++++++++++++++++++++---
  3 files changed, 145 insertions(+), 19 deletions(-)

diff --git a/src/pcm/pcm_iec958.c b/src/pcm/pcm_iec958.c
index d81b0a1..facf683 100644
--- a/src/pcm/pcm_iec958.c
+++ b/src/pcm/pcm_iec958.c
@@ -416,6 +416,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,

@@ -434,6 +463,39 @@ static const snd_pcm_ops_t snd_pcm_iec958_ops = {
   .set_chmap = snd_pcm_generic_set_chmap,
  };



+static const snd_pcm_fast_ops_t snd_pcm_iec958_fast_ops = {

.status = snd_pcm_plugin_status,
.state = snd_pcm_generic_state,
.hwsync = snd_pcm_generic_hwsync,
.delay = snd_pcm_plugin_delay,
.prepare = snd_pcm_plugin_prepare,
.reset = snd_pcm_plugin_reset,
.start = snd_pcm_generic_start,
.drop = snd_pcm_generic_drop,
.drain = snd_pcm_generic_drain,
.pause = snd_pcm_generic_pause,
.rewindable = snd_pcm_plugin_rewindable,
.rewind = snd_pcm_iec958_rewind,
.forwardable = snd_pcm_plugin_forwardable,
.forward = snd_pcm_iec958_forward,
.resume = snd_pcm_generic_resume,
.link = snd_pcm_generic_link,
.link_slaves = snd_pcm_generic_link_slaves,
.unlink = snd_pcm_generic_unlink,
.writei = snd_pcm_plugin_writei,
.writen = snd_pcm_plugin_writen,
.readi = snd_pcm_plugin_readi,
.readn = snd_pcm_plugin_readn,
.avail_update = snd_pcm_plugin_avail_update,
.mmap_commit = snd_pcm_plugin_mmap_commit,
.htimestamp = snd_pcm_generic_htimestamp,
.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
.poll_descriptors = snd_pcm_generic_poll_descriptors,
.poll_revents = snd_pcm_generic_poll_revents,
.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,

+};

/**
\brief Creates a new IEC958 subframe conversion PCM
\param pcmp Returns created PCM handle



@@ -495,7 +557,7 @@ 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;


pcm->fast_ops = &snd_pcm_iec958_fast_ops;

Instead of making all pcm_plugin functions public, you can consider 
something like this:
ptr = malloc(sizeof(snd_pcm_fast_ops_t));
*ptr = snd_pcm_plugin_fast_ops;
ptr->rewind = snd_pcm_iec958_rewind;
ptr->forward = snd_pcm_iec958_forward;
pcm->fast_ops = ptr;
(and deallocate ptr somewhere else)
...
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..022d908 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -137,7 +137,7 @@ void snd_pcm_plugin_init(snd_pcm_plugin_t *plugin)
   snd_atomic_write_init(&plugin->watom);
  }
-static int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
+int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
  {
   snd_pcm_plugin_t *plugin = pcm->private_data;
   snd_pcm_sframes_t sd;
@@ -154,7 +154,7 @@ static int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
   return 0;
  }
-static int snd_pcm_plugin_prepare(snd_pcm_t *pcm)
+int snd_pcm_plugin_prepare(snd_pcm_t *pcm)
  {
   snd_pcm_plugin_t *plugin = pcm->private_data;
   int err;
@@ -175,7 +175,7 @@ static int snd_pcm_plugin_prepare(snd_pcm_t *pcm)
   return 0;
  }
-static int snd_pcm_plugin_reset(snd_pcm_t *pcm)
+int snd_pcm_plugin_reset(snd_pcm_t *pcm)
  {
   snd_pcm_plugin_t *plugin = pcm->private_data;
   int err;
@@ -196,12 +196,12 @@ static int snd_pcm_plugin_reset(snd_pcm_t *pcm)
   return 0;
  }
-static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm)
+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);
@@ -224,12 +224,12 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
   return (snd_pcm_sframes_t) sframes;
  }
-static snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm)
+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);
@@ -347,7 +347,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_read_areas(snd_pcm_t *pcm,
  }
-static snd_pcm_sframes_t
+snd_pcm_sframes_t
  snd_pcm_plugin_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size)
  {
   snd_pcm_channel_area_t areas[pcm->channels];
@@ -356,7 +356,7 @@ snd_pcm_plugin_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size
   			   snd_pcm_plugin_write_areas);
  }
-static snd_pcm_sframes_t
+snd_pcm_sframes_t
  snd_pcm_plugin_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
  {
   snd_pcm_channel_area_t areas[pcm->channels];
@@ -365,7 +365,7 @@ snd_pcm_plugin_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
   			   snd_pcm_plugin_write_areas);
  }
-static snd_pcm_sframes_t
+snd_pcm_sframes_t
  snd_pcm_plugin_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size)
  {
   snd_pcm_channel_area_t areas[pcm->channels];
@@ -374,7 +374,7 @@ snd_pcm_plugin_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size)
   			  snd_pcm_plugin_read_areas);
  }
-static snd_pcm_sframes_t
+snd_pcm_sframes_t
  snd_pcm_plugin_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
  {
   snd_pcm_channel_area_t areas[pcm->channels];
@@ -383,7 +383,7 @@ snd_pcm_plugin_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
   			  snd_pcm_plugin_read_areas);
  }
-static snd_pcm_sframes_t
+snd_pcm_sframes_t
  snd_pcm_plugin_mmap_commit(snd_pcm_t *pcm,
   		   snd_pcm_uframes_t offset ATTRIBUTE_UNUSED,
   		   snd_pcm_uframes_t size)
@@ -452,7 +452,7 @@ snd_pcm_plugin_mmap_commit(snd_pcm_t *pcm,
   return xfer;
  }
-static snd_pcm_sframes_t snd_pcm_plugin_avail_update(snd_pcm_t *pcm)
+snd_pcm_sframes_t snd_pcm_plugin_avail_update(snd_pcm_t *pcm)
  {
   snd_pcm_plugin_t *plugin = pcm->private_data;
   snd_pcm_t *slave = plugin->gen.slave;
@@ -516,7 +516,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_avail_update(snd_pcm_t *pcm)
   }
  }
-static int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
+int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
  {
   snd_pcm_plugin_t *plugin = pcm->private_data;
   snd_pcm_sframes_t err;
diff --git a/src/pcm/pcm_plugin.h b/src/pcm/pcm_plugin.h
index 7ee7c7f..7734369 100644
--- a/src/pcm/pcm_plugin.h
+++ b/src/pcm/pcm_plugin.h
@@ -50,14 +50,42 @@ typedef struct {
  } snd_pcm_plugin_t;
/* make local functions really local */
-#define snd_pcm_plugin_init \

snd1_pcm_plugin_init

-#define snd_pcm_plugin_fast_ops \

snd1_pcm_plugin_fast_ops
#define snd_pcm_plugin_undo_read_generic \
snd1_pcm_plugin_undo_read_generic
#define snd_pcm_plugin_undo_write_generic \
snd1_pcm_plugin_undo_write_generic

+#define snd_pcm_plugin_init \

snd1_pcm_plugin_init

+#define snd_pcm_plugin_delay \

snd1_pcm_plugin_delay

+#define snd_pcm_plugin_prepare \

snd1_pcm_plugin_prepare

+#define snd_pcm_plugin_reset \

snd1_pcm_plugin_reset

+#define snd_pcm_plugin_rewindable \

snd1_pcm_plugin_rewindable

+#define snd_pcm_plugin_rewind \

snd1_pcm_plugin_rewind

+#define snd_pcm_plugin_forwardable \

snd1_pcm_plugin_forwardable

+#define snd_pcm_plugin_forward \

snd1_pcm_plugin_forward

+#define snd_pcm_plugin_writei \

snd1_pcm_plugin_writei

+#define snd_pcm_plugin_writen \

snd1_pcm_plugin_writen

+#define snd_pcm_plugin_readi \

snd1_pcm_plugin_readi

+#define snd_pcm_plugin_readn \

snd1_pcm_plugin_readn

+#define snd_pcm_plugin_mmap_commit \

snd1_pcm_plugin_mmap_commit

+#define snd_pcm_plugin_avail_update \

snd1_pcm_plugin_avail_update

+#define snd_pcm_plugin_status \

snd1_pcm_plugin_status

+#define snd_pcm_plugin_fast_ops \

snd1_pcm_plugin_fast_ops
void snd_pcm_plugin_init(snd_pcm_plugin_t *plugin);


@@ -77,6 +105,42 @@ snd_pcm_sframes_t snd_pcm_plugin_undo_write_generic
        snd_pcm_uframes_t res_size,		/* size of result areas */
        snd_pcm_uframes_t slave_undo_size);
+int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp);
+int snd_pcm_plugin_prepare(snd_pcm_t *pcm);
+int snd_pcm_plugin_reset(snd_pcm_t *pcm);
+snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm);
+snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
+snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm);
+snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);



+snd_pcm_sframes_t snd_pcm_plugin_writei

 (snd_pcm_t *pcm,


  const void *buffer,


  snd_pcm_uframes_t size);




+snd_pcm_sframes_t snd_pcm_plugin_writen

 (snd_pcm_t *pcm,


  void **bufs,


  snd_pcm_uframes_t size);




+snd_pcm_sframes_t snd_pcm_plugin_readi

 (snd_pcm_t *pcm,


  void *buffer,


  snd_pcm_uframes_t size);




+snd_pcm_sframes_t snd_pcm_plugin_readn

 (snd_pcm_t *pcm,


  void **bufs,


  snd_pcm_uframes_t size);




+snd_pcm_sframes_t snd_pcm_plugin_mmap_commit

 (snd_pcm_t *pcm,


  snd_pcm_uframes_t offset,


  snd_pcm_uframes_t size);




+snd_pcm_sframes_t snd_pcm_plugin_avail_update(snd_pcm_t *pcm);
+int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status);

/* make local functions really local */
#define snd_pcm_linear_get_index	snd1_pcm_linear_get_index
#define snd_pcm_linear_put_index	snd1_pcm_linear_put_index

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

    