Closed vBm closed 11 years ago
Hm, I really don't know how this stuff should work.
Maybe the choice of the config dir on Windows should be a compile time option altogether.
Note that you could always just clear the HOME environment variable for mpv, then it should revert to config subdir in the directory mpv.exe is located. (Maybe use a .bat file to redirect it?)
I'm not a coder. I can only answer from user's point of view.
If config is present with mpv.exe use it. Many apps work like that. btw. isn't this sort of connected ? -> https://github.com/mpv-player/mpv/commit/c921166bf67746f19906a61b62bf6f68146d9e45
If config is present with mpv.exe use it. Many apps work like that.
Could do that. This would mean that if a config dir is present in the mpv.exe dir, the other config directory is ignored. (Or alternatively, the first config file found anywhere is used.) In my opinion it's far too easy to create complicated and confusing behavior, so I want to be careful with this.
So here are two separate suggestions:
btw. isn't this sort of connected ? -> c921166
It is, but this code was never merged.
Both suggestions make sense. Do what you think it's the best :) Thanks for your hard work.
I'm not fully understanding this, and I certainly can't reproduce it. Running mpv.exe as-is always creates the mpv config subdirectory in the folder mpv.exe resides in (so if mpv's directory is on the PATH, it still creates it wherever mpv is, not where the user is). It only changes if you're using a different shell: cmd.exe creates it in the current folder, MSys' and Cygwin's bash (and probably rxvt) create the .mpv directory in their respective /home because that's normal *nix behavior, and msysgit's version of bash creates the subdirectory in %HOME%. Not sure where PowerShell creates it, but I'd guess it does it identifically to cmd.exe.
Unless this has something to do with running mpv under a non-Admin account, in which case it should rightly be expected to generate it in %HOME%, as that's the directory the user should actually be able to control without needing UAC verification.
Or, what you're suggesting is not using a subdirectory at all, which would be breaking with how the mplayer family has handled configuration for at least as long as I've been using them (close to 10 years if memory serves). And then the original issue is not actually about where mpv is generating the config file when it doesn't find an existing one, but that it's regenerating it when you're trying to put config into a directory that it shouldn't be put in.
Nothing but misunderstandings in this thread until the last paragraph of qyot27's post. :stuck_out_tongue: I've dealt with this issue for close to 10 years myself by symlinking, but I created this account after seeing this thread and realizing the current management might actually accept a patch!
In lieu of a diff, I'll just say this goes in mp_find_user_config_file in core/path.c:
#if defined(__MINGW32__) || defined(__CYGWIN__)
char exedir[260];
/* Hack to get fonts etc. loaded outside of Cygwin environment. */
int i, imax = 0;
int len = (int)GetModuleFileNameA(NULL, exedir, 260);
for (i = 0; i < len; i++)
if (exedir[i] == '\\') {
exedir[i] = '/';
imax = i;
}
exedir[imax] = '\0';
if (filename && mp_path_exists(mp_path_join(NULL, bstr0(exedir),
bstr0(filename)))) {
homedir = exedir;
config_dir = "";
}
else
#endif
if ((homedir = getenv("MPV_HOME")) != NULL) {
config_dir = "";
} else if ((homedir = getenv("HOME")) == NULL) {
#if defined(__MINGW32__) || defined(__CYGWIN__)
homedir = exedir;
#else
return NULL;
#endif
}
This is completely untested because I've never bothered setting up a mplayer build environment. It's supposed to behave more like a typical Windows program while staying completely out of the way.
As an aside, there's nothing Windows users hate more than the fontconfig font cache! I always passed -nofontconfig with mplayer, mplayer2 didn't create the cache, but mpv did. (Although it's possible the mpv build I used was compiled against fontconfig but mplayer2 wasn't.) Based on the advice from http://comments.gmane.org/gmane.comp.video.mplayer.user/69692 I created the following fonts.conf:
<?xml version="1.0"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig/>
This works just fine, but could there possibly be a way to obviate the need for this extra file? Could someone please add an option that achieves the same effect?
I noticed in mp_ass_configure_fonts in sub/ass_mp.c there's ass_set_fonts(priv, default_font, opts->font, 1, config, 1);
. What about calling ass_set_fonts(priv, default_font, opts->font, 1, config, 0);
(don't update the cache) or even ass_set_fonts(priv, default_font, opts->font, 0, NULL, 0);
(don't use fontconfig)? I wonder if this is what the -nofontconfig switch in vanilla mplayer does. Anyway, I'd love to hear your thoughts!
In lieu of a diff, I'll just say this goes in mp_find_user_config_file in core/path.c:
OK, that patch looks like what has been suggested: if the directory with the mpv.exe has a .mpv
directory, use that, otherwise use whatever else there is. (Note that mp_find_user_config_file
can be called with an empty filename to find the config dir itself - this is not so obvious.)
If nobody complains, I can accept a patch. One obvious bug: mp_path_join()
allocates memory, which has to be free'd with talloc_free()
.
This works just fine, but could there possibly be a way to obviate the need for this extra file? Could someone please add an option that achieves the same effect?
Normally that's the responsibility of whoever packages this. We can add an example fonts.conf, though. The bad news is that mpv relies a lot on libass for sub and OSD rendering, and libass in turn relies a lot on fontconfig. Things can easily break. In the worst case, OSD as well as text subtitles (in any format) will be invisible. In some cases, the OSD symbols (like the pause icon, or the volume icon when changing volume etc.) will be incorrect. The reason is that the OSD symbols come from a special font embedded in the mplayer binaries (mpv has it in sub/osd_font.otf
).
ass_set_fonts(priv, default_font, opts->font, 1, config, 0); (don't update the cache) or even
Looking at libass, this controls whether FcConfigBuildFonts()
is called. Maybe you can check whether this actually stops fontconfig from scanning fonts, without breaking anything?
ass_set_fonts(priv, default_font, opts->font, 0, NULL, 0); (don't use fontconfig)
Then it doesn't call fontconfig at all. There's a good chance fontconfig won't be involved at all. We could probably map this to an option, but then the user needs to provide a custom subfont, and OSD will probably be broken. I suspect libass without fontconfig doesn't work that great.
I wonder if this is what the -nofontconfig switch in vanilla mplayer does.
Vanilla mplayer has its own custom (and really bad) text rendering code, using freetype and fontconfig directly. At least for OSD fontconfig will not be used (normally it's used to find a font by name). Note that unlike mplayer2 and mpv, it won't have any trouble with the OSD font, because it passes the OSD font directly to freetype, or something like this.
OK, that patch looks like what has been suggested: if the directory with the mpv.exe has a .mpv directory, use that, otherwise use whatever else there is. (Note that mp_find_user_config_file can be called with an empty filename to find the config dir itself - this is not so obvious.)
First off, thanks for replying and also catching my stupid mistake! I should have created a temp variable...
Windows programs typically check user-specific places like %APPDATA% or %HOME% for subdirectories for config files, then the application directory itself, then if nothing is found create new files in the preferred location. (If the application directory is non-writable, the result is still intended behavior.) mpv determines the appropriate location (%MPV_HOME% then %HOME%/mpv then ./mpv), then opens the config files, creating new files if necessary. The patch prefers ./ on Windows if-and-only-if the file being searched for already exists there. Again, this is standard behavior.
I will use my situation as an example to make sure we are on the same page. I have %HOME% set, so my subfont.ttf is found at %HOME%/mpv/subfont.ttf. I would like to place it in c:\foo\subfont.ttf to live next to c:\foo\mpv.exe. This patch allows that one possibility while retaining all previous behavior. mp_find_user_config_file("") works exactly as before.
Then it doesn't call fontconfig at all. There's a good chance fontconfig won't be involved at all. We could probably map this to an option, but then the user needs to provide a custom subfont, and OSD will probably be broken. I suspect libass without fontconfig doesn't work that great.
I understand that libass without fontconfig is a bad idea. It would make more sense to map ass_set_fonts(priv, default_font, opts->font, 1, config, 0); to --no-fontcache. It could always be reverted if it breaks things.
Vanilla mplayer has its own custom (and really bad) text rendering code, using freetype and fontconfig directly. At least for OSD fontconfig will not be used (normally it's used to find a font by name). Note that unlike mplayer2 and mpv, it won't have any trouble with the OSD font, because it passes the OSD font directly to freetype, or something like this.
I think I remember I specified the font path in sherpya's mplayer builds and used --no-fontconfig --no-ass with subfont.ttf in lachs0r's mplayer2 builds. Well anyway, enough about the past, thanks once again for mpv! Sooo many longstanding stupid and obvious bugs have finally been fixed. I was always the most incredulous with the end of file issues with dsound. My "favorite" that I saw was VK_F12 being typo'd as VK_F1. Who doesn't accept that patch?? :unamused:
Sorry to keep bringing stuff up, but I think this is it. When listening to mp3 albums with --gapless-audio, it definitely works, but there's still a tiny gap. Is this due to the audio output (dsound for me) not buffering enough ahead, or is this unavoidable? Is there anything that can be done in userland or maybe with set_min_out_buffer_size or something? If you're prioritizing this, it's not very important to me, but I imagine it is to some people.
Let's say that the user has no prior configuration files or subdirectories, meaning that mpv has to generate them. What will the behavior be then? IMO, it should stay the same as it is now: if no configuration is found at all, mpv generates the subdirectory and config is generated inside the subdirectory (with the location of the subdirectory itself subject to the shell issue I mentioned earlier). Even on Windows.
I'll add here that I mostly use Windows and most of my mpv usage is under Windows, but that it needs to act the same as it always has to A) not confuse anyone with new behavior and B) to keep it consistent across platforms. I simply don't believe in giving Windows preferential treatment just because of something that's not mission-critical - especially in the case of a cross-platform application, not to mention that there's plenty of examples of Windows software that prefer different styles of housekeeping in regard to their generated files. That's something that's really up to the discretion of the developers.
If the patch would - in the absence of configuration subdirs/files - suddenly start generating them in the mpv root directory, that needs to be special behavior only activated at ./configure time (something like a --win32-root-config option) and not enabled by default, even on Windows. If it merely allows mpv to accept the config file from that location but still generates it in a subdir if it can't find it at all, then I don't object, although it'd seem a little weird to accept it from mpv's root dir but not generate them there; so again, it may be desirable to keep Windows-centric behavior confined to a purely opt-in (rather than autodetected) ./configure parameter.
If it merely allows mpv to accept the config file from that location but still generates it in a subdir if it can't find it at all, then I don't object
This is exactly how it works.
I agree with most of your post, but as a UX guy I am deeply opposed to this patch being a configure-time option. I think it's already purely opt-in because the user has to move the config files there intentionally, and if anything, not being detected would be the surprising behavior.
I would like to point out that generating config files in ./mpv is already Windows-only. Other platforms only accept %MPV_HOME% and %HOME%/mpv. Also, I don't think it's strange that readable and generated locations are not identical, as this is only ever true if there is exactly one readable location. As it stands, since I have %HOME% set, ./mpv is a valid location to read config files but mpv will never generate there.
I meant that if there was additional behavior to generate the files in mpv's root directory instead of in a subdirectory, that would be better handled as a configure option, as the concern is which method would take precedence. The blurb after that about keeping it all together was more just general musing than any real objection.
Generating it in ./mpv is something I'd consider a reduced form of the *nix behavior of generating it in $HOME as a hidden directory, mainly because PATH and HOME situations on Windows were an absolute mess for a long time, and in some ways it still is. I mean, at least Vista and above actually do more faithfully make the user understand HOME² (albeit in the simplest ways by pairing it with UAC and non-Administrator accounts by default, rather than by trying to explain what an environment variable is), but PATH is just a nightmare. I sometimes wonder - whenever ReactOS gets mature enough to start having daughter distros - if one of them might abandon the Windows hierarchy and use the FHS instead. I'd laugh.
²even if said 'HOME' is really %USERPROFILE%\Documents. But that's beside the point.
Generating it in ./mpv is something I'd consider a reduced form of the *nix behavior of generating it in $HOME as a hidden directory, mainly because PATH and HOME situations on Windows were an absolute mess for a long time, and in some ways it still is.
That is in part why I gave up on the "configpath" branch. Nobody could tell me how exactly it's supposed to work, so I just left it like it was. To be fair, on Linux it isn't simple either: should it be ~/.mpv or ~/.config/mpv? Actually, the latter is incorrect too, because according to XDG you can change ~/.config to something else using an environment variable. (Now why would you allow that? It just sounds like bloat to me.)
Can anyone tell me under what circumstances $HOME is set on Windows, and where does it actually point to? There are separate functions to get the "actual" location where application config data should be stored, like SHGetFolderPathW with CSIDL_LOCAL_APPDATA.
I understand that libass without fontconfig is a bad idea. It would make more sense to map ass_set_fonts(priv, default_font, opts->font, 1, config, 0); to --no-fontcache. It could always be reverted if it breaks things.
If you can confirm that it at least inhibits font scanning, I can add it. I still think it's a pretty bad idea though. It would be better to specify a custom fontconfig configuration file that gives access to the subfont only. Then at least OSD and fonts embedded in mkv wouldn't break.
When listening to mp3 albums with --gapless-audio, it definitely works, but there's still a tiny gap. Is this due to the audio output (dsound for me) not buffering enough ahead, or is this unavoidable?
ao_dsound should have a rather large default buffer size (500 ms or so). I think you can specify the ao_dsound buffer size with --abs (in bytes). (Most AOs don't respect this option, though.) You could also try ao_portaudio. Though this one is relatively new and not battle-tested, so I don't know how well it fares.
But you really should open new issues for these things... issues are free and don't cost anything, so use them.
By the way, if you have a (fully tested) patch, I could apply it.
Can anyone tell me under what circumstances $HOME is set on Windows, and where does it actually point to? There are separate functions to get the "actual" location where application config data should be stored, like SHGetFolderPathW with CSIDL_LOCAL_APPDATA.
I'm guessing that certain programs might set %HOME%, or else it's something that the user does of their own volition. I've never personally encountered it, although there is a %HOMEPATH% variable that seems to just be an alias for %USERPROFILE% (http://ss64.com/nt/syntax-variables.html).
RE: the fontconfig talk, libass can still use fontconfig even if mpv itself doesn't, or so it would seem. The result being that the only thing that works are embedded fonts, not ones installed to the system. As it stands, the entire contents of C:\Program Files\mpv-player on my computer are: mpv.exe mpv/ mpv/config mpv/subfont.ttf
It has not once regenerated the fontconfig cache over the seven months I've been using mpv (even after I completely purged all traces of the caches generated by mplayer2), but I know it does have an internal effect on libass because it can still render the OSD and subs with embedded fonts correctly (and embedded fonts did break when 2.10.92 was used, then fixed with fontconfig-git as noted in the relevant issue this was discussed in). The further absence of fonts.conf doesn't seem to cause any issues either. So to be honest, I'm not sure under what conditions I could force it to actually use fontconfig directly, and thereby start caching fonts (which would only occur whenever the installed fonts change anyway).
To remove possible confusion: mpv itself doesn't use fontconfig, only libass does. But sicne libass is used for OSD and subtitles, it's a rather important part. libass' API allows you to specify a fonts.conf or whether to use fontconfig at all too.
I sometimes wonder - whenever ReactOS gets mature enough to start having daughter distros - if one of them might abandon the Windows hierarchy and use the FHS instead. I'd laugh.
That'd be almost as funny as GoboLinux already is!
Can anyone tell me under what circumstances $HOME is set on Windows, and where does it actually point to?
If %HOME% is set (which is what I've done), that is used. Otherwise, %HOMEDIR%%HOMEDRIVE% should be used. %HOMEDIR% is typically C: and %HOMEDRIVE% is \Users\Something that when you concatenate the two, you get a full path. These are system environmental variables and are guaranteed to exist. This concatenation is almost always the same as %USERPROFILE% but this is not strictly guaranteed. FOSS programs are typically the only ones to use %HOME%; Windows programs tend to go straight to %USERPROFILE%. It's all just a bit daft.
If you can confirm that it at least inhibits font scanning, I can add it. I still think it's a pretty bad idea though. It would be better to specify a custom fontconfig configuration file that gives access to the subfont only. Then at least OSD and fonts embedded in mkv wouldn't break.
Finally got around to testing this. It works exactly as hoped. With ass_set_fonts(priv, default_font, opts->font, 1, config, 0);, you get "[ass] FcConfigGetFonts failed". The font cache is not created or scanned, and everything seems to work as expected.
By the way, with ass_set_fonts(priv, default_font, opts->font, 0, NULL, 0);, you get "[ass] Fontconfig disabled, only default font will be used." That pretty much behaves as described -- the OSD still works though. Now, I also tried compiling without libass, and that breaks the OSD as expected.
As I said earlier, I recommend mapping the first one to an option (maybe --no-fontscan?). That way, people who never want to see the font cache can use the option all the time, while I'm pretty sure others will be able to run without the option to get a font cache, then run with it to prevent future scanning when, say, one font is added to a huge font collection (not a problem on Linux, of course). I don't see any point in allowing fontconfig to be completely disabled, but I could be wrong.
I'm afraid if I touch anything (like the .rst's), I'll say something ignorant, so if you don't mind, I'll let you handle how to tell potential users of the option that it's a Bad Idea.
ao_dsound should have a rather large default buffer size (500 ms or so). I think you can specify the ao_dsound buffer size with --abs (in bytes). (Most AOs don't respect this option, though.) You could also try ao_portaudio. Though this one is relatively new and not battle-tested, so I don't know how well it fares.
Sorry for the false alarm, I was being moronic. I tried setting --abs to some ridiculous value and it had a hilarious effect on buffering, but still the gap. Then I realized I should try lossless sources, and yeah, that was it. Now I remember why I don't care about gapless audio. We would also need some type of Winamp-style silence skipping for lossy sources.
By the way, I'm sorry for not creating new issues, but I didn't think they were worth it or would create much discussion. I'll get it right next time.
By the way, if you have a (fully tested) patch, I could apply it.
Yes, I've finally been able to fully test it, and I'm glad, because I caught a couple really dumb bugs on my part. For future reference: passing a null pointer filename to mp_find_user_config_file must be caught, but we don't sanitize it only so that we can indicate a null pointer was passed. Otherwise if (!filename) filename = ""; would be simpler. Maybe better? Passing null pointers to mp_path_exists and mp_path_isdir eventually get passed as null pointers as lpMultiByteStr in MultiByteToWideChar, which tests for that and returns 0, which is caught by mp_from_utf8, which then aborts.
Do you need me to clone and then make a pull request? Seems like overkill, here's the patch instead:
EDIT: For posterity, here's a link to the commit.
https://github.com/elevengu/mpv/commit/935378054ed376876e2b97369602078b99e9cb71
Finally got around to testing this. It works exactly as hoped. With ass_set_fonts(priv, default_font, opts->font, 1, config, 0);, you get "[ass] FcConfigGetFonts failed". The font cache is not created or scanned, and everything seems to work as expected.
I added a --no-fontconfig-scan option to the branch named fontconfig
, but on Linux it completely fails. It doesn't render any fonts... no OSD, no subs.
passing a null pointer filename to mp_find_user_config_file must be caught, but we don't sanitize it only so that we can indicate a null pointer was passed.
Calling it with NULL should normally mean that only the config directory should be returned. But I'm not sure if that still can happen.
Do you need me to clone and then make a pull request?
That would be appreciated. I just tried to copy&paste your patch and get it applied somehow, but patch as well as git-apply refused to accept it and returned stuff like "fatal: corrupt patch at line 52". The diff header is probably wrong. I concluded that pasting patch into a github issue is the single worst way to post a patch. Using git properly is so much simpler (for both you and me), and also liberates me from thinking what to put into the author field.
Posting it as a gist actually works pretty nicely (just make sure the patch is the proper output from git format-patch), even though the smoothest option is just to issue a normal pull request from a forked repository.
I'd say a wgetable format-patch is fine (maybe use http://sprunge.us/). I don't insist on pull requests, but having to copy&paste patches manually and having to fix them because the formatting is broken in obscure ways is a bit, well, not so great.
I added a --no-fontconfig-scan option to the branch named fontconfig, but on Linux it completely fails. It doesn't render any fonts... no OSD, no subs.
That's too bad. Figures it was too good to be true, or someone would have tried it before. Well, actually Linux is the one platform where you would never want to --no-fontconfig-scan anyway, so maybe it doesn't matter that much.
What are the chances this eventually gets merged into the main branch? This question is purely out of curiosity, not begging at all, as the fonts.conf solution already works for me (and qyot27 apparently doesn't even need that).
Calling it with NULL should normally mean that only the config directory should be returned. But I'm not sure if that still can happen.
Now that I know that's what should happen (not that it ever happens in the code, they pass empty string instead), I made one last adjustment.
That would be appreciated.
Done! I should have done it this way originally, but I'd never used github before, or anything other than "git clone".
I concluded that pasting patch into a github issue is the single worst way to post a patch.
Well, there are always worse ways. :stuck_out_tongue: I could have told you which lines to go to, how to edit them...
To be more specific about my particular scenario, not having fonts.conf results in mpv (or well, libass most likely, but exposed through mpv all the same) throwing messages that it can't find it and therefore to use fallback. I did, on curiosity, paste the sample fonts.conf from above into the mpv config subdirectory and it no longer gives those seemingly-harmless error messages, but neither does it cache the fonts, restricting itself to just those included as attachments in MKV files.
These were pretty much vanilla static builds of all the dependencies involved, so like I mentioned, I'm not sure. I wouldn't be surprised if it's actually a routine in fonts.conf that might trigger the caching, as the copy of fonts.conf from a mencoder build I have does have system font directory info in it. I'll have to play around and see if I can get it to cache them, because like I said, even though embedded fonts in MKV files work, ones called from the system do not.
So, this should be fixed with b2c2fe7a3782c.
As for the fontconfig issue: well, from what lachs0r said, --no-fontconfig-scan will usually do the same thing as on Linux: break fonts completely. lachs0r has a mpv build with a specific fonts.conf that avoids scanning all system fonts here: http://mpv.srsfckn.biz/mpv-i686-latest.7z
Thanks, wm4!
I did, on curiosity, paste the sample fonts.conf from above into the mpv config subdirectory and it no longer gives those seemingly-harmless error messages, but neither does it cache the fonts, restricting itself to just those included as attachments in MKV files.
Yes, I wanted to have the same functionality as you have (except you don't need fonts.conf for whatever reason): OSD works, embedded fonts work, subfont.ttf works, no fontcache (so no system fonts). If you want the system fonts back, you should put <dir>WINDOWSFONTDIR</dir>
inside <fontconfig>
in fonts.conf.
As for the fontconfig issue: well, from what lachs0r said, --no-fontconfig-scan will usually do the same thing as on Linux: break fonts completely. lachs0r has a mpv build with a specific fonts.conf that avoids scanning all system fonts
Well, it worked just fine for me in testing, but I'm not going to argue the point. Your suggestion on the mailing list (what lachs0r has done with CUSTOMFONTDIR) is surely the most sensible for a packager to provide by default. I only want one user font, so I use a dummy fonts.conf, but others might want a small collection of fonts.
Back to the original topic: I know you probably already know this, but for completeness, SHGetFolderPath (the "proper" way to do things instead of checking %APPDATA% or %USERPROFILE% or %HOME%) has been deprecated and wraps to SHGetKnownFolderPath since Vista. Forgot to mention that earlier.
Yes, I wanted to have the same functionality as you have (except you don't need fonts.conf for whatever reason): OSD works, embedded fonts work, subfont.ttf works, no fontcache (so no system fonts). If you want the system fonts back, you should put
WINDOWSFONTDIR insidein fonts.conf.
With -v -v -v it shows it getting stuck at [ass] Setting up fonts... and then outputs no more messages
As you can see in ass_mp.c, it must be stuck in ass_set_fonts(), so it hangs inside libass or (more likely) fontconfig. Would require more investigation.
Then it may be due to some of the compilation toolchain stuff. This is already well off topic, so if I can narrow it down (and it's not something as simple as a toolchain issue), then I'll open a separate issue for it.
It would be quite nice if mpv would use
config
if it was inside the same folder asmpv.exe
At the moment if you move
config
to the same dir wherempv.exe
is located, empty config gets recreated at%home%\mpv\