[AE] - Make sure, that m_channelLayout is updated, when user has set too... #1344

Merged
merged 1 commit into from Sep 1, 2012

Projects

None yet

5 participants

@fritsch
Member
fritsch commented Aug 30, 2012

If a user wrongly sets 6 channels but his output device does only support 2 channels, video playback will get really choppy. AE currently does not handle this error and I did not find a way to do this more sane. So at least give alsa a chance ...

See also the following comment, I think this was intended to get fixed at this point?

Member
fritsch commented Aug 31, 2012

To reply to myself. The real problem is that SoftAE does not handle the channels error correctly. We know this error from the Alsa Sink, as it is explicitely checked - but afterwards not communicated to SoftAE. It tries to open a new sink with "newformat" as parameters - but this newformat does not take the number of channels that the device can do into account nor the error. So it would fail again ...

Perhaps this "newformat" is to be changed in order to be more failsafe? I can't really say this as I did not look into AE for very long. Happy for input.

@ghost
ghost commented Aug 31, 2012

hi peter, really bad time right now. our developer conference starts today
and most devs are on a plane. so don't be discoraged by a lack of immediate
answers.

Member
fritsch commented Aug 31, 2012

@cptspiff:

np at all. I wish you a great time in Vienna.

Owner
Memphiz commented Aug 31, 2012

Well i'm sitting in a Train atm ;)

Contributor

Hi fritsch - is there a log posted somewhere? I can check into handling correctly in SoftAE if that situation arises.

Contributor

FYI - the sink is supposed to override the requested format with what its actual capabilites are and the engine is supposed to adjust accordingly. If the engine requests 5.1 based on the stream content and the sink returns 2.0 the via newformat the engine should automatically downmix.

Member
fritsch commented Aug 31, 2012

Hi DDDamian,

thx for your comment. The (Alsa) sink at least does not overwrite it, cause the function terminates too early, see the patched out "return false". I will upload a xbmc.log when I come home.

Member
fritsch commented Aug 31, 2012

As requested a xbmc.log: http://xbmclogs.com/show.php?id=7617
Also see Line 426 that seems highly critical.

Channel Count does not get reduced.

Contributor

Thx fritsch - yes, that's definitely the problem - sink should return what it can handle (not necessarily what was requested based on the stream). In this case the error is entirely within the sink code, not SoftAE. If the sink overwrites the channel count with 2 the engine will adjust accordingly.

If that is occuring after this patch you should be successful.

FYI the enumeration code we added (and that dumps the info at the start of the log) was meant to be expanded upon so the user selects the device and we default to its capabilities unless overridden by the user. In this case the channel count would be automatically set to 2.0 as reported during enumeration. That was/is the plan going forward, but haven't got there yet.

Can you confirm your patch does indeed pass 2.0 back to SoftAE? If so lets merge this asap.

Member
fritsch commented Aug 31, 2012

@DDDamian:
http://xbmclogs.com/show.php?id=7619 <- new xbmc log with the patch applied. Playback is working fine and also the NULL device is not openened anymore, but - SoftAE still thinks that it has to put out 5.1 Speakers.

Edit: See Line 1157.

Contributor

Ah yes - I see where in the code. It's written to allow the user to override the reported information via the GUI setting (the user is always right lol). So it's using CStdLayout from the GUI settings which is 5.1 - so your initial statement about incorrect user settings is true.

Question is do we allow user to override what the device reports? It's an easy change if we instead assume the user has setup incorrectly and that the reported channel count is correct. Although I guess they can still screw up in their asound.conf setup.

Thoughts from the team? Who's right - user or asound.conf? Go with GUI setting or asound setting?

Member
fritsch commented Aug 31, 2012

@DDDamian:

My thoughts:
Concerning the stuttering that 6 channels output produce on a wrongly opened device, i see no point in keeping the user values. But AC3 / DTS should be cared as those output "5.1" over just 2 channels and this should be possible also for remixing stuff (DTS-HD to AC3) and the like.

@ghost
ghost commented Aug 31, 2012

users are their own worst enemy. asound.conf being hard to use is a good thing. I fail to see the need for this on our side.

Member

Thoughts from the team? Who's right - user or asound.conf? Go with GUI setting or asound setting?

Users often have no clue what those settings are all about. Ideally we should only allow reasonable settings in the GUI. asound.conf is system configuration and should not be overruled by a user application.

Member
wsnipex commented Aug 31, 2012

probably not popular suggestion: when a user sets a speaker setup and audio device in the GUI, check the sink capabilities and pop a warning/disable wrong settings.

Member

In my opinion this is a very good suggestion and not only for audio. I often thought about checking entire system capabilities at startup and not giving users the chance to activate e.g. vdpau on an Intel system.
The time required for this we would get back by not having to answer all those questions in the forum :)

Contributor
DDDamian commented Sep 1, 2012

@wsnipex @fernetMenta - very popular suggestion with me - that's why we put all the effort into the enumeration code ;) We just didn't get to the next point yet which was to auto-set the defaults based on that. It was still intended to be subject to user overrride but....

IMHO we should use the sink's return value in the meantime instead of m_stdChLayout (user setting) and pop up a KaiToast as wsnipex suggests if there's a mismatch.

I'll doctor SoftAE accordingly and pull this in so at least we get the return value from the Alsa sink.

Thx fritsch :)

@DDDamian DDDamian merged commit 12840c2 into xbmc:master Sep 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment