[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: |
Mon, 9 Sep 2024 15:23:39 -0400 |
Since there has not been any comment on this, I have just merged the code
into master. I hope sometime this year to come out with a long overdue
release of all the changes that have happened over the last 7 years.
Even though the code is merged, if there are problems we can address these
down the line. And discussion on the code is always open whenever or if
ever it happens.
On Thu, Sep 5, 2024 at 8:40 PM Rocky Bernstein <rocky@gnu.org> wrote:
> 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
>>
>