On Tue, Dec 11, 2018 at 1:48 PM Thomas Schmitt <scdbac...@gmx.net> wrote:
> Hi, > > lagging behind my promises i today combined vetx01/libcdio with > vext01/libcdio-paranoia. > > Bugs found and fixed so far: > > - libcdio/lib/driver/gnu_linux.c puts the highest track number into the > track counter _img_private_t.i_tracks (netbsd.c does it right). > This caused ill track access, but also enough attempts with cd-paranoia > -Q. > > - The loop in display_toc() of src/cd-paranoia.c is wrong. It begins to > count from 1 up to track counter (which was wrongly the highest track > number on gnu_linux.c). > It should count from first track to last track plus 1 for lead-out. > This caused ill track access. > > - The loop in lib/cdda_interface/common_interface.c function > data_bigendianp() > should iterate over nominal track numbers but rather does over > disc_toc[] index plus 1. > Symptom is the message: > Cannot determine CDROM drive endianness. > > The following run now works for me without raising further suspicions: > > cd-paranoia -d /dev/sr4 -Q > > I could need exact instructions about what further tests to run. > > > Edd: How to proceed git-wise now ? > > Shall i branch from your branches and collect fixes ? (New clone from > better address ?) > Shall i produce patches which you put into your branches ? > (Patch content is shown in the text below. Give me instructions about > their format if you want them.) > > ------------------------------------------------------------------------ > Dirty details in chronological sequence: > > install_dir=/...absolute.path.../install_dir > > git clone https://github.com/vext01/libcdio > cd libcdio > git checkout openbsd_fixes_to_master > sh ./autogen.sh > ./configure --prefix=$install_dir > make > (cd src ; touch cd-drive.1 cd-info.1 cd-read.1 iso-info.1 iso-read.1) > make > make install > > cd ../libcdio-paranoia > make clean > ./configure --prefix=$install_dir > make > make install > > without much errors (except the traditional problem with non-existing man > pages which i solved by a run of "touch"). > > Surprisingly > > ldd "$install_dir"/bin/cd-paranoia > > shows two libcdio.so binaries: > > libcdio.so.16 => /usr/lib/libcdio.so.16 (0x00007f7b2970a000) > libcdio.so.18 => /...install_dir.../lib/libcdio.so.18 > (0x00007f7b28821000) > > (and also > libcdio_cdda.so.2 => /...install_dir.../lib/libcdio_cdda.so.2 > (0x00007f7b29502000) > libcdio_paranoia.so.2 => /...install_dir.../lib/libcdio_paranoia.so.2 > (0x00007f7b292fb000) > ) > > A test message put into gnu_linux.c of the freshly cloned libcdio does not > show up in a run of cd-paranoia. So it's still using the old libcdio.so. > > This libcdio.so.16 is pointed to by the link /usr/lib/libcdio.so. > > Is this double libcdio due to my ignorance towards PKG_CONFIG_PATH ? > (To what value should i set it ? I tried "$install_dir", but nothing > changes after "make clean ; make ; make install" of libcdio-paranoia.) > > I tried with hidden /usr/lib/libcdio.so. It tried to use libcdio.a and > failed with relocation incompatibility (because trying to make an .so ?). > When removing /usr/bin/libcdio.a, it failed because not finding libcdio. > > I finally had to point /usr/lib/libcdio.so to $install_dir/lib/libcdio.so > before i could see my test message from gnu_linux.c. > > -------------------------------------------------------------------------- > > Whatever, the wrong output still persists > > $ "$install_dir"/bin/cd-paranoia -d /dev/sr4 -Q > ... > track length begin copy pre ch > =========================================================== > 4. 2682 [00:35.57] 0 [00:00.00] no no 2 > 5. 0 [00:00.00] 2682 [00:35.57] no no 2 > 6. 0 [00:00.00] 2682 [00:35.57] no no 2 > TOTAL 2682 [00:35.57] (audio only) > > But at least it should now run the code that is to be tested. > > In src/cd-paranoia.c, d->tracks is 6, i.e. the highest track number. > Its variable name rather suggests that it shall be the track count. > (The loop takes profit.) > In cdio/paranoia/cdda.h the definition of cdrom_drive_t prescibes to > subtract cdio_get_first_track_num() from track number to get the index > for d->disc_toc[]. So d->tracks should be 3 or 4 (modulo lead-out track). > > lib/cdda_interface/cddap_interface.c has: > d->tracks = cdio_get_num_tracks(d->p_cdio) ; > libcdio/.../gnu_linux.c points to > get_num_tracks_generic() > whereas netbsd.c points to > get_num_tracks_netbsd() > > get_num_tracks_generic() returns generic_img_private_t.i_tracks . > get_num_tracks_netbsd() returns TOTAL_TRACKs which in netbsd.c is the same > as .i_tracks. > > But gnu_linux.c happily puts the highest track number into .i_tracks. > The number of tracks is computed from both. E.g. in read_toc_linux(): > u_tracks = p_env->gen.i_tracks - p_env->gen.i_first_track + 1; > > > The definition of generic_img_private_t in libcdio/lib/driver/generic.h > says: > > track_t i_first_track; /**< The starting track number. */ > track_t i_tracks; /**< The number of tracks. */ > > So gnu_linux.c is wrong. > > -------------------------------------------------------------------------- > > Then of course, the loop in display_toc() of src/cd-paranoia.c is wrong: > > for( i=1; i<=d->tracks; i++) > if ( cdda_track_audiop(d,i) > 0 ) { > > lsn_t sec=cdda_track_firstsector(d,i); > > cdda_track_audiop() in libcdio-paranoia/lib/cdda_interface/toc.c uses "i" > for cdio_get_track_format() which in gnu_linux.c and in netbsd.c expects > nominal track numbers. > cdda_track_firstsector() subtracts from "i" the first nominal track number > and uses the result as index to cdrom_drive_t.disc_toc[]. > > So the loop should rather be > > for( i = cdio_get_first_track_num(d->p_cdio); > i <= cdio_get_last_track_num(d->p_cdio); i++) > > But with gnu_linux.c as it is now, i get > > d->tracks= 6, first= 4, last= 9 > > So cdio_get_last_track_num() is broken on gnu_linux.c.i > Probably by the .i_tracks bug. > > And access to the first three tracks is still not correct: > > 4. 2682 [00:35.57] 0 [00:00.00] no no 2 > 5. 0 [00:00.00] 2682 [00:35.57] no no 2 > 6. 0 [00:00.00] 2682 [00:35.57] no no 2 > 7. 0 [00:00.00] 2682 [00:35.57] no no 4 > 8. 0 [00:00.00] 2682 [00:35.57] no no 4 > 9. 5364 [01:11.39] 2682 [00:35.57] no no 4 > TOTAL 8046 [01:47.21] (audio only) > > I have to fix gnu_linux.c first: > > ============================================================================ > --- lib/driver/gnu_linux.c.orig 2018-12-11 13:59:31.849293595 +0100 > +++ lib/driver/gnu_linux.c 2018-12-11 15:58:51.897320630 +0100 > @@ -1126,7 +1126,7 @@ static bool > read_toc_linux (void *p_user_data) > { > _img_private_t *p_env = p_user_data; > - int i; > + int i, i_last_track; > unsigned int u_tracks; > > /* read TOC header */ > @@ -1137,9 +1137,10 @@ read_toc_linux (void *p_user_data) > } > > p_env->gen.i_first_track = p_env->tochdr.cdth_trk0; > - p_env->gen.i_tracks = p_env->tochdr.cdth_trk1; > + i_last_track = p_env->tochdr.cdth_trk1; > + p_env->gen.i_tracks = i_last_track - p_env->gen.i_first_track + 1; > > - u_tracks = p_env->gen.i_tracks - p_env->gen.i_first_track + 1; > + u_tracks = p_env->gen.i_tracks; > > if (u_tracks > CDIO_CD_MAX_TRACKS) { > cdio_log(CDIO_LOG_WARN, "Number of tracks exceeds maximum (%d vs. > %d)\n", > @@ -1149,7 +1150,7 @@ read_toc_linux (void *p_user_data) > > > /* read individual tracks */ > - for (i= p_env->gen.i_first_track; i<=p_env->gen.i_tracks; i++) { > + for (i= p_env->gen.i_first_track; i <= i_last_track; i++) { > struct cdrom_tocentry *p_toc = > &(p_env->tocent[i-p_env->gen.i_first_track]); > > > ============================================================================ > > Next i fix cd-paranoia.c: > > > ============================================================================ > > --- src/cd-paranoia.c.orig 2018-12-09 21:59:02.956749766 +0100 > +++ src/cd-paranoia.c 2018-12-11 19:09:31.601363825 +0100 > @@ -251,7 +251,8 @@ display_toc(cdrom_drive_t *d) > "track length begin copy pre ch\n" > "==========================================================="); > > - for( i=1; i<=d->tracks; i++) > + for( i=cdio_get_first_track_num(d->p_cdio); > + i<=cdio_get_last_track_num(d->p_cdio); i++) > if ( cdda_track_audiop(d,i) > 0 ) { > > lsn_t sec=cdda_track_firstsector(d,i); > > > ============================================================================ > > Now i get a much better result with cd-paranoia -Q: > > 4. 2682 [00:35.57] 0 [00:00.00] no no 2 > 5. 2682 [00:35.57] 2682 [00:35.57] no no 2 > 6. 2682 [00:35.57] 5364 [01:11.39] no no 2 > TOTAL 8046 [01:47.21] (audio only) > > (d->tracks= 3, first= 4, last= 6) > > But now the CD with track 1 to 3 gets stuck for quite a while and then > reports > > 006: Could not read any data from drive > > Cdparanoia could not find a way to read audio from this drive. > > This is reproducible until i eject and reload the tray. > Then i get the correct track list: > > ts_debug: d->tracks= 3, first= 1, last= 3 > 1. 2682 [00:35.57] 0 [00:00.00] no no 2 > 2. 2682 [00:35.57] 2682 [00:35.57] no no 2 > 3. 2682 [00:35.57] 5364 [01:11.39] no no 2 > TOTAL 8046 [01:47.21] (audio only) > > More runs succeed, but last very long. What does it do for 45 seconds ? > The drive is busy blinking. > With option -v the last message before the long waiting is > > Attempting to determine drive endianness from data.... > > when it continues, it says > > Data appears to be coming back Little Endian. > certainty: 100% > > libcdio-paranoia/lib/cdda_interface/common_interface.c has > > /* find a block with nonzero data * > > This explains a lot. My data are dummies, not real audio. > > At least it nothing to do with track numbers. > But why does it sometimes last 15 seconds and sometimes 45 ? > > And why is the CD with track 4 to 6 reliably fast ? > Aha: > Cannot determine CDROM drive endianness. > > Reason is that data_bigendianp() in common_interface.c loops over > track indice from 0 to (d->tracks - 1) and converts them into track > numbers by adding 1. > > > ============================================================================ > > It seems odd to count from first track minus 1 to last track minus 1 > only to add 1 with any usage of "i". So i also changed the "i+1" gestures. > > --- lib/cdda_interface/common_interface.c.orig 2018-12-09 > 21:57:17.716749368 +0100 > +++ lib/cdda_interface/common_interface.c 2018-12-11 > 18:56:01.209360765 +0100 > @@ -70,12 +70,13 @@ data_bigendianp(cdrom_drive_t *d) > > cdmessage(d,"\nAttempting to determine drive endianness from data..."); > d->enable_cdda(d,1); > - for(i=0,checked=0;i<d->tracks;i++){ > + for(i=cdio_get_first_track_num(d->p_cdio), checked=0; > + i<=cdio_get_last_track_num(d->p_cdio); i++){ > float lsb_energy=0; > float msb_energy=0; > - if(cdda_track_audiop(d,i+1)==1){ > - long firstsector=cdda_track_firstsector(d,i+1); > - long lastsector=cdda_track_lastsector(d,i+1); > + if(cdda_track_audiop(d,i)==1){ > + long firstsector=cdda_track_firstsector(d,i); > + long lastsector=cdda_track_lastsector(d,i); > int zeroflag=-1; > long beginsec=0; > > > ============================================================================ > > Now i get from the CD with track 4 to 6: > > Attempting to determine drive endianness from data...... > Data appears to be coming back Little Endian. > certainty: 100% > > But this decision is made in less than a second, each time i try. > Shrug. > > I close the cd-paranoia shop for today. Instructions about how to submit > my changes would be appreciated. > For libcdoi changes, when you and Edd are satisfied that things are the way they should be merge that into a branch in savannah libcdio and then merge that branch into master. For cdio_paranoia changes, use the usual github process of submitting pull request. Also, if Edd hasn't filled out a FSF copyright release form, he should do that. > > Have a nice day :) > > Thomas > > > ᐧ
