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

AESinkAudioTrack: Verify ENCODING_IEC61937 before using it #12862

Merged
merged 3 commits into from Oct 7, 2017

Conversation

@fritsch
Copy link
Member

commented Sep 30, 2017

There are some devices out there implementing Android 7 half-baked. Check if IEC is usable before using it as default.

CLog::Log(LOGDEBUG, "AESinkAUDIOTrack: Using IEC PT mode: %d", CJNIAudioFormat::ENCODING_IEC61937);
if (supports_192khz)
// check if we support openening an IEC sink at all:
bool supports_iec = IsSupported(48000, CJNIAudioFormat::CHANNEL_OUT_STEREO, CJNIAudioFormat::ENCODING_IEC61937);

This comment has been minimized.

Copy link
@hudokkow

hudokkow Sep 30, 2017

Member

typo s/openening/opening/

This comment has been minimized.

Copy link
@fritsch

fritsch Sep 30, 2017

Author Member

thx :-)

@fritsch fritsch force-pushed the fritsch:ieccheck branch from 1797bfc to 756fcd0 Sep 30, 2017

@fritsch

This comment has been minimized.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2017

Could someone please test this on a Sony / Phillip TV with Android 7 but not supporting IEC?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

@CiNcH83 you had a broken philips correct?

@MartijnKaijser MartijnKaijser requested a review from koying Oct 2, 2017

@CiNcH83

This comment has been minimized.

Copy link

commented Oct 2, 2017

No. It is a Sony on Marshmallow. No Nougat yet. So can‘t test unfortunately.

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

I have tried the build on my Philips TV with Android 7.1.2

passthrough does not work.
the debug log seems to have the same issues as before.

could I attach the debug log here?

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

Just the link to the pastebin. Passthrough not working is not what this PR fixes. Philips is known very bad in even supporting the basics. If you have luck AC3 will work - nothing else.

But let's see your debuglog

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

I know the passthrough limit of philips version of Android
here is part of the debug log, the whole debug file was to large
https://pastebin.com/wzSMLpWJ

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

Not funny :-(

See:

07:46:40.594 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 32000 supported
07:46:40.595 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 44100 supported
07:46:40.596 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 48000 supported
07:46:40.596 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 88200 supported
07:46:40.597 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 96000 supported
07:46:40.598 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 176400 supported
07:46:40.598 T:18446744072074406176   DEBUG: AESinkAUDIOTRACK - 192000 supported
07:46:40.599 T:18446744072074406176   DEBUG: Firmware implements AC3 RAW
07:46:40.599 T:18446744072074406176   DEBUG: Firmware implements EAC3 RAW
07:46:40.599 T:18446744072074406176   DEBUG: Firmware implements DTS RAW
07:46:40.599 T:18446744072074406176   DEBUG: Firmware implements DTS-HD RAW
07:46:40.599 T:18446744072074406176   DEBUG: Firmware implements TrueHD RAW
07:46:40.600 T:18446744072074406176   DEBUG: AESinkAUDIOTrack: Using IEC PT mode: 13
07:46:40.601 T:18446744072074406176   DEBUG: 8 Channel PT via IEC61937 is supported

That device also happily opens 8 channels in IEC mode ... :-)

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

I am currently doing another build for you via: http://jenkins.kodi.tv/job/Android-ARM/6729/ - it will have ieccheck2 in its name and will appear here: http://mirrors.kodi.tv/test-builds/android/arm/

Thanks much for testing in advance. This build will disable IEC, so that we can see which formats the Philip supports - perhaps now DTS / EAC3 s also working

@CiNcH83

This comment has been minimized.

Copy link

commented Oct 3, 2017

What you hoped AudioTrack::getMinBufferSize to do for you is not documented anywhere. You just made an assumption which turned out to be wrong. Actually the documentation already indicates that you might be wrong:

The size is an estimate because it does not consider either the route or the sink, since neither is known yet.

AudioDeviceInfo::getEncodings is the only documented API which tells you the audio/PT capabilities per audio device.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

@CiNcH83 Documented and "might" in the same sentence made me laugh, but yeah - while you have written this, even you might have figured that this is exactly the reason why we test it :-)

For testing the RAW API, here is a testbuild: http://mirrors.kodi.tv/test-builds/android/arm/kodi-20171003-152e7f1-ieccheck2-armeabi-v7a.apk

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

After the pure RAW API test, I would be very intesterested in: http://mirrors.kodi.tv/test-builds/android/arm/kodi-20171003-3c86966-ieccheck2-armeabi-v7a.apk where I implemented a new method for checking the capabilities, curious.

@fritsch fritsch force-pushed the fritsch:ieccheck branch from 756fcd0 to 4bb6f74 Oct 3, 2017

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

The raw api version worked as expected on my philips tv. ( but still no DTS )

Tested 4 of my testfiles
AC3-384 kbit - OK
AC3-448 kbit - OK
AC3-640 kbit - OK
DTS-1536Kbit- no sound

https://pastebin.com/ffPi9Bvt

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

Thx - please also compare this version to http://mirrors.kodi.tv/test-builds/android/arm/kodi-20171003-3c86966-ieccheck2-armeabi-v7a.apk which again tries to enumerate IEC for your device.

DTS is clear - it's broken by firmware - nothing we can do about it.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

If this would work - I would also try to "deep enumerate" the other PT capabilities.

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

your VerifySinkConfiguration build seems to working for my android tv.
As I get Dolby Digital AC3 to my receiver as before

Works as good as your raw api test build

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

Perfect. I will now add another commit, that will als try to enumerate the RAW formats - that way even the settings should be missing if not supported. Let's see what happens.

Can you for completeness post a debuglog with the former build?

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

yes
I had to revome alot of debug info while playing the files to fit the logfile within 500k
https://pastebin.com/BAPY3qQe

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

Thank you very much. For a final test, please give this build (after it's up a chance): http://mirrors.kodi.tv/test-builds/android/arm/kodi-20171003-ef01537-ieccheck-armeabi-v7a.apk

When everything works as it should, you should only have AC3 left to choose from and this should work. Thanks very much for your support!

@CiNcH83

This comment has been minimized.

Copy link

commented Oct 3, 2017

EAC3 should also work..

}
if (CJNIAudioFormat::ENCODING_DOLBY_TRUEHD != -1)
{
CLog::Log(LOGDEBUG, "Firmware implements TrueHD RAW");
m_info.m_streamTypes.push_back(CAEStreamInfo::STREAM_TYPE_TRUEHD);
if (VerifySinkConfiguration(48000, AEChannelMapToAUDIOTRACKChannelMask(AE_CH_LAYOUT_7_1), CJNIAudioFormat::ENCODING_DOLBY_TRUEHD))

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 3, 2017

Member

Should introduce some constants for these magic numbers

This comment has been minimized.

Copy link
@fritsch

fritsch Oct 3, 2017

Author Member

Which one is magic?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 3, 2017

Member

48000 used here and on the other places and 192000 etc.

This comment has been minimized.

Copy link
@fritsch

fritsch Oct 3, 2017

Author Member

Yeah. Those are the two default rates when doing DTS / AC3 passthrough or when doing DTS-HD, TRUEHD Passthrough. Are used this way all over the sinks. I don't really see the point in changing this as it's clear for every audio guy.

This comment has been minimized.

Copy link
@fritsch

fritsch Oct 3, 2017

Author Member

Never the less. If it's very important for you - I can do that (at least for the enumeration code).

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 3, 2017

Member

It's your choice. For me an advantage for using constants is finding references instead of grepping the number.

This comment has been minimized.

Copy link
@fritsch

fritsch Oct 3, 2017

Author Member

I'll keep as is, cause of the following: if you wake me up at night and asked me about the default DTS samplerate when doing Passthrough I would answerk 48k. if you'd ask me how I named the 48000 in AESinkAudioTrack I needed to think :-) - in fact I would search for 48000 to find the var name and from there I would search for the varName .

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

Last test, I only had the option to enable passthrough on AC3 and EAC3 in the audio settings menu.
enabled transcoding.
so my dts clip was output to my receiver as AC3

https://pastebin.com/C6p7Rpu5

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

@jlinden79 perfect. That's what I wanted to have. Thanks very much.

@jlinden79

This comment has been minimized.

Copy link

commented Oct 3, 2017

Thank you @fritsch
for making kodi usable on my philips tv.

I saw this PR is on master not krypton branch. Is it also be merged to the krypton branch?

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

@jlinden79 we need to verify on non broken hardware to see if we run into regressions. Then it's on @MartijnKaijser to decide. In general: yes, this code is rather simple, only impacts on Enumeration and removes non working settings from the menu.

In the meantime I can build you a Krypton build as a "thanks" for your help!

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

@jlinden79 http://mirrors.kodi.tv/test-builds/android/arm/kodi-20171003-26ff01d-kryptverify-armeabi-v7a.apk <- as promissed. Please give me offline feedback as it needed heavy fixing of merge-conflicts and backporting of a lot of AudioTrack stuff - so that we don't spam the others without our private conversation.

Thanks again for your help with above testing. Highly appreciated.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

jenkins build this please

@fritsch fritsch force-pushed the fritsch:ieccheck branch from 723a385 to e7e66a4 Oct 7, 2017

@Rechi

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

old revision was build
jenkins build this please

@MartijnKaijser MartijnKaijser merged commit bedc63f into xbmc:master Oct 7, 2017

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the L 18.0-alpha1 milestone Oct 7, 2017

@CiNcH83

This comment has been minimized.

Copy link

commented Dec 6, 2018

So your check fails now for the first time with Sony Oreo firmware:

11:38:52.931 T:18446744071739586928  NOTICE: Enumerated AUDIOTRACK devices:
11:38:52.931 T:18446744071739586928  NOTICE:     Device 1
11:38:52.932 T:18446744071739586928  NOTICE:         m_deviceName      : AudioTrack
11:38:52.932 T:18446744071739586928  NOTICE:         m_displayName     : android
11:38:52.932 T:18446744071739586928  NOTICE:         m_displayNameExtra: audiotrack
11:38:52.932 T:18446744071739586928  NOTICE:         m_deviceType      : AE_DEVTYPE_HDMI
11:38:52.932 T:18446744071739586928  NOTICE:         m_channels        : FL, FR, FC, LFE, SL, SR, BL, BR, BC, BLOC, BROC
11:38:52.933 T:18446744071739586928  NOTICE:         m_sampleRates     : 32000,44100,48000,88200,96000,176400,192000
11:38:52.933 T:18446744071739586928  NOTICE:         m_dataFormats     : AE_FMT_S16LE,AE_FMT_FLOAT,AE_FMT_RAW
11:38:52.933 T:18446744071739586928  NOTICE:         m_streamTypes     : STREAM_TYPE_AC3,STREAM_TYPE_EAC3,STREAM_TYPE_DTSHD_CORE,STREAM_TYPE_DTS_1024,STREAM_TYPE_DTS_2048,STREAM_TYPE_DTS_512,STREAM_TYPE_DTSHD,STREAM_TYPE_DTSHD_MA

Sink verification returns true for DTS and DTS-HD. It does not work though. Full log can be found here.

12:54:56.661 T:2103454064   ERROR: CDVDAudio::AddPacketsRenderer - timeout adding data to renderer

Audio capabilities on Android are meant to be checked this way. I am not saying that this method works with all devices. But it is the intended way to check audio capabilities.

@fritsch

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@CiNcH83

This comment has been minimized.

Copy link

commented Jan 30, 2019

Seems I was wrong. I just installed latest Oreo and the BRAVIA indeed pretends to support DTS and DTS-HD which both do not work.

SPMC via Android's AudioDeviceInfo::getEncodings:

--- Found device: BRAVIA 4K GB ATV3
id: 12, type: HDMI_ARC, isSink: true, isSource: false
channel counts: 1 / 2 / 6 / 8 /
channel index masks: any
channel masks: 4 / 12 / 252 / 6396 /
encodings: PCM_16BIT / 10 / 11 / 12 / AC3 / E_AC3 / DTS / DTS_HD /
sample rates: 8000 / 11025 / 16000 / 22050 / 24000 / 32000 / 44100 / 48000 /

So your check is still valid. Screw Sony!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.