calculates wrong cddb id due to missing pregap

The CDDB code is the same in 1.2.0 and 1.1.5, so far.

See http://git.videolan.org/?p=vlc.git;a=blob;f=modules/access/cdda.c;hb=HEAD#l604 for the code.

added 1 deleted label

I forgot to adjust the disc length for the pregap, so the last cddb query calc should be:

$ cddb_query calc 2769 13 150 17585 31226 44585 60540 76935 95823 104452 125344 140880 148974 167108 193724

CD disc ID is bd0acf0d

which is correct.

cddb_query uses libcdio to read the TOC and uses cdte_format = CDROM_MSF, whereas vlc does it in modules/access/vcd/cdrom.c and uses cdte_format = CDROM_LBA. Could that be the difference?

I'm not entirely clear, but I've read some definitions of LBA that ignore the pregap. libcdio seems to use LSN as a frame offset that is pregap-aware. But sometimes the difference seems a bit muddled.

VLC doesn't use libcdio, IIRC.

Sorry for my later mistake, now I am editing this comment only to correctly state that Function "GetCDDBInfo()" is not really part of libcddb, but is part of VLC source code, like Get_Tracks() the routine that calls it, both of which are definied into VLC "modules/access/cdda.c" source file. Of course, GetCDDBInfo() does make use of data structures declared into "cddb/cddb.h" and makes calls to CDDB related functions which are all dynamically linked to libcddb ( http://libcddb.sourceforge.net/tutorial.html ).

For example, one of this libcddb functions,namely "cddb_disc_calc_discid()", is precisely responsable for calculating the discid that is used, along with the frame offsets of every CD track, for querying the CDDB...

The apparently inexact match returned by the CDDB server, is caused ( as oliverhenshaw found out and fisrt explained in this bug tracking report ) by a missing pregap in all track frame offsets that VLC is passing to libcddb functions responsable for the query, like libcddb:cddb_query :

cdda.c:GetTracks -> cdda.c:GetCDDBInfo( access_t *p_access, int i_titles, int *p_sectors ) -> libcddb:cddb_query( p_cddb, p_disc )

The reason why ( as oliverhenshaw further explained earlier ) the starting frame numbers in all tracks of the "*p_sectors" data structure are missing these 150 frames ( equivalent to the 2 seconds pregap ) is because of always using macro "MSF_TO_LBA2" ( which explicitly subtracts this gap ) instead of macro "MSF_TO_LBA" ( which does not alter the values provided by Native OS API functions, like "DeviceIoControl_()" in the case of MS Windows OSes family ).

Doing a bit more of research, it shows that prior to calling GetCDDBInfo(), the CD Table of Contents, named "cdrom_toc" ( structure of typedef CDROM_TOC ) is initiallized in this call sequence :

cdda.c:GetTracks -> cdrom.c:ioctl_GetTracksMap -> Native API funtion call -- OS Dependant function that retrieves CD TOC depending on ARCHITECTURE ( for APPLE: CDTOCGetDescriptorCount(), WIN32: DeviceIOControl() , OS2: DosDevIOCtl() , etc. )

So, after this OS-dependant API call, made under modules/access/vcd/cdrom.c:ioctl_GetTracksMap() implementation , an array of elements representing all tracks in the CD is dynamically created and pointed by "pp_sectors":

(source file: cdrom.c, LINE 371 )

 *pp_sectors = calloc( i_tracks + 1, sizeof(**pp_sectors) );

This array is inmediately filled inside a loop, using a macro, "MSF_TO_LBA2":

(source file: cdrom.c, LINE 375 )

for( int i = 0 ; i <= i_tracks ; i++ )
            {
                (*pp_sectors)[ i ] = MSF_TO_LBA2(
                                           cdrom_toc.TrackData[i].Address[1],
                                           cdrom_toc.TrackData[i].Address[2],
                                           cdrom_toc.TrackData[i].Address[3] );
                msg_Dbg( p_this, "p_sectors: %i, %i", i, (*pp_sectors)[i]);
             }

As you can see from "cdrom.h":

/**** Misc. Macros   ****/

/* LBA = msf.frame + 75 * ( msf.second + 60 * msf.minute ) */
#define MSF_TO_LBA(min, sec, frame) ((int)frame + 75 * (sec + 60 * min))

/* LBA = msf.frame + 75 * ( msf.second - 2 + 60 * msf.minute ) */
#define MSF_TO_LBA2(min, sec, frame) ((int)frame + 75 * (sec -2 + 60 * min))

This macro, MSF_TO_LBA2 , receives CD track data in the format Minutes, Seconds and Frames, and then calculates the total frame offset by using a formula similar to macro MSF_TO_LBA, but also explicitly subtracting the 2 seconds pregap!!!...

I really dont understand yet why , but just suppose that the choice of macro "MSF_TO_LBA2" instead of "MSF_TO_LBA" ( which does not subtract the pregap ) was made because of being more convenient for VCD, o maybe for controlling CD playback or seek functions... After all, this macro call is made from "modules/access/vcd/cdrom.c" ... I think we need to carefully understand this prior to propose any changes to VLC source code, that maybe could fix CDDB support but at the same time, create a new bug with VCD playback!

So we should change back to MSF_TO_LBA for CDDB?

I am not really sure yet ( need to do some more research, ja! ), but in the meantime I will try this dirty trick on "modules/access/vcd/cdrom.h":

/**** Misc. Macros   ****/

/* LBA = msf.frame + 75 * ( msf.second + 60 * msf.minute ) */
#define MSF_TO_LBA(min, sec, frame) ((int)frame + 75 * (sec + 60 * min))

/* LBA = msf.frame + 75 * ( msf.second - 2 + 60 * msf.minute ) */
#define MSF_TO_LBA2(min, sec, frame) ((int)frame + 75 * (sec + 60 * min))

As you see, I will take away the "-2" term in MSF_TO_LBA2 macro definition, and then proceed to compile ( in fact, crosscompile Win32 version of VLC under linux ). Then I will report back if CDDB support is finally working properly or not... But, of course, as I said earlier, I dont think this would be a proper way to fix CDDB without potencially introducing some new bug related with VCD or even CDDA playback ...

Hi,

I would like to share the way I have found to ( hopefully ) fix this bug preventing VLC from correctly retrieving Audio CD info from any HTTP CDDB Server, that is caused by this missing pregap when building the CDDB database query ( cgi cmd line via http protocol )...

The best fix I could figure out consist on modifying two existing code lines inside "modules/access/cdda.c", part of VLC source code:

Current line 648 of "cdda.c" source:

 /* in order to properly calculate the discid value and also to get the cgi CDDB query correctly built, for every track starting sector, we need to add 150 frames, that comes from the 2 seconds pregap at the begining of every audio CD, and were previuosly substracted while convertings track starting times ( Minutes:Seconds:Frames ) to Logical Block Addresses ( LBA ), using macro MSF_TO_LBA2, defined in <vcd/cdrom.h> and <vcd/cdrom_internals.h>  */
 
        cddb_track_set_frame_offset( t, p_sectors[i] + 150 );  /* adding 150,for the pregap, to every track frame offset */

And finally at current line 665 of "cdda.c" source:

    cddb_disc_set_length( p_disc, (int)(i_length/1000000 + 2 ) );    /* here we add the 2 seconds pregap to the total Audio CD length, in order to get the discid calculated right   */

With only these two code lines modified, every track start frame and total CD length passed to the following calls to libcddb functions via structure "p_cddb" are now correct, so the discid calculation and then the query http cmd line will truely reflect the Audio CD track structure, matching that existing on the internet server database ( like freedb.org or musicbrainz.org ).

This is all that is needed to fix this bug, at least if building VLC for linux platforms... If you want to be able to successfully use the VLC CDDB feature on Windows, then you will also need to fix this other somehow related ( but not dependant ) bug : #13465 (closed) ( https://trac.videolan.org/vlc/ticket/13465 )

Regards!

MSF_TO_LBA2 retrieves 150 from the computation, like CDConvertMSFToLBA does.

So, I cannot change the macros, without breaking macos.

Fixed. Please test vlc.git

commit ab0080f6 Author: Jean-Baptiste Kempf jb@videolan.org Date: Fri Jan 2 15:30:59 2015 +0100

CDDB: add more debug to find the potential issues

Ref [#4370](https://code.videolan.org/videolan/vlc/-/issues/4370)

Original author: jb@videolan.org

commit a4271ec9 Author: Jean-Baptiste Kempf jb@videolan.org Date: Fri Jan 2 15:20:57 2015 +0100

CDDB: add the correct pregap for calculation

150sectors or 2 seconds

Should close [#4370](https://code.videolan.org/videolan/vlc/-/issues/4370)

Original author: jb@videolan.org

added Status::fixed label

closed

Hi, Congrats! Just checked the 03-Ene-2015 nightly build and now the bug seems solved!

Only one suggestion about the msg_Debug() added in commit ab0080f6: to display the discid in the right format (an 8 digit hexa number, as used in search engines like http://www.freedb.org/freedb_discid_check.php ) it should be:

Current line 400, in modules/access/cdda.c:

msg_Dbg( p_access, "Disc ID: %u", cddb_disc_get_discid( p_disc ) );

Proposed modification :

msg_Dbg( p_access, "Disc ID: %08x", cddb_disc_get_discid( p_disc ) );

Regards!

Done.

commit 2d61d6fb Author: Jean-Baptiste Kempf jb@videolan.org Date: Mon Jan 5 17:36:15 2015 +0100

CDDB: display CDDB ID in the right format

Aka a 8 digit hexa number, as used in search engines like
http://www.freedb.org/freedb_discid_check.php

Ref [#4370](https://code.videolan.org/videolan/vlc/-/issues/4370)

Original author: jb@videolan.org

Great :)

calculates wrong cddb id due to missing pregap

use values from vlc gdb session

add 2 second pregap to above values and adjust offsets accordingly

Child items 0

Activity