-
Notifications
You must be signed in to change notification settings - Fork 30
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
FG-2933: Add CompressedAudio schema #159
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -843,6 +843,44 @@ Specifically, the requirements for different \`format\` values are: | |
], | ||
}; | ||
|
||
const CompressedAudio: FoxgloveMessageSchema = { | ||
type: "message", | ||
name: "CompressedAudio", | ||
description: "A single frame of a compressed audio bitstream", | ||
fields: [ | ||
{ | ||
name: "timestamp", | ||
type: { type: "primitive", name: "time" }, | ||
description: "Timestamp of audio frame", | ||
}, | ||
{ | ||
name: "data", | ||
type: { type: "primitive", name: "bytes" }, | ||
description: `Compressed audio frame data.`, | ||
}, | ||
{ | ||
name: "format", | ||
type: { type: "primitive", name: "string" }, | ||
description: "Audio format.\n\nSupported values: `opus`.", | ||
}, | ||
{ | ||
name: "type", | ||
type: { type: "primitive", name: "string" }, | ||
description: "Frame type.\n\n Supported values: `key`, `delta`.", | ||
}, | ||
Comment on lines
+866
to
+870
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We historically chose not to include this for CompressedVideo, although I don't think I know the whole reasoning behind the decision. It's certainly caused us a lot of pain having to detect key frames ourselves, but on the other hand it's nicer for users. cc @defunctzombie @snosenzo for any thoughts / additional historical context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice with the MCAP recording demo as it is in foxglove/mcap#1293, the frames are all keyframes, which would make this field somewhat unnecessary. That might not be the case for all users though. It definitely pains me to include this extra field in every message (more data => more storage costs), but it does mean there is no logic of detecting keyframes like you said. A cursory search did not come up with how to detect audio keyframes, so I'd be worried about the complexity of the logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I pushed for fewer fields (like dropping isKeyframe and other metadata config). My arguments were similar to those discussed here: a) easier for the user and b) fewer fields that did not matter 99.9% of the time c) information which was already present in the data. I would have the same arguments for
Do we need to support this "description" concept? https://developer.mozilla.org/en-US/docs/Web/API/AudioDecoder/configure#description |
||
{ | ||
name: "sample_rate", | ||
type: { type: "primitive", name: "uint32" }, | ||
description: "Number of audio samples per second.", | ||
}, | ||
{ | ||
name: "number_of_channels", | ||
type: { type: "primitive", name: "uint32" }, | ||
description: "Number of audio channels in the input.", | ||
}, | ||
], | ||
}; | ||
|
||
const RawImage: FoxgloveMessageSchema = { | ||
type: "message", | ||
name: "RawImage", | ||
|
@@ -1482,6 +1520,7 @@ export const foxgloveMessageSchemas = { | |
Color, | ||
CompressedImage, | ||
CompressedVideo, | ||
CompressedAudio, | ||
CylinderPrimitive, | ||
CubePrimitive, | ||
FrameTransform, | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 added this so that CI runs on my branches with a slash in the branch name. I'm happy to remove this if you're not interested in this change.
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've been using slashes in my branch names and didn't have an issue with CI running: #158
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.
This PR is targeting a branch other than
main
(my other branch,fgwt202412/ubuntu-22_04-python-3_7
), which does have a slash in it.Docs
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, makes sense, this is for the target branch rather than the head branch. In some other repos we use
pull_request: {}
which may do the same thing by default? Anyhow, this seems fine