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

[win32] Update rendering system to DirectX11. #6987

Merged
merged 28 commits into from Jul 4, 2015

Conversation

Projects
None yet
5 participants
@afedchin
Member

afedchin commented Apr 20, 2015

This is work to porting Kodi rendering system to DirectX 11. Here is testing thread on the forum http://forum.kodi.tv/showthread.php?tid=218274

New features (compared to dx9 version):

  • Caching font vertices on GPU side (see #4143)
  • Hardwarebased stereoscopic 3D (requires Win8).
  • Support interleaved stereoscopic mode.
  • New checkerboard stereoscopic mode.
  • No longer need to install the DX to the target platform, but for properly work video API this requires updates (see below)

This also require changes in ffmpeg use D3D11 Video API to decoding, see afedchin/FFmpeg@d335d99.

To properly work video API (dxva rendering and decoding) on Windows Vista/7 this requires:

need to find a way to trigger user to get that installed.

PS. The first few commits not fully reflect their essence, everything was done chaotically without a clear plan of action. Some things are reworked multiple times, as an example GUI sahders. Currently I don't see how can I change (split or squash) commits to make them more clear.

PS. This breaks bisect windows builds.

@Paxxi

This comment has been minimized.

Member

Paxxi commented Apr 20, 2015

I also work chaotically so once finished I tend to make a new branch and cherry pick related changes and squish them to make it cleaner. It's quite a lot of work though on something big like this.

Just wanna say awesome work!

@Memphiz

This comment has been minimized.

Member

Memphiz commented Apr 20, 2015

You could for example git reset all your commits (so you have them as pending changes) and then pick some added files with a nice commit message and then commit maybe removed files together and at the end when only modified files are left - git commit -p relevant chunks of them (if possible). Just an idea on how to do it. Looks like you should try to reduce the number of commits before it can be reviewed properly.

@afedchin

This comment has been minimized.

Member

afedchin commented Apr 20, 2015

Decrease amount of commits and made them more relevant to their description.

@Paxxi @Memphiz I was already thinking about both ways. But all of them make me cry.

@afedchin

This comment has been minimized.

Member

afedchin commented Apr 22, 2015

Here is branch rewritten which is easier to review master...afedchin:dx11_new, but it can break bisect. @Memphiz @Paxxi What do you think?

@Paxxi

This comment has been minimized.

Member

Paxxi commented Apr 23, 2015

Looks a lot better, I'm not a user of bisect so personally I don't care but some might disagree about it I guess.
Will try to get time during the weekend to go through it all 😊

@afedchin

This comment has been minimized.

Member

afedchin commented Apr 24, 2015

I've reviewed this and found what this broke existing screensavers/visualizations on other platforms. I've also found a few bugs. So I suggest to move to the new branch (or update this) because it contains all new fixes/improvements/cosmetics. This branch has become too difficult for support and fix. Yes I know what dx11_new break bisect, but only for windows builds.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 24, 2015

nice work! regarding

This also require changes in ffmpeg use D3D11 Video API to decoding, see afedchin/FFmpeg@d335d99.

could you ask at ffmpeg whether they prefer a configure switch or something else?

@afedchin

This comment has been minimized.

Member

afedchin commented Apr 26, 2015

I've sent the question. But I think this patch will not be accepted because it is not full. It is enough to work decoders but not enough to compile full ffmpeg.

@afedchin

This comment has been minimized.

Member

afedchin commented Apr 27, 2015

@FernetMenta I've reworked dxva related code as Hendrik suggested. No more need changes to ffmpeg.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Apr 30, 2015

No more need changes to ffmpeg.

great!

@Paxxi

This comment has been minimized.

Member

Paxxi commented May 10, 2015

It's time we get started on reviewing this properly so let's start with some ground rules

  1. There are style issues such as braces, indent, concistency and some spelling, We don't talk about that now, it can be handled in a later stage.
  2. I don't think we should spend any time on Effects11 and wether it's in the correct place, it doesn't matter at this point
  3. We don't need to discuss dxerr, same as Effects11, see http://blogs.msdn.com/b/chuckw/archive/2012/04/24/where-s-dxerr-lib.aspx for explanation of why it's needed.

My initial thoughts

I've spent the day going through all kodi code, haven't touched the visualizations yet and I find it very solid. The design seems sound and well encapsulated, it touches very little or no shared code and the separations are clean. I will spend some more time over the week going back over it as it's fairly big.

@FernetMenta I hope you have some time and or energy to review as well as you probably know the architecture of this code the best

isues so far

I though initially I'd put these as line comments but my browser doesn't agree with that so I'll stick it here instead
RenderSystemDX#532 The loop will iterate outside the array if no match is found.
RenderSystemDX#584 Setting gpu thread prio to 7 might need some consideration as it might negatively impact other applications on the system. MSDN's advise is to not touch it without lots of profiling showing that it's needed.

DXVA.cpp#1077 According to msdn no extensions are defined for DX11 so we probably shouldn't set this to a value for previous dx versions https://msdn.microsoft.com/en-us/library/windows/desktop/hh447645(v=vs.85).aspx

WinRender.cpp#1033 leaks memory if the error condition is true.

There's a few missing virtual destructors that may leads to memory leaks, mainly in RenderCapture.cpp

@afedchin

This comment has been minimized.

Member

afedchin commented May 10, 2015

@Paxxi I've added most of your comments to the commits.

@afedchin

This comment has been minimized.

Member

afedchin commented May 10, 2015

As for WinRender.cpp#1033. I agree with you. But I suggest to fix it in separate PR before Isengard release.

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented May 23, 2015

@FernetMenta @fritsch i would appreciate if you could comment if the architectural changes are within reason (if there are at all). This would give @afedchin time to do any alterations by the time we can merge this for 16.0

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented May 24, 2015

Regarding player and videorenderer there are no significant architectural changes, apart from DX9->DX11 transition of course. nice solution for keeping compatibility with ffmpeg.
should go in as soon window is open again

@afedchin

This comment has been minimized.

Member

afedchin commented Jun 3, 2015

damn, after rebase fritsch's comment was lost. it can be found here afedchin@5e8a30d#commitcomment-11474698

@afedchin

This comment has been minimized.

Member

afedchin commented Jul 3, 2015

so time to final review I think.
@FernetMenta @Paxxi objection? suggestions?

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Jul 4, 2015

if nothing substantial has changed here, I am still ok woth this

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 4, 2015

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jul 4, 2015

Needs a rebase

@afedchin

This comment has been minimized.

Member

afedchin commented Jul 4, 2015

rebased
jenkins build this please

@afedchin afedchin changed the title from [RFC] Update rendering system to DirectX11. to [win32] Update rendering system to DirectX11. Jul 4, 2015

@Paxxi

This comment has been minimized.

Member

Paxxi commented Jul 4, 2015

You have my ok, merge when possible to avoid having to rebase this more than necessary

@afedchin

This comment has been minimized.

Member

afedchin commented Jul 4, 2015

just to be sure jenkins build this please again

afedchin added a commit that referenced this pull request Jul 4, 2015

Merge pull request #6987 from afedchin/dx11
[win32] Update rendering system to DirectX11.

@afedchin afedchin merged commit f3b994f into xbmc:master Jul 4, 2015

1 check passed

default Merged build finished.
Details

@afedchin afedchin deleted the afedchin:dx11 branch Feb 17, 2016

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