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

[Reimar Döffinger]

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, &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);


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.