Cannot select DVD title

[bergfried]

I cannot select the DVD title using dvd://TITLE as it will always switch to the same title for the same DVD, eg:

$ mpv --dvd-device "path/to/dvd" dvd://9
Playing: dvd://9
libdvdnav: Using dvdnav version 5.0.3
[...]
[dvdnav] Selecting title 3.
[dvdnav] DVDNAV, switched to title: 4

It does not seem to matter whether or not path/to/dvd points to an actual DVD device. Using mpv 0.21.

[kevmitch]

does the same thing happen with

$ mpv --dvd-device "path/to/dvd" dvdread://9

[bergfried]

No, dvdread://TITLE seems to work fine. Besides this, it seems that dvd://TITLE always selects the same title as if no TITLE were given in the first place, i.e. it works the same as dvd://. (Note that the default titles for dvd:// and dvdread:// are different, though.)

[kevmitch]

Yeah, dvd:// is a bit of a mess. It's using dvdnav which is meant to be used for menus, except mpv doesn't do menus. The dvdread:// codepath is quite a bit nastier, but it actually works correctly. I'm pretty sure dvdread:// selects the longest title if none is specified.

[OceanWolf]

Do you have plans to implement menus in the future to solve this issue? Just learnt about mpv having used mplayer for years and glad to see many bugs fixed and a much improved experience, apart from this regression over title, and the disappearance of a dvd menu.

P.S. the menu did appear after the track finished, but I could not navigate within the menu, neither with the mouse, nor with the arrow keys which just scrubs the video playing on the menu screen.

[kevmitch]

There was menu support for a while, but it was removed because its code was not self contained (it involved hacks in core player code) and there isn't anyone who both understands it and is motivated to put in the substantial work required to make it work correctly. The same goes for dvd support in general. It is essentially unmaintained. It was even removed for a few days or a week before being restored to quell the resulting angry mob.

[ghost]

Menu support itself was also first removed, then added differently, then removed again.

[OceanWolf]

Ahh, okay, having refactored large code-bases before, I figured it would go on the todo list of things to re-implement later once the main parts were complete (the refactor and comment out the parts that don't play nice with the new refactor to come back to at another time approach), but that doesn't seem the case.

I would volunteer to take a look at the code base myself, but I have a general aversion to C (mainly because the code looks ugly to read, perhaps not a problem here), and I lack the time right now. If I find the time I will take a look.

[wiiaboo]

the refactor and comment out the parts that don't play nice with the new refactor to come back to at another time approach

No need for dead code with git, just look for the commit before it was removed and git checkout to it.

[OceanWolf]

Well the next step, yes, comment out until it works, then remove and commit :)

[micrococo]

Hi.

I have used git bisect to find the commit that introduced this bug. It is the first time I have used this tool, so I hope I didn't make a mistake.

5e30e7a04125e3c503160a76bbfe9361bff561fd is the first bad commit
commit 5e30e7a04125e3c503160a76bbfe9361bff561fd
Author: wm4 <wm4@nowhere>
Date:   Thu Sep 8 21:46:48 2016 +0200

    stream_dvd, stream_dvdnav: remove weird option parsing stuff

    Same deal as with stream_bluray.

    Untested because I don't give a fuck about your shitty DVDs.

:040000 040000 d26d8b974b587e1d7c9b558a02d9f903c1a04ff4 df77352f8456d0ab63c07ced462fdccb7c400ba5 M      options
:040000 040000 7cf857363eaf6b246c07789c1519bf5fe84c50b6 4657a64dbe48812404eca4e5f7affd338ceca543 M      player
:040000 040000 bebea11dfe711688522b9722bbd3e1ca02ceb2f9 5b5f5686bb20c6f41fa01bde437c5484b3c6c7d1 M      stream

Greetings.

[ghost]

Probably just some values unintentionally overwritten somewhere, or so.

[OceanWolf]

Urgh, I have just taken a look at that commit to see if I could figure something out, and it all looks pretty terse to me, many levels of defines creating spaghetti code, so I haven't got very far yet with reading it.

What did jump out at me though was the different ordering of options within dvd_conf . opts

const struct m_sub_options dvd_conf =
{
    .opts = (const struct m_option[])
    {
        OPT_STRING("dvd-device", device, M_OPT_FILE),
        OPT_INT("dvd-speed", speed, 0),
        OPT_INTRANGE("dvd-angle", angle, 0, 1, 99),
        {0}
    },
    ...

and dvd_opts

struct dvd_opts
{
    int angle;
    int speed;
    char *device;
};

At this moment I doubt that causes anything other than my triggering my OCD, but I still need to read through the code a bit more to see how the two structs interact to make sure.

[ghost]

That's the option parser. It's pretty magic code. What you need to know is that this puts the global options (e.g. --dvd-angle) into the according struct fields. mp_get_config_group() returns a snapshot of the current values of those global options.

[OceanWolf]

Yes, I figured out that it was meant to do that through help of the trusty man page and some deductive reasoning. The problem comes with double checking that it actually does do that. @micrococo said that this commit broke these options, so assuming that, it comes down to rooting through this commit.

When I get some time I will figure out how to build mpv and begin prodding, which will no doubt involve some documentation of this magic. I go by the general rule of thumb, the more magical the code, the more documentation it requires so that future people don't have to get lost in magical code forests.

Did you reverse the order of the options for a particular reason? And the {0}? That does not look like an m_option to me...

[ghost]

The commit tries to merge global options and "URL" options (parameters like dvd://123), which might have gone wrong somewhere.

The order of the m_option or struct entries doesn't matter. The `{0}' sets all fields of the m_option struct to 0. Its function is to terminate the m_option array.

[wiiaboo]

7fe7583a7f56626a57ea181bd213064374738c27 probably fixed this.

[OceanWolf]

Ahh, thanks @wiiaboo, I look forward to testing this out in the next release.