[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libcdio-devel] RFC on two CD-TEXT patches by Serge Pouliquen
|
From: |
Rocky Bernstein |
|
Subject: |
Re: [Libcdio-devel] RFC on two CD-TEXT patches by Serge Pouliquen |
|
Date: |
Sun, 20 May 2018 12:50:05 -0400 |
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 <address@hidden> 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 address@hidden:/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 <address@hidden>
>
> ==========================================================================
>
> --- 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
>
>
>