[confirm] TV Guide border highlight causing render exception (2 Viewers)

[tourettes]

Texture atlas as a cache will be removed. Performance loss because of texture switching is minimal compared to our current D3D state handling.

Actually disabling the texture atlas will increase the performance (I did some local testing). This is because the vertex buffers aren't constantly getting updated, which seems to be a no go for modern GPUs. It is faster to switch between textures than to update vertices in our case.

I didn't remove the texture atlas in my font engine speed improvement since GUIImage is so ugly class that keeping it working was too much annoying

 

[tourettes]

Yes, picture viewer will be as fast as data can be loaded from the HDD and send to the GPU. Rescaling will be done by the GPU upon rendering.

There might be one limiting case - there are GPUs that wont allow texture sizes that the images could have. So we need to scale down to the ma GPU resolution at least.

As not every frame needs to be processed again it makes sense to render pictures using a shader to another texture that is than displayed and not processed again. And timing isn't essential here, so we can go for a very high quality shader here. No need to render at 60fps all the time.

Ken Burns -> need to render at the refresh rate to get anything nice out of it

 

[tourettes]

... how about using the RenderLayer lock in Add/Remove methods in GUIWindow.cs?

lock (GUIGraphicsContext.RenderLock)
 
{...}

You need to be really careful with those locks. As far as I remember there was a pacth in GIT at some point that tried to fix the flickering with locking -> MP deadlocks with some user interaction (cannot remember what).

 

[elliottmc]

Best practice in such cases is running the for-next starting with highest menber and going down - like that:

          for (int i = Children.Count - 1; i >= 0; i--)
          {...}

But I think we must not do that in the renderloop, as the order of the controls must be like that (ascending).

Would it not be possible to do something like

dummy = Children.Count;
 
for (int i = dummy - 1; i >= 0; i--)
{
GUIControl control = Children[dummy+1-i];
control.UpdateVisibility();
control.DoRender(timePassed, currentTime);
}

so that the loop goes in the right order, and the controls are also updated in the right order.

 

[tourettes]

Best practice in such cases is running the for-next starting with highest menber and going down - like that:

          for (int i = Children.Count - 1; i >= 0; i--)
          {...}

But I think we must not do that in the renderloop, as the order of the controls must be like that (ascending).

Would it not be possible to do something like

dummy = Children.Count;
 
for (int i = dummy - 1; i >= 0; i--)
{
GUIControl control = Children[dummy+1-i];
control.UpdateVisibility();
control.DoRender(timePassed, currentTime);
}

so that the loop goes in the right order, and the controls are also updated in the right order.

Some other thread could still modify the collection and Children[dummy+1-i] could be accessing wrong or non-existing items.

 

[elliottmc]

Best practice in such cases is running the for-next starting with highest menber and going down - like that:

          for (int i = Children.Count - 1; i >= 0; i--)
          {...}

But I think we must not do that in the renderloop, as the order of the controls must be like that (ascending).

Would it not be possible to do something like

dummy = Children.Count;
 
for (int i = dummy - 1; i >= 0; i--)
{
GUIControl control = Children[dummy+1-i];
control.UpdateVisibility();
control.DoRender(timePassed, currentTime);
}

so that the loop goes in the right order, and the controls are also updated in the right order.

Some other thread could still modify the collection and Children[dummy+1-i] could be accessing wrong or non-existing items.

But I thought the problem was that the list could be shorter, so if we start at the top we should be okay?

Let's look at it another way. Is Guzzi's fix an improvement on current code, which is definitely having a problem?

 

[Guzzi]

Yes, picture viewer will be as fast as data can be loaded from the HDD and send to the GPU. Rescaling will be done by the GPU upon rendering.

There might be one limiting case - there are GPUs that wont allow texture sizes that the images could have. So we need to scale down to the ma GPU resolution at least.

I think that is what we're doing now (downscaling) - and this is of course VERY slow - typical example here: Pic from camera with e.g. 3000x2000 to be sc scaled down to 1920x1080 -> takes ages. I played a bit with the settings (as it is even worse, when using the "use highest thumb quality" of MP (which I think is not necessary for viewing on TV), which makes it half, but still too much for "fluent browsing" of the pictures.
So either we find a way for faster conversion or have to do it in the thumb creator process - but I think that's another thread/topic ;-)

 

[Guzzi]

... how about using the RenderLayer lock in Add/Remove methods in GUIWindow.cs?

lock (GUIGraphicsContext.RenderLock)
 
{...}

You need to be really careful with those locks. As far as I remember there was a pacth in GIT at some point that tried to fix the flickering with locking -> MP deadlocks with some user interaction (cannot remember what).

Yep, I tried it and got a deadlock quickly ;-) - the suggestion was nonsense. I tried using a private object for locking in the collection class, but also wasn't working as I expected...

 

[Guzzi]

Would it not be possible to do something like

dummy = Children.Count;
 
for (int i = dummy - 1; i >= 0; i--)
{
GUIControl control = Children[dummy+1-i];
control.UpdateVisibility();
control.DoRender(timePassed, currentTime);
}

so that the loop goes in the right order, and the controls are also updated in the right order.

Some other thread could still modify the collection and Children[dummy+1-i] could be accessing wrong or non-existing items.

But I thought the problem was that the list could be shorter, so if we start at the top we should be okay?

Let's look at it another way. Is Guzzi's fix an improvement on current code, which is definitely having a problem?

It is, as it "reduces" the timeferame WHEN an error / exception can occur from the lifetime of the whole enumeration to the cycle of a possibly wrong enumeration. And as the cause here was TVplugin doing an remove/add, that should mostly work.
But: It is NOT the correct way and not curing the root cause, that the data processed is not thread safe and processed properly.
As noted above, I was playing with some locking, but it always brings additional risks of blocking the processing (stutter !) - so from users perspective I think we could live with such a "hack" - from dev POV it is not a clean solution. Any better suggestions of course welcome ...

 

[elliottmc]

Some other thread could still modify the collection and Children[dummy+1-i] could be accessing wrong or non-existing items.

But I thought the problem was that the list could be shorter, so if we start at the top we should be okay?

Let's look at it another way. Is Guzzi's fix an improvement on current code, which is definitely having a problem?

It is, as it "reduces" the timeferame WHEN an error / exception can occur from the lifetime of the whole enumeration to the cycle of a possibly wrong enumeration. And as the cause here was TVplugin doing an remove/add, that should mostly work.
But: It is NOT the correct way and not curing the root cause, that the data processed is not thread safe and processed properly.
As noted above, I was playing with some locking, but it always brings additional risks of blocking the processing (stutter !) - so from users perspective I think we could live with such a "hack" - from dev POV it is not a clean solution. Any better suggestions of course welcome ...

So perhaps the correct course of action for now is to use this fix in 1.3.0RC, create a new mantis highlighting this as a temporary fix and keep the original mantis.