C2 error support.

[Kurt Roeckx]

Hi,

I'm looking into adding support for reading the C2 error
information, and probably some other things like CD-TEXT,
CD+MIDI, CD+G, ISRC, whatever info I can read from the disc.

It looks like I need to extend the API for this.  For instance,
for the C2 error information you get 1 bit per read byte
to indicate if there was an error reading that byte or not.

I see several ways to do this:
- Create new versions of exising functions with extra parameters
  (and different name)
- Create functions that return the extra info from the last
  call.
- Setting reading C2/subchannel info default to off, and return
  the data exactly like the drive returns it, so just after the
  main data.  It looks like this atleast requires a new function
  to set the options.
- Read the data by default, if supported by the drive, and instead
  of adding it after the main data of each sector, change it so
  that the main data of all sectors are still returned first,
  and then the other data.

The R-W subchannels can contain interleaved/error corrected
info like CD-TEXT, and not all drives support returning
that data deinterleaved and error corrected, but most do
support reading the RAW data.  So we might need to implement
that ourself.

So I'm wondering what you think the best option is.  I'm currently
in favour of the second option.


Kurt

_______________________________________________
Paranoia-dev mailing list
Paranoia-dev@...
http://lists.xiph.org/mailman/listinfo/paranoia-dev

[Kurt Roeckx]

On Fri, May 22, 2009 at 02:57:24PM +0200, Kurt Roeckx wrote:
> Hi,
>
> I'm looking into adding support for reading the C2 error
> information [...]

Here is an initial patch.  It would be nice if people could test
this and give feedback.

Some comments about the patch:
- I've merged all the i_read_mmc() functions to 1 function
  so that not all of them need to be modified.
- rs was set wrong in verify_read_command().  They all had
  the DAP bit inverted.
- There are still a few TODOs in it.

I've been testing this with a CD with lots of scratches.  What
I've seen:
- My plextor reads this CD at full speed (40x) every time, always
  identical, never returns C2 errors.
- The other drives have problems reading this, even at speed 1.
  Without this patch I got 20+ different files.  With this patch
  they all either generate the same file as the plextor, or fail
  to read it / generate a skip.  Once I got a total of 1.8 million
  C2 errors for 1 track, but the result was still good.
- The current patch marks all samples belonging to 1 byte of c2
  error information as having a problem while it should probably
  be possible to only mark the sample that is affected.  For some
  reason that's currently unclear to me this always results in a
  good file and trying to mark only the bad one resulted in a bad
  file.  (Maybe I should mark both left and right instead of only
  left or right?)


Kurt

---

Index: paranoia/paranoia.c
===================================================================
--- paranoia/paranoia.c (revision 16022)
+++ paranoia/paranoia.c (working copy)
@@ -2329,7 +2329,9 @@
   long sofar;
   long dynoverlap=(p->dynoverlap+CD_FRAMEWORDS-1)/CD_FRAMEWORDS;
   long anyflag=0;
+  unsigned char *c2_buffer=NULL;
 
+  c2_buffer=malloc(sectatonce * CD_C2BUFFERSIZE);
 
   /* Calculate the first sector to read.  This calculation takes
    * into account the need to jitter the starting point of the read
@@ -2444,6 +2446,7 @@
     if(new)free_c_block(new);
     if(buffer)free(buffer);
     if(flags)free(flags);
+    if(c2_buffer)free(c2_buffer);
     return NULL;
   }
   thisread=0;
@@ -2482,6 +2485,29 @@
   flags[sofar*CD_FRAMEWORDS+i]|=FLAGS_EDGE;
       }
 
+      if (cdda_read_c2_errors(p->d, c2_buffer, thisread) == 0)
+      {
+ int i;
+ for (i = 0; i < thisread * CD_C2BUFFERSIZE; i++)
+ {
+  int j;
+  /* The c2_buffer contains 1 bit for every byte from the
+   * buffer.  The buffer is in 16 bit words.  So 8 byte from
+   * the c2_buffer map to 4 values of the flags/buffer.
+   * It's not clear in which order the bits in the c2 buffer are,
+   * is the first byte in the MSB or LSB?  Proably all firmware won't
+   * agree.  We just mark all 4 values in the flags as invalid.
+   */
+  if (c2_buffer[i] != '\0')
+  {
+    for (j = 0; j < 4; j++)
+    {
+      flags[sofar*CD_FRAMEWORDS+i*4+j]|=FLAGS_UNREAD;
+    }
+  }
+ }
+      }
+
       if(adjread+secread-1==p->current_lastsector)
  new->lastsector=-1;
       
@@ -2519,6 +2545,7 @@
     free(flags);
     new=NULL;
   }
+  if(c2_buffer)free(c2_buffer);
   return(new);
 }
 
Index: interface/cdda_interface.h
===================================================================
--- interface/cdda_interface.h (revision 16022)
+++ interface/cdda_interface.h (working copy)
@@ -17,6 +17,7 @@
 #define CD_FRAMESIZE_RAW 2352
 #endif
 #define CD_FRAMESAMPLES (CD_FRAMESIZE_RAW / 4)
+#define CD_C2BUFFERSIZE (CD_FRAMESIZE_RAW / 8)
 
 #include <sys/types.h>
 #include <signal.h>
@@ -133,6 +134,7 @@
        long beginsector, long sectors);
 extern long cdda_read_timed(cdrom_drive *d, void *buffer,
     long beginsector, long sectors, int *milliseconds);
+extern int cdda_read_c2_errors(cdrom_drive *d, void *buffer, int sectors);
 
 extern long cdda_track_firstsector(cdrom_drive *d,int track);
 extern long cdda_track_lastsector(cdrom_drive *d,int track);
Index: interface/scsi_interface.c
===================================================================
--- interface/scsi_interface.c (revision 16022)
+++ interface/scsi_interface.c (working copy)
@@ -58,7 +58,7 @@
   sprintf(buffer,"\tDMA scatter/gather table entries: %d\n\t"
   "table entry size: %d bytes\n\t"
   "maximum theoretical transfer: %d sectors\n",
-  table, reserved, table*(reserved/CD_FRAMESIZE_RAW));
+  table, reserved, table*(reserved/d->private_data->data_size));
   cdmessage(d,buffer);
 
   cur=reserved; /* only use one entry for now */
@@ -82,11 +82,11 @@
       "\t\tforcing maximum possible sector size.  This can break\n"
       "\t\tspectacularly; use with caution!\n");
   }
-  d->nsectors=cur/CD_FRAMESIZE_RAW;
+  d->nsectors=cur/d->private_data->data_size;
   d->bigbuff=cur;
 
   sprintf(buffer,"\tSetting default read size to %d sectors (%d bytes).\n\n",
-  d->nsectors,d->nsectors*CD_FRAMESIZE_RAW);
+  d->nsectors,d->nsectors*d->private_data->data_size);
 
   if(cur==0) exit(1);
 
@@ -912,90 +912,120 @@
   return(0);
 }
 
+/* READ CD command (0xBE)
+ * byte 1: bit 0:
+ *   - mmc1-mmc4: RELADR: must be 0
+ *   - mmc5-    : obsolete
+ * byte 1: bit 1:
+ *   - mmc1-mmc3: Reserved
+ *   - mmc4-    : Digital Audio Play: When set to 1 the drive will do things
+ *                like mute and interpolate on errors
+ * byte 1: bit 4-2: sector type
+ *   - 000: All types (mandotary)
+ *   - 001: CD-DA (mmc1-mmc3: optional, mmc4-: mandotary)
+ *   - 002: Mode 1 (mandotary)
+ *   - 003: Mode 2 formless (mmc1-mmc3: madotary, mmc4-: optional)
+ *   - 004: Mode 2 form 1 (mandotary)
+ *   - 005: Mode 2 form 2 (mandotary)
+ * byte 2-5: Start LBA
+ * byte 6-8: Length
+ * byte 9: bit 0: reserved
+ * byte 9: bit 2-1: C2 error info:
+ *   - 00: No C2 info
+ *   - 01: 1 bit for all 2352 bytes
+ *   - 10: Bitwise or of C2 bytes + pad + 1 bit for all 2352 bytes.
+ * byte 9: bit 3: EDC & ECC
+ * byte 9: bit 4: User Data
+ * byte 9: bit 6-5: Header Codes
+ *   - 00: No header info
+ *   - 01: 4 byte sector header
+ *   - 10: 8 byte sector sub-header (mode 2)
+ *   - 11: both header and sub-header
+ * byte 9: bit 7: Sync
+ * byte 10: bit 2-0: sub channel selection:
+ *   - 000: No sub channel
+ *   - 001: RAW P-W channel (only mentioned in mmc1 - mmc4, but others still
+ *          talk about it.)
+ *   - 010: Q channel data
+ *   - 100: Corrected and deinterleaved R-W subchannel
+ *
+ * Note: Setting any of the bits 7 - 3 from byte 9 for should act the same
+ *       and only return the user data (0x10) for CDDA.
+ *
+ */
+
 static int i_read_mmc (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
   int ret;
-  unsigned char cmd[12]={0xbe, 0x2, 0, 0, 0, 0, 0, 0, 0, 0x10, 0, 0};
+  unsigned char cmd[12]={0xbe, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
 
-  cmd[3] = (begin >> 16) & 0xFF;
-  cmd[4] = (begin >> 8) & 0xFF;
-  cmd[5] = begin & 0xFF;
-  cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
-    return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
-  return(0);
-}
+  if(d->private_data->mmc_use_dap)
+  {
+    cmd[1] |= 0x02;
+  }
 
-static int i_read_mmcB (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
-  int ret;
-  unsigned char cmd[12]={0xbe, 0x0, 0, 0, 0, 0, 0, 0, 0, 0x10, 0, 0};
+  if(d->private_data->mmc_use_sectortype_cdda)
+  {
+    cmd[1] |= 0x04; /* 001 in bit 4-2 */
+  }
 
-  cmd[3] = (begin >> 16) & 0xFF;
-  cmd[4] = (begin >> 8) & 0xFF;
-  cmd[5] = begin & 0xFF;
-  cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
-    return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
-  return(0);
-}
+  if(d->private_data->mmc_use_channel_selection_all) /* bit 7-3 */
+  {
+    cmd[9] |= 0xF8; /* get sync, all headers, user data, EDC/ECC */
+  }
+  else
+  {
+    cmd[9] |= 0x10; /* get user data */
+  }
 
-static int i_read_mmc2 (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
-  int ret;
-  unsigned char cmd[12]={0xbe, 0x2, 0, 0, 0, 0, 0, 0, 0, 0xf8, 0, 0};
+  if (d->private_data->mmc_use_c2)
+  {
+    /* Git 1 bit error indication per byte after c2 error correction. */
+    cmd[9] |= 0x02;
+  }
 
   cmd[3] = (begin >> 16) & 0xFF;
   cmd[4] = (begin >> 8) & 0xFF;
   cmd[5] = begin & 0xFF;
   cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
+  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * d->private_data->data_size,'\177',1,sense)))
     return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
+  if(p)
+  {
+    int i;
+    for (i = 0; i < sectors; i++)
+    {
+      if (d->private_data->mmc_use_c2 && d->private_data->c2_buffer != NULL)
+      {
+ memcpy(d->private_data->c2_buffer+i*CD_C2BUFFERSIZE,
+  d->private_data->sg_buffer + i * (d->private_data->data_size)
+    + CD_FRAMESIZE_RAW,
+  CD_C2BUFFERSIZE);
+      }
+      memcpy(p+i*CD_FRAMESIZE_RAW,
+ d->private_data->sg_buffer + i * (d->private_data->data_size),
+ CD_FRAMESIZE_RAW);
+    }
+  }
   return(0);
 }
 
-static int i_read_mmc2B (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
-  int ret;
-  unsigned char cmd[12]={0xbe, 0x0, 0, 0, 0, 0, 0, 0, 0, 0xf8, 0, 0};
-
-  cmd[3] = (begin >> 16) & 0xFF;
-  cmd[4] = (begin >> 8) & 0xFF;
-  cmd[5] = begin & 0xFF;
-  cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
-    return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
-  return(0);
+/* TODO: Remove the sectors parameter? */
+extern int cdda_read_c2_errors(cdrom_drive *d, void *buffer, int sectors)
+{
+  if (!d->is_mmc)
+  {
+    /* TODO: Use different error number? */
+    return -405;
+  }
+  if (!d->private_data->mmc_use_c2 || d->private_data->c2_buffer == NULL)
+  {
+    /* TODO: Is this a good error?  Document it. */
+    return -11;
+  }
+  memcpy(buffer, d->private_data->c2_buffer, CD_C2BUFFERSIZE*sectors);
+  return 0;
 }
 
-static int i_read_mmc3 (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
-  int ret;
-  unsigned char cmd[12]={0xbe, 0x6, 0, 0, 0, 0, 0, 0, 0, 0xf8, 0, 0};
-
-  cmd[3] = (begin >> 16) & 0xFF;
-  cmd[4] = (begin >> 8) & 0xFF;
-  cmd[5] = begin & 0xFF;
-  cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
-    return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
-  return(0);
-}
-
-static int i_read_mmc3B (cdrom_drive *d, void *p, long begin, long sectors, unsigned char *sense){
-  int ret;
-  unsigned char cmd[12]={0xbe, 0x4, 0, 0, 0, 0, 0, 0, 0, 0xf8, 0, 0};
-
-  cmd[3] = (begin >> 16) & 0xFF;
-  cmd[4] = (begin >> 8) & 0xFF;
-  cmd[5] = begin & 0xFF;
-  cmd[8] = sectors;
-  if((ret=handle_scsi_cmd(d,cmd,12,0,sectors * CD_FRAMESIZE_RAW,'\177',1,sense)))
-    return(ret);
-  if(p)memcpy(p,d->private_data->sg_buffer,sectors*CD_FRAMESIZE_RAW);
-  return(0);
-}
-
 /* straight from the MMC3 spec */
 static inline void LBA_to_MSF(long lba,
       unsigned char *M,
@@ -1227,31 +1257,17 @@
   return(scsi_read_map(d,p,begin,sectors,i_read_mmc));
 }
 
+/* This function gets called in the exception list.  We probably just want to
+ * turn on DAP. */
 long scsi_read_mmc2 (cdrom_drive *d, void *p, long begin,
        long sectors){
-  return(scsi_read_map(d,p,begin,sectors,i_read_mmc2));
+  d->private_data->mmc_use_sectortype_cdda = 0;
+  d->private_data->mmc_use_dap = 1;
+  d->private_data->mmc_use_channel_selection_all = 1;
+  d->private_data->mmc_use_c2 = 0;
+  return(scsi_read_map(d,p,begin,sectors,i_read_mmc));
 }
 
-long scsi_read_mmc3 (cdrom_drive *d, void *p, long begin,
-       long sectors){
-  return(scsi_read_map(d,p,begin,sectors,i_read_mmc3));
-}
-
-long scsi_read_mmcB (cdrom_drive *d, void *p, long begin,
-       long sectors){
-  return(scsi_read_map(d,p,begin,sectors,i_read_mmcB));
-}
-
-long scsi_read_mmc2B (cdrom_drive *d, void *p, long begin,
-       long sectors){
-  return(scsi_read_map(d,p,begin,sectors,i_read_mmc2B));
-}
-
-long scsi_read_mmc3B (cdrom_drive *d, void *p, long begin,
-       long sectors){
-  return(scsi_read_map(d,p,begin,sectors,i_read_mmc3B));
-}
-
 long scsi_read_msf (cdrom_drive *d, void *p, long begin,
        long sectors){
   return(scsi_read_map(d,p,begin,sectors,i_read_msf));
@@ -1305,7 +1321,7 @@
        long sectors);
   unsigned char density;
   
-  int16_t *buff=malloc(CD_FRAMESIZE_RAW);
+  int16_t *buff=malloc(d->private_data->data_size);
 
   cdmessage(d,"Verifying CDDA command set...\n");
 
@@ -1401,33 +1417,51 @@
   
   /* 2 through 10 do not allow/require density */
  case 2:
-  d->read_audio=scsi_read_mmcB;
-  rs="be 02,10";
+  d->private_data->mmc_use_sectortype_cdda = 0;
+  d->private_data->mmc_use_dap = 0;
+  d->private_data->mmc_use_channel_selection_all = 0;
+  d->read_audio=scsi_read_mmc;
+  rs="be 00,10";
   if(i==0)break;
  case 3:
   j=3;
-  d->read_audio=scsi_read_mmc2B;
-  rs="be 02,f8";
+  d->private_data->mmc_use_sectortype_cdda = 0;
+  d->private_data->mmc_use_dap = 0;
+  d->private_data->mmc_use_channel_selection_all = 1;
+  d->read_audio=scsi_read_mmc;
+  rs="be 00,f8";
   if(i==0)break;
  case 4:
   j=4;
-  d->read_audio=scsi_read_mmc3B;
-  rs="be 06,f8";
+          d->private_data->mmc_use_sectortype_cdda = 1;
+          d->private_data->mmc_use_dap = 0;
+          d->private_data->mmc_use_channel_selection_all = 1;
+          d->read_audio=scsi_read_mmc;
+          rs="be 04,f8";
   if(i==0)break;
  case 5:
   j=5;
+  d->private_data->mmc_use_sectortype_cdda = 0;
+  d->private_data->mmc_use_dap = 1;
+  d->private_data->mmc_use_channel_selection_all = 0;
   d->read_audio=scsi_read_mmc;
-  rs="be 00,10";
+  rs="be 02,10";
   if(i==0)break;
  case 6:
   j=6;
-  d->read_audio=scsi_read_mmc2;
-  rs="be 00,f8";
+  d->private_data->mmc_use_sectortype_cdda = 0;
+  d->private_data->mmc_use_dap = 1;
+  d->private_data->mmc_use_channel_selection_all = 1;
+  d->read_audio=scsi_read_mmc;
+  rs="be 02,f8";
   if(i==0)break;
  case 7:
   j=7;
-  d->read_audio=scsi_read_mmc3;
-  rs="be 04,f8";
+  d->private_data->mmc_use_sectortype_cdda = 1;
+  d->private_data->mmc_use_dap = 1;
+  d->private_data->mmc_use_channel_selection_all = 1;
+  d->read_audio=scsi_read_mmc;
+  rs="be 06,f8";
   if(i==0)break;
  case 8:
   j=8;
@@ -1547,11 +1581,7 @@
   long i;
 
   if(!(d->read_audio==scsi_read_mmc ||
-     d->read_audio==scsi_read_mmc2 ||
-     d->read_audio==scsi_read_mmc3 ||
-     d->read_audio==scsi_read_mmcB ||
-     d->read_audio==scsi_read_mmc2B ||
-       d->read_audio==scsi_read_mmc3B)){
+     d->read_audio==scsi_read_mmc2)){
 
     cdmessage(d,"This command set may use a Force Unit Access bit.");
     cdmessage(d,"\nChecking drive for FUA bit support...\n");
@@ -1627,17 +1657,25 @@
     if((b[0]&0x3F)==0x2A){
       /* MMC style drive! */
       d->is_mmc=1;
+      d->private_data->mmc_has_c2 = 0;
+      d->private_data->mmc_use_c2 = 0;
       
       if(b[1]>=4){
  if(b[5]&0x1){
   cdmessage(d,"\tDrive is MMC style\n");
-  return(1);
  }else{
   cdmessage(d,"\tDrive is MMC, but reports CDDA incapable.\n");
   cdmessage(d,"\tIt will likely not be able to read audio data.\n");
-  return(1);
  }
+ if (b[5]&0x10)
+ {
+  d->private_data->mmc_has_c2 = 1;
+  /* TODO: Make this optional? */
+  d->private_data->mmc_use_c2 = 1;
+  cdmessage(d,"\tDrive supports reporting C2 errors\n");
+ }
       }
+      return 1;
     }
   }
   
@@ -1683,16 +1721,29 @@
   d->enable_cdda = Dummy;
   d->read_audio = scsi_read_D8;
   d->fua=0x0;
+  d->private_data->data_size = CD_FRAMESIZE_RAW;
+  d->private_data->c2_buffer=NULL;
   if(d->is_atapi)d->lun=0; /* it should already be; just to make sure */
       
   if(d->is_mmc){
-    d->read_audio = scsi_read_mmc2B;
+    d->private_data->mmc_use_sectortype_cdda = 0;
+    d->private_data->mmc_use_dap = 0;
+    d->private_data->mmc_use_channel_selection_all = 1;
+    d->read_audio = scsi_read_mmc;
     d->bigendianp=0;
     check_exceptions(d,mmc_list);
+    if (d->private_data->mmc_use_c2)
+    {
+       /* 1 bit for every byte */
+       d->private_data->data_size += CD_C2BUFFERSIZE;
+    }
   }else{
     if(d->is_atapi){
       /* Not MMC maybe still uses 0xbe */
-      d->read_audio = scsi_read_mmc2B;
+      d->private_data->mmc_use_sectortype_cdda = 0;
+      d->private_data->mmc_use_dap = 0;
+      d->private_data->mmc_use_channel_selection_all = 1;
+      d->read_audio = scsi_read_mmc;
       d->bigendianp=0;
       check_exceptions(d,atapi_list);
     }else{
@@ -1725,8 +1776,11 @@
   check_cache(d);
 
   d->error_retry=1;
-  d->private_data->sg_hd=realloc(d->private_data->sg_hd,d->nsectors*CD_FRAMESIZE_RAW + SG_OFF + 128);
+  d->private_data->sg_hd=realloc(d->private_data->sg_hd,d->nsectors*d->private_data->data_size + SG_OFF + 128);
   d->private_data->sg_buffer=((unsigned char *)d->private_data->sg_hd)+SG_OFF;
+  /* TODO: Should get reallocated?  But where should the initial allocation
+   * be in that case?  Never gets free()d. */
+  d->private_data->c2_buffer=malloc(d->nsectors*(CD_C2BUFFERSIZE));
   d->report_all=1;
   return(0);
 }
Index: interface/low_interface.h
===================================================================
--- interface/low_interface.h (revision 16022)
+++ interface/low_interface.h (working copy)
@@ -100,8 +100,17 @@
 struct cdda_private_data {
   struct sg_header *sg_hd;
   unsigned char *sg_buffer; /* points into sg_hd */
+  unsigned char *c2_buffer;
   clockid_t clock;
   int last_milliseconds;
+  int data_size;
+
+  int mmc_has_c2;
+  int mmc_use_c2;
+
+  int mmc_use_sectortype_cdda;
+  int mmc_use_dap;
+  int mmc_use_channel_selection_all;
 };
 
 #define MAX_RETRIES 8

---

_______________________________________________
Paranoia-dev mailing list
Paranoia-dev@...
http://lists.xiph.org/mailman/listinfo/paranoia-dev

[xiphmont]

> Here is an initial patch.  It would be nice if people could test
> this and give feedback.

C2 error information is on the short-term feature list (along with
more sophisticated reconstruction than offered by the redbook spec),
so allow me to try to bend this all into that direction :-)

> Some comments about the patch:
> - I've merged all the i_read_mmc() functions to 1 function
>  so that not all of them need to be modified.

The dispatch design currently there is cute and badly out of date, and
the next major release with new features (including C2) should ditch
the old interface entirely.

We need to make sure to probe for the capability carefully, pay
attention to whether or not it is actually working, and not attempt to
use it on dives that don't support it correctly.  It's less common
that newer drives will brick if you ask them for something they can't
do, but it's a regular problem in old drives.

> - rs was set wrong in verify_read_command().  They all had
>  the DAP bit inverted.

Oh, ha, you're right.  I had to look three times to see it even after
you told me. "Functional testing does not necessarily find
cosmetic/informational errors"

> - There are still a few TODOs in it.

Good to get it working, play with it, and submit for review before
going too all out.

> I've been testing this with a CD with lots of scratches.  What
> I've seen:
> - My plextor reads this CD at full speed (40x) every time, always
>  identical, never returns C2 errors.

That sounds problematic.  Unless it's changed, the Plextors should be
among the drives that return full corrected and uncorrected C2
information.  Even a pristine disc has corrected errors.

> - The other drives have problems reading this, even at speed 1.
>  Without this patch I got 20+ different files.

I have not yet looked at what you're doing with the C2 information you
collect-- how is the matching/verification algorithm modified?

>  Once I got a total of 1.8 million
>  C2 errors for 1 track, but the result was still good.

That sounds about right :-)  Are you leveraging any sort of your own
reconstruction or relying on the drive to interpolate?

> - The current patch marks all samples belonging to 1 byte of c2
>  error information as having a problem while it should probably
>  be possible to only mark the sample that is affected.

*yes*.  Especially as, on a general purpose CPU, we have access to far
more powerful audio reconstruction techniques than the drive can do in
firmware.  The better job we do seperating the good samples from the
bad (even the good bits from the bad bits within samples!), the
better.

The physical structure of the CD is going to scatter adjacent samples
to different parts of the surface such that runs of bad bytes are
rare.  usually you get interleaved errors, which naturally makes
recovery earlier.  We must not toss that advantage away.

>  For some
>  reason that's currently unclear to me this always results in a
>  good file and trying to mark only the bad one resulted in a bad
>  file.  (Maybe I should mark both left and right instead of only
>  left or right?)

Well, we'd need to figure that out :-)  Probably has to do with
exactly how the matching's been modified to deal with the flags.

Monty
_______________________________________________
Paranoia-dev mailing list
Paranoia-dev@...
http://lists.xiph.org/mailman/listinfo/paranoia-dev

[Kurt Roeckx]

On Wed, May 27, 2009 at 04:58:28PM -0400, xiphmont@... wrote:
> We need to make sure to probe for the capability carefully, pay
> attention to whether or not it is actually working, and not attempt to
> use it on dives that don't support it correctly.  It's less common
> that newer drives will brick if you ask them for something they can't
> do, but it's a regular problem in old drives.

The current code looks at the (legacy) 2A page to see if C2 is
supported or not, and then always turns it on.  There are a few
things wrong with it:
- If you look at the mmc1 spec it says that being able to request
  it is mandatory, but drives that don't support it should just
  return 0 bits, giving you the impression that there is no
  error.  But it's not because that's what the spec says, that
  that is what all drives do.  So we might need a test to see
  how much data we really receive when we requested it.
- If your drive fully supports it, you might want to test the
  effect of having it turned on or off
- We should probably not rely on the legacy code page to detect
  it for new drives.

> > I've been testing this with a CD with lots of scratches.  What
> > I've seen:
> > - My plextor reads this CD at full speed (40x) every time, always
> >  identical, never returns C2 errors.
>
> That sounds problematic.  Unless it's changed, the Plextors should be
> among the drives that return full corrected and uncorrected C2
> information.  Even a pristine disc has corrected errors.

C2 error information is only uncorrected errors, there is no
indication of corrected errors.

Looking at documentation of plextor software, it seems they can
also read C1 and C2 error rates, but they probably get that using
vendor specific commands.

Also not that you do not get the C1/C2 error correcting bytes
themself (8*98=784), just 1 bit for every byte, so 24/8*98=294
bytes.

> > - The other drives have problems reading this, even at speed 1.
> >  Without this patch I got 20+ different files.
>
> I have not yet looked at what you're doing with the C2 information you
> collect-- how is the matching/verification algorithm modified?

The only thing it does it marking samples that had an error
as unread.  The code does the same thing if the whole read
wasn't complete.  I did not really look into how all the
algorithmes work, but it seems this was enough for me to get
it working.

> >  Once I got a total of 1.8 million
> >  C2 errors for 1 track, but the result was still good.
>
> That sounds about right :-)  Are you leveraging any sort of your own
> reconstruction or relying on the drive to interpolate?

As far as I know, based on the debug info I added, cdparanoia just
reads it multiple times until it got all samples (twice?) without
an error.

If it realy fails to read only some bytes, we could interpolate
ourself.

> > - The current patch marks all samples belonging to 1 byte of c2
> >  error information as having a problem while it should probably
> >  be possible to only mark the sample that is affected.
>
> *yes*.  Especially as, on a general purpose CPU, we have access to far
> more powerful audio reconstruction techniques than the drive can do in
> firmware.  The better job we do seperating the good samples from the
> bad (even the good bits from the bad bits within samples!), the
> better.

The C1/C2 error correction works on byte basis.  Either the whole
byte is good or bad.  It's designed to handle burst errors and not
single bit errors.

> The physical structure of the CD is going to scatter adjacent samples
> to different parts of the surface such that runs of bad bytes are
> rare.  usually you get interleaved errors, which naturally makes
> recovery earlier.  We must not toss that advantage away.

If I remember correctly, they have sample 0, 2, 4, 6 in subframe 0
and 1, 3, 5, 7 in the next.  If subframe 0 can't be recovered, you
can interpolate the samples based on the other subframe.

> >  For some
> >  reason that's currently unclear to me this always results in a
> >  good file and trying to mark only the bad one resulted in a bad
> >  file.  (Maybe I should mark both left and right instead of only
> >  left or right?)
>
> Well, we'd need to figure that out :-)  Probably has to do with
> exactly how the matching's been modified to deal with the flags.

I was thinking that maybe the matching now only looks at the left
channel and assumes that if you have a sample for left you also
have the right sample.  But I'll look into that.


Kurt

_______________________________________________
Paranoia-dev mailing list
Paranoia-dev@...
http://lists.xiph.org/mailman/listinfo/paranoia-dev