Android - Passthrough Changes with v17

  Thread Rating:
  • 5 Vote(s) - 4.6 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Post Reply
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #31
The fix koying did is correct. As the write method changed from before v23 to now. I will add this.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #32
Mmmh - reading more exact, I need to take that back, see:

Quote:int write (short[] audioData,
int offsetInShorts,
int sizeInShorts)
Writes the audio data to the audio sink for playback (streaming mode), or copies audio data for later playback (static buffer mode). The format specified in the AudioTrack constructor should be ENCODING_PCM_16BIT to correspond to the data in the array.

As we explicitely don't open the device in ENCODING_PCM_16BIT mode, we now have undocumented behaviour. Please also backport the correct signed int writing method. Is that possible?

In other words:
If vendor A backports it correctly, it cannot correctly use it, cause vendor B missed this one method.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
(This post was last modified: 2016-09-19 08:16 by fritsch.)
find quote
mo123 Offline
Fan
Posts: 363
Joined: May 2013
Reputation: 6
Post: #33
(2016-09-19 08:15)fritsch Wrote:  Mmmh - reading more exact, I need to take that back, see:

Quote:int write (short[] audioData,
int offsetInShorts,
int sizeInShorts)
Writes the audio data to the audio sink for playback (streaming mode), or copies audio data for later playback (static buffer mode). The format specified in the AudioTrack constructor should be ENCODING_PCM_16BIT to correspond to the data in the array.

As we explicitely don't open the device in ENCODING_PCM_16BIT mode, we now have undocumented behaviour. Please also backport the correct signed int writing method. Is that possible?

In other words:
If vendor A backports it correctly, it cannot correctly use it, cause vendor B missed this one method.

Hi

I post some information here, don't know if it can help perhaps?

#define ENCODING_IEC61937 10 is used
Koying said it will be better to use #define ENCODING_IEC61937 13 in the SDK, but it shouldn't make a difference?
I can make changes to the SDK myself if needed to test.

IEC Firmware patches used
https://drive.google.com/file/d/0B2JoqOT...sp=sharing

System SDK Audio source code files
https://drive.google.com/file/d/0B2JoqOT...B6LVk/view

Kodi 16.1 logs where IEC 61937 pass-through worked.
https://drive.google.com/file/d/0B2JoqOT...JxRjA/view

Spreadsheet with Kodi Test Videos with Kodi 16.1 IEC pass-through based on RKMC.
https://drive.google.com/file/d/0B2JoqOT...FkUWM/view
(This post was last modified: 2016-09-19 08:30 by mo123.)
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #34
https://developer.android.com/reference/...G_IEC61937 <- it is 13 (d), but we call
Code:
GetStaticValue(c, CJNIAudioFormat::ENCODING_IEC61937, "ENCODING_IEC61937");

So that gets overwritten with the value you actually used yourself. But it's better to use 13 - as this is how it is defined in the API - for compatibility arround _all_ mediaplayers.


Quote:Kodi 16.1 logs where IEC 61937 pass-through worked.
https://drive.google.com/file/d/0B2JoqOT...JxRjA/view

Spreadsheet with Kodi Test Videos with Kodi 16.1 IEC pass-through based on RKMC.
https://drive.google.com/file/d/0B2JoqOT...FkUWM/view
^^ does not matter at all. We don't talk about "what works" but about how to do it properly to make it work once and for all in the way the API wants it.


The patch is fine (besides the 13 (d in hex)). Please also backport the int16 API - which is very easy to do.

For your code, add:

Code:
static jint android_media_AudioTrack_write_short(JNIEnv *env,  jobject thiz,
                                                  jshortArray javaAudioData,
                                                  jint offsetInShorts, jint sizeInShorts,
                                                  jint javaAudioFormat,
                                                  jboolean isWriteBlocking) {

    sp<AudioTrack> lpTrack = getAudioTrack(env, thiz);
    if (lpTrack == NULL) {
        jniThrowException(env, "java/lang/IllegalStateException",
            "Unable to retrieve AudioTrack pointer for write()");
        return 0;
    }

    jshort* cAudioData = NULL;
    if (javaAudioData) {
        cAudioData = (jshort *)env->GetShortArrayElements(javaAudioData, NULL);
        if (cAudioData == NULL) {
            ALOGE("Error retrieving source of audio data to play, can't play");
            return 0; // out of memory or no data to load
        }
    } else {
        ALOGE("NULL java array of audio data to play, can't play");
        return 0;
    }
    jint written = writeToTrack(lpTrack, javaAudioFormat, (jbyte *)cAudioData,
                                offsetInShorts * sizeof(short), sizeInShorts * sizeof(short),
                                isWriteBlocking == JNI_TRUE /* blocking */);
    env->ReleaseShortArrayElements(javaAudioData, cAudioData, 0);

    if (written > 0) {
        written /= sizeof(short);
    }

    return written;
}
Don't forget to add the method definition to the proper file.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
(This post was last modified: 2016-09-19 08:53 by fritsch.)
find quote
mo123 Offline
Fan
Posts: 363
Joined: May 2013
Reputation: 6
Post: #35
(2016-09-19 08:49)fritsch Wrote:  https://developer.android.com/reference/...G_IEC61937 <- it is 13 (d), but we call
Code:
GetStaticValue(c, CJNIAudioFormat::ENCODING_IEC61937, "ENCODING_IEC61937");

So that gets overwritten with the value you actually used yourself. But it's better to use 13 - as this is how it is defined in the API - for compatibility arround _all_ mediaplayers.


Quote:Kodi 16.1 logs where IEC 61937 pass-through worked.
https://drive.google.com/file/d/0B2JoqOT...JxRjA/view

Spreadsheet with Kodi Test Videos with Kodi 16.1 IEC pass-through based on RKMC.
https://drive.google.com/file/d/0B2JoqOT...FkUWM/view
^^ does not matter at all. We don't talk about "what works" but about how to do it properly to make it work once and for all in the way the API wants it.


The patch is fine (besides the 13 (d in hex)). Please also backport the int16 API - which is very easy to do.

For your code, add:

Code:
static jint android_media_AudioTrack_write_short(JNIEnv *env,  jobject thiz,
                                                  jshortArray javaAudioData,
                                                  jint offsetInShorts, jint sizeInShorts,
                                                  jint javaAudioFormat,
                                                  jboolean isWriteBlocking) {

    sp<AudioTrack> lpTrack = getAudioTrack(env, thiz);
    if (lpTrack == NULL) {
        jniThrowException(env, "java/lang/IllegalStateException",
            "Unable to retrieve AudioTrack pointer for write()");
        return 0;
    }

    jshort* cAudioData = NULL;
    if (javaAudioData) {
        cAudioData = (jshort *)env->GetShortArrayElements(javaAudioData, NULL);
        if (cAudioData == NULL) {
            ALOGE("Error retrieving source of audio data to play, can't play");
            return 0; // out of memory or no data to load
        }
    } else {
        ALOGE("NULL java array of audio data to play, can't play");
        return 0;
    }
    jint written = writeToTrack(lpTrack, javaAudioFormat, (jbyte *)cAudioData,
                                offsetInShorts * sizeof(short), sizeInShorts * sizeof(short),
                                isWriteBlocking == JNI_TRUE /* blocking */);
    env->ReleaseShortArrayElements(javaAudioData, cAudioData, 0);

    if (written > 0) {
        written /= sizeof(short);
    }

    return written;
}
Don't forget to add the method definition to the proper file.

I see in line 669, android_media_AudioTrack.cpp file I posted(in the above first attachment, I double zipped the file?) your code was implemented, only difference I found was this from my code to yours.

line 672 - jint javaAudioFormat) { to
jint javaAudioFormat,
jboolean isWriteBlocking) {
&
line 695 - true /*blocking write, legacy behavior*/); to
isWriteBlocking == JNI_TRUE /* blocking */);

Is that all that is needed to be changed or am I missing something?

I will try to build some new firmware with the changes.

Thanks
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #36
Take care:

Old, since API v3, you still need that:
Code:
int write (short[] audioData,
                int offsetInShorts,
                int sizeInShorts)
New since API v23, you will need that for our PT:
Code:
int write (short[] audioData,
                int offsetInShorts,
                int sizeInShorts,
                int writeMode)

It's a simple overwrite, needed so that the jni can map it correctly.

Edit: If you are really lazy (not recommended), just add the second method and call your first method ommiting the additional parameter. Not recommended, cause of the blocking mode that you cannot handle then. We assume blocking mode in the sink. As this is where the loop for adding packages stop.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
(This post was last modified: 2016-09-19 11:28 by fritsch.)
find quote
mo123 Offline
Fan
Posts: 363
Joined: May 2013
Reputation: 6
Post: #37
(2016-09-19 11:26)fritsch Wrote:  Take care:

Old, since API v3, you still need that:
Code:
int write (short[] audioData,
                int offsetInShorts,
                int sizeInShorts)
New since API v23, you will need that for our PT:
Code:
int write (short[] audioData,
                int offsetInShorts,
                int sizeInShorts,
                int writeMode)


It's a simple overwrite, needed so that the jni can map it correctly.

Edit: If you are really lazy (not recommended), just add the second method and call your first method ommiting the additional parameter. Not recommended, cause of the blocking mode that you cannot handle then. We assume blocking mode in the sink. As this is where the loop for adding packages stop.

So

If I look at Audiotrack.java:
In the Marshmallow version it is the same as your code and correct?
https://github.com/geekboxzone/mmallow_f...java#L1839

In the Lollipop version it is missing the 'int writeMode)' and I should add it.
https://github.com/geekboxzone/lollipop_...java#L1288

Should these lines also be added to Lollipop as it is in Marshmallow or not needed and will cause other problems?
https://github.com/geekboxzone/mmallow_f...1846-L1849

I plan to move to Marshmallow soon but would be great in the meantime if it can work on Lollipop.

Thanks
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #38
I'd say: if you can add it. The reason is not the functionality but what "blocking" does. This basically means: block until data is consumed. Depending on other implementations it would run into fragmentation with non blocking. E.g. it will return and application needs to fragment the data. In kodi's case we rely on a blocking sink to consume data.

Quote:Should these lines also be added to Lollipop as it is in Marshmallow or not needed and will cause other problems?
https://github.com/geekboxzone/mmallow_f...1846-L1849

I'd handle this gracefully, e.g. if (whatever) do whatever else do the opposite.

There is something I don't like about the API. Both short[] methods are "documented" with opening the sink with PCM_16bit, which we can't - as we want to use IEC format.

What would you suggest?

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #39
Btw, something to add. When I look at your code I see that your native method adds the samples "bytewise". The whole reason to use shorts is that this fits the sample size and doing it bytewise might fragment within one sample - which should never happen. But that's on your side of the "hidden" implementation, but take care.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #40
Any news on your side?

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
find quote
mo123 Offline
Fan
Posts: 363
Joined: May 2013
Reputation: 6
Post: #41
(2016-09-20 08:34)fritsch Wrote:  Any news on your side?

Hi

Can you perhaps help to modify this file correctly and upload perhaps?
https://github.com/geekboxzone/lollipop_...java#L1288
I got a compile error about write mode and think I did something wrong.
I will send the complete error log later, something about MediaAudiotest java file that don't correspond with the changes in above Audiotrack.java file, short has 3 int values but returned 3 int + boolean(write mode) values.

Isn't there perhaps a way to make a test build with the change Koying made so I can perhaps test it without above file changes or won't it work?

I will try to compile everything on Marshmallow but have to download the SDK on my slow connection that will take several hours.
So will only be able to provide feedback on MM in 2-3 days.
I think there it will work correct.
(This post was last modified: 2016-09-20 08:59 by mo123.)
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #42
I know that the build works, cause it works on Nexus with Android N.

You just have to add the new method in there.

Easiest:
Code:
public int write(short[] audioData, int offsetInShorts, int sizeInShorts
                      @WriteMode int writeMode) {
  // lazy way to ignore the blocking argument
  return write(audioData, offsetInShorts, sizeInShorts);
}

But as you support blocking for float anyways, it's just the code I gave you above already.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
(This post was last modified: 2016-09-20 09:12 by fritsch.)
find quote
mo123 Offline
Fan
Posts: 363
Joined: May 2013
Reputation: 6
Post: #43
(2016-09-20 09:11)fritsch Wrote:  I know that the build works, cause it works on Nexus with Android N.

You just have to add the new method in there.

Easiest:
Code:
public int write(short[] audioData, int offsetInShorts, int sizeInShorts
                      @WriteMode int writeMode) {
  // lazy way to ignore the blocking argument
  return write(audioData, offsetInShorts, sizeInShorts);
}

But as you support blocking for float anyways, it's just the code I gave you above already.

Hi

I tried to do the 'short' the same as for 'float'
But I get an Android compile error for Android LP.
I'f I use your method above, I get the same compile error also as below.
Something is not right in android_media_AudioTrack.cpp

Error
"frameworks/base/media/java/android/media/AudioTrack.java:45: error 101: Unresolved link/see tag "#write(short[], int, int)" in android.media.AudioTrack
DroidDoc took 427 sec. to write docs to out/target/common/docs/doc-comment-check
make: *** [out/target/common/docs/doc-comment-check-timestamp] Error 45"

Android compile log
http://pastebin.com/K30JXbz2

android_media_AudioTrack.cpp (Problem possiblly here)
http://pastebin.com/sbLGHT5J

AudioTrack.java
http://pastebin.com/N3R2jpCW

Do you have any advise?
I don't have a lot of Android coding knowledge, so for me it is very difficult if I run into a problem.

I asked another user to compile Marshmallow firmware with the IEC patches where the 'short' is already included correctly, so waiting for that also.
(This post was last modified: 2016-09-21 07:40 by mo123.)
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #44
From the log you patched out the 3 param method. That you are not allowed to do! You need both methods.
The warning might also be missing generated doc, cause annotation missing?

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
(This post was last modified: 2016-09-21 07:47 by fritsch.)
find quote
fritsch Offline
Team-Kodi Developer
Posts: 17,723
Joined: Aug 2011
Reputation: 544
Location: Stuttgart*
Post: #45
To speedup things: http://jenkins.kodi.tv/job/Android-ARM/9262/ please test this.

Also tell me about the plan on how you inmplement GetHeadPosition for dts-hd / truehd - so that I can implement that. We open 2 channels, but send 8 channels 192 khz - while AE thinks we consume 8 channels, that we need to account for.

"Your most vocal users are not reflective of your userbase" J.M.
"Of course, they [XP Users and people with outdated hardware] need to tell the world about the kind of hero they are, and block innovation for everyone else because their decades old OS / hardware needs to work =p" nevcairiel (ffmpeg)
No Debug Log no issue.
find quote
Post Reply