Thanks for undertaking the more agressive change. A first reading it all looks good. I'll review the code in more detail though later...
On Sun, May 20, 2018 at 12:18 PM, Thomas Schmitt <[email protected]> wrote: > Hi, > > the tentacles of my changeset proposal became invasive. > I tried hard to curb them. > > The few lines which collide with Serge Pouliquen's CD have contact with > three problems: > > - CDTEXT_LANGUAGE_UNKNOWN not only marks language code 0x00 but also > invalid language codes (0x80 to 0xFF) and unused language block slots. > > - There is no way to directly select a language block from the reply array > of cdtext_list_languages(). cdtext_list_languages() does not reliably > tell the block index of a language, anyway. > So one has to use cdtext_select_language() which searches in cdtext_t > for the language code. This works correctly only if no language code > appears more than once. > By mapping language codes 0x80 - 0xFF to a default value, this assumption > is put even more into question. > > - cdtext_list_languages(cdtext_t *p_cdtext) returns a pointer to a static > array, although it gets a parameter from which it reads the content > of that array. > If ever two different cdtext_t objects get used interchangeably, then > they would alter each other's returned language list while the > previous callers are not aware. > > Since we already need a new API call cdtext_list_languages_v2() for solving > the first problem, and since it will then inherit the other two problems, > i decided to address all three by one consistent changeset. > > It will address the problems by: > > + A new API call cdtext_list_languages_v2() which > > + distinguishes CDTEXT_LANGUAGE_UNKNOWN = 0x00, > CDTEXT_LANGUAGE_INVALID = 0x80 to 0xFF, and > CDTEXT_LANGUAGE_BLOCK_UNUSED > > + takes care to keep the indexing in its reply array the same as the > indexing by cdtext_t.block_i ("index of active block"). > > + returns a pointer to a new struct cdtext_s member rather than a pointer > to a static array. > > + A new API call cdtext_set_language_index() which selects a language block > by its index in the reply array of cdtext_list_languages_v2(). > It checks the submitted index and, if ok, sets cdtext_t.block_i to it. > So no search by language code is needed. > > + The already mentioned new struct member in cdtext_s (aka cdtext_t) > cdtext_lang_t languages[CDTEXT_NUM_BLOCKS_MAX]; > /**< return value of cdtext_list_languages_v2() */ > > (I dropped the idea of defining 128 dummy language codes CDTEXT_LANGUAGE_80 > to CDTEXT_LANGUAGE_FF. This would avoid the problem of non-injective > mapping but not the problem of externally provided data which potentially > use multiple blocks with the same language code.) > > ------------------------------------------------------------ > --------------- > > The following proposal passed "make" with gcc 4.9.2. > I then did > > export LD_LIBRARY_PATH="$(pwd)/lib/iso9660/.libs:$(pwd)/lib/udf/. > libs:$(pwd)/lib/driver/.libs" > valgrind --leak-check=full ./src/.libs/cd-info -C /dev/sr4 > > and got the expectable result from our two-language Katzenmusik CD: > > cd-info version 2.0.0 x86_64-unknown-linux-gnu > ... > CD driver name: GNU/Linux > access mode: IOCTL > ... > Disc mode is listed as: CD-DA > CD-ROM Track List (1 - 3) > ... > Language 0 'English': > CD-TEXT for Disc: > TITLE: Night Cats II > PERFORMER: United Cat Orchestra > ... > Language 1 'German': > CD-TEXT for Disc: > TITLE: Nachtkratz II > PERFORMER: Vereinigtes Katzenorchester > ... > ==8028== All heap blocks were freed -- no leaks are possible > ... > ==8028== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) > > ------------------------------------------------------------ > --------------- > > I could now need instructions how to help preparing a test version for > Serge Pouliquen and his CD. > I Cc: Serge in the hope that he can make use of the diff below directly. > > The changes currently sit uncommitted in a tree made by > git clone [email protected]:/srv/git/libcdio.git > > My git knowledge is restricted to very few gestures. If i shall clone > again with a better git command, just tell me. > > ------------------------------------------------------------------------ > Changelog text: > > New API call cdtext_list_languages_v2() to be preferred over now deprecated > cdtext_list_languages(). New API call cdtext_set_language_index(). > > Signed-off-by: Thomas Schmitt <[email protected]> > > ========================================================================== > > --- lib/driver/libcdio.sym.orig 2018-05-18 22:27:56.125019530 +0200 > +++ lib/driver/libcdio.sym 2018-05-20 15:41:07.829579832 +0200 > @@ -194,8 +194,10 @@ cdtext_get_last_track > cdtext_init > cdtext_data_init > cdtext_list_languages > +cdtext_list_languages_v2 > cdtext_set > cdtext_select_language > +cdtext_set_language_index > debug_cdio_mmc_feature > debug_cdio_mmc_feature_interface > debug_cdio_mmc_feature_profile > --- include/cdio/cdtext.h.orig 2018-05-18 22:27:56.121019530 +0200 > +++ include/cdio/cdtext.h 2018-05-20 16:51:54.877595869 +0200 > @@ -192,7 +192,16 @@ typedef enum { > CDTEXT_LANGUAGE_ASSAMESE = 0x7C, > CDTEXT_LANGUAGE_ARMENIAN = 0x7D, > CDTEXT_LANGUAGE_ARABIC = 0x7E, > - CDTEXT_LANGUAGE_AMHARIC = 0x7F > + CDTEXT_LANGUAGE_AMHARIC = 0x7F, > + > + /* libcdio-internal pseudo codes: */ > + > + /* Language code was not one of the above */ > + CDTEXT_LANGUAGE_INVALID = 0x100, > + > + /* A block which is characterized by this language code must be ignored > */ > + CDTEXT_LANGUAGE_BLOCK_UNUSED = 0x101 > + > } cdtext_lang_t; > > /*! > @@ -300,15 +309,59 @@ track_t cdtext_get_last_track(const cdte > */ > bool cdtext_select_language(cdtext_t *p_cdtext, cdtext_lang_t language); > > -/* > +/*! > + > + *** Deprecated. Use cdtext_list_languages_v2() *** > + > Returns a list of available languages or NULL. > > + WARNING: The indice in the returned array _ do not _ match the indexing > + as expected by cdtext_set_language_index(). > + Use cdtext_select_language() with the values of array elements. > + > Internally the list is stored in a static array. > > @param p_cdtext the CD-TEXT object > + @return NULL if p_cdtext is NULL. > + Else an array of 8 cdtext_lang_t elements: > + CDTEXT_LANGUAGE_UNKNOWN not only marks language code 0x00 > + but also invalid language codes and invalid language blocks. > */ > cdtext_lang_t *cdtext_list_languages (const cdtext_t *p_cdtext); > > +/*! > + Returns an array of available languages or NULL. > + The index of an array element may be used to select the corresponding > + language block by call cdtext_set_language_index(). > + > + The return value is a pointer into the memory range of *p_cdtext. > + Do not use it after having freed that memory range. > + > + @param p_cdtext the CD-TEXT object > + @return NULL if p_cdtext is NULL. > + Else an array of 8 cdtext_lang_t elements: > + CDTEXT_LANGUAGE_UNKNOWN to CDTEXT_LANGUAGE_AMHARIC mark valid > + language blocks with valid language codes. > + CDTEXT_LANGUAGE_INVALID marks valid language blocks with invalid > + language codes. > + CDTEXT_LANGUAGE_BLOCK_UNUSED marks invalid language blocks > which do > + not exist on CD or could not be read for some reason. > +*/ > +cdtext_lang_t *cdtext_list_languages_v2(cdtext_t *p_cdtext); > + > +/*! > + Select the given language by block index. See > cdtext_list_languages_v2(). > + If the index is bad, or no language block with that index was read: > + select the default language at index 0 and return false. > + > + @param p_cdtext the CD-TEXT object > + @param idx the desired index: 0 to 7. > + > + @return true on success, false if no language block is associated to idx > +*/ > +bool > +cdtext_set_language_index(cdtext_t *p_cdtext, int idx); > + > /*! > Sets the given field at the given track to the given value. > > --- lib/driver/cdtext_private.h.orig 2018-05-18 22:27:56.125019530 > +0200 > +++ lib/driver/cdtext_private.h 2018-05-20 12:52:19.461541589 +0200 > @@ -126,6 +126,7 @@ struct cdtext_block_s { > */ > struct cdtext_s { > struct cdtext_block_s block[CDTEXT_NUM_BLOCKS_MAX]; /**< CD-TEXT for > block 0..7 */ > + cdtext_lang_t languages[CDTEXT_NUM_BLOCKS_MAX]; /**< return value > of cdtext_list_languages_v2() */ > uint8_t block_i; /**< index of > active block */ > }; > > --- lib/driver/cdtext.c.orig 2018-05-18 22:27:56.125019530 +0200 > +++ lib/driver/cdtext.c 2018-05-20 16:51:20.797595740 +0200 > @@ -314,7 +314,7 @@ cdtext_lang_t > cdtext_get_language(const cdtext_t *p_cdtext) > { > if (NULL == p_cdtext) > - return CDTEXT_LANGUAGE_UNKNOWN; > + return CDTEXT_LANGUAGE_BLOCK_UNUSED; > return p_cdtext->block[p_cdtext->block_i].language_code; > } > > @@ -344,12 +344,22 @@ cdtext_get_last_track(const cdtext_t *p_ > return p_cdtext->block[p_cdtext->block_i].last_track; > } > > -/* > +/*! > + *** Deprecated. Use cdtext_list_languages_v2() *** > + > Returns a list of available languages or NULL. > > + WARNING: The indice in the returned array _ do not _ match the indexing > + as expected by cdtext_set_language_index(). > + Use cdtext_select_language with the values of array elements. > + > Internally the list is stored in a static array. > > @param p_cdtext the CD-TEXT object > + @return NULL if p_cdtext is NULL. > + Else an array of 8 cdtext_lang_t elements: > + CDTEXT_LANGUAGE_UNKNOWN not only marks language code 0x00 > + but also invalid language codes and invalid language blocks. > */ > cdtext_lang_t > *cdtext_list_languages(const cdtext_t *p_cdtext) > @@ -363,7 +373,9 @@ cdtext_lang_t > for (i=0; i<CDTEXT_NUM_BLOCKS_MAX; i++) > { > avail[i] = CDTEXT_LANGUAGE_UNKNOWN; > - if (CDTEXT_LANGUAGE_UNKNOWN != p_cdtext->block[i].language_code) > + if (CDTEXT_LANGUAGE_UNKNOWN != p_cdtext->block[i].language_code && > + CDTEXT_LANGUAGE_INVALID != p_cdtext->block[i].language_code && > + CDTEXT_LANGUAGE_BLOCK_UNUSED != p_cdtext->block[i].language_code) > avail[j++] = p_cdtext->block[i].language_code; > } > > @@ -371,6 +383,62 @@ cdtext_lang_t > } > > /*! > + Returns an array of available languages or NULL. > + The index of an array element may be used to select the corresponding > + language block by call cdtext_set_language_index(). > + > + The return value is a pointer into the memory range of *p_cdtext. > + Do not use it after having freed that memory range. > + > + @param p_cdtext the CD-TEXT object > + @return NULL if p_cdtext is NULL. > + Else an array of 8 cdtext_lang_t elements: > + CDTEXT_LANGUAGE_UNKNOWN to CDTEXT_LANGUAGE_AMHARIC mark valid > + language blocks with valid language codes. > + CDTEXT_LANGUAGE_INVALID marks valid language blocks with invalid > + language codes. > + CDTEXT_LANGUAGE_BLOCK_UNUSED marks invalid language blocks > which do > + not exist on CD or could not be read for some reason. > +*/ > +cdtext_lang_t > +*cdtext_list_languages_v2(cdtext_t *p_cdtext) > +{ > + int i; > + > + if (NULL == p_cdtext) > + return NULL; > + for (i = 0; i < CDTEXT_NUM_BLOCKS_MAX; i++) > + { > + p_cdtext->languages[i] = p_cdtext->block[i].language_code; > + } > + return p_cdtext->languages; > +} > + > +/*! > + Select the given language by block index. See > cdtext_list_languages_v2(). > + If the index is bad, or no language block with that index was read: > + select the default language at index 0 and return false. > + > + @param p_cdtext the CD-TEXT object > + @param idx the desired index: 0 to 7. > + > + @return true on success, false if no language block is associated to idx > +*/ > +bool > +cdtext_set_language_index(cdtext_t *p_cdtext, int idx) > +{ > + if (NULL == p_cdtext) > + return false; > + p_cdtext->block_i = 0; > + if (idx < 0 || idx > 7) > + return false; > + if (p_cdtext->block[idx].language_code == CDTEXT_LANGUAGE_BLOCK_UNUSED) > + return false; > + p_cdtext->block_i = idx; > + return true; > +} > + > +/*! > Try to select the given language. > Select default language if specified is not available or invalid and > return false. > @@ -386,7 +454,7 @@ cdtext_select_language(cdtext_t *p_cdtex > if(NULL == p_cdtext) > return false; > > - if (CDTEXT_LANGUAGE_UNKNOWN != language) > + if (CDTEXT_LANGUAGE_BLOCK_UNUSED != language) > { > int i; > for (i=0; i<CDTEXT_NUM_BLOCKS_MAX; i++) { > @@ -395,9 +463,8 @@ cdtext_select_language(cdtext_t *p_cdtex > return true; > } > } > - } else { > - p_cdtext->block_i = 0; > } > + p_cdtext->block_i = 0; > return false; > } > > @@ -425,7 +492,7 @@ cdtext_t > } > } > p_cdtext->block[i].genre_code = CDTEXT_GENRE_UNUSED; > - p_cdtext->block[i].language_code = CDTEXT_LANGUAGE_UNKNOWN; > + p_cdtext->block[i].language_code = CDTEXT_LANGUAGE_BLOCK_UNUSED; > } > > p_cdtext->block_i = 0; > @@ -459,9 +526,13 @@ cdtext_is_field (const char *key) > > Internal function. > > + >>> Seems to have no caller. > + >>> Actually "supported" should rather be "listed in specs". > + >>> ??? Shall we throw it out as long as we still can ? > + > @param lang language to test > > - @return CDTEXT_LANGUAGE_UNKNOWN if language is not supported > + @return CDTEXT_LANGUAGE_INVALID if language is not supported > */ > cdtext_lang_t > cdtext_is_language(const char *lang) > @@ -472,7 +543,7 @@ cdtext_is_language(const char *lang) > if (0 == strcmp(cdtext_language[i], lang)) { > return i; > } > - return CDTEXT_LANGUAGE_UNKNOWN; > + return CDTEXT_LANGUAGE_INVALID; > } > > /*! > @@ -626,6 +697,8 @@ cdtext_data_init(cdtext_t *p_cdtext, uin > /* set Language */ > if(blocksize.langcode[i_block] <= 0x7f) > p_cdtext->block[i_block].language_code = > blocksize.langcode[i_block]; > + else > + p_cdtext->block[i_block].language_code = > CDTEXT_LANGUAGE_INVALID; > > /* determine encoding */ > switch (blocksize.charcode){ > --- include/cdio++/cdtext.hpp.orig 2018-05-18 22:27:56.121019530 +0200 > +++ include/cdio++/cdtext.hpp 2018-05-20 15:57:02.197583436 +0200 > @@ -90,13 +90,33 @@ bool selectLanguage(cdtext_lang_t lang) > } > > /*! > - returns a list of available languages > + selects a language by index rather than language code > +*/ > +bool setLanguageIndex(int idx) > +{ > + return cdtext_set_language_index(p_cdtext, idx); > +} > + > +/*! > + > + *** Deprecated. Use listLanguagesV2(), see cdio/cdtext.h *** > + > + returns a list of available languages (which has various problems) > */ > cdtext_lang_t *listLanguages() > { > return cdtext_list_languages(p_cdtext); > } > > +/*! > + returns a pointer to an array with 8 elements which indicate available > + languages > +*/ > +cdtext_lang_t *listLanguagesV2() > +{ > + return cdtext_list_languages_v2(p_cdtext); > +} > + > > /* > * Local variables: > --- src/cd-info.c.orig 2018-05-18 22:27:56.129019530 +0200 > +++ src/cd-info.c 2018-05-20 16:28:49.625590638 +0200 > @@ -450,10 +450,11 @@ print_cdtext_info(CdIo_t *p_cdio, track_ > return; > } > > - languages = cdtext_list_languages(p_cdtext); > + languages = cdtext_list_languages_v2(p_cdtext); > + /* The API promises that non-NULL p_cdtext yields non-NULL languages */ > for(i=0; i<8; i++) > - if ( CDTEXT_LANGUAGE_UNKNOWN != languages[i] > - && cdtext_select_language(p_cdtext, languages[i])) > + if ( CDTEXT_LANGUAGE_BLOCK_UNUSED != languages[i] > + && cdtext_set_language_index(p_cdtext, i)) > { > printf("\nLanguage %d '%s':\n", i, cdtext_lang2str(languages[i])); > > --- example/cdtext.c.orig 2018-05-18 22:27:56.121019530 +0200 > +++ example/cdtext.c 2018-05-20 16:24:17.657589611 +0200 > @@ -69,9 +69,10 @@ print_disc_info(CdIo_t *p_cdio) { > > printf("-- CD-Text available in: "); > > - languages = cdtext_list_languages(cdtext); > + languages = cdtext_list_languages_v2(cdtext); > + /* The API promises that non-NULL p_cdtext yields non-NULL > languages */ > for(i=0; i<8; i++) > - if ( CDTEXT_LANGUAGE_UNKNOWN != languages[i]) > + if ( CDTEXT_LANGUAGE_BLOCK_UNUSED != languages[i]) > printf("%s ", cdtext_lang2str(languages[i])); > printf("\n"); > } > --- example/cdtext-raw.c.orig 2018-05-18 22:27:56.121019530 +0200 > +++ example/cdtext-raw.c 2018-05-20 16:22:48.921589276 +0200 > @@ -62,10 +62,12 @@ print_cdtext_info(cdtext_t *p_cdtext) { > > int i, j; > > - languages = cdtext_list_languages(p_cdtext); > + languages = cdtext_list_languages_v2(p_cdtext); > + if (NULL == languages) > + return; > for(i=0; i<8; i++) > - if ( CDTEXT_LANGUAGE_UNKNOWN != languages[i] > - && cdtext_select_language(p_cdtext, languages[i])) > + if ( CDTEXT_LANGUAGE_BLOCK_UNUSED != languages[i] > + && cdtext_set_language_index(p_cdtext, i)) > { > printf("\nLanguage %d '%s':\n", i, cdtext_lang2str(languages[i])); > > --- example/C++/OO/cdtext.cpp.orig 2018-05-18 22:27:56.117019530 +0200 > +++ example/C++/OO/cdtext.cpp 2018-05-20 16:25:03.357589784 +0200 > @@ -69,9 +69,10 @@ print_disc_info(CdioDevice *device) > > printf("CD-Text available in: "); > > - languages = cdtext->listLanguages(); > + languages = cdtext->listLanguagesV2(); > + /* The API promises that non-NULL p_cdtext yields non-NULL languages > */ > for(i=0; i<8; i++) > - if ( CDTEXT_LANGUAGE_UNKNOWN != languages[i]) > + if ( CDTEXT_LANGUAGE_BLOCK_UNUSED != languages[i]) > printf("%s ", cdtext->lang2str(languages[i])); > printf("\n"); > } > > ========================================================================== > > Have a nice day :) > > Thomas > > >
