FG-2933: Add CompressedAudio schema #159
Conversation
fgwt202412
force-pushed
the
fgwt202412/compressed-audio-schema
branch
from
af32368 to
9bf7142
Compare
|
The CI / python workflow is failing: https://github.com/foxglove/schemas/actions/runs/12341162294/job/34439471378?pr=159 It seems like Python 3.7 is not supported for Ubuntu 24.04 x64 - not sure if runners recently changed. According to this issue, we would need to fix to |
internal/schemas.ts
Outdated
There was a problem hiding this comment.
This CompressedAudio schema was the only addition - the rest was auto-generated by yarn update-generated-files
fgwt202412
force-pushed
the
fgwt202412/compressed-audio-schema
branch
from
e2cd86e to
4cd9d40
Compare
fgwt202412
changed the base branch from
main
to
fgwt202412/ubuntu-22_04-python-3_7
| @@ -5,7 +5,7 @@ on: | |||
| branches: [main] | |||
| tags: ["releases/**"] | |||
| pull_request: | |||
| branches: ["*"] | |||
| branches: ["**"] | |||
There was a problem hiding this comment.
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.
🤔 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.
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.
There was a problem hiding this comment.
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
fgwt202412
force-pushed
the
fgwt202412/compressed-audio-schema
branch
from
810146c to
e5232a8
Compare
fgwt202412
force-pushed
the
fgwt202412/compressed-audio-schema
branch
from
e5232a8 to
fbb9ccf
Compare
| { | ||
| name: "type", | ||
| type: { type: "primitive", name: "string" }, | ||
| description: "Frame type.\n\n Supported values: `key`, `delta`.", | ||
| }, |
There was a problem hiding this comment.
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.
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.
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
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 type here. It seems like we should be able to identify this from the stream (and the proposed panel code does not use this field so why add it now - we can add it later).
sample_rate and num_channels seem to behave differently and are not present in the data itself which means they need to be present somewhere. One could imagine this information could have been something in the mcap channel metadata but we have not leaned on that and it makes it harder to reason about how to craft the messages.
Do we need to support this "description" concept? https://developer.mozilla.org/en-US/docs/Web/API/AudioDecoder/configure#description
Changelog
Add CompressedAudio schema
Docs
None
Description
Foxglove does not currently support audio messages of any kind. In order to support playback of audio in Foxglove Studio, this schema provides the wire format for audio frames that Foxglove can play.
Tested locally using the MCAP recording demo and a new Audio panel.
Used in:
FG-2933