Hello, On Tue, Jun 26, 2007 at 09:19:32PM -0400, The Wanderer wrote:

Reimar Döffinger wrote:

...

Please test and comment attached patch.

On limited testing: it does as described for me, but with the unfortunate side effect of removing the ability to have the previous behavior if desired. Make this an option (preferably disabled by default, though that's not absolutely required), and unless more negatives show up it has my approval for the little that's worth.

I don't believe the old behaviour to be desirable enough to keep. Note that per-file -fs/-nofs still works as long as you don't change the setting during playback. If anything, I'd advocate a "reset display" command that is also useful if you accidentially resize the video. Greetings, Reimar Doeffinger

Hello, On Thu, Jun 28, 2007 at 12:16:25AM +0800, Stanley wrote: [...]

if (i >= pos1 && i < pos2) { *dest = level_hi * (*src); } else if (i >= pos2 && i < pos3) { float level = (float)(pos3 - i) / (pos3 -pos2) * (level_hi - level_low) + level_low; *dest = level * *src; } else { *dest = *src; } } else { if (i >= pos1 && i < pos2) { float level = (float)(i - pos1) / (pos2 -pos1) * (level_hi - level_low) + level_low; *dest = level * *src; } else if (i >= pos2 && i < pos3) { *dest = level_hi * *src; } else { *dest = *src; } }

This is horribly inefficient, you are doing two floating point multiplications and one division per pixel. You should do something like this IMO: unsigned delta = (level_hi - level_low)/(pos2 - pos1) * (1 << 24); unsigned level_low_int = level_low * (1 << 24); in config, then in the loop: unsigned level = level_low_int; *dest = (level * *src) >> 24; level += delta; Should reduce it to one multiply, one shift and one add and all integer. The calculations may be one-off, please check yourself (though it should hardly be visible anyway). Doing the comparisions in the inner loop is also less than optimal, you could process the four different areas one after the other, thus also taking advantage of the accelerated memcpy function for the copying. [...]

static int config(struct vf_instance_s* vf, int width, int height, int d_width, int d_height, unsigned int flags, unsigned int outfmt){

// convert percentage to pixel vf->priv->pos1 = (int)(vf->priv->pos1 / 100 * width); vf->priv->pos2 = (int)(vf->priv->pos2 / 100 * width); vf->priv->pos3 = (int)(vf->priv->pos3 / 100 * width);

This will break horribly when config is called more than once, which I think might easily happen. At least there is no reason not to just add a few ints to the struct to hold the values in pixel.

#define ST_OFF(f) M_ST_OFF(struct vf_priv_s,f) static m_option_t vf_opts_fields[] = { {"level_hi", ST_OFF(level_hi), CONF_TYPE_FLOAT, 0, 0 ,0, NULL}, {"level_low", ST_OFF(level_low), CONF_TYPE_FLOAT, 0, 0 ,0, NULL}, {"pos1", ST_OFF(pos1), CONF_TYPE_FLOAT, 0, 0 ,0, NULL}, {"pos2", ST_OFF(pos2), CONF_TYPE_FLOAT, 0, 0 ,0, NULL}, {"pos3", ST_OFF(pos3), CONF_TYPE_FLOAT, 0, 0 ,0, NULL},

Since all these have a valid range you should specify it.

{"direction", ST_OFF(direction), CONF_TYPE_INT, 0, 0 ,0, NULL},

And this allows only for two values for all I can see, specifying horizontal or vertical. So make it a flag and name it e.g. horizontal so everyone can see which value is which direction without having to look at the manual. Greetings, Reimar Doeffinger

2007/6/27, Reimar Doeffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:

Hello, On Tue, Jun 26, 2007 at 09:19:32PM -0400, The Wanderer wrote:

...

Reimar Döffinger wrote:

...

Please test and comment attached patch.

On limited testing: it does as described for me, but with the unfortunate side effect of removing the ability to have the previous behavior if desired. Make this an option (preferably disabled by default, though that's not absolutely required), and unless more negatives show up it has my approval for the little that's worth.

I don't believe the old behaviour to be desirable enough to keep. Note that per-file -fs/-nofs still works as long as you don't change the setting during playback. If anything, I'd advocate a "reset display" command that is also useful if you accidentially resize the video.

Just by looking at the patch. I don't see where VO_USER_OVERRIDE_POS flag is set and where it's user_x/y parameters are filled. Also I'd have too see the actual code where vo_width/height initialization are removed, but it doesn't look right. Anyway, I think we should preserve the FS state, unless the command line says something different. I don't like the current approach as it requires additional code for each vo that wants this kind of behavior, creating huge non-intuitive differences among vo's.

On Thu, 2007-06-28 at 19:12 +0200, Reimar Döffinger wrote:

It is still ugly insofar as it only affects X11 vos, but I do not really like the alternatives of hacking this into vf_vo or the option parser code (probably will result in a very bloated and/or hackish approach), do you see any of these as clearly better? And why?

I think the fullscreen case could be best handled as an option parser feature. Make the option behave so that a global value is set at the start of the first file but is not restored at the start of later files. The above change would affect the behavior of "-fs file1 -nofs file2", as the -nofs from file1 would stay for file2 which does not explicitly specify a new value. I'm not sure whether this is something worth "fixing".

Hello, On Thu, Jun 28, 2007 at 08:56:43PM +0300, Uoti Urpala wrote:

On Thu, 2007-06-28 at 19:12 +0200, Reimar Döffinger wrote:

...

It is still ugly insofar as it only affects X11 vos, but I do not really like the alternatives of hacking this into vf_vo or the option parser code (probably will result in a very bloated and/or hackish approach), do you see any of these as clearly better? And why?

I think the fullscreen case could be best handled as an option parser feature. Make the option behave so that a global value is set at the start of the first file but is not restored at the start of later files.

The above change would affect the behavior of "-fs file1 -nofs file2", as the -nofs from file1 would stay for file2 which does not explicitly specify a new value. I'm not sure whether this is something worth "fixing".

That puts the whole command-line concept upside down. I'd considered that even more hackish than the ideas I had (among them things like a new option type that would not be overwritten on restore when bit 31 is set and another option parsing pass that would read vo_user_override and restore the options according to it - the later one probably combining the disadvantages of all approaches). I actually think that Ivan's objection can probably easiest be fixed by setting the vo_user stuff in mp_property_fullscreen while leaving the other code as is. Greetings, Reimar Döffinger

Reimar Döffinger wrote:

Hello, On Thu, Jun 28, 2007 at 05:07:44PM +0300, Ivan Kalvachev wrote:

...

2007/6/27, Reimar Doeffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:

[...]

...

Anyway, I think we should preserve the FS state, unless the command line says something different.

Two examples are hardly enough for a rule, but it feels as if every time you touch the interface-level code, it changes something in a way I don't like...

...

I don't like the current approach as it requires additional code for each vo that wants this kind of behavior, creating huge non-intuitive differences among vo's.

So how about this version then?

I don't see any indication of a way (e.g. by config file option) to produce the previous behaviour, which - as I said in the earlier thread - I think follows intuitively from the option-positioning rules used for the rest of MPlayer. I have no objection to providing the ability to have this kind of behaviour, but I do object to removing the ability to have the current behaviour and the option consistency which it provides. I would not even object strongly to making the behaviour you are aiming for default if you think it's that important, but I certainly do not want to have to use it myself. -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.

Reimar Döffinger wrote:

Hello, On Fri, Sep 07, 2007 at 07:12:57AM -0400, The Wanderer wrote:

...

Two examples are hardly enough for a rule, but it feels as if every time you touch the interface-level code, it changes something in a way I don't like...

Then maybe you try more to explain your likes so either they can be satisfied without a new commandline option or I can at least start to see the need for it.

In the previous case, I think I've already argued it as far as I can, and you don't seem to have been swayed in the slightest; my objection there was not so much on like/dislike as on the fact that something which had worked before (and seems like it should be common) had stopped working, and the thing which had been fixed by the change seemed like it should be much more rare than the thing which broke. And, of course, I used the behaviour which broke but am unlikely to ever use the thing which was fixed. In this case, my only real argument aside from "it's a change, change is bad" (which I do not really agree with) is that the current behaviour *does* make sense in context of the other MPlayer options, and while the behaviour I think I understand you as changing this to may also be desirable, it does not to me seem desirable enough to break the existing paradigm.

...

I don't see any indication of a way (e.g. by config file option) to produce the previous behaviour, which - as I said in the earlier thread - I think follows intuitively from the option-positioning rules used for the rest of MPlayer.

Huh? They only mail that said something like this was in reply to a suggestion for a _completely different_ approach at this,

As long as the effect is the same (in terms of removing the existing behaviour), which as far as I can tell from scanning the patch it appears to be, the same rationale applies. I don't honestly remember discussion of multiple approaches, or at least not ones which would be different in any significant respect in terms of the things I care about here.

and the other mail from you in this thread I replied to but never got an answer to...

I have made two previous posts in this thread; you replied to one of them, which was itself a reply to your post which started the thread, but not to the other. Your reply did not, so far as I could tell, contain anything which invited a response; you specifically said "I don't believe the old behaviour to be desirable enough to keep", which A: does not exactly inspire confidence in your willingness to be convinced on the issue, and B: is hard to argue against, as it's primarily a matter of opinion. My only argument against it (at least so far) is the very matter of option consistency which I mentioned above.

Adding a command line option is a possibility, but it is a very bad solution and should not be done without good reasons. If there was a sensible way to add it as a suboption I'd be more open to it, it keeps the complexity at least a bit more manageable.

...can suboptions be specified in the config file to take effect whenever their associated option is specified, without simultaneously having to specify the option itself in the config file and so turn it on by default? Because if not, having a suboption would not allow me (or anyone else) to have this default to the current behaviour, and that ability is my criterion for not objecting to the addition of new behaviour: that it be an addition, not a replacement. I agree that this is far too clunky a sort of thing to be well suited for a separate command-line option, but since which behaviour is desired is a matter of individual preference *and* we handle user preference-like settings in the config file *and* the program is designed such that all config-file entries inherently correspond to command line options... -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.

Reimar Doeffinger wrote:

Hello, On Fri, Sep 07, 2007 at 05:53:56PM -0400, The Wanderer wrote:

...

Reimar Döffinger wrote:

...

Then maybe you try more to explain your likes so either they can be satisfied without a new commandline option or I can at least start to see the need for it.

In the previous case, I think I've already argued it as far as I can, and you don't seem to have been swayed in the slightest; my objection there was not so much on like/dislike as on the fact that something which had worked before (and seems like it should be common) had stopped working, and the thing which had been fixed by the change seemed like it should be much more rare than the thing which broke. And, of course, I used the behaviour which broke but am unlikely to ever use the thing which was fixed.

Then why not give me the specific use case? An example of a scenario that would make MPlayer work worse for you.

...I thought I *had* given the use case at the time, at least if I'm understanding the term correctly. The previous case was the one corresponding to r18718. Before that change, MPlayer was compatible with window-manager window position memory; afterwards, it was not. Before the change, if I told the window manager "remember the location of this window", then the window would be in the same location next time even for a different file. After the change, the window always appears in the middle of the screen regardless. It is, of course, possible to override this with -geometry, if one knows the correct coordinates (although figuring those out is considerably less convenient than just having the WM handle it) - but the fact remains that failing to be compatible with window-manager location memory constitutes loss of functionality. The change which was fixed by that commit was that, if you specified -fs, -fixed-vo and multiple files, then changed back from fullscreen on something other than the first file, the window would still have the resolution of the first file. I did not and do not see how situations in which the pre-r18718 behaviour is a problem is likely to crop up anywhere near as often as situations in which the post-r18718 behaviour is a problem. The specific context in which I was using window-manager position memory is in placing the MPlayer window in a specific location with respect to my Japanese-English dictionary program, for convenience when I am translating text from e.g. anime title screens. It has become much more difficult to do that conveniently since the change took effect, particularly since I frequently need to seek back to the exact same location repeatedly and there is no convenient way to do that without quitting and restarting - which, now, results in having to move the window again (and so is much less convenient).

...

In this case, my only real argument aside from "it's a change, change is bad" (which I do not really agree with) is that the current behaviour *does* make sense in context of the other MPlayer options, and while the behaviour I think I understand you as changing this to may also be desirable, it does not to me seem desirable enough to break the existing paradigm.

I don't consider it breaking any paradigm, but actually "fixing" an existing one: That interactive changes should override commandline options.

Yes, they should. The question is to what extent. I am saying that the answer may be different in different contexts, and so both behaviours should be available.

The change is that such an override should also persist beyond file change, so it could be argued that this is more that just extending current behaviour.

I don't know if I agree with that "should". Are there any other command-line options which also correspond to interactive commands? Volume, OSD level and audio delay are the only ones I can think of offhand (aside from seeking, which is not persistent), and all of those reset to their command-line-specified values at file change. Would you argue that those should also persist between files? I can see a few cases where it might be useful for them to, but I can also see other cases where it would definitely not be desirable. Again, at best I could support providing both behaviours.

...

...

Huh? They only mail that said something like this was in reply to a suggestion for a _completely different_ approach at this,

As long as the effect is the same (in terms of removing the existing behaviour), which as far as I can tell from scanning the patch it appears to be, the same rationale applies.

Well, the difference is that my patches only changes current behaviour when the user explicitly switch to fullscreen.

...then perhaps I've missed something. I thought that *all* of the possible patches under discussion (whether created yet or not) were changing that and only that. I am saying that I do *not* want to have toggling the current file to or from fullscreen automatically set all subsequent files to the same. Just because I have explicitly made the change for this particular file does not necessarily mean that I want to make the change for all others. There are times when it does, but there are also times when it might not.

...

...

and the other mail from you in this thread I replied to but never got an answer to...

I have made two previous posts in this thread; you replied to one of them, which was itself a reply to your post which started the thread, but not to the other. Your reply did not, so far as I could tell, contain anything which invited a response; you specifically said "I don't believe the old behaviour to be desirable enough to keep", which A: does not exactly inspire confidence in your willingness to be convinced on the issue, and B: is hard to argue against, as it's primarily a matter of opinion. My only argument against it (at least so far) is the very matter of option consistency which I mentioned above.

Obviously there was quite some misunderstanding. Firstly I used "believe" as in "I don't believe the old behaviour is useful to anyone" - which (in that sentence obviously) does not mean I can't be convinced but just that I do not know.

It sounded to me like an expression of fixed opinion - specifically, the opinion that the old behaviour was in no way desirable. This is probably another manifestation of the difficulty of nuanced communication in a purely text medium; however, the phrasing used simply sounds dismissive to me. For what it's worth, I would (and did) read "I don't believe X" as being a much stronger negative statement than "I don't know X". Belief is much harder to overcome than simple lack of knowledge.

Also your argument about consistency did not concern me much, because I would tend towards the "interaction overrides commandline (permanently)" approach which would be (somewhat) consistent as well.

Yes, it would be consistent in a different way, and it would be good to have that available - but unless there is some specific reason *to* remove the existing behaviour rather than keep it, I see no reason to break the existing (and I hesitate to use the word) paradigm.

The much better argument is that your objections aren't purely theoretical but based on your actual use.

...in all honesty, I'm forced to admit that I'm not absolutely sure that that's true. Unlike the window-positioning issue, in this case I rarely do do anything which would be broken by the change you are proposing.

That is also better, because it allows proposing and discussing alternatives to an option, as there are: 1) a command to reset such overrides (my suggestion in the email I complained you did not answer to)

I don't remember seeing such a suggestion, and I don't see one now that I go looking for it - but while this would not be ideal, it would still be acceptable.

2) a different command, one which overrides options and one which does not. I am not sure how to implement this, properties do not seem to like an additional option, and a command/property with a completely new name seems a bit ugly to me.

Seems kind of ugly to me as well, but for that matter, so does my own suggestion of an option to choose which behaviour is desired - which, for the record, I would still prefer to anything else suggested so far (even over simply not changing the behaviour at all). -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.

Reimar Döffinger wrote:

Hello, On Fri, Sep 07, 2007 at 08:16:17PM -0400, The Wanderer wrote:

...

The previous case was the one corresponding to r18718. Before that change, MPlayer was compatible with window-manager window position memory; afterwards, it was not.

[...]

...

The change which was fixed by that commit was that, if you specified -fs, -fixed-vo and multiple files, then changed back from fullscreen on something other than the first file, the window would still have the resolution of the first file.

I did not and do not see how situations in which the pre-r18718 behaviour is a problem is likely to crop up anywhere near as often as situations in which the post-r18718 behaviour is a problem.

In addition the old code also cause the window to wander further down with each aspect change and I think also with each time I switched from fullscreen to windowed mode.

I don't remember this being mentioned at the time, but since last time I searched I couldn't find the original (pre-commit) discussion I'll take your word for it.

But the bigger problem I as the one wrangling with the code is that while you preferred this behaviour it was not at all what the code was _supposed_ to do, and what it actually did was mostly a function of the window manager leading to hard to reproduce behaviour and complaints.

I do not see how it is desirable to be explicitly incompatible with a fairly standard window-manager feature. Most of the time, the user will not have requested the window manager to remember the window location, and in that case I think the current behaviour is good - but if the user *has*, we should *not* ignore that. Window position is, in many respects, properly the domain of the window manager anyway...

There was an attempt to fix this properly again, but nobody is interested enough to keep the issue alive (and if necessary beat me to accept a solution I don't like), the only thing coming out of it was yet another ugly bloaty hack added to a window manager...

I don't know if I remember the attempt, and I'm not sure I understand what constitutes "proper" in your view in this context, but at this point - as long as nothing else gets broken in the process - anything which restores compatibility with WM position memory should be fine by me.

...

Volume, OSD level and audio delay are the only ones I can think of offhand (aside from seeking, which is not persistent), and all of those reset to their command-line-specified values at file change. Would you argue that those should also persist between files?

Volume does not reset (at least not if it's a hardware mixer, not sure about -softvol), but there is no way to set volume from commandline anyway.

-softvol is what I"m talking about (since I don't have a hardware mixer), and indeed it does reset - or at least it did last I checked. And yes, you can in some sense set volume from the command line, using -af volume.

...

...

2) a different command, one which overrides options and one which does not. I am not sure how to implement this, properties do not seem to like an additional option, and a command/property with a completely new name seems a bit ugly to me.

Seems kind of ugly to me as well, but for that matter, so does my own suggestion of an option to choose which behaviour is desired - which, for the record, I would still prefer to anything else suggested so far (even over simply not changing the behaviour at all).

I forgot 3) make it depend on -fixed-vo: old behaviour with -nofixed-vo, while -fixed-vo keeps fullscreen state.

This would sort-of fix things for me for the short term, since I don't presently use -fixed-vo, but since that will not necessarily remain the case I'm not sure it's good in the long term. I also have the understanding that there is the possibility of dropping -nofixed-vo entirely at some point...

4) add INITED_USER_FLAGS or similar to uninit_player that resets these, thus keeping the current behaviour and only "fixing" aspect changes (or other mid-stream resolution etc. changes).

I'm not sure that I understand this, but if I do, then it sounds almost ideal for my purposes. Then again, I'm not sure how this would address the issue at hand, which is that some people want fullscreen status (once toggled) to persist between files and others do not. I had another idea last night for a possible approach, but I seem to have forgotten it in my sleep. Yet a separate idea would be to do something comparable to e.g. the "pausing_keep" slave-mode command prefix - make it possible to specify e.g. a "persistent" prefix (or not prefix, but you get the idea) in the keybindings file. Then you give the new behaviour if that flag has been specified for the "toggle fullscreen" key, and the old one if not. This has the advantage that it could be extended to do the same kind of thing for all other potentially relevant commands, not just fullscreen. It has the disadvantage that it might be horrendous to code cleanly. -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.

Reimar Döffinger wrote:

Hello, On Sat, Sep 08, 2007 at 09:34:30AM -0400, The Wanderer wrote: [...]

...

Yet a separate idea would be to do something comparable to e.g. the "pausing_keep" slave-mode command prefix - make it possible to specify e.g. a "persistent" prefix (or not prefix, but you get the idea) in the keybindings file. Then you give the new behaviour if that flag has been specified for the "toggle fullscreen" key, and the old one if not.

This has the advantage that it could be extended to do the same kind of thing for all other potentially relevant commands, not just fullscreen. It has the disadvantage that it might be horrendous to code cleanly.

I admit that this is the solution I think I'd prefer

Good, because after some thought, I think it's the one I like best as well. ^_^

(also since it is more flexible for gui frontends), I just don't like fiddling with the whole input system etc. and the string parsing, and lastly I don't know how to pass that info to the property functions...

Well, from what I can tell based on svn blame and the contents of MAINTAINERS, Alban Bedel seems to be "in charge" of the input and config systems. Alban, if you're around, do you have any advice or suggestions? (I would not be inherently unwilling to do at least some of the work myself, but A: I'm not sure I have time, B: I don't know if I'd know where to start looking at the input system code, and C: I would have no clue how to hook it in to the actually functional part.) -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.