Prevent unnecessary colorspace conversions (regression in #4163) #4192

Merged
merged 1 commit into from Feb 23, 2014

Conversation

Projects
None yet
9 participants

Shine- commented Feb 11, 2014

Previous PR disables all color spaces except YV12 for all renderers in Windows, not just for DXVA2/-HD. See forum for details: http://forum.xbmc.org/showthread.php?tid=183975&pid=1621870#pid1621870
This is the same patch minus the debug code.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 12, 2014

Member

Voyager1 commented Feb 12, 2014

patch looks fine to me. @wsoltys @a11599 - your thoughts?
jenkins build this please

Contributor

a11599 commented Feb 12, 2014

Looks good to me as well.

+ {
+ m_formats.push_back(RENDER_FMT_YUV420P10);
+ m_formats.push_back(RENDER_FMT_YUV420P16);
+ }
@elupus

elupus Feb 12, 2014

Member

This is rather ugly.. You disable HI10P acceleration when gui setting for DXVA is enabled.

@Voyager1

Voyager1 Feb 12, 2014

Member

Hi10P acceleration? not sure I understand this comment: I didn't think there was any HW for Hi10P, and we're talking about rendering here, which we found out not to render in any other format than RENDER_FMT_YUV420P - see #4163

@Shine-

Shine- Feb 12, 2014

I think I know what he means (and I think I misunderstood him in my previous comment). With acceleration, he means "render speedup by not converting color space", which is what he introduced with #845. @elupus: Yes, I disable your render speedup because none of the two DXVA renderers currently available can render anything else than YV12-8bit, resulting in in a black screen on attempted Hi10P playback. This is more of a workaround right now - the proper fix would be to completely overhaul the DXVA2 and DXVA-HD renderers to (1) accept any color space the processor supports instead of just YV12 and (2) do the color space conversion as late as possible to maximize quality. Probably a post-Gotham ToDo...

@elupus

elupus Feb 12, 2014

Member

Or just skip DXVA renderers if player tries to configure with YUV420P10/16 and fall back to non DXVA based rendering.

@elupus

elupus Feb 12, 2014

Member

Which from what i can tell: "CWinRenderer::SelectRenderMethod()" will already do. So there is no reason to not advertise support for these formats. If DXVA renderer support it doesn't matter.

@elupus

elupus Feb 13, 2014

Member

Why should it it accept a format it doesn't support and later possible
crash? That is beyond me.

When player configures renderer it is too late to make any further
decisions.

I can buy an argument that it should disable support in the supports call,
to force colorspace conversion in player.

But that still doesn't say that we should crash if something else is
requested.

That is the whole point of the fall through code after processor open.
On Feb 13, 2014 7:40 AM, "Voyager1" notifications@github.com wrote:

In xbmc/cores/VideoRenderers/WinRenderer.cpp:

{

  • m_formats.push_back(RENDER_FMT_YUV420P10);
  • m_formats.push_back(RENDER_FMT_YUV420P16);
  • if (g_Windowing.IsTextureFormatOk(D3DFMT_L16, 0))
  • {
  •  m_formats.push_back(RENDER_FMT_YUV420P10);
    
  •  m_formats.push_back(RENDER_FMT_YUV420P16);
    
  • }

And so it always ends... (Sigh)
@Shine- https://github.com/Shine- please be a bit more patient and
persistent, some team devs are quite difficult to work with. This is not
the moment to give up and who knows ultimately differences of opinion and
language barriers may be removed and an even better solution could come out
of this.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/4192/files#r9699251
.

@Voyager1

Voyager1 Feb 13, 2014

Member

I believe @Shine- 's point is that when you select DXVA for rendering it's going to be chosen all the time, so there is no fallback option. Prior to #4163 Hi10p gave black screen on DXVA rendering, but worked fine when choosing "Automatic" rendering - which essentially uses software rendering for anything not-HW-decoded, and DXVA rendering for all DXVA-decoded material. Some users have DXVA rendering forced ON because they want to take advantage of (better) HW scalers and deinterlacers e.g. for DVDs. However, I don't see the point for Hi10p to use DXVA rendering, because there's usually no need for deinterlacing or scaling.
That said, current code has an issue either way, and I thought the proposed solution here is simple and would do the trick for Gotham, and we can address it properly after G.

@elupus

elupus Feb 13, 2014

Member

HI10P is heavy heavy heavy to decode. Any little extra performance helps. If you force a color space conversion to yv12 (or something else non 16bit) you, not only, loose quality, you also likely make it unplayable on some devices.

And as you say, there is no point in using DXVA for hi10p. We should avoid options that users have to toggle on and off based on what file they are playing.

With my proposed change: DXVA would be selected for all non hi10p formats and non DXVA renderer would be used for the other. Ie keeping pretty much best of both worlds.

@Shine-

Shine- Feb 13, 2014

There is no "later possible crash". In Pre-Init(), we make sure that only color space(s) are allowed that are actually supported, ie. YV12 here. You either convert in the renderer, right before output (not yet implemented in the DXVA2/-HD renderer), or you do it before processing if you have to (this PR).

As for your proposed change, no, it will be a major performance hit for people with low-performance GPUs (during fallback to PS rendering) or low-performance CPUs (during fallback to SW rendering), effectively disabling fluent Hi10P playback for them.

Performance was the whole point why I have been using this patch for almost 2 years now: I have an Intel G45 and using PS scalers for 10bit is a much worse performance impact than converting to 8-bit and using DXVA scalers, especially when actual scaling is involved (e.g. 720p->1080p). Color space conversion and subsequent DXVA scaling gives fluent playback, always, PS scaling does not.
That's why I said we should never, ever, fallback to non-DXVA rendering when a user actively chooses DXVA in the GUI settings.

@elupus

elupus Feb 13, 2014

Member

I'm a bit surprised that you get higher performance with that software color space conversion. I'm not going to argue that since I've not tested myself.

I'm fine with not reporting the 16bit formats as supported when dxva is forced. Thus most of this patch (i have some cosmetic comments).

That however does not change the fact that the DXVA processors should not accept formats they don't support. If we don't allow fallback. The Winrenderer::Configure call should fail so the player is notified of it.

Member

wsoltys commented Feb 12, 2014

@Voyager1 can't comment on that one as its not my area of knowledge. Pull it in if you guys and elupus are fine with it.

xbmc/cores/VideoRenderers/WinRenderer.cpp
- else
- return 0;
+ if (!m_processor->PreInit())
+ CLog::Log(LOGNOTICE, "CWinRenderer::Preinit - could not init DXVA2 processor - skipping");
@afedchin

afedchin Feb 12, 2014

Member

I think here should be SAFE_DELETE(m_processor) to prevent memory leak.

@Shine-

Shine- Feb 12, 2014

I was wondering about that as well, but I assumed you left it out for a reason in #3589. Could you double-check again? I will gladly add it to this PR if needed!

@afedchin

afedchin Feb 12, 2014

Member

I've checked this. You are right. No need destroy processor here. It used in SelectRenderMethod. Or we can destroy it here to not keep in memory partially initialised processor and check m_processor for null value in SelectRenderMethod. So leave as is.

xbmc/cores/VideoRenderers/WinRenderer.cpp
-
- if (g_Windowing.IsTextureFormatOk(D3DFMT_L16, 0))
+ // allow other color spaces if DXVA rendering is not used or not available
+ if (!processor_ok || m_iRequestedMethod != RENDER_METHOD_DXVA && m_iRequestedMethod != RENDER_METHOD_DXVAHD)
@elupus

elupus Feb 13, 2014

Member

Switch processor_ok for m_processor == NULL
Drop the m_iRequestedMethod check (if we have a processor that should be used, as mentioned in other discussion)

@elupus

elupus Feb 13, 2014

Member

Also.. it would be nice if the CProcessor interface had a way to get a list of supported render formats. It could differ between DXVA and DXVAHD. But can be done later.

@Shine-

Shine- Feb 13, 2014

"Drop the m_iRequestedMethod check" - that's basically what #4163 did, introducing the whole regression because g_advancedSettings.m_DXVAForceProcessorRenderer is always true (because of another bug that causes XBMC to crash when trying to PS-scale a DXVA decoded frame).

@Shine-

Shine- Feb 13, 2014

Also, "Switch processor_ok for m_processor == NULL" won't work, because m_processor isn't NULL - it's used in SelectRenderMethod() later, so we can't SAFE_DELETE it yet. I could add afedchin's proposed change, though (see earlier comment). I will have a look at it after work. Thanks for the suggestion!

Shine- commented Feb 18, 2014

Not sure whether this still has a chance of getting merged or #4201 is preferred now.
Either way, I cleaned it up as requested, removed the uninitialized processor from memory as suggested by afedchin and fixed mergeability with the current tree.

xbmc/cores/VideoRenderers/WinRenderer.cpp
+ {
+ m_renderMethod = RENDER_DXVA;
+ break;
+ }
@elupus

elupus Feb 19, 2014

Member

Strange indent

xbmc/cores/VideoRenderers/WinRenderer.cpp
-
- if (g_Windowing.IsTextureFormatOk(D3DFMT_L16, 0))
+ // allow other color spaces besides YV12 in case DXVA rendering is not used or not available
+ if (!m_processor || m_iRequestedMethod != RENDER_METHOD_DXVA && m_iRequestedMethod != RENDER_METHOD_DXVAHD)
@elupus

elupus Feb 19, 2014

Member

Some (brackets) would be good here. It's correct as it is. But can be read wrong easily.

xbmc/cores/VideoRenderers/WinRenderer.cpp
@@ -396,8 +400,7 @@ unsigned int CWinRenderer::PreInit()
m_iRequestedMethod = CSettings::Get().GetInt("videoplayer.rendermethod");
- if (g_advancedSettings.m_DXVAForceProcessorRenderer || m_iRequestedMethod == RENDER_METHOD_DXVA
- || m_iRequestedMethod == RENDER_METHOD_DXVAHD)
+ if (g_advancedSettings.m_DXVAForceProcessorRenderer || m_iRequestedMethod == RENDER_METHOD_DXVA || m_iRequestedMethod == RENDER_METHOD_DXVAHD)
@elupus

elupus Feb 19, 2014

Member
if (g_advancedSettings.m_DXVAForceProcessorRenderer 
||  m_iRequestedMethod == RENDER_METHOD_DXVA 
||  m_iRequestedMethod == RENDER_METHOD_DXVAHD)

or

if (g_advancedSettings.m_DXVAForceProcessorRenderer ||
    m_iRequestedMethod == RENDER_METHOD_DXVA ||
    m_iRequestedMethod == RENDER_METHOD_DXVAHD)

Isn't m_iRequestedMethod implied by m_DXVAForceProcessorRenderer? Just a thought.

@Shine-

Shine- Feb 19, 2014

It's not. That's what escaped me in the original patch #4163 as well. m_DXVAForceProcessorRenderer is, I'd say, misnamed, it should better be called "m_ForceProcessorRendererAfterDXVADecode" because that's what it does (and nothing else). It has no effect at all when SW decoding is used.

{
CLog::Log(LOGNOTICE, "D3D: unable to open DXVA processor");
- m_processor->Close();
+ if (m_processor)
+ m_processor->Close();
@elupus

elupus Feb 19, 2014

Member

Why not SAFE_DELETE here?

Member

elupus commented Feb 19, 2014

I noticed an issue. CWinRenderer::Configure can be called multiple times with different foramts without PreInit being called inbetween.

Thus SelectRenderMethod can not delete m_processor if we don't create it there.

Shine- commented Feb 19, 2014

I guess that's the answer why "m_processor->Close();" instead of SAFE_DELETE in SelectRenderMethod(). Admitted, I didn't think that far, I just figured it's that way for a reason and kept it as is... ;-)

Shine- commented Feb 19, 2014

Fixed indent (grml...), changed formatting and added brackets for readability.

Member

elupus commented Feb 19, 2014

I'm alright with this pull now. I've not tested it thou, so that needs to be done.

jenkins build this please

Contributor

t-nelson commented Feb 20, 2014

@Voyager1 @wsoltys @a11599 mind testing and reporting back?

Member

Voyager1 commented Feb 20, 2014

just tested with hi10p material, on the following renderer settings dxva, pixelshaders, automatic. All works fine, the only thing is I cannot see in PS mode whether the color space is converted or not, my eyes aren't that precise :)
Is there anything to look for in a log or in the on screen stats (when pressing o) ?

Shine- commented Feb 20, 2014

Not when pressing 'o', but if you check the debug log. There's a line that tells you which color space is being used as destination. If you play a Hi10P file w/ PS rendering without this second patch, it's going to say that the destination color space is "YV12". With this patch, it says "YV12P10". Can't tell you the exact wording since I'm still at work and can't check my saved logs. But I believe that it says "YV12" and "YV12P10", not "YUV420P" and "YUV420P10". Should be easy to find the line.

Member

Voyager1 commented Feb 20, 2014

ok, thanks that was easy:
DXVA

19:58:48 T:6112   DEBUG: CDVDPlayerVideo::OutputPicture - change configuration. 1920x1038. framerate: 23.98. format: YV12

all others (Software, PS, Auto)

20:02:24 T:7364   DEBUG: CDVDPlayerVideo::OutputPicture - change configuration. 1920x1038. framerate: 23.98. format: YV12P10

And indeed I also tested without the patch and I'm only getting YV12.
From my perspective it's good to go.

Contributor

t-nelson commented Feb 20, 2014

@elupus Your button

Member

wsoltys commented Feb 20, 2014

Don't have much to test with but I'm fine with @Voyager1 tests.

@Voyager1 Voyager1 closed this Feb 21, 2014

@Voyager1 Voyager1 reopened this Feb 21, 2014

Member

Voyager1 commented Feb 21, 2014

Apologies button accident... Reopened

@Voyager1 Voyager1 modified the milestones: H* 14.0-alpha1, Pending for inclusion Feb 21, 2014

Member

Voyager1 commented Feb 22, 2014

@elupus - if no further comments, please hit the merge button.

jmarshallnz added a commit that referenced this pull request Feb 23, 2014

Merge pull request #4192 from Shine-/dxvarenderer
Prevent unnecessary colorspace conversions (regression in #4163)

@jmarshallnz jmarshallnz merged commit 62d6cff into xbmc:master Feb 23, 2014

1 check passed

default Merged build #247 succeeded in 58 min
Details

@Shine- Shine- deleted the Shine-:dxvarenderer branch Feb 24, 2014

@MartijnKaijser MartijnKaijser modified the milestones: Gotham13.0-rc1, Helix 14.0-alpha1 May 21, 2014

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