[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libcdio-devel] [PATCH v2] cdtext: Count empty fields as tracks
|
From: |
Rocky Bernstein |
|
Subject: |
Re: [Libcdio-devel] [PATCH v2] cdtext: Count empty fields as tracks |
|
Date: |
Thu, 5 Sep 2024 20:40:41 -0400 |
I have committed this patch to the branch count-empty-tracks-as-field-v2;
see
https://git.savannah.gnu.org/cgit/libcdio.git/log/?h=count-empty-tracks-as-field-v2
I invite review of this. If after a week or so there are no objections, I
will merge this into the master branch.
On Thu, Sep 5, 2024 at 10:13 AM John Ernberg <j@j-ernberg.se> wrote:
> 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
>