Skip to content

1. PulseAudioDirectSound, 2. ffmpeg/vaapi fix, 3. bad variable name causing crash in Linux #409

Closed
wants to merge 28 commits into from

3 participants

@cbxbiker61

Hello xbmc team,

I've got three commits that should be solid for inclusion. They all fix bugs in xbmc.
I'm not too savvy on how exactly to separate these commits from my other commits so I've got the links here.

  1. Fix PulseAudioDirectSound constructor:

cbxbiker61@da4fd96

  1. ffmpeg/vaapi can't handle multi-threaded decode, so avoid that:

cbxbiker61@f07548b

  1. Duplicate variable name causes undefined behaviour (real-world error in shared_ptr):

cbxbiker61@7faeb9f

@cbxbiker61

OK, a fix for item 3 has just been committed, so cross that one off the list.

@elupus elupus commented on an outdated diff Sep 6, 2011
xbmc/cores/dvdplayer/DVDAudio.cpp
@@ -75,14 +76,24 @@ bool CDVDAudio::Create(const DVDAudioFrame &audioframe, CodecID codec)
{
CLog::Log(LOGNOTICE, "Creating audio device with codec id: %i, channels: %i, sample rate: %i, %s", codec, audioframe.channels, audioframe.sample_rate, audioframe.passthrough ? "pass-through" : "no pass-through");
- // if passthrough isset do something else
+ int sample_rate(audioframe.sample_rate);
+ AudioFilter::HeaderInfo hi;
+ AudioFilter::DtsHeaderParser hp;
+
+ if ( hp.parseHeader(audioframe.data, &hi) && hi.isHd() )
+ sample_rate *= 4;
@elupus
Team Kodi member
elupus added a note Sep 6, 2011

This should have been handled in the passthrough codec already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus
Team Kodi member
elupus commented on 75ddf2b Sep 6, 2011

We've already tried this and found it to be to slow to react to large errors.

@elupus
Team Kodi member
elupus commented on f07548b Sep 6, 2011

This is not okey.. Just cause you CAN do vaapi doesn't me it will be used. Fix so ffmpeg doesn't use threading if it is using vaapi decoder. Secondly that call is redundant in newer ffmpeg i think since it will do it itself anyway.

The reason I conditionally disabled threads was that the ffmpeg/vaapi demo at http://www.splitted-desktop.com/~gbeauchesne/hwdecode-demos/ ensures that avcodec does not init multiple threads.

If you are saying the call to avcodec_thread_init is redundant, then eliminating the call completely would be the way to go.

Team Kodi member

it will be redundant on the next ffmpeg bump.

@elupus
Team Kodi member
elupus commented on 7faeb9f Sep 6, 2011

Looks okey.

@elupus
Team Kodi member
elupus commented on 7beccae Sep 6, 2011

absolutely not okey. find the bug instead or give a better reason why this fixes it.

Actually this is more of a "temporary" fix than anything. I plan on tracking down the real problem when I get time.

Team Kodi member

I can't see how this can possibly fix any issue, PAPlayer will sleep for quite a bit of the audio output buffers size this is true. Perhaps it is sleeping to much and perhaps PAPlayer should check the minreq before setting sleep time so that it isn't cutting it to close, i.e. it should only sleep until buffer is X times minreq, to avoid it accidently sleeping longer and getting underruns.

Not sure if its possible to fix it better (if this is the issue) using the push model we have. I guess the other option is to rewrite paplayer sleep to behave closer to dvdplayer and inject packets closer to playback speed.

@elupus
Team Kodi member
elupus commented on 92937bf Sep 6, 2011

We absolutely don't need a third way to handle passthrough.. ffmpeg should handle it just fine.

@topfs2
Team Kodi member
topfs2 commented on 1410053 Sep 7, 2011

Not sure I like the WaitForStreamReady, mainly how its handling the lock state of the mainloop, the lock state is not necessarily the same when entering and when leaving. While I can see the value in splitting it out to a reusable function considering its only ever used twice (and unlikely to be used more) I'd rather have it explicit in both and not split out (as its altering the lock state and is better to be explicit in code right there in the function which does it).

From what I can tell in the patch you seem to be altering the buffering strategies in both passthrough and non-passthrough, I'd prefer that to be a seperate commit with a reasoning behind it. (I have altered the strategy in AE pulse but not in mainline so I'm not against the alteration it just looks messy in the same commit)

@elupus
Team Kodi member
elupus commented Apr 10, 2012

This seem to have sidetracked from original pull.

@elupus elupus closed this Apr 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.