Re: [Libcdio-devel] Fedora bug report concerning CD-TEXT

[Thomas Schmitt]

Hi,

Rocky Bernstein wrote:
> Thomas: yes mmc_get_track_isrc_private should interpret this bit before
> returning and ISRC string. Please make it so.

I do not feel ready for a patch yet. Too many loose ends.

But i can make a list of problems and implementation proposals for
discussion:

- The demanded reply size with READ SUB-CHANNEL is larger than the amount
  of data which we actually want with MCN and ISRC and larger than MMC-1
  and MMC-4 promise to deliver.
  We want the 4 bytes of Sub-Q Channel Data Header (MMC-4, table 425),
  and the 5 header bytes from Sub-channel data (tables 431 and 432),
  and the 13 bytes of MCN or the 12 bytes of ISRC (tables 431 and 432).

  I.e. 22 bytes for MCN and 21 bytes for ISRC.

-  Neither the Sub-Channel Data Length (table 425) nor the bits MCVAL
  and TCVAL (tables 431 and 432) are interpreted to verify that the
  whole reply is valid.

- The gesture
    return strdup(&buf[9]);
  bets memory integrity on the MMC promise that byte 17 of Sub-channel
  Data is "Zero".

- cd-info or libcio levels inbetween do not obey the following demand
  of mmc_get_mcn_private() and mmc_get_track_isrc_private() :
    Note: string is malloc'd so caller should free() then returned
    string when done with it.

(I wonder whether the code in libburn yields the same density of sins.
 Not unlikely, given the current bug count of 3 : 3 in this thread.)

There is an overall problem lurking at the horizon:
 
- The command READ SUB-CHANNEL is not part of MMC-5 and MMC-6 any more.
  MMC-5 was released in 2007.
  cdrdao scans the sub-channel data which are replied by READ CD when
  reading the whole track.
  One could stop reading when the first instance of MCN or ISRC is found
  in the track. MMC-5 7.5.4.18, 7.5.4.19 promise that one has to read
  at most 100 blocks in order to get such an instance.

Well, exchanging READ SUB-CHANNEL by a READ CD scan loop is a larger job
from which i refrain for now.

-------------------------------------------------------------------------

I hope to have addressed the first three problems in this proposal

/**
  Return the media catalog number MCN.

  Note: string is malloc'd so caller should free() then returned
  string when done with it.

*/
char *
mmc_get_mcn_private ( void *p_env,
                      const mmc_run_cmd_fn_t run_mmc_cmd
                      )
{
  mmc_cdb_t cdb = {{0, }};
  char buf[22]; /* Up to last MCN byte in reply: 4 + 5 + 13 */
  int i_status;

  if ( ! p_env || ! run_mmc_cmd )
    return NULL;

  CDIO_MMC_SET_COMMAND(cdb.field, CDIO_MMC_GPCMD_READ_SUBCHANNEL);
  CDIO_MMC_SET_READ_LENGTH8(cdb.field, sizeof(buf));

  cdb.field[1] = 0x0;
  cdb.field[2] = 0x40;
  cdb.field[3] = CDIO_SUBCHANNEL_MEDIA_CATALOG;

  memset(buf, 0, sizeof(buf));

  i_status = run_mmc_cmd(p_env, mmc_timeout_ms,
                              mmc_get_cmd_len(cdb.field[0]),
                              &cdb, SCSI_MMC_DATA_READ,
                              sizeof(buf), buf);
  if(i_status == 0) {
    if((((unsigned int) buf[2]) << 8) + (unsigned int) buf[3] < 18)
      return NULL;        /* Not enough data replied */
    if(!(buf[8] & 0x80))
      return NULL;        /* MCN not valid */

    return strndup(&buf[9], 13);
  }
  return NULL;
}

The proposal for ISRC would look nearly the same.

-----------------------------------------------------------------------

I wonder whether it would be desirable to unite both function bodies.

char *
mmc_read_mcn_isrc ( void *p_env,
                    const mmc_run_cmd_fn_t run_mmc_cmd,
                    track_t i_track,
                    unsigned char sub_chan_param,
                    int length
                  )
{
  mmc_cdb_t cdb = {{0, }};
  char buf[22]; /* Up to last MCN byte in reply: 4 + 5 + 13 */
  int i_status;

  if ( ! p_env || ! run_mmc_cmd )
    return NULL;
  if (length > sizeof(buf) - 9)
    return NULL;          /* Too many reply bytes requested */

  CDIO_MMC_SET_COMMAND(cdb.field, CDIO_MMC_GPCMD_READ_SUBCHANNEL);
  CDIO_MMC_SET_READ_LENGTH8(cdb.field, sizeof(buf));

  cdb.field[1] = 0x0;
  cdb.field[2] = 0x40;
  cdb.field[3] = sub_chan_param;
  cdb.field[6] = i_track;

  memset(buf, 0, sizeof(buf));

  i_status = run_mmc_cmd(p_env, mmc_timeout_ms,
                              mmc_get_cmd_len(cdb.field[0]),
                              &cdb, SCSI_MMC_DATA_READ,
                              sizeof(buf), buf);
  if(i_status == 0) {
    if((((unsigned int) buf[2]) << 8) + (unsigned int) buf[3] < 5 + length)
      return NULL;        /* Not enough data replied */
    if(!(buf[8] & 0x80))
      return NULL;        /* not valid */

    return strndup(&buf[9], length);
  }
  return NULL;            /* SCSI command failed */
}

and to call this one by the existing frontend functions:

char *
mmc_get_mcn ( const CdIo_t *p_cdio )
{
  if ( ! p_cdio )  return NULL;
  return mmc_read_mcn_isrc (p_cdio->env, p_cdio->op.run_mmc_cmd,
                            (track_t) 0,
                            CDIO_SUBCHANNEL_MEDIA_CATALOG, 13 );
}

char *
mmc_get_track_isrc ( const CdIo_t *p_cdio, track_t i_track )
{
  if ( ! p_cdio )  return NULL;
  return mmc_read_mcn_isrc (p_cdio->env, p_cdio->op.run_mmc_cmd,
                            i_track,
                            CDIO_SUBCHANNEL_TRACK_ISRC, 12 );
}

driver_return_code_t
mmc_isrc_track_read_subchannel (CdIo_t *p_cdio,  /*in*/ const track_t track,
                                /*out*/ char *p_isrc)
{
  ...
  p_isrc_int = mmc_read_mcn_isrc(p_cdio->env, p_cdio->op.run_mmc_cmd, track,
                                 CDIO_SUBCHANNEL_TRACK_ISRC, 12);
  ...
}

At least it would better explain the constant "18". :))

-----------------------------------------------------------------------

In another stage of metamorphosis, we could split the new function
into an SCSI transport part and an MMC interpreter part, in order to
avoid requesting more reply bytes than the drive might be willing to
return.
This might be necessary to avoid problems with operating system SCSI
passthrough drivers. (There once were and there might still be in Linux.)

char *
mmc_read_sub_channel_transp ( void *p_env,
                              const mmc_run_cmd_fn_t run_mmc_cmd,
                              track_t i_track,
                              unsigned char sub_chan_param,
                              unsigned int *num_data,
                              char *buf
                            )
{
  mmc_cdb_t cdb = {{0, }};
  int i_status;

  if ( ! p_env || ! run_mmc_cmd )
    return -1;
  if (*num_data < 4)
    return -1;

  CDIO_MMC_SET_COMMAND(cdb.field, CDIO_MMC_GPCMD_READ_SUBCHANNEL);
  CDIO_MMC_SET_READ_LENGTH8(cdb.field, sizeof(buf));

  cdb.field[1] = 0x0;
  cdb.field[2] = 0x40;
  cdb.field[3] = sub_chan_param;
  cdb.field[6] = i_track;

  memset(buf, 0, sizeof(buf));

  i_status = run_mmc_cmd(p_env, mmc_timeout_ms,
                              mmc_get_cmd_len(cdb.field[0]),
                              &cdb, SCSI_MMC_DATA_READ,
                              *num_data, buf);
  if(i_status == 0) {
    *num_data = (((unsigned int) buf[2]) << 8) + (unsigned int) buf[3] + 4;
    return 0;
  }
  return -1;
}

char *
mmc_read_mcn_isrc ( void *p_env,
                    const mmc_run_cmd_fn_t run_mmc_cmd,
                    track_t i_track,
                    unsigned char sub_chan_param,
                    int length
                  )
{
  char buf[24]; /* Maximum reply size as of MMC-4 */
  int i_status;
  unsigned int num_data;

  if (length > sizeof(buf) - 9)
    return NULL;              /* Too many reply bytes requested */

  /* inquire number of available reply bytes */
  num_data = 4;
  memset(buf, 0, sizeof(buf));
  if (mmc_read_sub_channel_transp (p_env, run_mmc_cmd, i_track,
                                   sub_chan_param, &num_data, buf) == -1)
    return NULL;
  if (num_data < 9 + length)
    return NULL;              /* Not enough data replied */
  if (num_data > sizeof(buf))
    num_data = sizeof(buf);

  /* now fetch data */ 
  memset(buf, 0, sizeof(buf));
  if (mmc_read_sub_channel_transp (p_env, run_mmc_cmd, i_track,
                                   sub_chan_param, &num_data, buf) == -1)
    return NULL;              /* SCSI command failed */

  /* pick MCN or ISRC */
  if (num_data < 9 + length)
    return NULL;              /* Not enough data replied */
  if ( ! (buf[8] & 0x80) )
    return NULL;              /* MCN/ISRC not valid */
  return strndup(&buf[9], length);
}

-----------------------------------------------------------------------

The code proposals are not tested yet. We first should decide for what
code architecture to strive.


Have a nice day :)

Thomas