-
Notifications
You must be signed in to change notification settings - Fork 23
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
Release 0.8.11 #386
Release 0.8.11 #386
Conversation
yatharthranjan
commented
Sep 9, 2024
•
edited by pvannierop
Loading
edited by pvannierop
- Add polar SDK schemas and specs
- update secret for snapshot publishing
- pRMT audio input schemas and spec
- Various security fixes
Add Polar schemas
Configure using github secret for snapshot publishing
Created New Schema And Specifications For Phone Audio Input Plugin
@yatharthranjan for the pRMT audio plugin, should we add button event markers to the schema? Or is this using something else? |
Hi @afolarin, I thought it could be separate from the audio plugin so it can be utilised with any plugin. It could be either its own plugin or part of the core application. |
Yes, it would be good to have a standard "user-triggered event" for RADAR-base generally. The semantics I guess could mean it is used anywhere (p/aRMT apps, wearables etc). I guess then it needs to have some capability to include some context, which would be useful as we could then specify as part of e.g. config what the event means in that specific use case/project etc. |
Ok sure, I have added an issue to track this -- RADAR-base/radar-prmt-android#149 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a few questions.
{ "name": "filePath", "type": "string", "doc": "Path of the audio file retrieved after uploading to S3 storage." }, | ||
{ "name": "deviceName", "type": "string", "doc": "Name of the input audio device used for routing during this recording." }, | ||
{ "name": "deviceId", "type": "string", "doc": "Identifier associated with the input device used for audio recording." }, | ||
{ "name": "deviceSampleRates", "type": "string", "doc": "Supported sample rates of the input audio device." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sample rates and encodings, this will be the string representation of the array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, @this-Aditya is this supposed to be an array or will it be like a comma-separated string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I planned to use an array for this, but I remember joris's comment to avoid using an array here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sampleRates, channelCounts and encodings are comma-separated string.
{ "name": "audioFileSize", "type": "long", "doc": "Size of the audio file (in bytes)." }, | ||
{ "name": "hadPlayback", "type": "boolean", "doc": "Whether the recorded audio file was played before uploading to s3 storage." }, | ||
{ "name": "audioFileExtension", "type": "string", "doc": "Extension of the audio file." }, | ||
{ "name": "configuredSampleRate", "type": "int", "doc": "Sample rate for audio recording configured by firebase remote configs in application."}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this always be int and no decimals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but @this-Aditya can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok I see, thanks!
To fix software vulnerability.
[Security] Fix software vulnerabilities for 0.8.11 release
I am merging this release because every reviewers comment appears to have been addressed. |