Skip to content
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

ActiveAE: add advanced setting for forcing multichannel layout #4967

Closed
wants to merge 2 commits into from

Conversation

anssih
Copy link
Member

@anssih anssih commented Jun 29, 2014

Anthem Statement D2 audio processor only supports 2.0 and 5.1 HDMI
playback properly, causing completely silent output if playback of e.g.
a 4.0 audio stream is being done.

Add "audio.fixedmultichannellayout" advanced setting for forcing the use
of the user-specified layout in audio settings for multichannel streams
in case the user has equipment that does not support all intermediate
layouts properly.
Stereo streams will continue to be played back as stereo and no
resampling is done.

Reported by Grant Warecki (gjwAudio).

@fritsch, @FernetMenta, WDYT? This basically becomes a oneliner in ApplySettingsToFormat() so IMHO it does not pollute the code too badly for a fix for obscure devices...

Anthem Statement D2 audio processor only supports 2.0 and 5.1 HDMI
playback properly, causing completely silent output if playback of e.g.
a 4.0 audio stream is being done.

Add "audio.fixedmultichannellayout" advanced setting for forcing the use
of the user-specified layout in audio settings for multichannel streams
in case the user has equipment that does not support all intermediate
layouts properly.
Stereo streams will continue to be played back as stereo and no
resampling is done.

Reported by Grant Warecki (gjwAudio).
@Memphiz
Copy link
Member

Memphiz commented Jun 29, 2014

Why does a fixed 5.1 setting in xbmc not do the same?

@anssih
Copy link
Member Author

anssih commented Jun 29, 2014

jenkins build this please

@anssih
Copy link
Member Author

anssih commented Jun 29, 2014

@Memphiz Two reasons AFAICS (feel free to correct me if I'm wrong):

  1. Fixed mode will resample all audio (if a non-matching sample rate), which is not really wanted here
  2. Fixed mode will upmix pad stereo audio to 5.1, which is not wanted either.

@Memphiz
Copy link
Member

Memphiz commented Jun 29, 2014

  1. yep thats it - thx for the reminder
  2. nope only if stereo upmix is enabled in addition

@anssih
Copy link
Member Author

anssih commented Jun 29, 2014

nope only if stereo upmix is enabled in addition

Oops, sorry, I meant that audio output will be at 5.1 (with silent channels, not upmix), so the receiving device will not see it as stereo, which is not wanted.

@FernetMenta
Copy link
Contributor

I don't think any consumer electronics device has such a setting. How do they work with the Anthem? Why does a 4.0 fail? Could you provide a log? Was that tested on more than one platform? Can we rule out an issue of the driver?

@anssih
Copy link
Member Author

anssih commented Jun 30, 2014

I don't think any consumer electronics device has such a setting. How do they work with the Anthem?

They don't. Note that 4.0 is rather unusual in consumer electronics, but googling reveals that apparently 4.0 is somewhat common with classical SACDs and they do not work with Anthem D2.

Here's a massive AVSforum thread where this issue is mentioned on several posts with SACD players (for some reason I can't find direct links): http://www.avsforum.com/t/678260/anthem-d2-d2v-avm50-avm50v-arc1-tweaking-guide

Why does a 4.0 fail?

Because the hw does not like HDMI CA values that have 3-5 active channels for some reason.

Could you provide a log?

I can get one if needed, don't have one right now.

Was that tested on more than one platform? Can we rule out an issue of the driver?

Well, as per above, this issue exists with hardware SACD players as well, and we tested the behavior very extensively with the user (with speaker-test and driver tweaks), and it seems to be exclusively the HDMI CA value that upsets the receiver. I guess I could ask him to test with another platform if needed/possible.

If this were some cheap Chinese receiver or something, I might be OK at leaving them broken and un-advanced-configurable, but this receiver is a rather high-end model (7000+ USD when it was new), albeit somewhat dated now (HDMI was still quite new at the time).

@fritsch
Copy link
Member

fritsch commented Jun 30, 2014

Can't that be fixed in driver by just returning either FL,FR or the corresponding 5.1 Layout else?

@anssih
Copy link
Member Author

anssih commented Jun 30, 2014

@fristch, based on what? EDID blacklist would probably be overkill (at least until/if we encounter other receivers with issues). A module parameter might be better, although it would probably be more code there than just adding the workaround in xbmc (and it isn't really driver-specific per-se, as this is about the receiver hw).

Plus it would leave this broken on other non-Linux platforms...

@FernetMenta
Copy link
Contributor

I am undecided here. Do we really want to add an advanced setting for a single user? At least a very limited number of users would benefit from this. How will we decide next time if a user wants a workaround for some buggy device?

If this were some cheap Chinese receiver or something, I might be OK at leaving them broken and un-advanced-configurable, but this receiver is a rather high-end model (7000+ USD when it was new),

In that range I would expect the manufacturer to move their asses and get this fixed.

@anssih
Copy link
Member Author

anssih commented Jun 30, 2014

I am undecided here. Do we really want to add an advanced setting for a single user? At least a very limited number of users would benefit from this. How will we decide next time if a user wants a workaround for some buggy device?

The same way we decide now, by performing cost/benefit-analysis, IMHO. There are three factors AFAICS:

  1. How many users are affected?
  2. How much code is needed / how invasive the change is?
  3. How easy/cheap it would be for the user to workaround or replace hardware without any changes in XBMC?

Agreeing on weights might be difficult, though :)

In this case, 1. Very few as far as we can estimate (1-3? assuming another use case for the option does not surface) => against option, 2. Not very => for option, 3. Quite infeasible => for option.

As you probably have guessed, I'm for the option (not too strongly, though - maybe I'd feel more strongly about it if I had this piece of hardware myself, though...).

Completely another way to go with this would be to make audio configuration more flexible in a more generic way, but I guess the benefits of that would be limited or even negative (unless there has been actual demand for (other) weird configs?)?

In that range I would expect the manufacturer to move their asses and get this fixed.

I guess the manufacturer considers it EOL at this point, unfortunately (or maybe, though unlikely, it is/was not software-fixable). Not sure if it was properly reported to them at the time when the model was more current, but there hasn't been firmware updates in years AFAICS.

@fritsch
Copy link
Member

fritsch commented Jun 30, 2014

Reading the patch again - I am not against it. It is clean, non ambigous and non intrusive. For what's worth it, it fixes someone's audio.

  1. 3 Users affected => --
  2. three lines => ++
  3. quite hard, therefore ++ again

wins by ++

@FernetMenta
Copy link
Contributor

Tomorrow the next user request an advanced setting in a similar context, the day after another, and so on, ....
There is another option for this user: change the code to his needs and build it

@fritsch
Copy link
Member

fritsch commented Jun 30, 2014

If we have a sane request on new settings that often, we should change our default settings capabilities, cause that would proof we are not general enough, btw.
\sum\limits_{i=1}^n i has no limit and therefore we would make \infty users unhappy.

You just gave a proof why this setting should go in :-)

@FernetMenta
Copy link
Contributor

I have cleaned out all this crappy advanced settings for audio not long ago. There must be a really really good reason for adding such a setting again. I don't consider 1-3 users with an EOL device as a good reason.
If you find some generic use case which covers this case and justifies a gui setting be my guest. But I don't agree to those types of workarounds for a single user.

@fritsch
Copy link
Member

fritsch commented Jun 30, 2014

I am quite interested in finding a more general configuration that can include this single user's hardware and make him happily use xbmc. The OSX guys workaround "different hardware" in the sink itself ... so what about doing that? With the disadvantage that this workaround is not platform agnostic.

@t-nelson
Copy link
Contributor

Dude has a $7k amp? I'll maintain a branch for him for $1k.

@FernetMenta
Copy link
Contributor

why don't you add this as an additional mode and add it to the spinner?

@anssih
Copy link
Member Author

anssih commented Jun 30, 2014

IMHO we should be able to avoid advancedsettings proliferation by periodically pinging out to see if a particular setting is used by anyone anymore (in addition to not adding them if the case is rare and easy to workaround/avoid)...

why don't you add this as an additional mode and add it to the spinner?

Because the mode seems somewhat arbitrary (as it is workarounding a specific hw issue), and adding it to the spinner and having translations for it etc. seems overkill (at least to me) for just a few users, while an advancedsetting would be less invasive, less confusing, and less developer-resource-intensive. As said, would be a different thing if we could come up with a more widely useful mode that happens to fix this as well.

Anyway, if we can't find a solution to agree on, me providing a customized openELEC build periodically to the reporter is not too much work, but I'd like to avoid that if possible.

@t-nelson, actually, this is a preprocessor - you get line level outputs so you'll need a separate amp

@t-nelson
Copy link
Contributor

Fine, $2k then. :)

@FernetMenta
Copy link
Contributor

"fixed" mode is actually pike mode which also fits into the category "somewhat arbitrary".

advancedsetting would less developer-resource-intensive

only because nobody cares cleaning them out after a while. It was resource-intensive for me doing this job, one reason I am reluctant in adding those without a compelling reason.

@jmarshallnz
Copy link
Contributor

Thinking slightly outside the box, would this be taken care of once the DSP stuff comes in? i.e. a very simple DSP that just adds blank channels for 3, 3.1, 4, 4.1, 5.0 -> 5.1 ?

If so, that seems a reasonable way forward given the code is somewhat on the way?

@FernetMenta
Copy link
Contributor

@AlwinEsch is this scenario covered by DSP?

@AlwinEsch
Copy link
Member

Yes, if the DSP system is enabled it is always on the selected output and unused channels leaved empty. Reason there was to have for every add-on DSP Master and Post Process Mode the possibility for channel correction.

Also if no up-mix (for stereo and upmix enabled) or down-mix (higher input layout, as output) is present on add-on side it becomes then performed by ffmpeg inside xbmc to the selected output channels.

@AlwinEsch
Copy link
Member

Also one point which must be checked with it. Is then channel alignment always correct?

On DSP it has for different output types wrong channel alignment (here on alsa correct on pulse the surround and lfe channels was wrong). There it is fixed with limitation to ffmpeg channel layout until correction from sink output.

see
https://github.com/AlwinEsch/xbmc/blob/audio-dsp-addon-handling/xbmc/cores/AudioEngine/Engines/ActiveAE/ActiveAE.cpp#L1456-1495
and
https://github.com/AlwinEsch/xbmc/blob/audio-dsp-addon-handling/xbmc/cores/AudioEngine/DSPAddons/ActiveAEDSP.cpp#L436-474

@fritsch
Copy link
Member

fritsch commented Jul 3, 2014

Can you emphasize on "right" or "wrong"? Is the mapping AE Channel Layout -> PA Layout faulty? https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Sinks/AESinkPULSE.cpp#L292

@AlwinEsch
Copy link
Member

From my side wrong but unsure if was only with the dsp processing system from me.

Mapping by me on every multichannel playback was wrong which have another channel alignment on sinks as from "CAEChannelInfo& CAEChannelInfo::operator=(const enum AEStdChLayout rhs)" channel layout.

@anssih Was the alignment for you correct or must you change something for Anthem Statement D2?

Performing currently a build for test of it.

@AlwinEsch
Copy link
Member

Fault confirmed, with a forced layout the alignment of the rear, side and LFE speaker is not correct here with ALSA and PULSE Audio.

Have seen, this fault also present if the fixed output configuration is enabled.

Can anyone else check it under different sound systems?

AlwinEsch pushed a commit to AlwinEsch/kodi that referenced this pull request Jul 4, 2014
@AlwinEsch
Copy link
Member

Have added advanced setting to my system to check with channel alignment (dsp system off) and with call of "CActiveAEDSP::Get().GetDSPChannelLayout(stdChannelLayout)" the channel alignment is correct.

Is there as temporary to see and check.

AlwinEsch pushed a commit to AlwinEsch/kodi that referenced this pull request Jul 4, 2014
AlwinEsch pushed a commit to AlwinEsch/kodi that referenced this pull request Jul 4, 2014
@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Jul 16, 2014
@MartijnKaijser MartijnKaijser removed this from the Pending for inclusion milestone Jun 14, 2015
@MartijnKaijser
Copy link
Member

Let me close it for now. New PR can be opened if wanted/needed and is rebased on current master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants