Re: C2 error support.
Kurt Roeckx <[email protected]> Sun, 24 May 2009 11:32:20 +0200
| Newsgroups | gmane.comp.audio.cd-paranoia.devel |
|---|---|
| Message-ID | <[email protected]> |
--FL5UXtIhxfXey3p5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
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
--FL5UXtIhxfXey3p5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="c2_errors.diff"
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
--FL5UXtIhxfXey3p5
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
Paranoia-dev mailing list
[email protected]
http://lists.xiph.org/mailman/listinfo/paranoia-dev
--FL5UXtIhxfXey3p5--