[DVDnav-discuss] [PATCH] "dvdnav_jump_to_sector" as an alternative to "dvdnav_time_search" (REV 1: handle delay when reading directly off disc)

[gnosygnu]

>> Ok. Thanks for reviewing the patch and providing detailed feedback. My
>> apologies for the issues: most of these were not obvious to me.
>> I've changed all below except for #5 (not sure what to change), part
>> of #6 (I explain below) and #7 (not sure how to change). I detail more
>> inline. The revised patch is attached.
>
> Thanks for reworking your patch. It helps a lot.

Cool. I'm glad I got it right.

>>> 5. Almost none of the new code adheres to the style of the
>>>    current file.
>> I'm not sure what this applies to. I reviewed the code and did the following
>> - changed all single line comments (//) to multi-line (/* */)
>> - removed the "ERR:" prefix from the messages
>> - removed the out_ prefix from all variables
>> If there is something else, please detail.
>
> You will be cover a majority of the style issues by removing // for /**/ and
> not using tabs. This is not as much of an issue now. But don't be surprised
> if we go through at least one more iteration just to get the style consistent.

Okay. Feel free to specify, and I will change accordingly.

>>> 6. The #defines added to searching.c should be in dvdnav_internal.h
>>>    and they should be enums.
>> Ok. I moved all #defines to dvdnav_internal.h.
>> I converted the CELL_FIND constants to an enum.
>> I did not convert the TMAP_IDX constants as they are not an enum, but
>> marker values.
>
> OK, I can live w/ that.

Okay.

>>> 7. Many of the functions take several arguments. For example:
>>>    dvdnav_tmap_calc_time_for_tmap_entry() takes 8 arguments.
>>>    They should be pared down if possible. Since they are internal,
>>>    this is the least of the issues. But think about how we have to
>>>    be able to read this stuff in the future.
>> Is a struct permissible? If so, I can create a struct to bundle some
>> of the arguments. I reuse admap, admap_len, tmap, tmap_len,
>> tmap_interval several times, and I can combine them into one struct. I
>> can also define another struct to handle the index/sector variables
>> for querying the admap and the tmap.
>>     I'm not sure if this is what you're looking for though....
>
> An internal struct is permissible. Especially if it tends to encapsulate
> all of the data you need. When using a struct try to use const references
> when the function doesn't need to modify the struct and pointers when
> the functions do. This will cut down on the function's stack size.
>

How about these structs? If you have no objections, I'll go ahead and
rework the code around them.

/*
 * Describes a given time, and the closest sector, vobu and tmap index
 */
typedef struct {
  uint64_t          time;
  uint32_t          sector;
  uint32_t          vobu;
  int32_t           tmap;
} dvdnav_pos_data;

/*
 * Encapsulates common variables used by internal functions of jump_to_time
 */
typedef struct {
  vts_tmap_t        *tmap;
  vobu_admap_t      *admap;
  int32_t           admap_len;
  int32_t           tmap_len;
  int32_t           tmap_interval;
  int32_t           cell_idx;
  dvdnav_pos_data   *cell_bgn;
  dvdnav_pos_data   *cell_end;
  dvdnav_pos_data   *jump;
} dvdnav_jump_time_args;


> Thanks again.
>
> E
>
> --
> Erik Hovland
> erik at hovland.org
> http://hovland.org/
> _______________________________________________
> DVDnav-discuss mailing list
> DVDnav-discuss at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
>