Re: [Libcdio-devel] RFC on two CD-TEXT patches by Serge Pouliquen

[Thomas Schmitt]

Hi,

here is a first sketch of a patch for
  https://savannah.gnu.org/bugs/?53928

This changeset is not even compiled yet. It is posted here to get the ok
of Rocky Bernstein about the overall plan and to get reviewed in respect
to its impact on the existing libcdio code.

Missing yet are at least:

- Registration of new public symbol cdtext_list_languages_v2 in
  lib/driver/libcdio.sym

- C++ API version of cdtext_list_languages_v2() for use in
  example/C++/OO/cdtext.cpp

- Using cdtext_list_languages_v2() in the three examples
  example/cdtext.c , example/cdtext-raw.c , example/C++/OO/cdtext.cpp

I added my "Signed-off-by:" to certify https://developercertificate.org/
Of course i do not claim own copyright for this code but rather acknowledge
the copyright of Rocky Bernstein.

------------------------------------------------------------------------
Changelog text:

Strip official CD-TEXT language code CDTEXT_LANGUAGE_UNKNOWN from its
libcdio specific meanings "invalid language code" and "non existent block"
by introducing pseudo language codes CDTEXT_LANGUAGE_INVALID and
CDTEXT_LANGUAGE_BLOCK_UNUSED.
API call cdtext_list_languages() keeps the old indistinction and gets
deprecated. A new call cdtext_list_languages_v2() distinguishes the
three codes and clarifies that in its result only array members with
CDTEXT_LANGUAGE_BLOCK_UNUSED shall be ignored. 

 Signed-off-by: Thomas Schmitt <address@hidden>

========================================================================

--- ./lib/driver/cdtext.c.orig  2018-05-18 22:27:56.125019530 +0200
+++ ./lib/driver/cdtext.c       2018-05-19 12:34:38.897211353 +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,17 @@ cdtext_get_last_track(const cdtext_t *p_
   return p_cdtext->block[p_cdtext->block_i].last_track;
 }
 
-/*
+/*!
+  *** Deprecated in API *** 
+
   Returns a list of available languages or NULL.
 
   Internally the list is stored in a static array.
 
   @param p_cdtext the CD-TEXT object
+  @return an array of 8 cdtext_lang_t items:
+          CDTEXT_LANGUAGE_UNKNOWN not only marks language code 0x00
+          but also invalid language codes and invalid text blocks.
 */
 cdtext_lang_t
 *cdtext_list_languages(const cdtext_t *p_cdtext)
@@ -363,7 +368,42 @@ 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;
+  }
+
+  return avail;
+}
+
+/*!
+  Returns a list of available languages or NULL.
+
+  Internally the list is stored in a static array.
+
+  @param p_cdtext the CD-TEXT object
+  @return an array of 8 cdtext_lang_t items:
+          CDTEXT_LANGUAGE_UNKNOWN to CDTEXT_LANGUAGE_AMHARIC mark valid
+          text blocks with valid language codes.
+          CDTEXT_LANGUAGE_INVALID marks valid text blocks with invalid
+          language codes.
+          CDTEXT_LANGUAGE_BLOCK_UNUSED marks invalid text blocks which do
+          not exist on CD or could not be read for some reason.
+*/
+cdtext_lang_t
+*cdtext_list_languages_v2(const cdtext_t *p_cdtext)
+{
+  static cdtext_lang_t avail[CDTEXT_NUM_BLOCKS_MAX];
+  int i, j=0;
+
+  if (NULL == p_cdtext)
+    return NULL;
+
+  for (i=0; i<CDTEXT_NUM_BLOCKS_MAX; i++)
+  {
+    avail[i] = CDTEXT_LANGUAGE_BLOCK_UNUSED;
+    if (CDTEXT_LANGUAGE_BLOCK_UNUSED != p_cdtext->block[i].language_code)
       avail[j++] = p_cdtext->block[i].language_code;
   }
 
@@ -386,7 +426,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 +435,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 +464,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 +498,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 +515,7 @@ cdtext_is_language(const char *lang)
     if (0 == strcmp(cdtext_language[i], lang)) {
       return i;
     }
-  return CDTEXT_LANGUAGE_UNKNOWN;
+  return CDTEXT_LANGUAGE_INVALID;
 }
 
 /*!
--- ./src/cd-info.c.orig        2018-05-18 22:27:56.129019530 +0200
+++ ./src/cd-info.c     2018-05-19 11:41:08.501199231 +0200
@@ -450,9 +450,9 @@ print_cdtext_info(CdIo_t *p_cdio, track_
     return;
   }
 
-  languages = cdtext_list_languages(p_cdtext);
+  languages = cdtext_list_languages_v2(p_cdtext);
   for(i=0; i<8; i++)
-    if ( CDTEXT_LANGUAGE_UNKNOWN != languages[i]
+    if ( CDTEXT_LANGUAGE_BLOCK_UNUSED != languages[i]
          && cdtext_select_language(p_cdtext, languages[i]))
     {
       printf("\nLanguage %d '%s':\n", i, cdtext_lang2str(languages[i]));
--- ./include/cdio/cdtext.h.orig        2018-05-18 22:27:56.121019530 +0200
+++ ./include/cdio/cdtext.h     2018-05-19 09:51:01.885174285 +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,37 @@ 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.
 
   Internally the list is stored in a static array.
 
   @param p_cdtext the CD-TEXT object
+  @return an array of 8 cdtext_lang_t items:
+          CDTEXT_LANGUAGE_UNKNOWN not only marks language code 0x00
+          but also invalid language codes and invalid text blocks.
 */
 cdtext_lang_t *cdtext_list_languages (const cdtext_t *p_cdtext);
 
+/*!
+  Returns a list of available languages or NULL.
+
+  Internally the list is stored in a static array.
+
+  @param p_cdtext the CD-TEXT object
+  @return an array of 8 cdtext_lang_t items:
+          CDTEXT_LANGUAGE_UNKNOWN to CDTEXT_LANGUAGE_AMHARIC mark valid
+          text blocks with valid language codes.
+          CDTEXT_LANGUAGE_INVALID marks valid text blocks with invalid
+          language codes.
+          CDTEXT_LANGUAGE_BLOCK_UNUSED marks invalid text blocks which do
+          not exist on CD or could not be read for some reason.
+*/
+cdtext_lang_t *cdtext_list_languages_v2(const cdtext_t *p_cdtext); 
+
 /*! 
   Sets the given field at the given track to the given value.

========================================================================


Have a nice day :)

Thomas