[PATCH 0/6] catch up to 0.9.6 and a couple of extras
In an attempt to work towards getting Jorgen's patches in, I have taken on the task of catching up the mplayer tree of libdvdread to 0.9.6. Next will be 0.9.7. But I figured that I can submit that in its own chunk. This patch set also includes some cosmetic fixes as well as a potential memory leak fix and a check of a few system calls. Enjoy E --- Erik Hovland (6): 0.9.6 included a few items to improve the sofware on the *BSD Add in DVDFileStat Add the version function DVDVersion(). The version should be bumped to at least 5. But I am going to go to 6 on this one. Make cosmetic changes to files. Some system calls that report errors are not being checked. msvc/include/pthreads/pthread.h | 6 - msvc/include/sys/time.h | 2 msvc/include/unistd.h | 4 - src/bitreader.c | 2 src/dvd_input.c | 2 src/dvd_reader.c | 258 ++++++++++++++++++++++++++++++++++++--- src/dvd_udf.c | 4 - src/dvdread/dvd_reader.h | 47 ++++++- src/dvdread/ifo_types.h | 2 src/dvdread/nav_types.h | 8 + src/ifo_print.c | 22 ++- src/md5.c | 4 - src/nav_print.c | 2 13 files changed, 302 insertions(+), 61 deletions(-) -- Signature
Not a big deal, but chdir, getcwd and fchdir all return values that could be checked. Signed-off-by: Erik Hovland <erik@hovland.org> --- src/dvd_reader.c | 21 ++++++++++++++++++--- 1 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/dvd_reader.c b/src/dvd_reader.c index 65900f7..66289e7 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -421,14 +421,29 @@ dvd_reader_t *DVDOpen( const char *ppath ) int cdir = open( ".", O_RDONLY ); if( cdir >= 0 ) { - chdir( path_copy ); + if ( chdir( path_copy ) == -1 ) { + close( cdir ); + return NULL; + } new_path = malloc(PATH_MAX+1); if(!new_path) { free(path); return NULL; } - getcwd(new_path, PATH_MAX ); - fchdir( cdir ); + if ( getcwd( new_path, PATH_MAX ) == NULL ) { + free( path ); + close( cdir ); + free( path_copy ); + free( new_path ); + return NULL; + } + if ( fchdir( cdir ) == -1 ) { + free( path ); + close( cdir ); + free( path_copy ); + free( new_path ); + return NULL; + } close( cdir ); free( path_copy ); path_copy = new_path;
On Fri, Feb 13, 2009 at 11:20:49AM -0800, Erik Hovland wrote:
diff --git a/src/dvd_reader.c b/src/dvd_reader.c index 65900f7..66289e7 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -421,14 +421,29 @@ dvd_reader_t *DVDOpen( const char *ppath ) int cdir = open( ".", O_RDONLY );
if( cdir >= 0 ) { - chdir( path_copy ); + if ( chdir( path_copy ) == -1 ) { + close( cdir ); + return NULL; + } new_path = malloc(PATH_MAX+1); if(!new_path) { free(path); return NULL; } - getcwd(new_path, PATH_MAX ); - fchdir( cdir ); + if ( getcwd( new_path, PATH_MAX ) == NULL ) { + free( path ); + close( cdir ); + free( path_copy ); + free( new_path ); + return NULL; + } + if ( fchdir( cdir ) == -1 ) { + free( path ); + close( cdir ); + free( path_copy ); + free( new_path ); + return NULL; + } close( cdir ); free( path_copy ); path_copy = new_path;
Obviously there are a whole lot of free/close etc. missing, and with more returns that IMO becomes completely unmaintainable. I think this is a perfect place for something like "goto error" which does all the necessary cleanup. Afterwards the error checks can be added.
Il giorno ven, 13/02/2009 alle 20.32 +0100, Reimar Döffinger ha scritto:
On Fri, Feb 13, 2009 at 11:20:49AM -0800, Erik Hovland wrote:
Obviously there are a whole lot of free/close etc. missing, and with more returns that IMO becomes completely unmaintainable. I think this is a perfect place for something like "goto error" which does all the necessary cleanup. Afterwards the error checks can be added.
I agree
Cosmetic fixes of spelling errors and some style inconsistencies. Signed-off-by: Erik Hovland <erik@hovland.org> --- msvc/include/pthreads/pthread.h | 6 +++--- msvc/include/sys/time.h | 2 +- msvc/include/unistd.h | 4 ++-- src/dvd_reader.c | 18 +++++++++--------- src/dvdread/dvd_reader.h | 8 ++++---- src/dvdread/ifo_types.h | 2 +- src/dvdread/nav_types.h | 8 ++++---- src/ifo_print.c | 22 +++++++++++----------- 8 files changed, 35 insertions(+), 35 deletions(-) diff --git a/msvc/include/pthreads/pthread.h b/msvc/include/pthreads/pthread.h index c2c2caa..9bb3de4 100644 --- a/msvc/include/pthreads/pthread.h +++ b/msvc/include/pthreads/pthread.h @@ -196,7 +196,7 @@ extern "C" * pthread_attr_setstackaddr * * _POSIX_THREAD_PRIORITY_SCHEDULING (set) - * If set, you can use realtime scheduling. + * If set, you can use real-time scheduling. * Indicates the availability of: * pthread_attr_getinheritsched * pthread_attr_getschedparam @@ -393,7 +393,7 @@ enum { PTHREAD_SCOPE_SYSTEM = 1, /* Default */ /* - * pthread_setcancelstate paramters + * pthread_setcancelstate parameters */ PTHREAD_CANCEL_ENABLE = 0, /* Default */ PTHREAD_CANCEL_DISABLE = 1, @@ -420,7 +420,7 @@ enum { /* * ==================== * ==================== - * Cancelation + * Cancellation * ==================== * ==================== */ diff --git a/msvc/include/sys/time.h b/msvc/include/sys/time.h index f519f5a..f7973c1 100644 --- a/msvc/include/sys/time.h +++ b/msvc/include/sys/time.h @@ -20,7 +20,7 @@ * WIN32 PORT, * by Matthew Grooms <elon@altavista.com> * - * sys/time.h - There is no seperate sys/time.h for win32 so we simply + * sys/time.h - There is no separate sys/time.h for win32 so we simply * include the standard time header as well as our xine * timer functions. */ diff --git a/msvc/include/unistd.h b/msvc/include/unistd.h index 0f6cec7..620017c 100644 --- a/msvc/include/unistd.h +++ b/msvc/include/unistd.h @@ -21,7 +21,7 @@ * by Matthew Grooms <elon@altavista.com> * * unistd.h - This is mostly a catch all header that maps standard unix - * libc calls to the equivelent win32 functions. + * libc calls to the equivalent win32 functions. * */ @@ -63,7 +63,7 @@ #define snprintf _snprintf #define vsnprintf _vsnprintf -// FIXME : I dont remember why this is here +// FIXME : I don't remember why this is here #define readlink #endif diff --git a/src/dvd_reader.c b/src/dvd_reader.c index 66289e7..408811d 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -249,7 +249,7 @@ static dvd_reader_t *DVDOpenImageFile( const char *location, int have_css ) if( have_css ) { /* Only if DVDCSS_METHOD = title, a bit if it's disc or if - * DVDCSS_METHOD = key but region missmatch. Unfortunaly we + * DVDCSS_METHOD = key but region mismatch. Unfortunately we * don't have that information. */ dvd->css_state = 1; /* Need key init. */ @@ -304,7 +304,7 @@ static char *sun_block2char( const char *path ) #endif #if defined(SYS_BSD) -/* FreeBSD /dev/(r)(a)cd0c (a is for atapi), recomended to _not_ use r +/* FreeBSD /dev/(r)(a)cd0c (a is for atapi), recommended to _not_ use r OpenBSD /dev/rcd0c, it needs to be the raw device NetBSD /dev/rcd0[d|c|..] d for x86, c (for non x86), perhaps others Darwin /dev/rdisk0, it needs to be the raw device @@ -415,7 +415,7 @@ dvd_reader_t *DVDOpen( const char *ppath ) #ifndef WIN32 /* don't have fchdir, and getcwd( NULL, ... ) is strange */ /* Also WIN32 does not have symlinks, so we don't need this bit of code. */ - /* Resolve any symlinks and get the absolut dir name. */ + /* Resolve any symlinks and get the absolute dir name. */ { char *new_path; int cdir = open( ".", O_RDONLY ); @@ -425,9 +425,9 @@ dvd_reader_t *DVDOpen( const char *ppath ) close( cdir ); return NULL; } - new_path = malloc(PATH_MAX+1); - if(!new_path) { - free(path); + new_path = malloc( PATH_MAX + 1 ); + if( new_path == NULL ) { + free( path ); return NULL; } if ( getcwd( new_path, PATH_MAX ) == NULL ) { @@ -445,15 +445,15 @@ dvd_reader_t *DVDOpen( const char *ppath ) return NULL; } close( cdir ); - free( path_copy ); - path_copy = new_path; + free( path_copy ); + path_copy = new_path; } } #endif /** * If we're being asked to open a directory, check if that directory - * is the mountpoint for a DVD-ROM which we can use instead. + * is the mount point for a DVD-ROM which we can use instead. */ if( strlen( path_copy ) > 1 ) { diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index 8a2a58d..93c5d2f 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -86,8 +86,8 @@ typedef struct dvd_file_s dvd_file_t; * path/VTS_01_1.VOB * path/vts_01_1.vob * - * @param path Specifies the the device, file or directory to be used. - * @return If successful a a read handle is returned. Otherwise 0 is returned. + * @param path Specifies the device, file or directory to be used. + * @return If successful a read handle is returned. Otherwise 0 is returned. * * dvd = DVDOpen(path); */ @@ -126,7 +126,7 @@ typedef enum { * @param dvd A dvd read handle. * @param titlenum Which Video Title Set should be used, VIDEO_TS is 0. * @param domain Which domain. - * @return If successful a a file read handle is returned, otherwise 0. + * @return If successful a file read handle is returned, otherwise 0. * * dvd_file = DVDOpenFile(dvd, titlenum, domain); */ dvd_file_t *DVDOpenFile( dvd_reader_t *, int, dvd_read_domain_t ); @@ -174,7 +174,7 @@ int32_t DVDFileSeek( dvd_file_t *, int32_t ); /** * Reads the given number of bytes from the file. This call can only be used * on the information files, and may not be used for reading from a VOB. This - * reads from and increments the currrent seek position for the file. + * reads from and increments the current seek position for the file. * * @param dvd_file A file read handle. * @param data Pointer to a buffer to write the data into. diff --git a/src/dvdread/ifo_types.h b/src/dvdread/ifo_types.h index b28fe7f..7130699 100644 --- a/src/dvdread/ifo_types.h +++ b/src/dvdread/ifo_types.h @@ -558,7 +558,7 @@ typedef struct { uint8_t unknown1; uint8_t unknown2; uint8_t unknown3; - uint8_t unknown4; /* ?? allways 0x30 language?, text format? */ + uint8_t unknown4; /* ?? always 0x30 language?, text format? */ uint8_t unknown5; uint16_t offset; /* from first */ diff --git a/src/dvdread/nav_types.h b/src/dvdread/nav_types.h index aa36b52..9aef6b1 100644 --- a/src/dvdread/nav_types.h +++ b/src/dvdread/nav_types.h @@ -73,7 +73,7 @@ typedef struct { * 1XXb: pan&scan buttons */ typedef struct { - uint16_t hli_ss; /**< status, only low 2 bits 0: no buttons, 1: different 2: equal 3: eual except for button cmds */ + uint16_t hli_ss; /**< status, only low 2 bits 0: no buttons, 1: different 2: equal 3: equal except for button cmds */ uint32_t hli_s_ptm; /**< start ptm of hli */ uint32_t hli_e_ptm; /**< end ptm of hli */ uint32_t btn_se_e_ptm; /**< end ptm of button select */ @@ -96,7 +96,7 @@ typedef struct { /** * Button Color Information Table - * Each entry beeing a 32bit word that contains the color indexs and alpha + * Each entry being a 32bit word that contains the color indexes and alpha * values to use. They are all represented by 4 bit number and stored * like this [Ci3, Ci2, Ci1, Ci0, A3, A2, A1, A0]. The actual palette * that the indexes reference is in the PGC. @@ -192,7 +192,7 @@ typedef struct { } ATTRIBUTE_PACKED sml_pbi_t; /** - * Seamless Angle Infromation for one angle + * Seamless Angle Information for one angle */ typedef struct { uint32_t address; /**< offset to next ILVU, high bit is before/after */ @@ -200,7 +200,7 @@ typedef struct { } ATTRIBUTE_PACKED sml_agl_data_t; /** - * Seamless Angle Infromation + * Seamless Angle Information */ typedef struct { sml_agl_data_t data[9]; diff --git a/src/ifo_print.c b/src/ifo_print.c index 4863b1b..68e4ce3 100644 --- a/src/ifo_print.c +++ b/src/ifo_print.c @@ -130,7 +130,7 @@ static void ifo_print_video_attributes(int level, video_attr_t *attr) { printf("(please send a bug report), "); } - // Wide is allways allowed..!!! + // Wide is always allowed..!!! switch(attr->permitted_df) { case 0: printf("pan&scan+letterboxed, "); @@ -328,7 +328,7 @@ static void ifo_print_audio_attributes(int level, audio_attr_t *attr) { case 1: // Normal audio printf("Normal Caption "); break; - case 2: // visually imparied + case 2: // visually impaired printf("Audio for visually impaired "); break; case 3: // Directors 1 @@ -685,7 +685,7 @@ static void ifoPrint_CELL_PLAYBACK(cell_playback_t *cell_playback, int nr) { break; case 2: case 3: - printf("(send bug repport) "); + printf("(send bug report) "); break; } } @@ -781,7 +781,7 @@ void ifoPrint_PGC(pgc_t *pgc) { } } - /* Memmory offsets to div. tables. */ + /* Memory offsets to div. tables. */ ifoPrint_PGC_COMMAND_TBL(pgc->command_tbl); ifoPrint_PGC_PROGRAM_MAP(pgc->program_map, pgc->nr_of_programs); ifoPrint_CELL_PLAYBACK(pgc->cell_playback, pgc->nr_of_cells); @@ -806,7 +806,7 @@ void ifoPrint_TT_SRPT(tt_srpt_t *tt_srpt) { printf("\tTitle playback type: (%02x)\n", *(uint8_t *)&(tt_srpt->title[i].pb_ty)); printf("\t\t%s\n", - tt_srpt->title[i].pb_ty.multi_or_random_pgc_title ? "Random or Shuffle" : "Sequencial"); + tt_srpt->title[i].pb_ty.multi_or_random_pgc_title ? "Random or Shuffle" : "Sequential"); if (tt_srpt->title[i].pb_ty.jlc_exists_in_cell_cmd) printf("\t\tJump/Link/Call exists in cell cmd\n"); if (tt_srpt->title[i].pb_ty.jlc_exists_in_prepost_cmd) printf("\t\tJump/Link/Call exists in pre/post cmd\n"); if (tt_srpt->title[i].pb_ty.jlc_exists_in_button_cmd) printf("\t\tJump/Link/Call exists in button cmd\n"); @@ -1109,7 +1109,7 @@ void ifo_print(dvd_reader_t *dvd, int title) { printf("No PGCI Unit table present\n"); } - printf("\nParental Manegment Information table\n"); + printf("\nParental Management Information table\n"); printf( "------------------------------------\n"); if(ifohandle->ptl_mait) { ifoPrint_PTL_MAIT(ifohandle->ptl_mait); @@ -1129,12 +1129,12 @@ void ifo_print(dvd_reader_t *dvd, int title) { printf("No Text Data Manager Information present\n"); } - printf("\nMenu Cell Adress table\n"); + printf("\nMenu Cell Address table\n"); printf( "-----------------\n"); if(ifohandle->menu_c_adt) { ifoPrint_C_ADT(ifohandle->menu_c_adt); } else { - printf("No Menu Cell Adress table present\n"); + printf("No Menu Cell Address table present\n"); } printf("\nVideo Manager Menu VOBU address map\n"); @@ -1176,12 +1176,12 @@ void ifo_print(dvd_reader_t *dvd, int title) { printf("No VTS Time Map table present\n"); } - printf("\nMenu Cell Adress table\n"); + printf("\nMenu Cell Address table\n"); printf( "-----------------\n"); if(ifohandle->menu_c_adt) { ifoPrint_C_ADT(ifohandle->menu_c_adt); } else { - printf("No Cell Adress table present\n"); + printf("No Cell Address table present\n"); } printf("\nVideo Title Set Menu VOBU address map\n"); @@ -1192,7 +1192,7 @@ void ifo_print(dvd_reader_t *dvd, int title) { printf("No Menu VOBU address map present\n"); } - printf("\nCell Adress table\n"); + printf("\nCell Address table\n"); printf( "-----------------\n"); ifoPrint_C_ADT(ifohandle->vts_c_adt);
Il giorno ven, 13/02/2009 alle 11.21 -0800, Erik Hovland ha scritto:
Cosmetic fixes of spelling errors and some style inconsistencies.
Signed-off-by: Erik Hovland <erik@hovland.org>
---
msvc/include/pthreads/pthread.h | 6 +++--- msvc/include/sys/time.h | 2 +- msvc/include/unistd.h | 4 ++-- src/dvd_reader.c | 18 +++++++++--------- src/dvdread/dvd_reader.h | 8 ++++---- src/dvdread/ifo_types.h | 2 +- src/dvdread/nav_types.h | 8 ++++---- src/ifo_print.c | 22 +++++++++++----------- 8 files changed, 35 insertions(+), 35 deletions(-)
Erik, did you check your patches? if so, what version of the code did you diff them against? One more thing: git is a git at sending patches;) Please, send them as attachment and with a proper subject and explanation. (btw, why are people so fond of git???) Moreover, I don't like changes like the one below:
@@ -425,9 +425,9 @@ dvd_reader_t *DVDOpen( const char *ppath ) close( cdir ); return NULL; } - new_path = malloc(PATH_MAX+1); - if(!new_path) { - free(path); + new_path = malloc( PATH_MAX + 1 ); + if( new_path == NULL ) { + free( path ); return NULL; } if ( getcwd( new_path, PATH_MAX ) == NULL ) { @@ -445,15 +445,15 @@ dvd_reader_t *DVDOpen( const char *ppath ) return NULL; } close( cdir ); - free( path_copy ); - path_copy = new_path; + free( path_copy ); + path_copy = new_path; } } #endif
Cosmetic fixes of spelling errors and some style inconsistencies.
Signed-off-by: Erik Hovland <erik@hovland.org>
---
msvc/include/pthreads/pthread.h | 6 +++--- msvc/include/sys/time.h | 2 +- msvc/include/unistd.h | 4 ++-- src/dvd_reader.c | 18 +++++++++--------- src/dvdread/dvd_reader.h | 8 ++++---- src/dvdread/ifo_types.h | 2 +- src/dvdread/nav_types.h | 8 ++++---- src/ifo_print.c | 22 +++++++++++----------- 8 files changed, 35 insertions(+), 35 deletions(-)
Erik, did you check your patches? if so, what version of the code did you diff them against?
The changes are based on a diff of libdvdread 0.9.5 and 0.9.6.
One more thing: git is a git at sending patches;) Please, send them as attachment and with a proper subject and explanation.
No problem. Will do. Will even resend this set that way.
(btw, why are people so fond of git???)
git-svn + stgit is about right for my workflow. My biggest issue w/ svn is that I am unable to evolve locally w/out polluting the main repo. But on top of that, it is pathetic at working with patches. Even quilt w/ svn is basically a dirty kludge.
Moreover, I don't like changes like the one below:
No problem. It is a cosmetic fix only. I am happy to drop it. E -- Erik Hovland erik@hovland.org http://hovland.org/
Il giorno dom, 15/02/2009 alle 10.24 -0800, Erik Hovland ha scritto:
The changes are based on a diff of libdvdread 0.9.5 and 0.9.6.
One more thing: git is a git at sending patches;) Please, send them as attachment and with a proper subject and explanation.
No problem. Will do. Will even resend this set that way.
(btw, why are people so fond of git???)
git-svn + stgit is about right for my workflow. My biggest issue w/ svn is that I am unable to evolve locally w/out polluting the main repo. But on top of that, it is pathetic at working with patches. Even quilt w/ svn is basically a dirty kludge.
Moreover, I don't like changes like the one below:
No problem. It is a cosmetic fix only. I am happy to drop it.
E
no reason to drop it. Applied by hand, thanks.
Up the version compiled in to 906. Signed-off-by: Erik Hovland <erik@hovland.org> --- src/dvdread/dvd_reader.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index 93c5d2f..be94cc7 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -43,7 +43,7 @@ /** * The current version. */ -#define DVDREAD_VERSION 904 +#define DVDREAD_VERSION 906 /** * The length of one Logical Block of a DVD.
Il giorno ven, 13/02/2009 alle 11.22 -0800, Erik Hovland ha scritto:
Up the version compiled in to 906.
Signed-off-by: Erik Hovland <erik@hovland.org>
---
src/dvdread/dvd_reader.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index 93c5d2f..be94cc7 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -43,7 +43,7 @@ /** * The current version. */ -#define DVDREAD_VERSION 904 +#define DVDREAD_VERSION 906
/** * The length of one Logical Block of a DVD.
Can you explain this patch? Is it supposed to expose a supposed compatibility with dvdread-0.9.6? If so it's not ok: I never applied the buffer-alignment stuff and I'm not going to apply it unless someone explains me why it's a good feature to have.
Up the version compiled in to 906.
Signed-off-by: Erik Hovland <erik@hovland.org>
---
src/dvdread/dvd_reader.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index 93c5d2f..be94cc7 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -43,7 +43,7 @@ /** * The current version. */ -#define DVDREAD_VERSION 904 +#define DVDREAD_VERSION 906
/** * The length of one Logical Block of a DVD.
Can you explain this patch? Is it supposed to expose a supposed compatibility with dvdread-0.9.6? If so it's not ok: I never applied the buffer-alignment stuff and I'm not going to apply it unless someone explains me why it's a good feature to have.
The only reason to have the patch in the set is for consistency's sake. In fact, I am all for removing this define macro altogether as it exists as a historical artifact to the original libdvdread. E -- Erik Hovland erik@hovland.org http://hovland.org/
Add in the DVDVersion function. Feel free to drop this patch as it really doesn't add anything helpful. Signed-off-by: Erik Hovland <erik@hovland.org> --- src/dvd_reader.c | 8 ++++++++ src/dvdread/dvd_reader.h | 4 ++++ 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/dvd_reader.c b/src/dvd_reader.c index 052d5b0..0147bbb 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -116,6 +116,14 @@ int UDFReadBlocksRaw( dvd_reader_t *device, uint32_t lb_number, int encrypted ); /** + * Returns the compiled version. (DVDREAD_VERSION as an int) + */ +int DVDVersion() +{ + return DVDREAD_VERSION; +} + +/** * Set the level of caching on udf * level = 0 (no caching) * level = 1 (caching filesystem info) diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index be94cc7..327c6f6 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -68,6 +68,10 @@ typedef struct dvd_reader_s dvd_reader_t; * Opaque type for a file read handle, much like a normal fd or FILE *. */ typedef struct dvd_file_s dvd_file_t; +/** + * Returns the compiled version of the library. (The value of DVDREAD_VERSION) + */ +int DVDVersion(); /** * Opens a block device of a DVD-ROM file, or an image file, or a directory
0.9.6 adds the library functions DVDFileStat and the structure dvd_stat_t. This also adds some internal helper functions. This patch differs from the original code in that it employs the _s vs. _t opacity trick. Signed-off-by: Erik Hovland <erik@hovland.org> --- src/dvd_reader.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++ src/dvdread/dvd_reader.h | 33 ++++++++ 2 files changed, 220 insertions(+), 0 deletions(-) diff --git a/src/dvd_reader.c b/src/dvd_reader.c index 0147bbb..f4be482 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -111,6 +111,12 @@ struct dvd_file_s { ssize_t filesize; }; +struct dvd_stat_s { + off_t size; /**< Total size of file in bytes */ + int nr_parts; /**< Number of file parts */ + off_t parts_size[9]; /**< Size of each part in bytes */ +}; + int UDFReadBlocksRaw( dvd_reader_t *device, uint32_t lb_number, size_t block_count, unsigned char *data, int encrypted ); @@ -910,6 +916,187 @@ void DVDCloseFile( dvd_file_t *dvd_file ) } } +static int DVDFileStatVOBUDF(dvd_reader_t *dvd, int title, + int menu, dvd_stat_t *statbuf) +{ + char filename[ MAX_UDF_FILE_NAME_LEN ]; + uint32_t size; + off_t tot_size; + off_t parts_size[9]; + int nr_parts = 0; + int n; + + if( title == 0 ) { + sprintf( filename, "/VIDEO_TS/VIDEO_TS.VOB" ); + } else { + sprintf( filename, "/VIDEO_TS/VTS_%02d_%d.VOB", title, menu ? 0 : 1 ); + } + if(!UDFFindFile( dvd, filename, &size )) { + return -1; + } + tot_size = size; + nr_parts = 1; + parts_size[0] = size; + + if( !menu ) { + int cur; + + for( cur = 2; cur < 10; cur++ ) { + sprintf( filename, "/VIDEO_TS/VTS_%02d_%d.VOB", title, cur ); + if( !UDFFindFile( dvd, filename, &size ) ) { + break; + } + parts_size[nr_parts] = size; + tot_size += size; + nr_parts++; + } + } + + statbuf->size = tot_size; + statbuf->nr_parts = nr_parts; + for(n = 0; n < nr_parts; n++) { + statbuf->parts_size[n] = parts_size[n]; + } + return 0; +} + + +static int DVDFileStatVOBPath( dvd_reader_t *dvd, int title, + int menu, dvd_stat_t *statbuf ) +{ + char filename[ MAX_UDF_FILE_NAME_LEN ]; + char full_path[ PATH_MAX + 1 ]; + struct stat fileinfo; + off_t tot_size; + off_t parts_size[9]; + int nr_parts = 0; + int n; + + + + if( title == 0 ) { + sprintf( filename, "VIDEO_TS.VOB" ); + } else { + sprintf( filename, "VTS_%02d_%d.VOB", title, menu ? 0 : 1 ); + } + if( !findDVDFile( dvd, filename, full_path ) ) { + return -1; + } + + if( stat( full_path, &fileinfo ) < 0 ) { + fprintf( stderr, "libdvdread: Can't stat() %s.\n", filename ); + return -1; + } + + + tot_size = fileinfo.st_size; + nr_parts = 1; + parts_size[0] = fileinfo.st_size; + + if( !menu ) { + int cur; + + for( cur = 2; cur < 10; cur++ ) { + + sprintf( filename, "VTS_%02d_%d.VOB", title, cur ); + if( !findDVDFile( dvd, filename, full_path ) ) { + break; + } + + if( stat( full_path, &fileinfo ) < 0 ) { + fprintf( stderr, "libdvdread: Can't stat() %s.\n", filename ); + break; + } + + parts_size[nr_parts] = fileinfo.st_size; + tot_size += parts_size[nr_parts]; + nr_parts++; + } + } + + statbuf->size = tot_size; + statbuf->nr_parts = nr_parts; + for(n = 0; n < nr_parts; n++) { + statbuf->parts_size[n] = parts_size[n]; + } + return 0; +} + + +int DVDFileStat(dvd_reader_t *dvd, int titlenum, + dvd_read_domain_t domain, dvd_stat_t *statbuf) +{ + char filename[ MAX_UDF_FILE_NAME_LEN ]; + char full_path[ PATH_MAX + 1 ]; + struct stat fileinfo; + uint32_t size; + + /* Check arguments. */ + if( dvd == NULL || titlenum < 0 ) { + errno = EINVAL; + return -1; + } + + switch( domain ) { + case DVD_READ_INFO_FILE: + if( titlenum == 0 ) { + sprintf( filename, "/VIDEO_TS/VIDEO_TS.IFO" ); + } else { + sprintf( filename, "/VIDEO_TS/VTS_%02i_0.IFO", titlenum ); + } + break; + case DVD_READ_INFO_BACKUP_FILE: + if( titlenum == 0 ) { + sprintf( filename, "/VIDEO_TS/VIDEO_TS.BUP" ); + } else { + sprintf( filename, "/VIDEO_TS/VTS_%02i_0.BUP", titlenum ); + } + break; + case DVD_READ_MENU_VOBS: + if( dvd->isImageFile ) { + return DVDFileStatVOBUDF( dvd, titlenum, 1, statbuf ); + } else { + return DVDFileStatVOBPath( dvd, titlenum, 1, statbuf ); + } + break; + case DVD_READ_TITLE_VOBS: + if( titlenum == 0 ) { + return -1; + } + if( dvd->isImageFile ) { + return DVDFileStatVOBUDF( dvd, titlenum, 0, statbuf ); + } else { + return DVDFileStatVOBPath( dvd, titlenum, 0, statbuf ); + } + break; + default: + fprintf( stderr, "libdvdread: Invalid domain for file stat.\n" ); + errno = EINVAL; + return -1; + } + + if( dvd->isImageFile ) { + if( UDFFindFile( dvd, filename, &size ) ) { + statbuf->size = size; + statbuf->nr_parts = 1; + statbuf->parts_size[0] = size; + return 0; + } + } else { + if( findDVDFile( dvd, filename, full_path ) ) { + if( stat( full_path, &fileinfo ) < 0 ) { + fprintf( stderr, "libdvdread: Can't stat() %s.\n", filename ); + } else { + statbuf->size = fileinfo.st_size; + statbuf->nr_parts = 1; + statbuf->parts_size[0] = statbuf->size; + return 0; + } + } + } + return -1; +} + /* Internal, but used from dvd_udf.c */ int UDFReadBlocksRaw( dvd_reader_t *device, uint32_t lb_number, size_t block_count, unsigned char *data, diff --git a/src/dvdread/dvd_reader.h b/src/dvdread/dvd_reader.h index 327c6f6..6ce1fb9 100644 --- a/src/dvdread/dvd_reader.h +++ b/src/dvdread/dvd_reader.h @@ -68,6 +68,12 @@ typedef struct dvd_reader_s dvd_reader_t; * Opaque type for a file read handle, much like a normal fd or FILE *. */ typedef struct dvd_file_s dvd_file_t; + +/** + * Opaque type that is used to provide statistics on a handle. + */ +typedef struct dvd_stat_s dvd_stat_t; + /** * Returns the compiled version of the library. (The value of DVDREAD_VERSION) */ @@ -121,6 +127,33 @@ typedef enum { } dvd_read_domain_t; /** + * Stats a file on the DVD given the title number and domain. + * The information about the file is stored in a dvd_stat_t + * which contains information about the size of the file and + * the number of parts in case of a multipart file and the respective + * sizes of the parts. + * A multipart file is for instance VTS_02_1.VOB, VTS_02_2.VOB, VTS_02_3.VOB + * The size of VTS_02_1.VOB will be stored in stat->parts_size[0], + * VTS_02_2.VOB in stat->parts_size[1], ... + * The total size (sum of all parts) is stored in stat->size and + * stat->nr_parts will hold the number of parts. + * Only DVD_READ_TITLE_VOBS (VTS_??_[1-9].VOB) can be multipart files. + * + * This function is only of use if you want to get the size of each file + * in the filesystem. These sizes are not needed to use any other + * functions in libdvdread. + * + * @param dvd A dvd read handle. + * @param titlenum Which Video Title Set should be used, VIDEO_TS is 0. + * @param domain Which domain. + * @param stat Pointer to where the result is stored. + * @return If successful 0, otherwise -1. + * + * int DVDFileStat(dvd, titlenum, domain, stat); + */ +int DVDFileStat(dvd_reader_t *, int, dvd_read_domain_t, dvd_stat_t *); + +/** * Opens a file on the DVD given the title number and domain. * * If the title number is 0, the video manager information is opened
On Friday 13 February 2009 20:24:59 Erik Hovland wrote:
0.9.6 adds the library functions DVDFileStat and the structure dvd_stat_t. This also adds some internal helper functions.
This patch differs from the original code in that it employs the _s vs. _t opacity trick.
Signed-off-by: Erik Hovland <erik@hovland.org>
Has there been any progress on this? I stumbled over this today while trying to compile dvdbackup, which still fails because this is missing. Another question: Would it be save for distributions to use that patch on libdvdread-4.1.3? Grs, Heinz
0.9.6 adds the library functions DVDFileStat and the structure dvd_stat_t. This also adds some internal helper functions.
This patch differs from the original code in that it employs the _s vs. _t opacity trick.
Signed-off-by: Erik Hovland <erik@hovland.org>
Has there been any progress on this?
It is currently stalled. Waiting for me to apply for svn write access. Give me a week to get my act together.
I stumbled over this today while trying to compile dvdbackup, which still fails because this is missing.
Another question: Would it be save for distributions to use that patch on libdvdread-4.1.3?
I plan to put this patch in ASAP. So it is probably pretty safe. E -- Erik Hovland erik@hovland.org http://hovland.org/
0.9.6 adds the library functions DVDFileStat and the structure dvd_stat_t. This also adds some internal helper functions.
This patch differs from the original code in that it employs the _s vs. _t opacity trick.
Signed-off-by: Erik Hovland <erik@hovland.org>
Has there been any progress on this? I stumbled over this today while trying to compile dvdbackup, which still fails because this is missing.
This was committed this week. Let me know if you are still having trouble w/ dvdbackup. E -- Erik Hovland erik@hovland.org http://hovland.org/
libdvdnav has a problem in that it is difficult to gracefully recover from a read error and continue on to subsequent blocks on the disc. An application would like to seek forward past the current block after getting a read error in order to attempt to get past the bad block(s). But dvdnav_sector_search() does not guarantee that a requested forward seek will actually move the current position forward. It truncates down to the nearest VOBU which will almost always cause a forward seek request by a single block (or a small number of blocks) to move the current position backward. This behaviour puts the application into a loop of: read failure, attempted forward seek (which results in backward seek), read failure ... Attached is a patch to dvdnav_sector_search which guarantees that an attempted forward seek always moves the current position forward.
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...
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...
On 09/12/2009 02:24 PM, Reimar Döffinger wrote:
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...
Allrightythen. Here's an update incorporating your suggestions. dvdnav_scan_admap didn't look like the right place for handling the cell boundary because it currently doesn't touch the cell information at all. In order to handle the boundary, it would have to replicate a lot of code that is already in dvdnav_sector_search. If that's what you want, I'm happy to do it. Just seems worse than the alternative.
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch. E -- Erik Hovland erik@hovland.org http://hovland.org/
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him. On 09/20/2009 11:08 AM, Erik Hovland wrote:
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
E
On Sunday, 20 September 2009 at 20:25, John Stebbins wrote:
On 09/20/2009 11:08 AM, Erik Hovland wrote:
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him.
Ping Nico. Could you approve/reject this? Regards, R. -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
On Sunday, 27 September 2009 at 00:54, Dominik 'Rathann' Mierzejewski wrote:
On Sunday, 20 September 2009 at 20:25, John Stebbins wrote:
On 09/20/2009 11:08 AM, Erik Hovland wrote:
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him.
Ping Nico. Could you approve/reject this?
Ping2. Regards, R. -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
On Friday, 05 March 2010 at 00:27, Dominik 'Rathann' Mierzejewski wrote:
On Sunday, 27 September 2009 at 00:54, Dominik 'Rathann' Mierzejewski wrote:
On Sunday, 20 September 2009 at 20:25, John Stebbins wrote:
On 09/20/2009 11:08 AM, Erik Hovland wrote:
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him.
Ping Nico. Could you approve/reject this?
Ping2.
Ping3. Anyone? Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
Ugh! That patch "worked" but was wrong. It was comparing an offset into the title to raw sector numbers. Here's a correction.
Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him.
Ping Nico. Could you approve/reject this?
Ping2.
Ping3. Anyone?
It is still in my tree. If John could address Reimar's last qualms: | 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... But I think that we could probably submit the patch w/out addressing these last issues. E -- Erik Hovland erik@hovland.org http://hovland.org/
On 10/06/2011 12:08 PM, Erik Hovland wrote:
> Ugh! That patch "worked" but was wrong. It was comparing an offset into > the > title to raw sector numbers. Here's a correction. > Just a follow up to let you know that this patch is in my tree currently. If you make any modifications, please feel free to update this thread w/ a new patch.
Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him. Ping Nico. Could you approve/reject this? Ping2. Ping3. Anyone? It is still in my tree. If John could address Reimar's last qualms: | 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...
But I think that we could probably submit the patch w/out addressing these last issues.
E
I thought I had. I incorporated his suggestions and responded to the issue about admap. From my previous emal on this patch: "dvdnav_scan_admap didn't look like the right place for handling the cell boundary because it currently doesn't touch the cell information at all. In order to handle the boundary, it would have to replicate a lot of code that is already in dvdnav_sector_search." I didn't get further feedback. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
>> Ugh! That patch "worked" but was wrong. It was comparing an offset into >> the >> title to raw sector numbers. Here's a correction. >> > Just a follow up to let you know that this patch is in my tree > currently. If you make any modifications, please feel free to > update this thread w/ a new patch. > Thanks. I've done quite a bit of testing with this version and a "bad" disc I created with a dry-erase marker. It's performing solidly. Also got it into the hands of one of our users that had a few problem discs and it worked well for him. Ping Nico. Could you approve/reject this? Ping2. Ping3. Anyone? It is still in my tree. If John could address Reimar's last qualms: | 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...
But I think that we could probably submit the patch w/out addressing these last issues.
E
I thought I had. I incorporated his suggestions and responded to the issue about admap. From my previous emal on this patch:
"dvdnav_scan_admap didn't look like the right place for handling the cell boundary because it currently doesn't touch the cell information at all. In order to handle the boundary, it would have to replicate a lot of code that is already in dvdnav_sector_search."
I didn't get further feedback.
Yes, I remember that it went pretty quiet in this thread. So I still think we should push this patch (and I will do it later if someone doesn't beat me to it). E -- Erik Hovland erik@hovland.org http://hovland.org/
On Thursday, 06 October 2011 at 23:13, Erik Hovland wrote:
>>> Ugh! That patch "worked" but was wrong. It was comparing an offset into >>> the >>> title to raw sector numbers. Here's a correction. >>> >> Just a follow up to let you know that this patch is in my tree >> currently. If you make any modifications, please feel free to >> update this thread w/ a new patch. >> > Thanks. I've done quite a bit of testing with this version and a "bad" > disc I created with a dry-erase marker. It's performing solidly. Also > got it into the hands of one of our users that had a few problem discs > and it worked well for him. Ping Nico. Could you approve/reject this? Ping2. Ping3. Anyone? It is still in my tree. If John could address Reimar's last qualms: | 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...
But I think that we could probably submit the patch w/out addressing these last issues.
E
I thought I had. I incorporated his suggestions and responded to the issue about admap. From my previous emal on this patch:
"dvdnav_scan_admap didn't look like the right place for handling the cell boundary because it currently doesn't touch the cell information at all. In order to handle the boundary, it would have to replicate a lot of code that is already in dvdnav_sector_search."
I didn't get further feedback.
Yes, I remember that it went pretty quiet in this thread. So I still think we should push this patch (and I will do it later if someone doesn't beat me to it).
Agreed. Please push. I'm taking it off my list, then. Thanks and regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
Agreed. Please push. I'm taking it off my list, then.
This patch is now pushed. Thanks John for another good one. Thanks for being two years patient. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Friday, 07 October 2011 at 19:09, Erik Hovland wrote:
Agreed. Please push. I'm taking it off my list, then.
This patch is now pushed. Thanks John for another good one. Thanks for being two years patient.
Hm, I've just tested it with some of my DVDs and seeking is still broken in MPlayer. With one DVD (home-made) with this patch, backward seek by 10s actually seeks forward about 10s, but with Akira Collector's Edition DVD forward seek by 10s actually seeks backwards at some points. I can provide the ISOs if anyone is willing to take a stab at fixing this for me. Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
On Fri, Oct 7, 2011 at 5:00 PM, Dominik 'Rathann' Mierzejewski <dominik@greysector.net> wrote:
On Friday, 07 October 2011 at 19:09, Erik Hovland wrote:
Agreed. Please push. I'm taking it off my list, then.
This patch is now pushed. Thanks John for another good one. Thanks for being two years patient.
Hm, I've just tested it with some of my DVDs and seeking is still broken in MPlayer. With one DVD (home-made) with this patch, backward seek by 10s actually seeks forward about 10s, but with Akira Collector's Edition DVD forward seek by 10s actually seeks backwards at some points.
I can provide the ISOs if anyone is willing to take a stab at fixing this for me.
As long as it isn't too large, I'll take a crack. There should be logic in the function to take care of forward vs. backward seeking. This is the if (forward) chunk in the code. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Tuesday, 11 October 2011 at 00:16, Erik Hovland wrote:
On Fri, Oct 7, 2011 at 5:00 PM, Dominik 'Rathann' Mierzejewski <dominik@greysector.net> wrote:
On Friday, 07 October 2011 at 19:09, Erik Hovland wrote:
Agreed. Please push. I'm taking it off my list, then.
This patch is now pushed. Thanks John for another good one. Thanks for being two years patient.
Hm, I've just tested it with some of my DVDs and seeking is still broken in MPlayer. With one DVD (home-made) with this patch, backward seek by 10s actually seeks forward about 10s, but with Akira Collector's Edition DVD forward seek by 10s actually seeks backwards at some points.
I can provide the ISOs if anyone is willing to take a stab at fixing this for me.
As long as it isn't too large, I'll take a crack.
Hm, oddly enough I can't reproduce it anymore, so let's forget about it for now. :) Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
Improve the port to *BSD and Solaris by a little. This patch is also dropable as it has not really been tried against a recent *BSD. Signed-off-by: Erik Hovland <erik@hovland.org> --- src/dvd_reader.c | 22 +++++++++++++++------- 1 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/dvd_reader.c b/src/dvd_reader.c index f4be482..dfa695e 100644 --- a/src/dvd_reader.c +++ b/src/dvd_reader.c @@ -317,12 +317,18 @@ static char *sun_block2char( const char *path ) #if defined(SYS_BSD) /* FreeBSD /dev/(r)(a)cd0c (a is for atapi), recommended to _not_ use r + update: FreeBSD and DragonFly no longer uses the prefix so don't add it. OpenBSD /dev/rcd0c, it needs to be the raw device NetBSD /dev/rcd0[d|c|..] d for x86, c (for non x86), perhaps others Darwin /dev/rdisk0, it needs to be the raw device - BSD/OS /dev/sr0c (if not mounted) or /dev/rsr0c ('c' any letter will do) */ + BSD/OS /dev/sr0c (if not mounted) or /dev/rsr0c ('c' any letter will do) + returns a string allocated with strdup. It should be freed when no longer + used. */ static char *bsd_block2char( const char *path ) { +#if defined(__FreeBSD__) || defined(__DragonFly__) + return (char *) strdup( path ); +#else char *new_path; /* If it doesn't start with "/dev/" or does start with "/dev/r" exit */ @@ -335,6 +341,7 @@ static char *bsd_block2char( const char *path ) strcat( new_path, path + strlen( "/dev/" ) ); return new_path; +#endif /* __FreeBSD__ || __DragonFly__ */ } #endif @@ -398,17 +405,18 @@ dvd_reader_t *DVDOpen( const char *ppath ) /** * Block devices and regular files are assumed to be DVD-Video images. */ + dvd_reader_t *dvd = NULL; #if defined(__sun) - ret_val = DVDOpenImageFile( sun_block2char( path ), have_css ); + dev_name = sun_block2char( path ); #elif defined(SYS_BSD) - ret_val = DVDOpenImageFile( bsd_block2char( path ), have_css ); + dev_name = bsd_block2char( path ); #else - ret_val = DVDOpenImageFile( path, have_css ); + dev_name = strdup( path ); #endif - + dvd = DVDOpenImageFile( dev_name, have_css ); + free( dev_name ); free(path); - return ret_val; - + return dvd; } else if( S_ISDIR( fileinfo.st_mode ) ) { dvd_reader_t *auth_drive = 0; char *path_copy;
participants (7)
-
Dominik 'Rathann' Mierzejewski -
Erik Hovland -
Heinz Wiesinger -
John Stebbins -
Nico Sabbi -
Reimar Döffinger -
Reimar Döffinger