Studio #2259

Merged
merged 4 commits into from Mar 1, 2013

Projects

None yet

7 participants

@elupus
Member
elupus commented Feb 18, 2013

This pull adds support for studio levels in the full xbmc GUI.

@jmarshallnz jmarshallnz commented on the diff Feb 19, 2013
xbmc/cores/VideoRenderers/VideoShaders/YUV2RGBShader.cpp
@@ -106,8 +107,20 @@ void CalculateYUVMatrix(TransformMatrix &matrix
coef.m[row][col] = conv[col][row];
coef.identity = false;
+
+ if(g_Windowing.UseLimitedColor())
+ {
+ matrix *= TransformMatrix::CreateTranslation(+ 16.0f / 255
+ , + 16.0f / 255
+ , + 16.0f / 255);
+ matrix *= TransformMatrix::CreateScaler((235 - 16) / 255.0f
+ , (235 - 16) / 255.0f
+ , (235 - 16) / 255.0f);
+ }
+
@jmarshallnz
jmarshallnz Feb 19, 2013 Member

IIRC our coefficients in the colour transforms include the translation+scaling to full range already, whilst most given in the various literature don't. It might be more readable, therefore, to have the coefficients defined on the reduced scale and move the translation + scaling to fullrange to here instead? (cosmetic issue ofc).

@elupus
elupus Feb 19, 2013 Member

Hmm. They don't really. They assume input is in the range 0 to 1 and -0.5
to 0.5. As far as I know that is how most define it.

So limited range is first scaled to that. That could be wrong of course.

@jmarshallnz jmarshallnz commented on the diff Feb 19, 2013
xbmc/guilib/GUIFontTTF.cpp
for(int i = 0; i < 4; i++)
{
- v[i].r = GET_R(color);
- v[i].g = GET_G(color);
- v[i].b = GET_B(color);
- v[i].a = GET_A(color);
+ v[i].r = r;
+ v[i].g = g;
+ v[i].b = b;
+ v[i].a = a;
}
@jmarshallnz
jmarshallnz Feb 19, 2013 Member

Something for later: Move this calculation out of RenderCharacter as it will be static for most text strings.

@jmarshallnz
Member

Looks good to me as a whole. Are you planning on GLES (shaders) and DX support or prefer someone else take a crack at that?

@elupus
Member
elupus commented Feb 19, 2013

Have no way to test gles, so would be nice to get some assistance there.
Will try to do win32.

@FernetMenta
Member

For what reason do we need this? I just dropped studio level in my new version of vdpau because it produced nothing but crap. If HDMI only supports limited range for RGB this can be set in the video driver, at least on Linux.

@elupus
Member
elupus commented Feb 19, 2013

If you set it in video driver, blacker than black and white than white is
lost.

Secondly, Intel drivers only support that in very new revisions.

And my TV don't support pc levels I without disabling bunch of other
things.

There really should not be Crap output from this, since we should not loose
any info for movies.

@FernetMenta
Member

Limited range HDMI does not define footroom/headroom space to be used for btb or wtw. Users reported entirely distorted output when when driver was set to full range and other and only supported limited. This may work but does not follow a spec. The options for vdpau did more harm than any good.

@elupus
Member
elupus commented Feb 19, 2013

I'm pretty sure it us foot room for btb, where have you read otherwise?

@FernetMenta
Member

I searched for the source but couldn't find. Maybe I mixed something up from other articles stating that footroom / headroom became less important with digital equipment.
According to CEA-861-D the source can identify itself and set the color range. In general this is full range for IT like a PC. Converting to limited range when the actual space is full range only works if the other side thinks the format is limited range. If this is the case there must have gone something wrong. I don't think this strategy will work for the most common case that both parties know about the other end.

@elupus
Member
elupus commented Feb 19, 2013

The problem is that there don't seem to be a way to indicate over hdmi what
the source is. At least I've yet to see a TV that handles that auto. I
don't think the standard specifies any method for it.

What make matters worse is that not all tvs allow the user a choice in what
the source is. So in my case it will assume our full range image is limited
range and "expand" the middle part on display.

When you change this setting in gfx, the only thing it does is compress the
full range rgb from the computer gfx. So then we end up with us
expanding,gfx compressing. But gfx can obviously not recreate the edge
values.

As you said the wtw, btb are less important in the digital erea, BUT they
do still help cover up invalid calibrated tv's. Ie brightness pulled too
high.

@FernetMenta
Member

The problem is that there don't seem to be a way to indicate over hdmi what the source is

I thought this is what's defined in CEA-861-D. HDMI reference this spec. My guess is that vdpau studio level stopped working with NVidia driver being more compliant to the spec and source and TV agreed on the same color space.

Maybe the options should be renamed to "fake limited color space" to warn users that this may not work.

@elupus
Member
elupus commented Feb 19, 2013

Eh? It will always work if your gfx isn't compressing.

But yea it's very possible nvidia changed their defaults to compress by
default given that almost all pc software output full range and that all
tvs default to assuming limited range on hdmi.

Also, I added the function for it in windowing system, the thought being
that if we can detect that gfx is compressing, we should not output
limited.

Note. even the ps3 have this option in their settings. I would assume the
xbox too.

Only reason I haven't added it earlier is that I thought I would need to
switch from the fixed pipeline to shaders. Turned out that we didn't. The
motivation increased when I needed it myself :-)

@elupus
Member
elupus commented Feb 19, 2013

That spec aught to be the one yea. But I suspect that always says limited
output. But doing that would be throwing away info for sources that is not.
Pictures, computer graphics and what not. Ie does not make sense for a
computer monitor connected via hdmi.

@FernetMenta
Member

VDPAU Studio Level ist not what it should be. I tried some test patterns and on my calibrated Plasma it gives me a incorrect image. Without it everything should be normal on a display that can process full RGB like every Panasonic Plasma or Samsung LCD TV...

I have studied more of those reports. The chance that this method fails is high enough.

HDMI spec state that limited range "shall" be used for almost all formats. Maybe some manufactures interpret this as "must" of default.

@FernetMenta
Member

I checked my living room tv on this. If there was a communication between source and tv as the spec suggests, it's not between my box and tv. At least the HDMI black level setting in the menu does not reflect the setting of the gfx.

Nevertheless, wouldn't make it sense to rename the option to something like limited range as this is more common than studio level?

@elupus
Member
elupus commented Feb 19, 2013

Yup, limited range may sound better.

@da-anda
Member
da-anda commented Feb 20, 2013

Did basic test on Windows, seems to work. Tested DXVA. The only thing I noticed is, that although the black of the video is brighter, the black background color of the video plane is still deep black, which looks weird (see http://images.elements-net.de/misc/xbmc-studio-level.png). With studio levels turned off it's the same black.

Another thing I noticed is that poster reflections in Confluence don't seem to work in this build - probably unrelated though.

@un1versal
Contributor

@elupus
I tested with this master + this PR
Without PR I also disabled VDPAU studio level conversion as it produced same black is greyish results.
only with this PR its not just vdpau accelerated content it applies even on GUI.

Essentially what @da-anda said holds. With setting on blacker is not blacker, and when you turn it off addons have no reflections. Without this PR (pure master compile) addon reflections are back to normal.

With setting on Black is greyish
screenshot011


With setting off Black is Black again
screenshot013

With setting off addons reflection is lost.
screenshot000

@elupus
Member
elupus commented Feb 21, 2013

Regarding reflections that need to be solved. Seem strange.

Regarding color. It's fully correct to become grey'er. You need to match this setting with your TV setting if you have one + make sure your video drivers are not doing same thing.

@elupus
Member
elupus commented Feb 21, 2013

Yea, the background still end up fully black. I'm not sure what is best. Your TV should be calibrated so you can't see a difference between the two anyway.

@jmarshallnz
Member

Looks like the colour mults are overriding the diffuse texture?

@elupus
Member
elupus commented Feb 21, 2013

Yea.. I forgot the old rule that you can do mults in any order but not addiotions in combination :). Anyway spotted some other weird stuff in there. We never setup the alpha sides of the texture stages.

@elupus elupus merged commit f65e61c into xbmc:master Mar 1, 2013
@ace20022 ace20022 commented on the diff Mar 2, 2013
xbmc/guilib/GUIFontTTFDX.cpp
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_ALPHAOP, D3DTOP_MODULATE );
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_ALPHAARG1, D3DTA_TEXTURE);
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_ALPHAARG2, D3DTA_DIFFUSE);
+ unit++;
+
+ if(g_Windowing.UseLimitedColor())
+ {
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_COLOROP , D3DTOP_ADD );
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG1, D3DTA_CURRENT) ;
+#if(1)
+ pD3DDevice->SetRenderState( D3DRS_TEXTUREFACTOR, D3DCOLOR_RGBA(16,16,16,0) );
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG2, D3DTA_TFACTOR );
+#else
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_CONSTANT , D3DCOLOR_RGBA(16,16,16,0) );
+ pD3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG2, D3DTA_CONSTANT );
+#endif
@ace20022
ace20022 Mar 2, 2013 Member

Have you forgotten to remove the #if toggle?

@ace20022 ace20022 commented on the diff Mar 2, 2013
xbmc/guilib/GUITextureD3D.cpp
{
- p3DDevice->SetTextureStageState( 1, D3DTSS_COLOROP, D3DTOP_DISABLE);
- p3DDevice->SetTextureStageState( 1, D3DTSS_ALPHAOP, D3DTOP_DISABLE);
+ m_col = D3DCOLOR_RGBA(GET_R(color) * (235 - 16) / 255
+ , GET_G(color) * (235 - 16) / 255
+ , GET_B(color) * (235 - 16) / 255
+ , GET_A(color));
+ p3DDevice->SetTextureStageState( unit, D3DTSS_COLOROP , D3DTOP_ADD );
+ p3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG1, D3DTA_CURRENT) ;
+#if(1)
+ p3DDevice->SetRenderState( D3DRS_TEXTUREFACTOR, D3DCOLOR_RGBA(16,16,16, 0) );
+ p3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG2, D3DTA_TFACTOR );
+#else
+ p3DDevice->SetTextureStageState( unit, D3DTSS_CONSTANT , D3DCOLOR_RGBA(16,16,16, 0) );
+ p3DDevice->SetTextureStageState( unit, D3DTSS_COLORARG2, D3DTA_CONSTANT );
+#endif
@ace20022
ace20022 Mar 2, 2013 Member

Same as above

@elupus
elupus Mar 2, 2013 Member

I suppose so. It's two ways of accomplishing the same thing. Wasn't sure if
the one I chose worked everywhere. But yea it should be removed.

@Memphiz
Member
Memphiz commented Apr 9, 2013

This pr has a cosmetic issue with DR. I guess once the setting is changed the whole screen has to be marked dirty. Else only the controls which are dirty reflect the range change ;) (seeing this on osx when turning the setting on and off)

@un1versal
Contributor

Memphiz I do believe you may be on to something, when using this and pressing volume up/down or mce button the screen color changes, wondering if this is what you discovered or something else.

@Memphiz
Member
Memphiz commented Apr 9, 2013

something else...

@elupus
Member
elupus commented Apr 9, 2013

It probably should force a skin reload. Shaders could need recreation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment