[DVDnav-discuss] [PATCH] Guarantee forward seek attempt moves position forward

[Reimar Döffinger]

On Sat, Sep 12, 2009 at 02:01:58PM -0700, John Stebbins wrote:
> diff -Naur libdvdnav.orig/src/searching.c libdvdnav/src/searching.c
> --- libdvdnav.orig/src/searching.c	2009-01-08 14:57:11.000000000 -0800
> +++ libdvdnav/src/searching.c	2009-09-12 13:47:33.880551499 -0700
> @@ -47,7 +47,7 @@
>  /* Return placed in vobu. */
>  /* Returns error status */
>  /* FIXME: Maybe need to handle seeking outside current cell. */
> -static dvdnav_status_t dvdnav_scan_admap(dvdnav_t *this, int32_t domain, uint32_t seekto_block, uint32_t *vobu) {
> +static dvdnav_status_t dvdnav_scan_admap(dvdnav_t *this, int32_t domain, uint32_t seekto_block, uint8_t next, uint32_t *vobu) {

IMO int should be used where no specific type is needed.

> @@ -89,7 +89,10 @@
>        vobu_start = next_vobu;
>        address++;
>      }
> -    *vobu = vobu_start;
> +    if(next)
> +      *vobu = next_vobu;
> +    else
> +      *vobu = vobu_start;

Use ?:

>    uint32_t first_cell_nr, last_cell_nr, cell_nr;
>    int32_t found;
> +  int32_t forward = 0;

int

> @@ -244,6 +250,8 @@
>      pthread_mutex_unlock(&this->vm_lock);
>      return DVDNAV_STATUS_ERR;
>    }
> +  if (target > current_pos)
> +    forward = 1;

forward = target > current_pos;

> @@ -270,6 +278,33 @@
>      } else {
>        /* convert the target sector from Cell-relative to absolute physical sector */
>        target += cell->first_sector;
> +      if (forward) {
> +        uint32_t vobu;
> +        /* if we are seeking forward from the current position, make sure
> +         * we move to a new position that is after our current position.
> +         * simply truncating to the vobu will go backwards */
> +        if (dvdnav_scan_admap(this, state->domain, target, 0, &vobu) == DVDNAV_STATUS_OK) {
> +          if (vobu <= current_pos) {
> +            if (dvdnav_scan_admap(this, state->domain, target, 1, &vobu) == DVDNAV_STATUS_OK) {
> +              if (vobu > cell->last_sector) {
> +                if (cell_nr == last_cell_nr) {
> +                  break;
> +                } else {
> +                    cell_nr++;
> +                    cell =  &(state->pgc->cell_playback[cell_nr-1]);
> +                    target = cell->first_sector;
> +                }
> +              } else {
> +                target = vobu;
> +              }
> +            } else {
> +              break;
> +            }
> +          }
> +        } else {
> +          break;
> +        }

Those extremely deep indentation levels make for an unreadable mess, not
to mention the inconsistent indentation.
        if (dvdnav_scan_admap(this, state->domain, target, 0, &vobu) != DVDNAV_STATUS_OK)
          break;
        if (vobu <= current_pos) {
          if (dvdnav_scan_admap(this, state->domain, target, 1, &vobu) != DVDNAV_STATUS_OK)
            break;
          if (vobu > cell->last_sector) {
            if (cell_nr == last_cell_nr)
              break;
            cell = state->pgc->cell_playback + cell_nr++;
            vobu = cell->first_sector;
          }
          target = vobu;
        }

Is a whole lot simpler but still feels suboptimal.
I also don't know if handling the cell change is quite right here or
if it wouldn't belong into dvdnav_scan_admap...