Skip to content
This repository

dvdplayer: Allow multithread decoding when software is used #2064

Merged
merged 1 commit into from over 1 year ago

5 participants

Peter Frühberger Joakim Plate davilla Rainer Hochecker Thomas Jager
Peter Frühberger
Collaborator

This allows decoding of some hi10p material on e.g. AMD Fusion with
both cores at the max.

@elupus: Do you see any problems with it?

Joakim Plate
Collaborator
Peter Frühberger
Collaborator

I cannot keep the "sense" point, as the code clearly sets m_bSoftware=true, when hi10p stuff is decoded.

if (hints.codec == CODEC_ID_H264)
{
switch(hints.profile)
{
case FF_PROFILE_H264_HIGH_10:
case FF_PROFILE_H264_HIGH_10_INTRA:
case FF_PROFILE_H264_HIGH_422:
case FF_PROFILE_H264_HIGH_422_INTRA:
case FF_PROFILE_H264_HIGH_444_PREDICTIVE:
case FF_PROFILE_H264_HIGH_444_INTRA:
case FF_PROFILE_H264_CAVLC_444:
m_bSoftware = true;
break;
}
}

So - I know for sure, that the above will work when HI10P is decoded.

With the other point you say, I did not come into contact (meaning no problems so far). I also think that new ffmpeg version will make this behaviour a lot better.

If you like I can integrate a "m_hi10p_is_decoded" and set this to true and rather check that flag?

Peter Frühberger
Collaborator

Update:
As you said: "It's always false otherwise." That is really nice for me and the patch then. As it does exactly what it shall do: Enable multi threading, when hi10p is decoded.

Cool thx for the tipp.

Peter Frühberger
Collaborator

I updated the PR to really only enable it for hi10p content.

As far as I know (TM), there is no dedicated GPU that can do hi10p content, furthermore it is not enabled by default, but can be set via advancedsettings.xml

I am also interested in a GUI Setting to ease this in the future for other codecs.

Joakim Plate
Collaborator
Peter Frühberger
Collaborator

@elupus:
Thx for your input. I updated the PR to rename the flag to disablehi10pmultithreading. I also let the ffmpeg default be logged when debugging - this will help us to find regressions, when users have problems with this new default.

I kept the SLICE decoding - in deed thread_type = FF_THREAD_SLICE | FF_THREAD_THREAD by default.

If you like the Logging or flagnames changed, please let me know.

davilla
Collaborator

I'm against adding things like g_advancedSettings.m_videoDisableHi10pMultithreading, they don't really solve the real issue and only add more techy tweaks that make xbmc more and more un-useable for anyone but tech geeks.
advancedsettings.xml is already filled with such thing and guisettings is getting confusing to mortals again.

Let's solve the real problems and stop adding work arounds and tweaks. If frame multithreading is crash prone for some sources, id those sources and exclude them from frame multithreaded decoding. Or id those source that are save and include them. ie. use a white list or a black list to pick. But a setting to enabled for Hi10p, come on, next it will be m_videoDisableH264PVRMultithreading and the list starts growing and becomes unmanageable and confusing.

Peter Frühberger
Collaborator

@davilla:
Yeah, you are totally right concerning the long term, this will be getting a mess, if we continue to add such stuff in the future. As you have seen in the code - we actually especially ask for Hi10p to disable HW decoding, as this format is not decoded on any GPU outside I know of.

If this check is already there, I thought it would be a good idea to use it, to make xbmc able to cope with this format in a multithreaded way. The new setting was thought as a backup - when there will be problems for some users.

This patch adresses "short term" improvement, as hi10p (sadly) is something people want now and want it working now. With this infrastructure already in place, it was easy to add.

I can remove the advancedsettings.xml changes - with the problem, that it cannot be disabled.

To summarize:
Short Term: solution to make (perhaps even frodo users happy) - think of a celeron or Atom (2.13ghz System) that would not be able to decode that stuff in frodo.
Long Term: Choose a whitelist or blacklist approach.

Rainer Hochecker
Collaborator

Well, currently this options is for tech geeks. As elupus pointed out multi-threaded decoding is error prone and needs to be optional. IMO it's an expert setting and fits perfectly into advanced settings (currently).

Joakim Plate
Collaborator
magao referenced this pull request in OpenELEC/OpenELEC.tv
Closed

Multi-threaded h264 decoding #1758

Thomas Jager

Would it be possible to check if VDPAU and VAAPI is disabled and then allow for framed based decoding? Wouldn't that be pretty safe?

Peter Frühberger
Collaborator

@thoj: This is not all that must be considered for a common multi processor approach. Think of the Windows / Mac hw decoders and so on.

In this specific case, the Hi10p check was introduced to disable hw accel at all - therefore it is safe by design. We are currently only discussing the advanced setting I think.

Thomas Jager

Yeah i agree.. It's only hi10p that needs more power! :) so this would be fine for me.

Peter Frühberger fritsch dvdplayer: Allow multithread decoding for hi10p content by default
This allows decoding of some hi10p material on e.g. AMD Fusion with
both cores at the max. This introduces a new advancedsetting named
disablehi10pmultithreading to disable hi10p decoded multithreaded.
42271fa
Peter Frühberger
Collaborator

What are we doing about this? Any input on the Whitelist / Blacklist approach?
Should we rather track the hw accelerators?

Open for input. H265 is stable since some days - so in the future there will be one format that can be done by newer graphic cards and not by older ones - so we need an infrastructure here anyways? This infrastructure could also solve the hi10p check, that i implemented in this patch.

Rainer Hochecker
Collaborator

@elupus it's more than a year back but I think I remember that disabling hw acceleration based on profile information was meant to be an intermediate solution. VDPAU supports chroma type 422. With the current code it won't get a change to decide by itself.

Joakim Plate
Collaborator
Joakim Plate
Collaborator
davilla
Collaborator

no hw decoder on the planet supports hi10p... :) not now, not every. Nvidia has already gone on record saying they have no interest in supporting nitch video formats.

Joakim Plate elupus merged commit 2c8e509 into from
Joakim Plate elupus closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jan 24, 2013
Peter Frühberger fritsch dvdplayer: Allow multithread decoding for hi10p content by default
This allows decoding of some hi10p material on e.g. AMD Fusion with
both cores at the max. This introduces a new advancedsetting named
disablehi10pmultithreading to disable hi10p decoded multithreaded.
42271fa
This page is out of date. Refresh to see the latest.
18 xbmc/cores/dvdplayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.cpp
@@ -138,6 +138,7 @@ CDVDVideoCodecFFmpeg::CDVDVideoCodecFFmpeg() : CDVDVideoCodec()
138 138 m_iScreenHeight = 0;
139 139 m_iOrientation = 0;
140 140 m_bSoftware = false;
  141 + m_isHi10p = false;
141 142 m_pHardware = NULL;
142 143 m_iLastKeyframe = 0;
143 144 m_dts = DVD_NOPTS_VALUE;
@@ -187,7 +188,10 @@ bool CDVDVideoCodecFFmpeg::Open(CDVDStreamInfo &hints, CDVDCodecOptions &options
187 188 case FF_PROFILE_H264_HIGH_444_PREDICTIVE:
188 189 case FF_PROFILE_H264_HIGH_444_INTRA:
189 190 case FF_PROFILE_H264_CAVLC_444:
  191 + // this is needed to not open the decoders
190 192 m_bSoftware = true;
  193 + // this we need to enable multithreading for hi10p via advancedsettings
  194 + m_isHi10p = true;
191 195 break;
192 196 }
193 197 }
@@ -247,8 +251,18 @@ bool CDVDVideoCodecFFmpeg::Open(CDVDStreamInfo &hints, CDVDCodecOptions &options
247 251 m_pCodecContext->codec_tag = hints.codec_tag;
248 252 /* Only allow slice threading, since frame threading is more
249 253 * sensitive to changes in frame sizes, and it causes crashes
250   - * during HW accell */
251   - m_pCodecContext->thread_type = FF_THREAD_SLICE;
  254 + * during HW accell - so we unset it in this case.
  255 + *
  256 + * When we detect Hi10p and user did not disable hi10pmultithreading
  257 + * via advancedsettings.xml we keep the ffmpeg default thread type.
  258 + * */
  259 + if(m_isHi10p && !g_advancedSettings.m_videoDisableHi10pMultithreading)
  260 + {
  261 + CLog::Log(LOGDEBUG,"CDVDVideoCodecFFmpeg::Open() Keep default threading for Hi10p: %d",
  262 + m_pCodecContext->thread_type);
  263 + }
  264 + else
  265 + m_pCodecContext->thread_type = FF_THREAD_SLICE;
252 266
253 267 #if defined(TARGET_DARWIN_IOS)
254 268 // ffmpeg with enabled neon will crash and burn if this is enabled
1  xbmc/cores/dvdplayer/DVDCodecs/Video/DVDVideoCodecFFmpeg.h
@@ -114,6 +114,7 @@ class CDVDVideoCodecFFmpeg : public CDVDVideoCodec
114 114
115 115 std::string m_name;
116 116 bool m_bSoftware;
  117 + bool m_isHi10p;
117 118 IHardwareDecoder *m_pHardware;
118 119 int m_iLastKeyframe;
119 120 double m_dts;
2  xbmc/settings/AdvancedSettings.cpp
@@ -112,6 +112,7 @@ void CAdvancedSettings::Initialize()
112 112 m_DXVANoDeintProcForProgressive = false;
113 113 m_videoFpsDetect = 1;
114 114 m_videoDefaultLatency = 0.0;
  115 + m_videoDisableHi10pMultithreading = false;
115 116
116 117 m_musicUseTimeSeeking = true;
117 118 m_musicTimeSeekForward = 10;
@@ -498,6 +499,7 @@ void CAdvancedSettings::ParseSettingsFile(const CStdString &file)
498 499 XMLUtils::GetBoolean(pElement,"enablehighqualityhwscalers", m_videoEnableHighQualityHwScalers);
499 500 XMLUtils::GetFloat(pElement,"autoscalemaxfps",m_videoAutoScaleMaxFps, 0.0f, 1000.0f);
500 501 XMLUtils::GetBoolean(pElement,"allowmpeg4vdpau",m_videoAllowMpeg4VDPAU);
  502 + XMLUtils::GetBoolean(pElement,"disablehi10pmultithreading",m_videoDisableHi10pMultithreading);
501 503 XMLUtils::GetBoolean(pElement,"allowmpeg4vaapi",m_videoAllowMpeg4VAAPI);
502 504 XMLUtils::GetBoolean(pElement, "disablebackgrounddeinterlace", m_videoDisableBackgroundDeinterlace);
503 505 XMLUtils::GetInt(pElement, "useocclusionquery", m_videoCaptureUseOcclusionQuery, -1, 1);
1  xbmc/settings/AdvancedSettings.h
@@ -164,6 +164,7 @@ class CAdvancedSettings
164 164 bool m_DXVAForceProcessorRenderer;
165 165 bool m_DXVANoDeintProcForProgressive;
166 166 int m_videoFpsDetect;
  167 + bool m_videoDisableHi10pMultithreading;
167 168
168 169 CStdString m_videoDefaultPlayer;
169 170 CStdString m_videoDefaultDVDPlayer;

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.