Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 23, 2014
Merged

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

merged 1 commit into from
Feb 23, 2014

Conversation

Shine-
Copy link

@Shine- 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
@Voyager1
Copy link

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

@a11599
Copy link

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);
}

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@wsoltys
Copy link

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.

else
return 0;
if (!m_processor->PreInit())
CLog::Log(LOGNOTICE, "CWinRenderer::Preinit - could not init DXVA2 processor - skipping");

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@Shine-
Copy link
Author

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.

{
m_renderMethod = RENDER_DXVA;
break;
}

This comment was marked as spam.

@elupus
Copy link
Contributor

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-
Copy link
Author

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-
Copy link
Author

Shine- commented Feb 19, 2014

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

@elupus
Copy link
Contributor

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

@t-nelson
Copy link
Contributor

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

@Voyager1
Copy link

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-
Copy link
Author

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.

@Voyager1
Copy link

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.

@t-nelson
Copy link
Contributor

@elupus Your button

@wsoltys
Copy link

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
@Voyager1
Copy link

Apologies button accident... Reopened

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

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

jmarshallnz added a commit that referenced this pull request Feb 23, 2014
Prevent unnecessary colorspace conversions (regression in #4163)
@jmarshallnz jmarshallnz merged commit 62d6cff into xbmc:master Feb 23, 2014
@Shine- Shine- deleted the dxvarenderer branch February 24, 2014 19:14
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants