Scythe42's fixes for 1.4.0 (2 Viewers)

[Scythe42]

I made this change on purpose because to be frank: you want the UI displayed at 60Hz and only for content use a different RR.

Why? I would say the UI should be rendered in the refresh rate that is current. My Setup runs at 50 Hz as I have most 50 Hz stuff here and this way avoid switching rr for the most part. MP should stick to what the user has Setup or have a selection itself, but not just say 60 Hz UI is best (for me it is not and I guess for most of Europe it is the same)...

I already added to use the current one when the D3D device gets created. But when refresh rate changes this magic value gets not adjusted. Never did. Missing feature.

It is not like I hardcoded it to 60. It is just the default value. It gets overriden on device creation anyway. But refresh rate changer does not adjust it and never did. The scrolling still work with the same value all the time.

But problem lies somewhere else in scolling speed setup that HomeY is seeing.

From his configuration:

 <entry name="ScrollSpeedRight">5</entry>

This is the problem. The scroll speed settings try to artificially limit how many frames are rendered for animations based on a magic value. And this does not work.

I bet if @HomeY sets these to the following, he will see no stuttering:

<entry name="ScrollSpeedRight">1</entry>

Problem here is that the scroll speed settings try to render at 32fps in his setup but that does not fit into 50Hz. The result is stuttering.

Solution: Proper algorithm on the Scroll Speed when calculating the numbers. It must be "every x" frames of the current refresh rate and not some fixed fps not matching what your display is set to. FPS of scrolling does not match Hz of Screen. 32.x fps on 50Hz cannot be smooth.

Might work on 1.3.0 as other stuff is broken there and there are more changes than just mine that have impact here.

@HomeY: can you experiment with the scroll speed a bit and tell me if it makes any difference, meaning my root cause analysis is correct? Especially interested in which numbers work and which not. I cannot test it, scrolling is never smooth for me, so I cannot see a difference.

 

[Scythe42]

@elliottmc: can you create a new installer please?

I like to verify some multi display things with users and check for regressions in that area. This is the most important thing and could be a merging show stopper.

Known Issues:

• Mouse Cursor Hiding is wrong, when you take focus away from MP during startup - have it reproducible and therefore can fix. This creates a imbalanced state that is fixed when you do something inside MP (cannot be fixed when not in focus).
• Threaded Plugins compatibility with an unknown plugin yet, causes MP to freeze on remote sessions (TVSeries fix posted above, IMDB+ bad API usage handled in Core). Disabled threaded plugin loading per default anyway for now, does not mean we cannot continue work on known issues.
• Probably existing resume/suspend issues we need to check with the users. Circumstances not yet clear.
• MCE remote code hangs on resuming with various remotes (so far Harmony has been identified) - working on that one.

The resume things could probably be solved by the user using the delay workaround on resuming. What seems to be common here is that MP starts right away, but some devices are available at a later point from Windows and MP code hangs here, because resuming is too fast now and some code cannot deal properly with something not being available and freezes without logging an error. Infinite retry loop I assume or even a crash and nothing is displayed because of the suspended state.

 

[HomeY]

@HomeY: can you experiment with the scroll speed a bit and tell me if it makes any difference, meaning my root cause analysis is correct? Especially interested in which numbers work and which not. I cannot test it, scrolling is never smooth for me, so I cannot see a difference.

I've tested ScrollSpeedRight with values from 1 - 5, leaving the ScrollSpeedDown @ 2.
All 5 testes resulted in GUI rendering around 32FPS.
Then did another 5 tests on ScrollSpeedRight with values from 1 - 5, setting ScrollSpeedDown @ 1.
Same results, always around 30-32FPS.

 

[Scythe42]

Then did another 5 tests on ScrollSpeedRight with values from 1 - 5, setting ScrollSpeedDown @ 1.
Same results, always around 30-32FPS.

Very interesting. So both values have no impact.

Let's go through the code:

GUIGraphicsContext.CurrentFPS.ToString("f2"),

That is what is displayed on screen. Let's check where it is used.

	protected void UpdateStats()
	{
	  long time	  = Stopwatch.GetTimestamp();
	  float diffTime = (float)(time - _lastTime) / Stopwatch.Frequency;
 
	  // Update the scene stats once per second
	  if (diffTime >= 1.0f)
	  {
		GUIGraphicsContext.CurrentFPS = Frames / diffTime;
		_lastTime = time;
		Frames = 0;
	  }

Here it gets updated. And it is used for nothing beside displaying the value in the Stats Renderer.

Frames is increased every time we call Render. The time span should be ok, the StopWatch is not precise enough. But that cannot explain that huge difference.

So we have to assume that we actually do not render frames. Frames is increased in MediaPorta.Render().

Let's check the conditions when we render a frame:

	if (!_suspended && AppActive && !_isRendering && GUIGraphicsContext.CurrentState != GUIGraphicsContext.State.LOST && GUIGraphicsContext.DX9Device != null)

These are ok, all the conditions are met, or we would see errors in the log.

So let's check where Render() is actually called:
When in OnPaintEvent() is triggered. That is ok, as a repaint is actaully requested. So leaves only FullRender()

There we enter the area of reduced frame rate rendering in MP, which I wanted to be removed completly.

We have two conditions here.

ActiveForm != this && _reduceFrameRate

Ok, MP is the active form, no doubt about it.

GUIGraphicsContext.SaveRenderCycles

This one seems more likely. This is the MP screen saver trying to reduce frames rendered.

Let's check your MediaPortal.xml. Ah, you are not using the MP screen blanker or frame rate reducer.

	<entry name="IdleTimer">no</entry>
	<entry name="IdleTimeValue">300</entry>
	<entry name="IdleBlanking">yes</entry>

So I cannot see something in code in relation to the settings. Must be something else.

Then we have this after each call of FullRender()

WaitForFrameClock();

What does it do:

	private static void WaitForFrameClock()
	{
	  // sleep as long as there are ticks left for this frame
	  ClockWatch.Stop();
	  long timeElapsed = ClockWatch.ElapsedTicks;
 
	  if (timeElapsed >= GUIGraphicsContext.DesiredFrameTime)
	  {
		return;
	  }
 
	  long milliSecondsLeft = (GUIGraphicsContext.DesiredFrameTime - timeElapsed) * 1000 / Stopwatch.Frequency;
	  DoSleep((int)milliSecondsLeft);
	}

So it sleeps if we did not the DesiredFrameTime. Let's follow it down the code path where it gets modified.

private static void SyncFrameTime()
{
  _desiredFrameTime = DXUtil.TicksPerSecond / _maxFPS;
}

And here comes my friend MaxFPS back into play, which according to your logs is set to 50.

So DXUtil.TicksPerSecond is next. Let's check where it is set or manipulated.

And here we enter a couple of more classes and methods of MP. Obviously what gets used here a base value your system is not what it should be or what is expected.

I need to stop here. Anyone else can reproduce this problem?

 

[HomeY]

So, when does the stuttering scrolling goes away exaclty? Meaning what steps did you need to execute after starting MP to make go it away? When it is gone is the on screen display also showing about 50fps?

Did a test with Shift + 1 open.

1. Started MP (it's rendering @ 32FPS)
2. Went into TV Series plugin and played an MKV (DRR got changed to 23.976) and stopped it. -> Rendering still @ 32 FPS
3. Went into TV Section and started LiveTV (DRR changed to 50Hz) and stopped playback -> Rendering @ 50FPS!

During this testrun i got Fontengine error (which you'll find in the attached log)

Does turning off plugins change things? Just start with core plugins enabled for a test. Just focus on this "avg. fames" for now.

Just started MP in debug mode with DW skin, which is also rendered @ 32FPS.

If not does using "Titan" as a Skin for example makes a difference?

Nope, Titan also renders @ 32FPS.

Does disabling the refresh rate changer makes a difference?

Disabled DRR completely, started MP -> GUI rendered @ 32FPS

 

[fforde]

IMDB+ calls SaveCache() during initialization because it modified options of main MP and wants them to be saved immediately. This is an absolute no go! This method should never be called by any 3rd party stuff, unless it is something that explicitly modifies MediaPortal.xml settings (like the GUISettings plugin for example).

If it shouldn't be called externally then the method should be marked internal. As far as I am concerned anything that is marked public is free game.

EDIT: Sorry, I missed Seco's comment above.

 

[RoChess]

IMDB+ calls SaveCache() during initialization because it modified options of main MP and wants them to be saved immediately. This is an absolute no go! This method should never be called by any 3rd party stuff, unless it is something that explicitly modifies MediaPortal.xml settings (like the GUISettings plugin for example).

IMDb+ can not wait for MediaPortal to exit before it updates the IMDb+ XML files though. It is done so that the IMDb+ scraper-script ends up using the correct values and has to be updated instant. But will look again at the process otherwise.

 

[Scythe42]

IMDB+ calls SaveCache() during initialization because it modified options of main MP and wants them to be saved immediately. This is an absolute no go! This method should never be called by any 3rd party stuff, unless it is something that explicitly modifies MediaPortal.xml settings (like the GUISettings plugin for example).

IMDb+ can not wait for MediaPortal to exit before it updates the IMDb+ XML files though. It is done so that the IMDb+ scraper-script ends up using the correct values and has to be updated instant. But will look again at the process otherwise.

Not talking about XML files of the Plugins itself, but actually saving the MP Settings. There is only one instance of it and settings, especially during startup might change if anything adds something new or updated something. This is result of another rather dirty practice of plugins want to store a lot of stuff in the MediaPortal.xml when it has not been set or determined during runtime instead of using their own XML.

It is this part in the code:

			#region Plugin
			using (Settings xmlwriter = new MPSettings())
			{
				xmlwriter.SetValue(cSection, cSyncInterval, SyncInterval.ToString());
				xmlwriter.SetValueAsBool(cSection, cSyncOnStartup, SyncOnStartup);
				xmlwriter.SetValue(cSection, cSyncLastDateTime, SyncLastDateTime.ToString());
				xmlwriter.SetValueAsBool(cSection, cDisableNotifications, DisableNotifications);
				xmlwriter.SetValue(cSection, cMoviesRefreshed, MoviesRefreshed.ToJSON());
			}
			Settings.SaveCache();
			#endregion

During startup you want to change configuration settings on MP and save the immediately to the main MediaPortal.xml. And that does not work and should not be allowed by MP. Having them in your plugin XML is no issue.

Anyway, the SaveCache() function now puts a lock on the object while saving. That avoids the issue for now.

Some time in the future SaveCache will be made internal. Probably with 1.5.0 when re-visiting more stuff in this area.

I am sure you have a valid reason for doing it. Not arguing against it. The fault of this being possible in the first place lies without doubt with MP.

If it shouldn't be called externally then the method should be marked internal. As far as I am concerned anything that is marked public is free game.

I fully understand this. And it is obviously a defect in MP having this method publicly available. It should never been this way.

Not blaming anyone, just telling that it is a no-go for a plugin and needs to be prevented for the future. Just stumbled across it with IMDB!+.

 

[seco]

See http://mantis.team-mediaportal.com/view.php?id=4377 about Settings problem

"Also MPSettings should be proper singleton. Currently there is MPSettings.Instance but it does not properly implement singleton pattern.

MPSettings() constructor is public which it should be not since it destroys the whole idea of singleton and the caching implementation does not work correctly (that is the reason why IMDB+ plugin is now using SaveCache() since it is using its own instance of MPSettings()).

Constructor already has comment:

// public constructor should be made/private protected, we should encourage the usage of Instance"

What this means is that during MP runtime there should be exactly one instance of MPSettings which reprents the current settings. When it is first created, the values are read from XML file. After this any modifications done to settings object go to cached implementation of Settings. Because MPSettings is single instance, all code using this instance will get those new modified setting values when requested. When MP closes/instance dies, current state is saved to XML file.

 

[Scythe42]

Fully agree.