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

[windows] Enable shader based HQ scalers for DXVA renderer #838

Merged
merged 1 commit into from Aug 31, 2013

Conversation

Projects
None yet
10 participants
@a11599

a11599 commented Apr 1, 2012

The patch enables pixel shader based scaling with DXVA rendering. This is practically a dusted down patch from CrystalP with an added GPU flush. The flush is required to prevent stuttering in ION (and possibly other cards). The impact on image quality depends on the GPU. Some do only bilinear even if they would have the ponies to do better; in those cases the quality improvement is very noticeable.

According to my tests, thanks to CrystalP's scaler speedup it can now do 1080p upscaling from any source up to 60 fps on the 40 shader Ati 3450 using 'optimized' scalers and tops out at ~45 fps using unoptimized ones. More powerful GPUs should do 60 fps even with unoptimized scalers easily.

By default, the DXVA renderer will default to DXVA scaling when scaling method is set to "Automatic" for a number of reasons (see in comments).

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Apr 1, 2012

Member

The settings nazi has to ask (without bothering to check the code in detail :p) whether it's reasonable to have the new setting. If it performs well enough on most hardware, why not just have it on? I presume the user can set it back to use whatever the DXVA default is anyway via the VideoOSD, right?

Member

jmarshallnz commented Apr 1, 2012

The settings nazi has to ask (without bothering to check the code in detail :p) whether it's reasonable to have the new setting. If it performs well enough on most hardware, why not just have it on? I presume the user can set it back to use whatever the DXVA default is anyway via the VideoOSD, right?

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Apr 2, 2012

Good point. The setting comes from times before CrystalP's scaler speedup, so it is not really needed, so I will remove it. However, I would keep DXVA scaling as a default for automatic for three reasons:

  1. Ati drivers come out of the box with edge enhancement enabled. This was designed to work after scaling. So unless the user explicitly disables EE in CCC, our HQ scalers look worse than even DXVA bilinear (at least to me).
  2. There are some really weak GPUs out there (Intel HD) that have minuscule shader performance and cannot do any of our PS based scalers.
  3. Newer cards actually offer very good DXVA scaling. My HD 6570 does look like spline, it has a very nice sharp scaler with very little ringing. I expect this trend to continue. ;)

People preferring these scalers can just set their preferred one as a default for all videos and that's it. :)

a11599 commented Apr 2, 2012

Good point. The setting comes from times before CrystalP's scaler speedup, so it is not really needed, so I will remove it. However, I would keep DXVA scaling as a default for automatic for three reasons:

  1. Ati drivers come out of the box with edge enhancement enabled. This was designed to work after scaling. So unless the user explicitly disables EE in CCC, our HQ scalers look worse than even DXVA bilinear (at least to me).
  2. There are some really weak GPUs out there (Intel HD) that have minuscule shader performance and cannot do any of our PS based scalers.
  3. Newer cards actually offer very good DXVA scaling. My HD 6570 does look like spline, it has a very nice sharp scaler with very little ringing. I expect this trend to continue. ;)

People preferring these scalers can just set their preferred one as a default for all videos and that's it. :)

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Apr 18, 2012

Removed the setting. If there are no objections, I would like to merge this in the May window.

a11599 commented Apr 18, 2012

Removed the setting. If there are no objections, I would like to merge this in the May window.

@ghost ghost assigned chadoe and CrystalP Apr 19, 2012

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Apr 19, 2012

Member

Looks good to me once squashed down - your thoughts @CrystalP

Member

jmarshallnz commented Apr 19, 2012

Looks good to me once squashed down - your thoughts @CrystalP

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Apr 27, 2012

Rebased, tested and squashed, I think this is ready to go now.

a11599 commented Apr 27, 2012

Rebased, tested and squashed, I think this is ready to go now.

@CrystalP

View changes

Show outdated Hide outdated xbmc/cores/VideoRenderers/WinRenderer.cpp Outdated
@CrystalP

This comment has been minimized.

Show comment
Hide comment
@CrystalP

CrystalP Apr 29, 2012

Contributor

Changing the scaling method while viewing a video gives me a black screen for the HQ scalers.

The point of the patch was originally to help with the quality of the upscaling on the Ion (was like bilinear). But it stuttered once in a while, making the exercise pointless. Since other GPUs already have similar quality to the xbmc scalers, the patch didn't make sense to apply.

If you verified that the playback is 100% smooth on Ion thanks to the GPU flush, fine. The important frame rates are 24 and 25.
Did you try what happens with interlaced material? The scaler selection code has a fallback to bilinear that could bite us since there is no bilinear scaler implemented here.

Contributor

CrystalP commented Apr 29, 2012

Changing the scaling method while viewing a video gives me a black screen for the HQ scalers.

The point of the patch was originally to help with the quality of the upscaling on the Ion (was like bilinear). But it stuttered once in a while, making the exercise pointless. Since other GPUs already have similar quality to the xbmc scalers, the patch didn't make sense to apply.

If you verified that the playback is 100% smooth on Ion thanks to the GPU flush, fine. The important frame rates are 24 and 25.
Did you try what happens with interlaced material? The scaler selection code has a fallback to bilinear that could bite us since there is no bilinear scaler implemented here.

@CrystalP

This comment has been minimized.

Show comment
Hide comment
@CrystalP

CrystalP Apr 30, 2012

Contributor

More info: on ATI 4550 this works, on nVidia G210 (similar to Ion G2) it doesn't.

Contributor

CrystalP commented Apr 30, 2012

More info: on ATI 4550 this works, on nVidia G210 (similar to Ion G2) it doesn't.

@CrystalP

View changes

Show outdated Hide outdated xbmc/cores/VideoRenderers/WinRenderer.cpp Outdated
@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 May 6, 2012

It worked originally on the ION and was 100% stutter free with the GPU flush (before the scaler speedup) for 24p/25p. I used it for months. Something must have happened in between that broke this on nVidia. Unfortunately I have no nVidia HW near me that makes it difficult to fix.

Agree with the rest of the comments, will do them, I need to adjust the patch anyways. If you have some suggestions on what to do to fix the nVidia issue that would be great.

Actually there are more GPUs that do awful scaling, Ati 3450, 3850 and 5145 are three examples (bilinear). So the patch might be beneficial for more people.

a11599 commented May 6, 2012

It worked originally on the ION and was 100% stutter free with the GPU flush (before the scaler speedup) for 24p/25p. I used it for months. Something must have happened in between that broke this on nVidia. Unfortunately I have no nVidia HW near me that makes it difficult to fix.

Agree with the rest of the comments, will do them, I need to adjust the patch anyways. If you have some suggestions on what to do to fix the nVidia issue that would be great.

Actually there are more GPUs that do awful scaling, Ati 3450, 3850 and 5145 are three examples (bilinear). So the patch might be beneficial for more people.

@PKOneTwo

This comment has been minimized.

Show comment
Hide comment
@PKOneTwo

PKOneTwo May 8, 2012

Just an FYI: using ATI 5450 and 6450. And the quality is more then visible (with patch integrated and) using Lanczos3 compared to DXVA setting!

PKOneTwo commented May 8, 2012

Just an FYI: using ATI 5450 and 6450. And the quality is more then visible (with patch integrated and) using Lanczos3 compared to DXVA setting!

@PKOneTwo

This comment has been minimized.

Show comment
Hide comment
@PKOneTwo

PKOneTwo May 13, 2012

Since i really love to have better quality then DXVA scaler (lanczos3 for example) i just edited the patch file, so it suits with the changes in master (lines, the one and the other changes of code):

http://pastebin.com/raw.php?i=qhSSYuQt
have fun

PKOneTwo commented May 13, 2012

Since i really love to have better quality then DXVA scaler (lanczos3 for example) i just edited the patch file, so it suits with the changes in master (lines, the one and the other changes of code):

http://pastebin.com/raw.php?i=qhSSYuQt
have fun

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Feb 12, 2013

Member

any love left for this PR? Having PVR now some HQ scalers (and deinterlacers) would really be nice to have.

Member

da-anda commented Feb 12, 2013

any love left for this PR? Having PVR now some HQ scalers (and deinterlacers) would really be nice to have.

@BrannonKing

This comment has been minimized.

Show comment
Hide comment
@BrannonKing

BrannonKing commented Feb 18, 2013

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser Aug 5, 2013

Member

@a11599
care to update this PR?

Member

MartijnKaijser commented Aug 5, 2013

@a11599
care to update this PR?

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Aug 6, 2013

Will do.

a11599 commented Aug 6, 2013

Will do.

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Aug 7, 2013

OK, here we go again. Rebased and adjusted code as @CrystalP requested. This is my last take on this subject. Please raise your concerns if any. I would be glad to receive test results too.

I tested progressive, interlaced, DXVA and software decoded video and have not found any problems but all my HW is ATI. Some more test results in this thread: http://forum.xbmc.org/showthread.php?tid=127174. I got reports from nV users (no black screen issue as reported by CrystalP).

a11599 commented Aug 7, 2013

OK, here we go again. Rebased and adjusted code as @CrystalP requested. This is my last take on this subject. Please raise your concerns if any. I would be glad to receive test results too.

I tested progressive, interlaced, DXVA and software decoded video and have not found any problems but all my HW is ATI. Some more test results in this thread: http://forum.xbmc.org/showthread.php?tid=127174. I got reports from nV users (no black screen issue as reported by CrystalP).

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser Aug 7, 2013

Member

@a11599
added test build including this patch in the forum thread.
Perhaps you could add to the thread what exactly needs testing (or perhaps start dedicated thread)
I have nvidia and an intel onboard GPU so i can do some testing as well. Played several video so far without problems

Member

MartijnKaijser commented Aug 7, 2013

@a11599
added test build including this patch in the forum thread.
Perhaps you could add to the thread what exactly needs testing (or perhaps start dedicated thread)
I have nvidia and an intel onboard GPU so i can do some testing as well. Played several video so far without problems

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Aug 8, 2013

@MartijnKaijser thanks for the testing. I added details on what to look for in the forum thread.

a11599 commented Aug 8, 2013

@MartijnKaijser thanks for the testing. I added details on what to look for in the forum thread.

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser
Member

MartijnKaijser commented Aug 8, 2013

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Aug 9, 2013

Spotted a rebase problem which probably broke 3D playback.

a11599 commented Aug 9, 2013

Spotted a rebase problem which probably broke 3D playback.

@a11599

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Aug 30, 2013

Last call; based on the positive feedbacks I am about to squash down and merge this in the September window. @MartijnKaijser thanks for your help in arranging the testing.

a11599 commented Aug 30, 2013

Last call; based on the positive feedbacks I am about to squash down and merge this in the September window. @MartijnKaijser thanks for your help in arranging the testing.

a11599 pushed a commit that referenced this pull request Aug 31, 2013

a11599
Merge pull request #838 from a11599/dxvahqscaler
[windows] Enable shader based HQ scalers for DXVA renderer

@a11599 a11599 merged commit 390def3 into xbmc:master Aug 31, 2013

@a11599 a11599 deleted the a11599:dxvahqscaler branch Aug 31, 2013

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Sep 17, 2014

Member

when using NVida gfx and render method DXVA this results into a black screen @a11599 any idea?

Member

FernetMenta commented on 3c31288 Sep 17, 2014

when using NVida gfx and render method DXVA this results into a black screen @a11599 any idea?

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Sep 17, 2014

Works for me with Helix alpha 2 on nV 840M just fine... Some change in between broke it then.

a11599 replied Sep 17, 2014

Works for me with Helix alpha 2 on nV 840M just fine... Some change in between broke it then.

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Sep 17, 2014

Member

can you fix it for current master?

Member

FernetMenta replied Sep 17, 2014

can you fix it for current master?

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Sep 17, 2014

Member

@ace20022 what NVidia card have you tested with. I have a Quadro K1000M which shows a black screen

Member

FernetMenta replied Sep 17, 2014

@ace20022 what NVidia card have you tested with. I have a Quadro K1000M which shows a black screen

This comment has been minimized.

Show comment
Hide comment
@a11599

a11599 Sep 17, 2014

No. It was originally CrystalP's patch, I just rebased it for Gotham.

a11599 replied Sep 17, 2014

No. It was originally CrystalP's patch, I just rebased it for Gotham.

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Sep 17, 2014

Member

GeForce 9500 GS

Member

ace20022 replied Sep 17, 2014

GeForce 9500 GS

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Sep 17, 2014

Member

@afedchin do you have an idea why this fails?

Member

FernetMenta replied Sep 17, 2014

@afedchin do you have an idea why this fails?

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