Re: [Libcdio-devel] CD-Text API changes

[Rocky Bernstein]

On Sat, Feb 25, 2012 at 5:57 PM, Leon Merten Lohse <address@hidden>wrote:

> Hi,
>
> I would like to present you the new CD-Text API for further discussion.
> With version 0.82 there was:
> cdtext.h:
>  const char *cdtext_field2str (cdtext_field_t i);
>  void cdtext_init (cdtext_t *cdtext);
>  void cdtext_destroy (cdtext_t *cdtext);
>  char *cdtext_get (cdtext_field_t key, const cdtext_t *cdtext);
>  const char *cdtext_get_const (cdtext_field_t key, const cdtext_t *cdtext);
>  cdtext_field_t cdtext_is_keyword (const char *key);
>  void cdtext_set (cdtext_field_t key, const char *value, cdtext_t *cdtext);
> track.h
>  cdtext_t *cdio_get_cdtext (CdIo_t *p_cdio, track_t i_track);
>
> Only the first block/language would be parsed, there was a seperate
> cdtext_t struct for each track.
>
> The new API supports all 8 blocks and language selection. To keep it
> simple everything is now stored in one central cdtext_t struct. This
> also makes sense for reasons other than simplicity of code. If an audio
> disc has CD-Text, it must be there for all tracks and it is stored in
> subchannels in the lead-in area. Libcdio reads and parses the
> information at once.
>
> Now there is (some of it not yet commited):
> cdtext.h:
>  const char *cdtext_genre2str (cdtext_genre_t i); // new
>  const char *cdtext_lang2str (cdtext_lang_t i); // new
>  const char *cdtext_field2str (cdtext_field_t i); // unchanged
>  void cdtext_init (cdtext_t *p_cdtext); // prototype unchanged
>  /* parses binary cd-text information into a cdtext_t struct */
>  int cdtext_data_init(cdtext_t *p_cdtext, uint8_t *wdata, size_t length);
> // new
>  void cdtext_destroy (cdtext_t *p_cdtext); // unchanged
>  /* new track parameter, changed order */
>  char *cdtext_get (const cdtext_t *p_cdtext, cdtext_field_t key, track_t
> track);
>  /* new track parameter, changed order */
>  const char *cdtext_get_const (const cdtext_t *p_cdtext, cdtext_field_t
> key, track_t track);
>  cdtext_lang_t cdtext_get_language (const cdtext_t *p_cdtext); // new, 1)
>  bool cdtext_select_language(cdtext_t *p_cdtext, const char *lang); //
> new, 2)
>  cdtext_lang_t *cdtext_languages_available (const cdtext_t *p_cdtext); //
> new, 3)
> disc.h
>  /* returns cd-text binary as it is */
>  uint8_t * cdio_get_cdtext_raw (CdIo_t *p_cdio); // new
>  /* calls cdtext_data_init on the return value of cdio_get_cdtext_raw */
>  cdtext_t *cdio_get_cdtext (CdIo_t *p_cdio); // unchanged
>
> New:
> 1), 2) and 3) allow to navigate the available languages.
> 1) returns the active language, 2) selects another languages, 3) prints
> the available languages.
>
> genre2str and lang2str are needed for numerical fields that were not
> read before.
> cdio_get_cdtext_raw's returned data can be directly saved into a file
> for further use with libburnia, cdrecord or cdrdao. It is the format
> the (now supported) CDTEXTFILE tag in a cue sheet expects.
> cdtext_data_init can be used for images that provide binary CD-Text.
>
> Changed:
> The cdtext_t struct now holds the CD-Text of the whole disc. So most of
> the functions need a track argument now.
> I moved the cdtext_t parameter to always be the first in every function.
>
> Deleted:
> I chose to move cdtext_is_keyword from the public header into the
> private one. I see no use of it other than image parsing.
> ( Do you think cdtext_set can go there, too? Why would anybody need it?)
>
>
> I hope I was able to explain the motives behind the changes a little
> better.
> If you can think of anything that should be (un)done before the new
> release, please comment.
>
> Currently I am still updating the documentation and example files but I
> hope I can send in the patch with the language selection in the next few
> days.
>
> Regards
> Leon
>
>
>
Thank you very much for this write up. I really appreciate it.

Some thoughts with regards to the questions I asked and the concerns
Nicolas raised. Your current corrected code reads all blocks whereas the
old code read only the first block. I imagine there are a number of cases
where the first block was good enough (or at least better than nothing). So
here, from a programming standpoint there is no change. And there are
features such as multiple language support that the old code just couldn't
handle. The main place that is incompatibility is in cdtext_get(). Although
I totally understand the reason for this change, if another name were used,
that might reduce incompatibility and confusion. Right? And the old
(incomplete) function could be simulated by the new one by supplying a
track parameter. Correct?

As far as places where the that used the old libcdio cdtext code is used
(outside of libcdio such as cd-info), I wrote some plugins for xine, and
vlc. The xine plugin I don't think was ever mainstream. Googling around for
CD-Text on vlc seems to show this was broken, so perhaps I will investigate
and update the code. There is also a libcdio-based plugin for gstreamer.
These are largely the uses I know of. If there are others, of course I'd
like to know about.

Nicolas: your thoughts here with respect to Leon's changes?

As for integration into the master code, I agreed we should try to not make
it harder on poor Pete Batard to get his existing changes in the pbatard
branch into master.

As I promised, I have working on this and it has been going slowly. When I
apply patches I want to be able to demonstrate improvement such as through
test cases that were failing no longer failing and demonstrate improvement
through more tests. As Pete previously mentioned, MinGW is already broken
on the tests. So I've been focusing a little on isolating those cases or
reducing the number of failures. Here it has meant applying those
pbatard-branch patches which specifically are for MinGW which might not
have been in the initial list.

At any rate, one safe thing to do is to check this into a branch and merge
it in after dealing with the bpatard branch. Alternatively if Pete is up
for it he can review the patches to determine breakage for his changes and
perhaps this can slip in.
As with incompatibility which Nicolas Boullis is (rightfully) concerned
with, it is largely up to you all.

And lastly something I will repeat something I have written before, so feel
free to skip the rest.

Sadly, the way progress has been made in libcdio is to put out flaky code
whether it is for retrieving CD-Text, issuing and handling MMC commands, or
OS-specific and image driver support , among many other places. I view this
unfortunately as a necessary evil, because otherwise nothing would have
ever gotten done. In too many cases, the rules by which one should code by
to were not previously publicly well documented. So I've been constantly in
the position of putting something out there in the hope that someone else
would find it good enough to graciously build upon. This increases the
likelihood incompatible API's and changes as we ultimately understand what
the reality is. The good news, I guess, is that as far as libcdio goes, I
-- the biggest offender of the need for incompatibilities -- don't plan on
extending libcdio in any new areas.