[MPlayer-dev-eng] [PATCH] stream_cdda.c bugs and such

[Ingo Brückl]

Reimar Döffinger wrote on Fri, 23 Dec 2011 17:48:53 +0100:

> On Tue, Dec 20, 2011 at 06:54:07PM +0100, Ingo Brückl wrote:
>> #3: -1 is never a valid track

> No, but -1 means "error". Your patch breaks the error case (e.g. broken
> TOC) by always returning 0 then.

Oh, I see, but no use is made of the error. The return value is instantly
used for calculation. Shouldn't there be a check and a STREAM_ERROR then?

  Index: stream/stream_cdda.c
  ===================================================================
  --- stream/stream_cdda.c	(revision 34466)
  +++ stream/stream_cdda.c	(working copy)
  @@ -247,6 +247,7 @@
       {
         int start_track = get_track_by_sector(p, p->start_sector);
         int end_track = get_track_by_sector(p, p->end_sector);
  +      if (start_track == -1 || end_track == -1) return STREAM_ERROR;
         *(unsigned int *)arg = end_track + 1 - start_track;
         return STREAM_OK;
       }
  @@ -256,6 +257,7 @@
         unsigned int track = *(unsigned int *)arg;
         int start_track = get_track_by_sector(p, p->start_sector);
         int end_track = get_track_by_sector(p, p->end_sector);
  +      if (start_track == -1 || end_track == -1) return STREAM_ERROR;
         int seek_sector;
         track += start_track;
         if (track > end_track) {
  @@ -273,6 +275,7 @@
       {
         int start_track = get_track_by_sector(p, p->start_sector);
         int cur_track = get_track_by_sector(p, p->sector);
  +      if (start_track == -1 || cur_track == -1) return STREAM_ERROR;
         *(unsigned int *)arg = cur_track - start_track;
         return STREAM_OK;
       }

>> #5: don't set paranoia mode if nothing is requested
>>     (This actually fixes a bug where starting sectors for tracks are wrongly
>>     changed which caused the off-by-one issue for GET_NUM_CHAPTERS. TOC
>>     reading seems more reliable without calling it. The manpage advises
>>     against paranoia values as well.)

> That doesn't make any sense to me at all.

There seems to be a bug(?) in cdparanoia. When calling paranoia_modeset()
with mode 0, it destroys start sector information. cdparanoia itself doesn't
call paranoia_modeset() for its TOC query option. (I know that rather the bug
in cdparanoia should be fixed, but since calling paranoia_modeset() with mode
0 is pointless anyway, we shouldn't do it.)

Two CD examples with tracks and their start sectors:

  cdparanoia -Q:

    1.     0
    2. 16767
    3. 37580
    4. 59745
    5. 70870

  debug output of mine from stream_cdda:

    ### before calling paranoia_modeset

    ### 1. 0
    ### 2. 16767
    ### 3. 37580
    ### 4. 59745
    ### 5. 70870

    ### mode = 0
    ### call to paranoia_modeset

    ### 1. 0
    ### 2. 0
    ### 3. 37580
    ### 4. 59745
    ### 5. 70870


  cdparanoia -Q:

    1.    33
    2. 18108
    3. 41740
    4. 60658
    5. 91370

  debug output of mine from stream_cdda:

    ### before calling paranoia_modeset

    ### 1. 33
    ### 2. 18108
    ### 3. 41740
    ### 4. 60658
    ### 5. 91370

    ### mode = 0
    ### call to paranoia_modeset

    ### 1. 33
    ### 2. 0
    ### 3. 41740
    ### 4. 60658
    ### 5. 91370

Ingo