Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Changes in Output settings take effect immediately #188

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

nparkanyi
Copy link

Updates the output settings page to automatically reload the current file at the same seek position after changing the video, audio, or subtitle renderers so the changes are visible immediately.

@@ -387,11 +387,19 @@ class CMainFrame : public CFrameWnd, public CDropClient
void StartWebServer(int nPort);
void StopWebServer();

int GetPlaybackMode() const { return m_iPlaybackMode; }
bool IsPlaybackCaptureMode() const { return GetPlaybackMode() == PM_ANALOG_CAPTURE || GetPlaybackMode() == PM_DIGITAL_CAPTURE; }
int GetPlaybackMode() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem unrelated. Please revert the unrelated changes and I'll have a look at astyle.

@XhmikosR XhmikosR requested a review from kasper93 August 3, 2017 07:03
@clsid2
Copy link
Contributor

clsid2 commented Aug 7, 2017

Please test with a live radio stream, or a raw audio file. In those cases duration is unknown, and seeking to last position may fail. Better restart from beginning if duration is unknown.

@kasper93
Copy link
Contributor

I think forcefully resetting playback is little bit too much. Restarting playback can be done with single hotkey (CTRL+E by default) and user can do it once he finish changing settings. I think it would be annoying for me if playback reset occur automatically, especially that some files/steam can't be recovered in the same position.

Also this patch is not complete, because if we want to reset playback there are many more options that should trigger this. Every other option on output page, subtitle renderer setting, all internal/external filters settings. Basically every change that can affect Directshow graph should reset playback with your approach. I don't like it. But if everyone thinks it is best we can think about merging this change.

@nparkanyi
Copy link
Author

The VMR and EVR output settings already take effect immediately, same with the subtitle settings, so in a sense it's already a bit inconsistent. To me, it seems a bit surprising that changing the renderer, then clicking 'Apply' only applies the change on the next file loaded. If you, say, want to compare the same image with different renderer backends, this is cumbersome because you have to change the setting, close the settings, restart the file, and seek to the same point.

@l4n9th4n9
Copy link

I think it would be annoying for me if playback reset occur automatically, especially that some files/steam can't be recovered in the same position.

I don't think users would change Output settings often in normal playback, so it's not that annoying. If someone tinkers with Output settings, they may try to see how different renderers behave, and having to restart playback for that every time may be actually more annoying. Maybe, if it's not too hard to implement, we could have a checkbox to disable/enable this behavior on the Output settings page.

@XhmikosR
Copy link
Contributor

An extra option for this doesn't make sense. We should decide if we want to change the default behavior or not. Perhaps we should show a warning message an if the user confirms then restart playback or something like that.

@nparkanyi
Copy link
Author

I wonder what would be a media stream that breaks this behaviour? As this is implemented, if the media has no duration, it simply "restarts" playback from the beginning, but generally if there is no duration, it's because it's a live stream (like internet radio), so even though we "restart" the playback, it's just continuing to stream it from the same position.

@kasper93
Copy link
Contributor

kasper93 commented Oct 2, 2017

Some streams that have duration are still not seekable. Anyway we can go with those changes, I still think we shouldn't restart playback, but I don't mind it really. I will take closer look soon.

stdedos pushed a commit to stdedos/mpc-hc that referenced this pull request Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
5 participants