[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: |
Thomas Schmitt |
|
Subject: |
Re: [Libcdio-devel] RFC on two CD-TEXT patches by Serge Pouliquen |
|
Date: |
Sun, 20 May 2018 18:18:07 +0200 |
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