[MPlayer-dev-eng] [PATCH] Add more ID_DVD_* style messages to dvdnav://

[Mike Castle]

On Tue, Feb 15, 2011 at 11:50 AM, Reimar Döffinger
<Reimar.Doeffinger at 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, &current, &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, &current, &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