Skip to content
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

CLI: mcap filter subcommand #445

Merged
merged 6 commits into from
Jul 7, 2022
Merged

CLI: mcap filter subcommand #445

merged 6 commits into from
Jul 7, 2022

Conversation

james-rms
Copy link
Collaborator

@james-rms james-rms commented Jun 27, 2022

Public-Facing Changes

  • New mcap filter subcommand added, which you can use to filter MCAPs by topic and time range.
  • Bug fixed in the go MCAP library when writing attachment indexes, which would cause the attachment
    content type to be malformed.

Description
This PR adds a new subcommand to the MCAP CLI tool that allows people to quickly filter topics into and out-of an MCAP file.

@james-rms james-rms marked this pull request as ready for review June 30, 2022 20:58
There was a bug where WriteAttachmentIndex would fail to
pre-allocate 8 bytes for one of its uint64 timestamp fields.

This would cause the write buffer to be too short to fit the
final field (the content type string). In our existing test cases,
the content type was more than 8 chars long, and the effect was
to truncate the content type string by 8 chars. However,
in implementing `mcap filter`, i created a test case with a 0-length
content type string, which caused a segfault.
@james-rms james-rms force-pushed the feature/cli-filter branch 2 times, most recently from bad50ee to 78204b6 Compare July 7, 2022 04:57
This subcommand allows command-line users to filter MCAP files
by time range and topic name.
@defunctzombie
Copy link
Contributor

What is the syntax for filter? Some examples?

@james-rms james-rms requested review from wkalt and jtbandes July 7, 2022 21:45
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
@james-rms
Copy link
Collaborator Author

@defunctzombie from the long help:

This subcommand filters an MCAP by topic and time range to a new file.
When multiple regexes are used, topics that match any regex are included (or excluded).
usage example:
  mcap filter in.mcap -o out.mcap -y /diagnostics -y /tf -y /camera_(front|back)

Some more examples:

# exclude topic by regex, using pipes
cat in.mcap | mcap filter -n /pose > out.mcap

# time ranges
mcap filter in.mcap -o out.mcap --start-secs 1657000000 --end-secs 1657030667

go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
}
} else {
channels[channel.ID] = markableChannel{channel, false}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we write the schema+channel after observing the channel, rather than after observing the first message, would that let us drop the "markable" aspect of the channel map, or maybe even drop the channel map altogether?

That would also result in us returning channels with matching topics even if they have no messages on them, which seems more correct on balance to me. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO a channel with no messages in it in an MCAP does not make a lot of sense to me, which is why I did it this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, a channel in an mcap with no messages is definitely something you can record - so why wouldn't the filter command preserve channels on the supplied topic?

go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
go/cli/mcap/cmd/filter.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wkalt wkalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few high-level comments - feel free to address as you like. I pulled it down and tested, seems to work fine with the stdout change.

@james-rms james-rms merged commit 38452d4 into main Jul 7, 2022
@james-rms james-rms deleted the feature/cli-filter branch July 7, 2022 23:20
@james-rms james-rms mentioned this pull request Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants