-
Notifications
You must be signed in to change notification settings - Fork 24
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
Foxglove h.264 support #22
Comments
Do you see a path to achieving compatibility without me outright adopting Foxglove's message format?
I believe Foxglove gets away with a simpler message format because they place restrictions on the streamed data that ffmpeg generally doesn't have. Like these: I would be loathe to forgo B frames. Seems easier for FoxGlove to support ffmpeg_image_transport_msgs. Not sure they want to do that either because if the customer selects an encoder that produces B frames, my plugin will work just fine, and later the customer will complain that they can't view the data they have collected. |
Thank you for the good answer and I agree with you points 👍 If I decide to move forward with this, I'll try to make a patch with a parameter to select the message format (and keep the original as default). You may close this issue and thanks! |
One further note: Foxglove also has an "experimental extension" with h.264 support using "sensor_msgs::msg::CompressedImage". I think Foxglove should implement a single "panel" with support for multiple message types instead of having multiple panels. Maybe this will happen in the future... |
Hi @pfavr2, did you end up moving forward with that? I'd be interested to have that patch as well.
Is it this one you are referring to? https://github.com/codewithpassion/foxglove-studio-h264-extension |
FYI https://github.com/angsa-robotics/ffmpeg_image_transport/pull/1/files. I did it hastily though, if you have any remarks please shoot |
@tonynajjar great to have a working implementation!
|
That's what I assumed, that the foxglove image panel implements its own decoding under the hood, unfortunately it's not open-source anymore to take a look. Your suggestion sounds generally good, it would be nice to not have my fork for long. Unfortunately I need to move on to something else for now so I won't be able to implement it but how about we open back this ticket and hopefully somebody picks it up? |
I do have the cycles to implement it. I contacted foxglove to clarify if they consider "foxglove_compressed_video_transport" a trademark violation. |
Correct. For our app (which can run in desktop and web) we do not assume or require the user have a "ros workspace" installed. This is great for being able to view data anywhere - but also has the side-effect that encodings and messages need to be more strictly specified than the typical ROS approach of "go install this image transport and it handles whatever it made up". This is why it is important for us to be clear in our message definitions what the "format" is - because we can only support certain things rather than anything any local image transport can do.
We'll get back to you on this and the above mentioned |
@defunctzombie @berndpfrommer Any updates on this? |
I was distracted by other projects. Will work a bit on it over the weekend. |
The core encoder/decoder modules have been lifted out now to the ffmpeg_encoder_decoder repo |
The foxglove image transport plugin is finished. Please check it out to see if it works for you. |
Thank you for a great ROS2 component!
This is a feature request: to support the foxglove.CompressedVideo message type in ffmpeg_image_transport
In this blog post Foxglove announces H.264 support in Foxglove:
https://foxglove.dev/blog/announcing-h264-support-in-foxglove
"...NAL units must be written to your log in a format that Foxglove will understand. For this, we’ve created the foxglove.CompressedVideo message schema – set the format field to “h264”, and the data field to contain the NAL units in Annex B format."
I think it would be nice to have this included in ffmpeg_image_transport when using h.264.
I could maybe help getting it done?
The text was updated successfully, but these errors were encountered: