Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

osc: fix forced-window detection during scaling #4102

Closed
wants to merge 1 commit into from
Closed

osc: fix forced-window detection during scaling #4102

wants to merge 1 commit into from

Conversation

Coacher
Copy link
Contributor

@Coacher Coacher commented Feb 2, 2017

Relying on "video" property being "no" isn't good enough because when
mpv starts playing a new file for a split second "video" is set to
"auto", which results in incorrect scaling being applied to a forced
window at this moment.

Then "video" property is set to "no" and correct scaling is applied,
which results in scaling differences. User sees this as OSC glitches.

Solve the problem by checking "force-window" option when trying to
detect forced-window mode.

Closes #4081.

Relying on "video" property being "no" isn't good enough because when
mpv starts playing a new file for a split second "video" is set to
"auto", which results in incorrect scaling being applied to a forced
window at this moment.

Then "video" property is set to "no" and correct scaling is applied,
which results in scaling differences. User sees this as OSC glitches.

Solve the problem by checking "force-window" option when trying to
detect forced-window mode.

Closes #4081.
@Coacher
Copy link
Contributor Author

Coacher commented Feb 2, 2017

This completely solves #4081 for me.
CC @wiiaboo.

@wiiaboo
Copy link
Member

wiiaboo commented Feb 2, 2017

I really don't think this is the right fix.
Built-in pseudo-gui always uses force-window and the point is not for the OSC to always have scale 2, but only when there's no video and force-window=yes, so you could change the if to include both checks.

If your point is always have consistent osc scaling even with video=no, just set scaleforcedwindow to 1 on your side.

@wiiaboo
Copy link
Member

wiiaboo commented Feb 2, 2017

so you could change the if to include both checks.

This also makes no sense, video=no without force-window doesn't create a window, so of course there's no osc.

Anyway, this isn't a bug, this is intended behavior. Just set scaleforcedwindow=1 if you don't like it.

@Coacher
Copy link
Contributor Author

Coacher commented Feb 2, 2017

Anyway, this isn't a bug, this is intended behavior.

It is intended to expect video property to be "no" with forced window, when it's not always the case?

@wiiaboo
Copy link
Member

wiiaboo commented Feb 2, 2017

scaleforcedwindow being different from scalewindowed and scalefullscreen is the intended behavior. I wasn't the one to do it, but I understand why it's different. If you want consistency, set the same value for all of them. What's so hard?

@Coacher
Copy link
Contributor Author

Coacher commented Feb 2, 2017

This is NOT about these 3 values being different. This is about osc.lua using incorrect checks to choose what scaling to apply. Have you read the commit message? Nothing there mentions 'consistency'.

@wiiaboo
Copy link
Member

wiiaboo commented Feb 2, 2017

I don't have the patience to keep repeating myself. Ask @ChrisK2 if you want to change the default scaleforcedwindow to 1. This PR is fixing the wrong thing.

@Coacher
Copy link
Contributor Author

Coacher commented Feb 2, 2017

I don't want to change scaleforcedwindow. I explicitly said so and my PR doesn't try to do it. I think you are mistaken here.

@Coacher
Copy link
Contributor Author

Coacher commented Feb 2, 2017

Built-in pseudo-gui always uses force-window and the point is not for the OSC to always have scale 2, but only when there's no video and force-window=yes.

Agree, I accidentally broke the case when there is actual video and --force-window is used.
Now whenever --force-window is used scaleforcedwindow is applied. This wasn't intended.
How one can reliably detect if mpv is in forced-window mode without any video?

wiiaboo added a commit to wiiaboo/mpv that referenced this pull request Feb 3, 2017
Will still hide playlist items with long enough filenames and osd-font-size
but not as soon.

osc messages should now preserve their scaling with fullscreen toggling and
cycling through audio-only files and files with video.

Closes mpv-player#4081, mpv-player#4083, mpv-player#4102
wiiaboo added a commit to wiiaboo/mpv that referenced this pull request Feb 3, 2017
Will still hide playlist items with long enough filenames and osd-font-size
but not as soon.

osc messages should now preserve their scaling with fullscreen toggling and
cycling through audio-only files and files with video.

Closes mpv-player#4081, mpv-player#4083, mpv-player#4102
wiiaboo added a commit to wiiaboo/mpv that referenced this pull request Feb 5, 2017
Will still hide playlist items with long enough filenames and osd-font-size
but not as soon.

osc messages should now preserve their scaling with fullscreen toggling and
cycling through audio-only files and files with video.

Closes mpv-player#4081, mpv-player#4083, mpv-player#4102
wiiaboo added a commit to wiiaboo/mpv that referenced this pull request Mar 24, 2017
Will still hide playlist items with long enough filenames and osd-font-size
but not as soon.

osc messages should now preserve their scaling with fullscreen toggling and
cycling through audio-only files and files with video.

Closes mpv-player#4081, mpv-player#4083, mpv-player#4102
wiiaboo added a commit to wiiaboo/mpv that referenced this pull request Mar 24, 2017
Will still hide playlist items with long enough filenames and osd-font-size
but not as soon.

osc messages should now preserve their scaling with fullscreen toggling and
cycling through audio-only files and files with video.

Closes mpv-player#4081, mpv-player#4083, mpv-player#4102
@wiiaboo
Copy link
Member

wiiaboo commented Mar 24, 2017

Fixed in bcb2db0, which didn't close this one for some reason.

@wiiaboo wiiaboo closed this Mar 24, 2017
@Coacher Coacher deleted the osc-fix-forced-window-scaling branch March 26, 2017 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants