Android: Use common CBuffer instead of custom RingBuffer #1528

j-santander · 2021-04-17T16:21:08Z

A couple of changes to the android code:

When I did my first round of modifications to the already existing android code, I needed some decoupling between receiving samples from the network and copying it to the oboe output buffers. To support that I brought a RingBuffer class without realizing there was already one in the common code (CBuffer class in buffer.h). Later I realized there were some mistakes in my ring buffer... so now I've taken the opportunity to remove it and use the common CBuffer instead.
Not sure why we were configuring oboe to use float samples. I believe I recall that at that time there was an ongoing effort to move to float everywhere. That change seems to not have been merged yet, so I've simplified the float to int16_t and int16_t to float code by simply configuring oboe to use int16_t samples.

I've verified the build on my terminal (Google Pixel 3a)

…6_t samples

ann0see

Thank you! Didn't check it logic wise, but please have a look at the styling.

ann0see · 2021-04-24T18:21:43Z

android/sound.cpp

-                static_cast<int16_t>(floatData[frmNum * oboeStream->getChannelCount() + channelNum] * _MAXSHORT);
-        }
-    }
+    memcpy(vecsTmpInputAudioSndCrdStereo.data(),floatData,sizeof(int16_t)*numFrames * oboeStream->getChannelCount());


I think the style is not correct here.

Thanks, is there a .clang-format I can use? I believe I saw something in the issues/discussions but I don't know if it finally materialized.

Yes, it's here: https://github.com/jamulussoftware/jamulus/pull/1127/files#diff-1026e0038b722990204a42bed8a6f7c0ec2302aa79e3fad1959d62ba968edfa2

It's still not merged and done for existing code yet, but new code can certainly be run through this.

ok, new commit after applying .clang-format as indicated by @hoffie
Thanks

pljones · 2021-04-25T12:09:10Z

android/sound.cpp

-               ", SampleRate: "                << sSampleRate <<
-               ", AudioFormat: "               << sAudioFormat <<
-               ", FramesPerCallback: "         << sFramesPerCallback << "]";
+    qInfo() << "Stream details: [sDirection: " << sDirection << ", FramesPerBurst: " << sFramesPerBurst << ", BufferSizeInFrames: " << sBufferSizeInFrames


OK, this is a good test for the .clang-format. The width is just a little too high - this exceeds Github's unified diff frame width.

pljones · 2021-04-25T12:28:34Z

android/sound.cpp


    return iOboeBufferSizeMono;
 }

 // This is the main callback method for when an audio stream is ready to publish data to an output stream
 // or has received data on an input stream. As per manual much be very careful not to do anything in this back that
 // can cause delays such as sleeping, file processing, allocate memory, etc.
-oboe::DataCallbackResult CSound::onAudioReady ( oboe::AudioStream* oboeStream,


And I guess this is one of the things we may have to live with going with the .clang-format.

pljones · 2021-04-25T12:33:50Z

@sthenos - as you did the original Android work, is there a chance you could review this, please?

Looks okay to me but I've no Android knowledge.

android/sound.cpp

pljones · 2021-06-26T07:23:01Z

OK, one more question raised in the code - but it's probably an example showing clearly someone who knows Android (i.e. not me!) needs to review this.

j-santander · 2021-06-26T08:07:27Z

Sorry, I'm currently away from my desk and I cannot check, but I thought it was the other way: Android code was using float while the rest of jamulus used pcm16. The original (non working) code used float. When I introduced the small modifications that make it start working I commented about that and I was told that there was an upcoming change to move everything to float. When I wrote this PR, I checked and the change to float hadn't happened, so I removed the extra code converting from float to int and configured oboe to use pcm16. When this week I updated the PR I didn't check again if jamulus was still using pcm16 or was already using floats. When I get back home I'll review that.

pljones · 2021-06-26T11:05:50Z

Thanks. It may have been as you say when the original Android branch was taken - it's not that long since Jamulus switched from int to float.

... and I could even be wrong! (I try to avoid the low-level audio stuff as much as I try to avoid the low-level network stuff: break those and you're not jamming :) )

j-santander · 2021-06-26T11:15:28Z

Ok, thanks. I'll review and modify accordingly and resubmit once is ready and retested. Cheers Julian El sáb., 26 jun. 2021 13:06, Peter L Jones ***@***.***> escribió:

…

Thanks. It may have been as you say when the original Android branch was taken - it's not that long since Jamulus switched from int to float. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUIC4RY4SRRMY25A2Y2GUTTUWYBTANCNFSM43DHFYKA> .

j-santander · 2021-06-26T18:33:17Z

Oh, well, I'm puzzled.

I see that on soundbase.h there's still

void ProcessCallback ( CVector<int16_t>& psData )

That, I believe, still indicates that (at least the input) is still expected to be int16_t data, not float.

I see that the Issue #544 was closed and the related PR #535 and #627 were merged on a separate branch (no longer active). In PR #660 it is mentioned that a previous attempt (#535) had to be reverted and this PR was closed with

The branch https://github.com/corrados/jamulus/tree/integrate_float2 is already in the Jamulus Git repo. So there is no need for a Pull Request here. As soon as I continue working on that topic, I'll update the branch and simply merge the code into the Git master branch. I'll close this Pull Request therefore now.

So.... I'd say that the change to float never happened.

pljones · 2021-06-27T07:29:57Z

So.... I'd say that the change to float never happened.

Thanks for checking (and no wonder I was confused by the sound of it!).

ann0see · 2021-11-03T20:50:50Z

@j-santander @pljones What's missing here?

pljones · 2021-11-03T22:22:08Z

Thanks for pinging me on this -- as it reminds me again that we're not yet float. At some point that work needs to be completed. But that's for a different time..!

In terms of review: it needs someone who knows the Android audio subsystem and how to do Android development to comment on it.

ann0see · 2022-01-02T17:59:24Z

@sthenos - as you did the original Android work, is there a chance you could review this, please?

It would be really great if someone with android knowledge could move this forward.

sthenos · 2022-01-02T23:48:39Z

I'll have a look, when i get a chance. Cheers Simon

…

On Sun, 2 Jan 2022, 17:59 ann0see, ***@***.***> wrote: @sthenos <https://github.com/sthenos> - as you did the original Android work, is there a chance you could review this, please? It would be really great if someone with android knowledge could move this forward. — Reply to this email directly, view it on GitHub <#1528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVI4UJJ6CAQSDGFESULBNDUUCHAPANCNFSM43DHFYKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

softins · 2022-01-10T13:15:24Z

Thanks for pinging me on this -- as it reminds me again that we're not yet float. At some point that work needs to be completed. But that's for a different time..!

The float work that was done was specifically in the mixing (CServer::MixEncodeTransmitData()), in order to avoid potential 16-bit overflow when summing channels. Volker's original commit to do this was 9c5a779, where he starting using an intermediate buffer of double instead of summing directly into the int16 output buffer. Using double was probably overkill, and there was a subsequent accepted commit 9171caa from HPS changing it from double to float.

Once the final sum of the samples was calculated, it then was converted back to int16 when writing to the output buffer.

Of course, the protocol audio data is Opus encoded, so the remaining question would be whether it is possible/better/worse to get the uncompressed audio in and out of Opus in float or int16 format. But as said above, that is for another discussion, not this PR.

softins · 2022-01-13T13:08:24Z

I have squashed this into a single commit from the branch point, and included a necessary fix to Jamulus.pro, in my own repo at https://github.com/softins/jamulus/tree/android-update. I retained @j-santander as author of the commit.

I will get it rebased to the current master and submit it as a new PR from my branch, to replace this PR.

Marking this PR as draft in the meantime.

Android update replacing PR #1528

Android: Use common CBuffer instead of custom RingBuffer and use int1…

b68e1cf

…6_t samples

ann0see reviewed Apr 24, 2021

View reviewed changes

Applied clang-format

75f0692

pljones reviewed Apr 25, 2021

View reviewed changes

hoffie mentioned this pull request Apr 25, 2021

add clang-format style definition #1127

Merged

j-santander mentioned this pull request Jun 24, 2021

android/sound.cpp: Fix overflow warning #1385

Closed

jsantander added 2 commits June 24, 2021 17:20

solved merge-conflicts

d728c90

fix clang-format

39943ad

pljones reviewed Jun 24, 2021

View reviewed changes

android/sound.cpp Outdated Show resolved Hide resolved

pljones reviewed Jun 24, 2021

View reviewed changes

android/sound.cpp Outdated Show resolved Hide resolved

j-santander added 2 commits June 24, 2021 22:59

address review comments

c94b2fc

rename floatData to intData

75f40cd

pljones reviewed Jun 26, 2021

View reviewed changes

android/sound.cpp Show resolved Hide resolved

softins self-assigned this Jan 12, 2022

softins marked this pull request as draft January 13, 2022 13:09

softins mentioned this pull request Jan 13, 2022

Android update replacing PR #1528 #2237

Merged

5 tasks

softins closed this in #2237 Jan 13, 2022

softins added a commit that referenced this pull request Jan 13, 2022

Merge pull request #2237 from softins/android-update

5eec44f

Android update replacing PR #1528

pljones added this to the Release 3.8.2 milestone Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Use common CBuffer instead of custom RingBuffer #1528

Android: Use common CBuffer instead of custom RingBuffer #1528

j-santander commented Apr 17, 2021

ann0see left a comment

ann0see Apr 24, 2021

j-santander Apr 24, 2021

hoffie Apr 24, 2021

j-santander Apr 25, 2021

pljones Apr 25, 2021

pljones Apr 25, 2021

pljones commented Apr 25, 2021

pljones commented Jun 26, 2021

j-santander commented Jun 26, 2021 via email •

edited

Loading

pljones commented Jun 26, 2021 •

edited

Loading

j-santander commented Jun 26, 2021 via email

j-santander commented Jun 26, 2021

pljones commented Jun 27, 2021

ann0see commented Nov 3, 2021

pljones commented Nov 3, 2021

ann0see commented Jan 2, 2022

sthenos commented Jan 2, 2022 via email

softins commented Jan 10, 2022

softins commented Jan 13, 2022 •

edited

Loading

Android: Use common CBuffer instead of custom RingBuffer #1528

Android: Use common CBuffer instead of custom RingBuffer #1528

Conversation

j-santander commented Apr 17, 2021

ann0see left a comment

Choose a reason for hiding this comment

ann0see Apr 24, 2021

Choose a reason for hiding this comment

j-santander Apr 24, 2021

Choose a reason for hiding this comment

hoffie Apr 24, 2021

Choose a reason for hiding this comment

j-santander Apr 25, 2021

Choose a reason for hiding this comment

pljones Apr 25, 2021

Choose a reason for hiding this comment

pljones Apr 25, 2021

Choose a reason for hiding this comment

pljones commented Apr 25, 2021

pljones commented Jun 26, 2021

j-santander commented Jun 26, 2021 via email • edited Loading

pljones commented Jun 26, 2021 • edited Loading

j-santander commented Jun 26, 2021 via email

j-santander commented Jun 26, 2021

pljones commented Jun 27, 2021

ann0see commented Nov 3, 2021

pljones commented Nov 3, 2021

ann0see commented Jan 2, 2022

sthenos commented Jan 2, 2022 via email

softins commented Jan 10, 2022

softins commented Jan 13, 2022 • edited Loading

j-santander commented Jun 26, 2021 via email •

edited

Loading

pljones commented Jun 26, 2021 •

edited

Loading

softins commented Jan 13, 2022 •

edited

Loading