Update comments for foxglove.CompressedVideo schema
#129
Conversation
| @@ -187,13 +187,13 @@ import { Time } from "./Time"; | |||
|
|
|||
| /** A single frame of a compressed video bitstream */ | |||
| export type CompressedVideo = { | |||
| /** Timestamp of image */ | |||
| /** Timestamp of video frame */ | |||
| timestamp: Time; | |||
|
|
|||
| /** Frame of reference for the image. The origin of the frame is the optical center of the camera. +x points to the right in the image, +y points down, and +z points into the plane of the image. */ | |||
There was a problem hiding this comment.
This is old right?
There was a problem hiding this comment.
oh I see the comment below still says image 👍 will fix
internal/schemas.ts
Outdated
| @@ -810,7 +810,7 @@ const CompressedVideo: FoxgloveMessageSchema = { | |||
| name: "data", | |||
| type: { type: "primitive", name: "bytes" }, | |||
| description: | |||
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame).", | |||
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). NOTE: It is strongly recommended to _not_ write B frames/slices to the H264 stream and instead solely use I and P frames because this more closely mirrors live data sources.", | |||
There was a problem hiding this comment.
Would this still make sense?
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). NOTE: It is strongly recommended to _not_ write B frames/slices to the H264 stream and instead solely use I and P frames because this more closely mirrors live data sources.", | |
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame, but not a B-frame).", |
I don't know the terminology that well but the "more closely mirrors live data sources" didn't feel like it clearly explained the reasoning. If you want to keep reasoning maybe something more like "to support streaming playback"?
There was a problem hiding this comment.
a B frame is technically a "delta" frame. What about something like:
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). NOTE: It is strongly recommended to _not_ write B frames/slices to the H264 stream and instead solely use I and P frames because this more closely mirrors live data sources.", | |
| "Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). B-frames should be avoided because of their incompatibility with live sources, due to them referencing a future frame.", |
There was a problem hiding this comment.
I think "live sources" is a bit unclear and it's also not obvious why someone who's just writing an mcap file should care about live sources
There was a problem hiding this comment.
I think a more accurate answer is because of their incompatibility with streaming playback. Displaying B-frame encoded video requires lookahead.
CI failure due to Docker outage |
| frame_id: string; | ||
|
|
||
| /** Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). */ | ||
| /** Compressed video frame data. For packet-based video codecs this data must begin and end on packet boundaries (no partial packets), and must contain enough video packets to decode exactly one image (either a keyframe or delta frame). B frames should be avoided because of their inconsistency with recordings directly written from live sources. */ |
There was a problem hiding this comment.
B frames are not inconsistent with live sources, live sources sometimes introduce a buffer and some latency to create them. We don't support them because of our source buffering code in studio, not because of some inherent unsuitability for the task. Therefore, i'd modify this to just say "Note: Foxglove Studio does not support display of video streams that include B frames".
|
Thanks for the feedback. I've updated the description to combine both @jhurliman and @james-rms suggestions.
|
Timestamp refers to video frame instead of image
Added note to
datafield to state that B-frames should not be used.