[PATCH] "dvdnav_jump_to_sector" as an alternative to "dvdnav_time_search" (REV 1: handle delay when reading directly off disc)
Revision to handle delay when reading directly off disc. See related email for more info
Revision to handle delay when reading directly off disc. See related email for more info
This patch has at least a few faults that will keep it from being accepted. 1. None of the internal functions are declared static to searching.c 2. There are tabs and extra spaces in the new code in the new patch. 3. RESULT_TRUE and RESULT_FALSE are obvious in C and shouldn't be used - try 0 and 1. 4. is_null() is a helper function that has little utility beyond: if (something_ptr == NULL) return 0; Remove it and use the C idiom. 5. Almost none of the new code adheres to the style of the current file. 6. The #defines added to searching.c should be in dvdnav_internal.h and they should be enums. 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. 8. The new code adds floating point. The file does not currently have any floating point code in it. This isn't a showstopper. But it is something to consider. 9. The patch comes in two pieces. It should span 3 files and be just one patch. If you want to have more then one patch, then you should layer the functionality. 10. It is OK to declare variables inside the function. But the rest of the file uses the idiom of declaring the function scope variables at the top of each function. Once the patch embodies at least these points we can start to discuss this patch on its merits. Thanks for the patch. I appreciate what you are trying to do w/ it. E -- Erik Hovland erik@hovland.org http://hovland.org/
Revision to handle delay when reading directly off disc.
I noticed the following with a particular DVD: ERR:cell not foundA-V: -0.200 ct: 0.047 18/ 18 13% 13% 32.8% 0 0 libdvdnav: get_PGCN failed. Was trying to find pgcN in domain 2 libdvdnav: chapter NOT FOUND! libdvdnav: get_PGCN failed. Was trying to find pgcN in domain 20 0 libdvdnav: chapter NOT FOUND! https://gist.github.com/1365954 I guess this is expected for certain DVD's? I'd be happy to send an exemplar or what not. -roger-
It seems about as fast now. Sweet!
Thanks! Glad it worked.
I noticed the following with a particular DVD: ERR:cell not foundA-V: -0.200 ct: 0.047 18/ 18 13% 13% 32.8% 0 0
I'm not sure, but I'm going to guess there is some sort of copy-protection issue with the DVD. There are errors early on in the stack trace (excerpted below) -- before it invokes the jump_to_time.
From what I can tell, vm.c is failing to find the PGC in the VTS_DOMAIN. The "cell not found" error occurs when trying to find a cell in the PGC. If dvdnav couldn't find a PGC before, then the cell would definitely not be found either.
If this is a commercial US DVD, I can hunt down a copy in my libraries. Alternatively, you can post the binary IFO files. They're generally shorter (< 100 KB), and I can investigate further. The files would be VIDEO_TS.IFO and VTS_01_0.IFO (based on the error trace). Hope this helps Excerpt: Starting playback... libdvdnav: get_PGCN failed. Was trying to find pgcN in domain 2 libdvdnav: chapter NOT FOUND!
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 again.
On Mon, Nov 14, 2011 at 1:18 PM, Erik Hovland <erik@hovland.org> wrote:
>> Revision to handle delay when reading directly off disc.
>> See related email for more info
>
> This patch has at least a few faults that will keep it from
> being accepted.
>
> 1. None of the internal functions are declared static to
> searching.c
Ok. Declared all internal functions static.
> 2. There are tabs and extra spaces in the new code in the
> new patch.
Ok. Removed all tabs. Removed all extra spaces at end of line.
> 3. RESULT_TRUE and RESULT_FALSE are obvious in
> C and shouldn't be used - try 0 and 1.
Ok. Changed to 0 and 1
> 4. is_null() is a helper function that has little utility beyond:
> if (something_ptr == NULL) return 0;
> Remove it and use the C idiom.
Ok. Removed is_null() and adopted guideline.
> 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.
> 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.
> 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....
> 8. The new code adds floating point. The file does not currently
> have any floating point code in it. This isn't a showstopper. But
> it is something to consider.
Okay. I changed these to integer types, and use multiply/divide by
1000 to get the same results. 1000 should be sufficient for precision.
> 9. The patch comes in two pieces. It should span 3 files and be
> just one patch. If you want to have more then one patch, then
> you should layer the functionality.
Okay. I combined to one patch.
> 10. It is OK to declare variables inside the function. But the rest of the
> file uses the idiom of declaring the function scope variables at the
> top of each function.
Okay. I moved all the variable declarations to the top of the function
> Once the patch embodies at least these points we can start to discuss
> this patch on its merits.
>
> Thanks for the patch. I appreciate what you are trying to do w/ it.
>
> E
> --
> Erik Hovland
> erik@hovland.org
> http://hovland.org/
> _______________________________________________
> DVDnav-discuss mailing list
> DVDnav-discuss@mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
> 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. >> 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. >> 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. >> 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. Thanks again. E -- Erik Hovland erik@hovland.org http://hovland.org/
>> 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@hovland.org
> http://hovland.org/
> _______________________________________________
> DVDnav-discuss mailing list
> DVDnav-discuss@mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
>
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.
Patch seems to apply fine and work as well as the previous. Also my one "failure" DVD still seeks fine/accurately, I was just wondering if the error messages were expected or not. -roger-
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.
enums named SECTOR, TIME and INDEX are too general. Feel free to append them w/ CELL_FIND_. E -- Erik Hovland erik@hovland.org http://hovland.org/
enums named SECTOR, TIME and INDEX are too general. Feel free to append them w/ CELL_FIND_.
Okay. I have changed above to CELL_FIND_SECTOR, CELL_FIND_TIME and CELL_FIND_INDEX for revision 3. Thanks.
I was just going over the new patch and saw this code: + /* HACK: ifo->vts_tmapt is NULL + * get ifo again by calling ifoOpen + * (ifoOpen will return an ifo with a timeMap) + * cache this ifo by overwriting this->vm->vtsi */ + if (tmapt == NULL) { + ifo = ifoOpen(this->vm->dvd, vts_idx); + this->vm->vtsi = ifo; + tmapt = ifo->vts_tmapt; + if (tmapt == NULL) return NULL; + } This is probably why there is a #include <dvdread/ifo_read.h>. Any reason why you feel you have to re-open the IFO? It seems like overkill or that you are workaround a bug elsewhere because vts_tmapt is NULL. E -- Erik Hovland erik@hovland.org http://hovland.org/
I was just going over the new patch and saw this code: + /* HACK: ifo->vts_tmapt is NULL + * get ifo again by calling ifoOpen + * (ifoOpen will return an ifo with a timeMap) + * cache this ifo by overwriting this->vm->vtsi */ + if (tmapt == NULL) { + ifo = ifoOpen(this->vm->dvd, vts_idx); + this->vm->vtsi = ifo; + tmapt = ifo->vts_tmapt; + if (tmapt == NULL) return NULL; + }
This is probably why there is a #include <dvdread/ifo_read.h>. Yup. Exactly. :)
Any reason why you feel you have to re-open the IFO? It seems like overkill or that you are workaround a bug elsewhere because vts_tmapt is NULL. I admit it is overkill. I only did it because the existing ifo's time map reference is null. Specifically "this->vm->vtsi->vts_tmapt" was null. In contrast the admap pointer was not null: "this->vm->vtsi->menu_vobu_admap".
I didn't know why tmap was null but I discovered if I call "ifo = ifoOpen(this->vm->dvd, vts_idx);" I get back an ifo with a time map reference that was valid. I admit this was kludgy. I put a note in the code, but I should probably have highlighted it better. Please let me know if there's another way I can get back a valid time map reference without having to reopen the ifo file. I'd be happy to change it. As always, thanks.
E
-- Erik Hovland erik@hovland.org http://hovland.org/ _______________________________________________ DVDnav-discuss mailing list DVDnav-discuss@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
On Wed, Nov 16, 2011 at 9:11 PM, gnosygnu <gnosygnu@gmail.com> wrote:
I was just going over the new patch and saw this code: + /* HACK: ifo->vts_tmapt is NULL + * get ifo again by calling ifoOpen + * (ifoOpen will return an ifo with a timeMap) + * cache this ifo by overwriting this->vm->vtsi */ + if (tmapt == NULL) { + ifo = ifoOpen(this->vm->dvd, vts_idx); + this->vm->vtsi = ifo; + tmapt = ifo->vts_tmapt; + if (tmapt == NULL) return NULL; + }
This is probably why there is a #include <dvdread/ifo_read.h>. Yup. Exactly. :)
Any reason why you feel you have to re-open the IFO? It seems like overkill or that you are workaround a bug elsewhere because vts_tmapt is NULL. I admit it is overkill. I only did it because the existing ifo's time map reference is null. Specifically "this->vm->vtsi->vts_tmapt" was null. In contrast the admap pointer was not null: "this->vm->vtsi->menu_vobu_admap".
I didn't know why tmap was null but I discovered if I call "ifo = ifoOpen(this->vm->dvd, vts_idx);" I get back an ifo with a time map reference that was valid.
I admit this was kludgy. I put a note in the code, but I should probably have highlighted it better. Please let me know if there's another way I can get back a valid time map reference without having to reopen the ifo file. I'd be happy to change it.
What about using the already existing ifo handle to re-read the tmap? int ifoRead_VTS_TMAPT(ifo_handle_t *); I am still concerned that the vts_tmapt is not already set up. E -- Erik Hovland erik@hovland.org http://hovland.org/
What about using the already existing ifo handle to re-read the tmap? int ifoRead_VTS_TMAPT(ifo_handle_t *);
Thanks. I should have found that myself. I've changed the code accordingly for revision 4.
I am still concerned that the vts_tmapt is not already set up.
I think the issue is in vm.c. From what I can tell, ifoOpenNewVTSI and vm_reset are not calling "ifoRead_VTS_TMAPT". In contrast, both procs call "ifoRead_VOBU_ADMAP". If you want, I can submit a separate patch for this. I think there would not be any adverse consequences of such a patch, aside from the miniscule performance hit in loading the tmap. Admittedly though, my familiarity with the vm.c is very little. Also, on a separate note, I'd like to hold off on combining the parameters into a struct. The changes are straightforward, but they involve a lot of lines, and I'd like not to have such a big moving target (i.e: keep submitting revisions that change lines upon lines). Let me know if you feel strongly about combining the parameters, and I will reprioritize. Thanks.
participants (3)
-
Erik Hovland -
gnosygnu -
Roger Pack