[Libcdio-devel] [PATCH v2] cdtext: Count empty fields as tracks

[John Ernberg]

The following CD-TEXT would fail when parsing due to an off-by-one in
the songwriter data type because the album-level field is empty:

    80 0 0 0 41 20 4d 65 6d 6f 69 72 20 6f 66 20 5f 23
    80 0 1 c 46 72 65 65 20 57 69 6c 6c 0 47 6f 5b 8d
    80 1 2 2 6e 65 20 42 75 74 20 4e 6f 74 20 46 b0 df
    80 1 3 e 6f 72 67 6f 74 74 65 6e 0 49 6e 73 9 18
    80 2 4 3 61 6e 69 74 79 20 41 20 4d 6f 6e 69 49 27
    80 2 5 f 6b 65 72 20 4f 66 20 4d 65 0 54 6f 13 d2
    80 3 6 2 20 50 65 72 73 69 73 74 20 6f 72 20 3b 87
    80 3 7 e 41 64 68 65 72 65 0 41 6e 20 45 6c 63 87
    80 4 8 5 65 67 79 20 66 6f 72 20 61 20 4d 61 bc e6
    80 4 9 f 6e 20 41 6c 69 76 65 0 50 73 79 63 93 89
    80 5 a 4 68 6f 74 69 63 6c 79 73 6d 0 41 6e d6 a
    80 6 b 2 20 49 6e 74 72 61 6d 75 72 61 6c 20 aa b6
    80 6 c e 4d 61 64 6e 65 73 73 0 51 75 65 73 b8 0
    80 7 d 4 74 69 6f 6e 73 20 6f 66 20 61 20 48 60 11
    80 7 e f 6f 6c 69 73 74 69 63 20 44 69 76 69 bb b8
    80 7 f f 6e 65 0 57 69 74 68 20 56 69 72 74 60 f5
    80 8 10 9 75 65 20 49 20 41 6d 20 46 72 65 65 21 6
    80 8 11 f 0 42 61 74 74 6c 65 73 20 41 72 65 a5 b9
    80 9 12 b 20 57 6f 6e 20 57 69 74 68 69 6e 0 b0 f3
    80 a 13 0 41 20 4d 65 6d 69 6f 72 20 6f 66 20 52 2f
    80 a 14 c 46 72 65 65 20 57 69 6c 6c 0 0 0 78 30
    81 0 15 0 4b 72 6f 73 69 73 0 4b 72 6f 73 69 91 f1
    81 1 16 5 73 0 9 0 9 0 9 0 9 0 9 0 80 2d
    81 7 17 0 9 0 9 0 9 0 9 0 0 0 0 0 bd f4
    82 0 18 0 0 4b 72 6f 73 69 73 0 9 0 9 0 47 9c
            ^^^
    82 4 19 0 9 0 9 0 9 0 9 0 9 0 9 0 1f 7f
    82 a 1a 0 9 0 0 0 0 0 0 0 0 0 0 0 1e f8
    83 0 1b 0 0 4b 72 6f 73 69 73 0 9 0 9 0 62 1b
    83 4 1c 0 9 0 9 0 9 0 9 0 9 0 9 0 31 9f
    83 a 1d 0 9 0 0 0 0 0 0 0 0 0 0 0 c6 da
    84 0 1e 0 0 4b 72 6f 73 69 73 0 9 0 9 0 8c 40
    84 4 1f 0 9 0 9 0 9 0 9 0 9 0 9 0 d4 a3
    84 a 20 0 9 0 0 0 0 0 0 0 0 0 0 0 98 d2
    8e 0 21 0 0 51 4d 37 32 38 31 39 30 35 32 30 2c cb
    8e 1 22 b 34 0 51 4d 37 32 38 31 39 30 35 32 31 28
    8e 2 23 a 30 35 0 51 4d 37 32 38 31 39 30 35 1a 5c
    8e 3 24 9 32 30 36 0 51 4d 37 32 38 31 39 30 53 af
    8e 4 25 8 35 32 30 37 0 51 4d 37 32 38 31 39 6c 0
    8e 5 26 7 30 35 32 30 38 0 51 4d 37 32 38 31 c6 9b
    8e 6 27 6 39 30 35 32 30 39 0 51 4d 37 32 38 39 e0
    8e 7 28 5 31 39 30 35 32 31 30 0 51 4d 37 32 48 67
    8e 8 29 4 38 31 39 30 35 32 31 31 0 51 4d 37 bf 19
    8e 9 2a 3 32 38 31 39 30 35 32 31 32 0 51 4d 17 fa
    8e a 2b 2 37 32 38 31 39 30 35 32 31 33 0 0 6f 43
    8f 0 2c 0 0 1 a 0 15 3 3 3 3 0 0 0 5f 7b
    8f 1 2d 0 0 0 0 0 0 0 b 3 2e 0 0 0 68 6c
    8f 2 2e 0 0 0 0 0 9 0 0 0 0 0 0 0 e7 87

With the album-level field empty cur_track was never incremented,
causing an off-by-one. Since the CD-TEXT additionally makes use of the
Tab Indicator the entire CD-TEXT is thrown out as invalid due to a tab
found in track 1 (that should be track 2).

Fix this by always incrementing cur_track on a termination, even if the
buffer contains nothing while still avoiding adding any blank fields to
the parsed CD-TEXT structure.

Include the above CD-TEXT in the test data.

---

v2:
 - Avoid regression with previous CD-TEXT tests by not adding the
   buffer if it's empty. (Rocky)
 - Include the CD-TEXT in the test suite.
 - Fix a confusion regarding the field IDs in the commit message.

According to the CD-TEXT specification says that if a field of type
80 to 85 or 8e is present for one track, it must be present for all of
them.

The regression in the tests was that MESSAGE and GENRE got added where
they weren't before.

In terms of MESSAGE that was probably more correct, but starting to add
them in such cases is maybe an API break. If you'd prefer that there
still needs to be some filtering in GENRE, I wonder if in that case it
should be limited to track 0 since it's not part of the list of fields
that need to be present for all tracks if used.

---
 lib/driver/cdtext.c         |  69 +++++++++++++++++----------------
 test/cdtext-krosis.right    |  75 ++++++++++++++++++++++++++++++++++++
 test/check_cdtext.sh        |   6 +++
 test/data/cdtext-krosis.cdt | Bin 0 -> 846 bytes
 4 files changed, 117 insertions(+), 33 deletions(-)
 create mode 100644 test/cdtext-krosis.right
 create mode 100644 test/data/cdtext-krosis.cdt

diff --git a/lib/driver/cdtext.c b/lib/driver/cdtext.c
index a80d5974..d92c9434 100644
--- a/lib/driver/cdtext.c
+++ b/lib/driver/cdtext.c
@@ -799,7 +799,7 @@ cdtext_data_init(cdtext_t *p_cdtext, uint8_t
*wdata, size_t i_data)
             buffer[i_buf++] = pack.text[j];
             if(pack.db_chars)
               buffer[i_buf++] = pack.text[j+1];
-          } else if(i_buf > 0) {
+          } else {
             /* if end of string */

             /* check if the buffer contains only the Tab Indicator */
@@ -815,38 +815,41 @@ cdtext_data_init(cdtext_t *p_cdtext, uint8_t
*wdata, size_t i_data)
                 buffer[i_buf++] = 0;
             }

-            switch (pack.type) {
-              case CDTEXT_PACK_TITLE:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_TITLE, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_PERFORMER:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_PERFORMER, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_SONGWRITER:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_SONGWRITER, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_COMPOSER:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_COMPOSER, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_ARRANGER:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_ARRANGER, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_MESSAGE:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_MESSAGE, buffer,
cur_track, charset);
-                break;
-              case CDTEXT_PACK_DISCID:
-                if (cur_track == 0)
-                  cdtext_set(p_cdtext, CDTEXT_FIELD_DISCID, buffer,
cur_track, NULL);
-                break;
-              case CDTEXT_PACK_GENRE:
-                cdtext_set(p_cdtext, CDTEXT_FIELD_GENRE, buffer,
cur_track, "ASCII");
-                break;
-              case CDTEXT_PACK_UPC:
-                if (cur_track == 0)
-                  cdtext_set(p_cdtext, CDTEXT_FIELD_UPC_EAN, buffer,
cur_track, "ASCII");
-                else
-                  cdtext_set(p_cdtext, CDTEXT_FIELD_ISRC, buffer,
cur_track, "ISO-8859-1");
-                break;
+            if ( ! CDTEXT_COMPARE_CHAR(buffer, '\0', pack.db_chars)) {
+              /* implies cur_track is in valid range */
+              switch (pack.type) {
+                case CDTEXT_PACK_TITLE:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_TITLE, buffer,
cur_track, charset);
+                  break;
+                case CDTEXT_PACK_PERFORMER:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_PERFORMER,
buffer, cur_track, charset);
+                  break;
+                case CDTEXT_PACK_SONGWRITER:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_SONGWRITER,
buffer, cur_track, charset);
+                  break;
+                case CDTEXT_PACK_COMPOSER:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_COMPOSER, buffer,
cur_track, charset);
+                  break;
+                case CDTEXT_PACK_ARRANGER:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_ARRANGER, buffer,
cur_track, charset);
+                  break;
+                case CDTEXT_PACK_MESSAGE:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_MESSAGE, buffer,
cur_track, charset);
+                  break;
+                case CDTEXT_PACK_DISCID:
+                  if (cur_track == 0)
+                    cdtext_set(p_cdtext, CDTEXT_FIELD_DISCID, buffer,
cur_track, NULL);
+                  break;
+                case CDTEXT_PACK_GENRE:
+                  cdtext_set(p_cdtext, CDTEXT_FIELD_GENRE, buffer,
cur_track, "ASCII");
+                  break;
+                case CDTEXT_PACK_UPC:
+                  if (cur_track == 0)
+                    cdtext_set(p_cdtext, CDTEXT_FIELD_UPC_EAN,
buffer, cur_track, "ASCII");
+                  else
+                    cdtext_set(p_cdtext, CDTEXT_FIELD_ISRC, buffer,
cur_track, "ISO-8859-1");
+                  break;
+              }
             }
             i_buf = 0;
             ++cur_track;
diff --git a/test/cdtext-krosis.right b/test/cdtext-krosis.right
new file mode 100644
index 00000000..3e15ea6b
--- /dev/null
+++ b/test/cdtext-krosis.right
@@ -0,0 +1,75 @@
+
+Language 0 'English':
+CD-TEXT for Disc:
+       TITLE: A Memoir of Free Will
+       PERFORMER: Krosis
+CD-TEXT for Track  1:
+       TITLE: Gone But Not Forgotten
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905204
+CD-TEXT for Track  2:
+       TITLE: Insanity A Moniker Of Me
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905205
+CD-TEXT for Track  3:
+       TITLE: To Persist or Adhere
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905206
+CD-TEXT for Track  4:
+       TITLE: An Elegy for a Man Alive
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905207
+CD-TEXT for Track  5:
+       TITLE: Psychoticlysm
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905208
+CD-TEXT for Track  6:
+       TITLE: An Intramural Madness
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905209
+CD-TEXT for Track  7:
+       TITLE: Questions of a Holistic Divine
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905210
+CD-TEXT for Track  8:
+       TITLE: With Virtue I Am Free
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905211
+CD-TEXT for Track  9:
+       TITLE: Battles Are Won Within
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905212
+CD-TEXT for Track 10:
+       TITLE: A Memior of Free Will
+       PERFORMER: Krosis
+       SONGWRITER: Krosis
+       COMPOSER: Krosis
+       ARRANGER: Krosis
+       ISRC: QM7281905213
diff --git a/test/check_cdtext.sh b/test/check_cdtext.sh
index dfbfc9a5..7ae0634d 100755
--- a/test/check_cdtext.sh
+++ b/test/check_cdtext.sh
@@ -68,4 +68,10 @@ test_cdtext_raw "$opts" ${srcdir}/${fname}.dump
${srcdir}/${fname}.right
 RC=$?
 check_result $RC "CD-Text libburnia roundtrip" "${CDTEXT_RAW} $opts"

+fname=cdtext-krosis
+opts="${srcdir}/data/${fname}.cdt"
+test_cdtext_raw "$opts" ${srcdir}/${fname}.dump ${srcdir}/${fname}.right
+RC=$?
+check_result $RC "CD-Text Krosis blanks + tab indicator" "${CDTEXT_RAW} $opts"
+
 exit 0
diff --git a/test/data/cdtext-krosis.cdt b/test/data/cdtext-krosis.cdt
new file mode 100644
index 
0000000000000000000000000000000000000000..eeb99672ce0a08bbb8b370fddbcf20b1be67b90d
GIT binary patch
literal 846
zcmY+BO-vI(7>2+7X}7z@Rz&1aVGtEm&~6ohc(JHJW2&G=4w}GP20C%Olk853jRyy4
zLQHgt@nWKh2jj(yoD4>dH!j|Y@!&;IdNyieJUL6D(3!(G^X$Ce&$}D|QYeG-l(>ja
zqlpd<Qq7QyF&Zbf4a4;M11`z3gVA7tp^KEEq2;e!QexDdp^RY%(vGJ$a#>X@hp-|N
z*{DO(om>vZZ($c*nilEdDy>`NN?1mt*!76V5OvY%MXqSE)Km_;7*Y;8Z)X>|8VTW<
zBAOQ8EtIj|ednqkcTmbEvlvFbV)oHJ9x~J%Wh84Cy}S^9Hh4HHqqM_ZD_?Le8@+tZ
z!!@ytHRWK>gRuhkR)A|eR7R-d1!G!hWU`iPu{gCwGbD@7ky-L~g=_J+gJGPo9J)qa
zHu;A~cIpL;(kPWjfh+A{9;uDPpv9PtJ(P0si#J@~6-DFJ5stAOabWouH|ol+5;2A-
zclkIsc5ibP;C5mT>fwS*g&F1W@aLSgC**-%*}P{?YpC4BkAnBV=b;fcWx>ngr}Ju4
z<xN1^?#>&{B5tkLzYEY(rMlU&pzf_wna>uCeO0Q}FC}QLQr$mOQroIjpPrWtRHb^n
z=0iKcSf(%8Zw@4SlZoz+zO+AjavL(ce7VC&^lqW%8@_TtPbN-P(3MxddN3m7eVfPz
zKXgb7GWKtv)hB+qGrV48po|8-`C3<Ky{?&9M<X+S<gi+9#WVx-p~=^eC>!0GNr4*v
u`NmOsvtjcD06n_|-2jpS^@>=+%85HmQqP8^QRSF8<?JOnh)|jT$KrpTmdKp|

literal 0
HcmV?d00001

-- 
2.45.1