[PATCH] Add more ID_DVD_* style messages to dvdnav://
I have some questions about this proposed patch that I'd like to see discussed. As I mentioned in an earlier post, the problem I'm trying to solve is that, as part of copy protection schemes, some discs are purposefully pressed to have bad sectors and libdvdread will simply lock up processing them, whereas libdvdnav is more successful. I guess I should clarify that, since dvdnav is built on top of dvdread: it does work, it's just that some calls into libdvdread invokes this problem. The way I approached this patch was to get the output of the following commands as nearly identical as I could: mplayer -identify -frames 0 -ao null -vo null dvd:// dvd://1 dvd://2 2> /dev/null | grep ^ID_ | sort -u > dvd.txt mplayer -identify -frames 0 -ao null -vo null dvdnav:// dvdnav://1 dvdnav://2 2> /dev/null | grep ^ID_ | sort -u > dvdnav.txt This patch is pretty good at achieving that goal, but there are some differences. First, dvd: gives us ID_DVD_DISC_ID=. However, that comes from dvd_reader API DVDDiscID(), and I could find no corresponding API in dvdnav. And since it is built up of items extracted from DVD meta information and that the whole problem I'm having is that locking up, I've decided not to try to provide that here. Second, when using -identify, stream_dvd.c will loop over all of the tracks by default, where as in stream_dvdnav.c, it ONLY does that on track ``0'' (i.e., when using dvdnav:// ). That is why I aggregated the results of {://, ://1, ://2} to get the output data. I figure a tool that uses mplayer to extract metadata would do something like: num_tracks=$(mplayer -identify dvdnav:// | grep ID_DVD_TITLES | sed s/ID_DVD_TITLES=//) tracks=$(printf 'dvdnav://%d ' $(seq 1 $num_tracks)) # dvdnav://1 dvdnav://2 ... dvdnav://$num_tracks random_output=$(mplayer -identify $tracks) not as great as just one call to mplayer but it'll work, I think. Thirdly, I noticed that the preexisting ID_DVD_TITLE_NN_LENGTH is _way_ off. dvd: and dvdnav: use two different ways to calculate the numbers, and this may demonstrate a bug in dvdnav. Some examples follow (first line dvd:, second dvdnav:) -ID_DVD_TITLE_10_LENGTH=32.000 +ID_DVD_TITLE_10_LENGTH=27.000 -ID_DVD_TITLE_35_LENGTH=60.033 +ID_DVD_TITLE_35_LENGTH=545.466 -ID_DVD_TITLE_36_LENGTH=244.800 +ID_DVD_TITLE_36_LENGTH=1113.133 Now, in another location, both dvd: and dvdnav: also dump ID_LENGTH which is supposed to contain the same information as ID_DVD_TITLE_NN_LENGTH, and it is correct in both locations. As noted, this is an existing issue, just one I noticed while doing this. I think on some discs, the chapter information from dvdnav is completely out of whack while the from dvdread is correct. Next, some information is now duplicated. For example, in the raw output we'll now see something like: Title: 'THE_THREE_MUSKETEERS' ID_DVD_VOLUME_ID=THE_THREE_MUSKETEERS I think this is OK. After all, some scripts will parse the old format, but I think having both will make it easier to port scripts from using dvd:// to dvdnav:// Next, since in order to dump the ANGLE information, we must actively loop over all of the titles, we have to save/restore the initial state. I don't know if it's safe to assume it will always be in title==0 (i.e, menu) when identify() is called in stream_dvdnav.c, so we have the little if/else there at the end. If we know it will always be in a menu that could be simplified. Also, for some reason, probably a misunderstanding of the API on my part, I have to call dvdnav_menu_call() twice. Reading the effects of this after the first call, the part is 0, then the second call sets the part to 3. Finally, in looking at all of this, I noticed that the for-loop in identify() had a fencepost error. If preferred, I can break that out into a separate patch. Sorry if this is a bit rambling, several interruptions while drafting it (toddler in the house). mrc
On Sun, 13 Feb 2011 14:30:43 -0800, Mike Castle wrote:
Finally, in looking at all of this, I noticed that the for-loop in identify() had a fencepost error. If preferred, I can break that out into a separate patch.
yes, seperate patches are preferred for bugs/cosmetics/features. patch sounds good (i am no coder). i assume you've seen lsdvd ? i heard it uses libdvdread, so maybe its not useful in your case. maybe it should be updated to use libdvdnav. -compn
On Sun, Feb 13, 2011 at 5:27 PM, compn <tempn@twmi.rr.com> wrote:
patch sounds good (i am no coder). i assume you've seen lsdvd ? i heard it uses libdvdread, so maybe its not useful in your case. maybe it should be updated to use libdvdnav.
Yes. My current process depends on lsdvd and lsdvd is the first tool that hung. Reworking lsdvd to use libdvdnav would require a major rewrite and when I recently contacted the author, he stated he was no long interested in maintaining it and suggested I take over. Instead I decided to investigate using mplayer -identify. mrc
On Sun, Feb 13, 2011 at 02:30:43PM -0800, Mike Castle wrote:
@@ -135,6 +135,9 @@ /* report the title?! */ if (dvdnav_get_title_string(priv->dvdnav,&title_str)==DVDNAV_STATUS_OK) { mp_msg(MSGT_IDENTIFY, MSGL_INFO,"Title: '%s'\n",title_str); + if (strlen(title_str)) { + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_VOLUME_ID=%s\n", title_str); + }
IMO please do this without the if, if the printout can sometimes not happen at all this makes it more difficult to write a program that reliably (e.g. without hanging) parses this info out. And the previous identify message was really bad, I think it would be better to remove it.
@@ -509,6 +512,7 @@ if(parts) { t = duration / 90; mp_msg(MSGT_IDENTIFY, MSGL_V, "ID_DVD_TITLE_%d_LENGTH=%d.%03d\n", title, t / 1000, t % 1000); + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_CHAPTERS=%d\n", title, n); mp_msg(MSGT_IDENTIFY, MSGL_INFO, "TITLE %u, CHAPTERS: ", title); for(i=0; i<n; i++) { t = parts[i] / 90000;
It would be best if you could send this, the previous and the ID_DVD_CURRENT_TITLE=%d chunk and just adding ID_DVD_TITLES=%d without the other changes as a separate patch, since those are so trivial they can be applied really quickly. The other part will probably need some more time from me to review.
@@ -519,16 +523,48 @@ } }
+static void identify_angles(dvdnav_t *nav, uint32_t title) +{ + uint32_t current, total; + + if (dvdnav_get_angle_info(nav, ¤t, &total) == DVDNAV_STATUS_OK) { + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_ANGLES=%d\n", + title, total); + } else { + mp_msg(MSGT_IDENTIFY, MSGL_ERR, + "Unable to get angle information for title %d.\n", title); + } +}
The error messages is rather bad to parse. My suggestion would be to either print nothing at all or (probably better) use a nonsense value like -1. I.e. int32_t current, total; if (dvdnav_get_angle_info(nav, ¤t, &total) != DVDNAV_STATUS_OK) total = -1; mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_ANGLES=%d\n", title, total); Note that dvdnav_get_angle_info actually does take int32_t, not uint32_t.
static void identify(dvdnav_priv_t *priv, struct stream_priv_s *p) { uint32_t titles=0, i; if(p->track <= 0) { + uint32_t title = 0; + uint32_t part = 0; + /* Save current position. */ + dvdnav_current_title_info(priv->dvdnav, &title, &part); + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "title=%d part=%d\n", title, part);
I do not like the formatting of that.
dvdnav_get_number_of_titles(priv->dvdnav, &titles); - for(i=0; i<titles; i++) + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLES=%d\n", titles); + for(i=1; i<=titles; i++) { identify_chapters(priv->dvdnav, i); + /* XXX: angle only work for current track, so switch */
work_s_ And I'd be in favour of fixing libdvdnav to support that kind of thing, at least assuming it's not a huge issue.
+ /* Restore saved position. */ + if (title) { + dvdnav_part_play(priv->dvdnav, title, part); + } else { + /* Have to do this twice for some reason. */ + dvdnav_menu_call(priv->dvdnav, part); + dvdnav_menu_call(priv->dvdnav, part);
That looks bad, we definitely should not do such hacks without fully understanding them. Also since a DVD has multiple menus.
On Tue, Feb 15, 2011 at 11:50 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On Sun, Feb 13, 2011 at 02:30:43PM -0800, Mike Castle wrote:
@@ -135,6 +135,9 @@ /* report the title?! */ if (dvdnav_get_title_string(priv->dvdnav,&title_str)==DVDNAV_STATUS_OK) { mp_msg(MSGT_IDENTIFY, MSGL_INFO,"Title: '%s'\n",title_str); + if (strlen(title_str)) { + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_VOLUME_ID=%s\n", title_str); + }
IMO please do this without the if, if the printout can sometimes not happen at all this makes it more difficult to write a program that reliably (e.g. without hanging) parses this info out.
For this case, I was exactly duplicating the experience from stream_dvd.c. Should that one be changed as well? I disagree with the statement about parsing. For example, in Python I would do something like this: title = 'unknown' for line in mplayer_stdout.readlines(): if line.startswith('ID_DVD_VOLUME_ID='): title = line.split('=')[1] print 'Title is:', title With your proposal I'd have to do: title = 'unknown' for line in mplayer_stdout.readlines(): if line.startswith('ID_DVD_VOLUME_ID='): sp = line.split('=') if len(sp) == 2: title = sp[1] print 'Title is:', title
And the previous identify message was really bad, I think it would be better to remove it.
Will do. I was nervous about breaking existing scripts right away, and was thinking about some sort of transition period.
@@ -509,6 +512,7 @@ if(parts) { t = duration / 90; mp_msg(MSGT_IDENTIFY, MSGL_V, "ID_DVD_TITLE_%d_LENGTH=%d.%03d\n", title, t / 1000, t % 1000); + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_CHAPTERS=%d\n", title, n); mp_msg(MSGT_IDENTIFY, MSGL_INFO, "TITLE %u, CHAPTERS: ", title); for(i=0; i<n; i++) { t = parts[i] / 90000;
It would be best if you could send this, the previous and the ID_DVD_CURRENT_TITLE=%d chunk and just adding ID_DVD_TITLES=%d without the other changes as a separate patch, since those are so trivial they can be applied really quickly. The other part will probably need some more time from me to review.
No problem. May be an evening or two.
@@ -519,16 +523,48 @@ } }
+static void identify_angles(dvdnav_t *nav, uint32_t title) +{ + uint32_t current, total; + + if (dvdnav_get_angle_info(nav, ¤t, &total) == DVDNAV_STATUS_OK) { + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_ANGLES=%d\n", + title, total); + } else { + mp_msg(MSGT_IDENTIFY, MSGL_ERR, + "Unable to get angle information for title %d.\n", title); + } +}
The error messages is rather bad to parse. My suggestion would be to either print nothing at all or (probably better) use a nonsense value like -1. I.e. int32_t current, total; if (dvdnav_get_angle_info(nav, ¤t, &total) != DVDNAV_STATUS_OK) total = -1; mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLE_%d_ANGLES=%d\n", title, total);
OK.
Note that dvdnav_get_angle_info actually does take int32_t, not uint32_t.
I was following the examples provided by surrounding code which passes in uint32_t to an API that takes int32_t. Would you like subsequent patches which updates those calls?
static void identify(dvdnav_priv_t *priv, struct stream_priv_s *p) { uint32_t titles=0, i; if(p->track <= 0) { + uint32_t title = 0; + uint32_t part = 0; + /* Save current position. */ + dvdnav_current_title_info(priv->dvdnav, &title, &part); + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "title=%d part=%d\n", title, part);
I do not like the formatting of that.
dvdnav_get_number_of_titles(priv->dvdnav, &titles); - for(i=0; i<titles; i++) + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_DVD_TITLES=%d\n", titles); + for(i=1; i<=titles; i++) { identify_chapters(priv->dvdnav, i); + /* XXX: angle only work for current track, so switch */
work_s_
Doh!
And I'd be in favour of fixing libdvdnav to support that kind of thing, at least assuming it's not a huge issue.
I had wondered if this was why ANGLE information wasn't provided before. This little bit of mplayer I felt comfortable jumping into. libdvdnav scares me a little. I should see what libdvdread is doing when it extracts angle information. I don't know if has to change position or not. Either way, a discussion for a different list.
+ /* Restore saved position. */ + if (title) { + dvdnav_part_play(priv->dvdnav, title, part); + } else { + /* Have to do this twice for some reason. */ + dvdnav_menu_call(priv->dvdnav, part); + dvdnav_menu_call(priv->dvdnav, part);
That looks bad, we definitely should not do such hacks without fully understanding them. Also since a DVD has multiple menus.
Yeah. I was going to try to delve into dvdnav for this. I assume the ``part'' parameter what handles the multiple menus. On the DVD I was testing, part was always == 3. I should test on more discs. But I can certainly understand the feeling here. Could be that libdvdnav is in some funny state at this point and that could come back to bite us in the future. mrc
On Tue, Feb 15, 2011 at 11:50 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
+ /* Save current position. */ + dvdnav_current_title_info(priv->dvdnav, &title, &part); + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "title=%d part=%d\n", title, part);
I do not like the formatting of that.
Mostly a note for myself here: that mp_msg() was a left over debug message I forgot to remove. mrc
participants (3)
-
compn -
Mike Castle -
Reimar Döffinger