RFC: Play10bit #845

merged 10 commits into from May 2, 2012


None yet

4 participants

elupus commented Apr 4, 2012

So, this branch adds support for avoiding the 10bit to 8bit color space conversion and letting the GPU get the full precision data.

I mainly want some comments on the WIP commit. I really don't like including that dvdplayer header in there. At the same time i'd rather not pull in renderer headers into dvdplayer.

Pulling in ffmpeg's pixfmt might be one option. But i'm not sure how we would handle any pixelformats not defined by that. Comments are welcome.


A separate header (or using rendermanager or something similar) seems the best way to go, yeah.

elupus commented Apr 11, 2012

Christ that made the diff of this thing grow... I suppose the first 2 commits are really quite separate since they are mainly refactoring.

elupus commented Apr 16, 2012

@CrystalP could you give this a whirl on windows with dxva? Just want to make sure i didn't break anything there.
@davilla A test on OSX would be nice

davilla commented Apr 16, 2012

borked in osx, no video and where the video would be is pure red.

davilla commented Apr 17, 2012

Any clues why I see red, I poke around and after climbing out of the m_format showing RENDER_FMT_NONE hole, I could not see anything obvious.

elupus commented Apr 17, 2012

No not really. Will give linux a go later. Tested on win last time I worked
on it. Can have missed something.

@CrystalP CrystalP commented on an outdated diff Apr 17, 2012
unsigned int texWidth;
DefinesMap defines;
- if (fmt == YV12)
+ if (fmt == RENDER_FMT_YUV420P16)
+ {
+ defines["XBMC_YV12"] = "";
+ texWidth = sourceWidth;
+ if(!m_YUVPlanes[0].Create(texWidth , m_sourceHeight , 1, 0, D3DFMT_L16, D3DPOOL_DEFAULT)
+ || !m_YUVPlanes[1].Create(texWidth / 2, m_sourceHeight / 2, 1, 0, D3DFMT_L16, D3DPOOL_DEFAULT)
+ || !m_YUVPlanes[2].Create(texWidth / 2, m_sourceHeight / 2, 1, 0, D3DFMT_L16, D3DPOOL_DEFAULT))
+ {
+ CLog::Log(LOGERROR, __FUNCTION__": Failed to create YV12 planes.");
CrystalP Apr 17, 2012 Contributor

would be nice to include the bit depth. same for 10bit.

@CrystalP CrystalP commented on the diff Apr 17, 2012
@@ -405,6 +377,17 @@ unsigned int CWinRenderer::PreInit()
if ((g_advancedSettings.m_DXVAForceProcessorRenderer || m_iRequestedMethod == RENDER_METHOD_DXVA) && !m_processor.PreInit())
CLog::Log(LOGNOTICE, "CWinRenderer::Preinit - could not init DXVA2 processor - skipping");
+ m_formats.push_back(RENDER_FMT_YUV420P);
+ if(g_Windowing.IsTextureFormatOk(D3DFMT_L16, 0))
CrystalP Apr 17, 2012 Contributor

OK. It could be a good idea to also call a new SupportedFormats function in CWinShader, to reduce the risk of problems later.
It's not a big deal and could be added later.

@CrystalP CrystalP commented on an outdated diff Apr 17, 2012
@@ -1200,26 +1189,40 @@ void YUVBuffer::Clear()
- case YV12:
+ case RENDER_FMT_YUV420P16:
+ {
+ wmemset((wchar_t*)planes[PLANE_Y].rect.pBits, 32768, planes[PLANE_Y].rect.Pitch * m_height / 2);
CrystalP Apr 17, 2012 Contributor

zero, not 32768 I think


I'll give dxva decoding and/or rendering a go tonight. Did you benchmark CPU/GPU usage?

elupus commented Apr 18, 2012

Sådär, hade blivit en drös buggar i senaste rebasen.. @davilla nu får du gärna försöka igen.

@CrystalP har inte gjort några speciella benchmarks, men det borde hjälpa en del. Det skippar en memcpy med shift på varje byte.

elupus commented Apr 18, 2012

lol.. i wonder why i wrote that in swedsh.. A translation might be in order:

There we go, had managed to introduce a bunch of bugs in the last rebase.. @davilla feel free to give it a try again.

@CrystalP havn't done any real benchmarks, but it should help some. It skips a memcpy with a shift on each byte.


OK, tried it and software decoding + pixel shaders or software render work
dxva decoding works.
software decoding + dxva renderer on 10 bit material doesn't work because a 10/16 bit copy to GPU memory is not implemented in CProcessor::Add() of dxva.cpp. But would it make sense to add, instead of letting ffmpeg convert to 8bit?

elupus commented Apr 19, 2012

Depends on if dxva processor can handle it. Our 10 bit data is stored in
the lower bits of a 16bit value. So one might need to scale it. If that
can't be done on gpu, its rather pointless.


I'll try it but I'd be surprised if they could.

In the meantime, something is screwing with dxva rendering after dxva decoding - nVidia drivers compensate, but ATI's do not.

At the beginning of CWinRenderer::SelectRenderMethod, m_Processor.Open() is called. Problem is that as its last parameter, it expects a D3D input data format for the processor, ie the format of the data after decoding (for ex. MAKEFOURCC('Y','V','1','2')). With this PR, it receives RENDER_FMT_DXVA, which means nothing to D3D and fails badly with ATI when querying processor capabilities, creating textures, ...

elupus commented Apr 20, 2012

Right. I knew there was something with that.

elupus commented Apr 20, 2012

@CrystalP could you give it a go again?

elupus commented Apr 22, 2012

Okey, so I've rebased and squashed in changes to their rightful place. Should be ready to go I think. Still needs a test on GLES platforms thou.

davilla commented Apr 23, 2012

gles and hw codec exclude fixes -> http://dl.dropbox.com/u/14341410/gles-10bit.patch

elupus commented Apr 23, 2012

what format was that? doesn't seem to be a text file?

davilla commented Apr 23, 2012

opps, my bad, dropped an alias instead of the real file. Try again with same URL.

elupus commented Apr 23, 2012

Thanx @davilla, have incorporated the changes. Noticed some ordering errors that would cause bisecting errors. I think they should be bisectable now (have not tested thou)

I think this should be ready to pull now.

@CrystalP CrystalP and 1 other commented on an outdated diff Apr 24, 2012
@@ -115,6 +117,13 @@ void CalculateYUVMatrix(TransformMatrix &matrix
, - 16.0f / 255
, - 16.0f / 255);
+ if(format == RENDER_FMT_YUV420P10)
+ {
+ matrix *= TransformMatrix::CreateScaler(64.0
+ , 64.0
+ , 64.0);
+ }
CrystalP Apr 24, 2012 Contributor

Shouldn't there be a 16 bit case as well?

elupus May 1, 2012 Member

And my response to this apperently got lost (this is getting annoying github). No.. For the 16bit case we are filling the full 16 bit. In the 10 bit case we are using the lower 10 bits of a 16bit value. That said. the 64 is somewhat wrong since we won't be getting the full 16bit range then. ie. 03FF will turn into FFD0 instead of FFFF.

davilla commented Apr 26, 2012

elupus, have you tried mpeg2 iso's ? I'm crashing in CopyPicture in iOS.


OK now for dxva decoding. I checked the dxva processors, they only accept a few 8 bit formats. Maybe future GPUs or drivers will add more but that's it for now, and they can't work properly with data direct from ffmpeg.

Gave a DVD a spin, all renderers are fine with libmpeg2 decoding on Windows.

davilla commented Apr 27, 2012

ok, mpeg2 iso's under iOS are fine with a recent pull. must have been some local issue.

OSX/iOS signoff for inject.

davilla commented Apr 30, 2012

ping elupus, found two more, see http://pastebin.com/qUR6phWY

elupus commented May 1, 2012

I intend to merge this in a day or two. I'll look at that copy & paste regarding profiles later and factor it out into DVDCodecUtils most likely.

elupus commented May 1, 2012

That profile change is technically separate from this pull request and could have been applied before this goes in. But it can go in with this pull too.

@elupus elupus merged commit 41887d5 into xbmc:master May 2, 2012
@HolgerW1 HolgerW1 pushed a commit to HolgerW1/xbmc that referenced this pull request Sep 26, 2014
Patrick Vos Merge branch 'PR-845_PP_filter_extensions' into development
* PR-845_PP_filter_extensions: closes gh-845
  Add extension filter to associated files for Post Processing
@HolgerW1 HolgerW1 pushed a commit to HolgerW1/xbmc that referenced this pull request Sep 26, 2014
Patrick Vos Merge branch 'development'
Updated **Womble** provider to use 'tv-sd' and 'tv-hd' categories 
instead of the now defunct 'TV-x264'.

Add extension filter to associated files for Post Processing #845
  * One case example is if you only want subs, you could create the 
    filter '.idx, .sub, .srt'. We strip spaces and ignore case when 
    matching against the filter.
  * This is useful for those that do not have the ability to setup a 
    cleanup script in their downloader.

Re-arrange snatched+download SQL to be easier to read and compare (split
snatched and download out).
  * Show **downloaded+snatched** on homepage rather than the combine
    show tooltip that explains it #842
  * Fix sorting shows with more than 100k episodes

Cleaned up season pack vs single episode searching logic
  * Added check to only consider a full season pack if the last episode
in the 
    season airdate is older than 7 days or if the last aired ep is older
than 'today'.
  * Fix **BTN** backlog search for single episode searches, remove
duplicate search parameters.

Fix **OMGWTFNZBS** proper/repack search

Post-Processing fixes
  * Fix release name not set on air-by-date shows during post-processing
  * Fix processing season 0

Change clean proper name to check against history
Should prevent a proper from being re-downloaded 
(such as the reposted '-RP' releases)

Change setting status to **UNAIRED** for future episodes (GC-2397)

Revised Roman Numerals conversion routine to be stricter
(ex: IIII is not 4, IL is not 1) and raise error instead of defaulting
to 0.

Regex tweaks. Pull #841
  * Add '480p' to our regex to reduce the chance of it being matched 
    as the season/ep or multi-ep reference.
  * Filter out releases like
    and 'Rome.S02.DiRFiXES.720p.BluRay.x264-SiNNERS' as they are useless
for our purposes.

Season 0 fix. Pull #843
  * Exclude specials from being changed to wanted automatically because
of the daily search.
  * When excluding specials use '> 0' rather than '!= 0' just to be more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment