-
Notifications
You must be signed in to change notification settings - Fork 42
RawImage: document encodings in schemas #273
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
base: main
Are you sure you want to change the base?
Conversation
4f62f0b
to
39fb06d
Compare
@@ -2335,7 +2333,52 @@ bytes | |||
</td> | |||
<td> | |||
|
|||
Raw image data | |||
Raw image data. |
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.
Reviewers should probably start here.
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 tend to dislike putting so much formatted text into table cells because it tends to make formatting a bit janky, especially on smaller screens.
I could also imagine putting these in a table on their own rather than a bulleted list. Something like this:
Encoding | Field value | Description | Step size |
---|---|---|---|
8-bit YUV | yuv422 |
U and V values are shared between horizontal pairs of pixels. Each pair of output pixels is encoded as [U, Y1, V, Y2]. | >= width * 2 |
8-bit RGB | rgb8 |
Each output pixel is encoded as [R, G, B] | >= width * 3 |
... | ... | ... | ... |
Raw image data. | |
Raw image data, encoded using the format specified by the `encoding` field. See below for a description of all supported encodings. |
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 agree, but I'm not sure if I can bite that off right now. The original text starts in schemas.ts
which goes through a generation step to be inserted into a table in the final markdown. I'd need to add a special case into the generation logic (here and in the schemas docusaurus plugin).
|
||
The requirements for each `encoding` value are: | ||
- `yuv422`, `uyvy`: | ||
- 8-bit [Y'UV](https://en.wikipedia.org/wiki/Y%E2%80%B2UV). |
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.
8-bit meaning what?
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.
signed or unsigned?
- Each output pixel is encoded as [R, G, B, Alpha]. | ||
- `step` must be greater than or equal to `width` * 4. | ||
- `bgr8`: | ||
- RGB, one byte per component. |
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.
RGB is backwards here relative to bgr8
, what does that mean? Also why does this say "one byte per component" whereas the other ones are 8-bit
, is there some difference?
- 8-bit brightness values from 0 (black) to 255 (white). | ||
- `step` must be greater than or equal to `width`. | ||
- `mono16`, `16UC1`: | ||
- 16-bit abstract per-pixel values. Rendering of these values is controlled in [Image panel color mode settings](https://docs.foxglove.dev/docs/visualization/panels/image#general). |
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.
Fascinating. How do we decode the values though? As a 16 bit unsigned number? signed? 16bit float?
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.
👍 seems like an improvement to me. There's a few more "ambiguities" that you could tighten up while you are here that I pointed out.
@@ -2294,7 +2294,7 @@ uint32 | |||
</td> | |||
<td> | |||
|
|||
Image height | |||
Image heigh in pixels |
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.
heigh
@@ -2335,7 +2333,52 @@ bytes | |||
</td> | |||
<td> | |||
|
|||
Raw image data | |||
Raw image data. |
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 tend to dislike putting so much formatted text into table cells because it tends to make formatting a bit janky, especially on smaller screens.
I could also imagine putting these in a table on their own rather than a bulleted list. Something like this:
Encoding | Field value | Description | Step size |
---|---|---|---|
8-bit YUV | yuv422 |
U and V values are shared between horizontal pairs of pixels. Each pair of output pixels is encoded as [U, Y1, V, Y2]. | >= width * 2 |
8-bit RGB | rgb8 |
Each output pixel is encoded as [R, G, B] | >= width * 3 |
... | ... | ... | ... |
Raw image data. | |
Raw image data, encoded using the format specified by the `encoding` field. See below for a description of all supported encodings. |
Raw image data. | ||
|
||
The requirements for each `encoding` value are: | ||
- `yuv422`, `uyvy`: |
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.
Are these (and other similar pairs) synonyms? It's not clear to me why we have two field values with the same description.
Raw image data | ||
Raw image data. | ||
|
||
The requirements for each `encoding` value are: |
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.
The requirements for each `encoding` value are: | |
The definitions for each `encoding` value are: |
- `yuv422`, `uyvy`: | ||
- 8-bit [Y'UV](https://en.wikipedia.org/wiki/Y%E2%80%B2UV). | ||
- U and V values are shared between horizontal pairs of pixels. Each pair of output pixels is encoded as [U, Y1, V, Y2]. | ||
- `step` must be greater than or equal to `width` * 2. |
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.
nit: remove all unnecessary periods (anywhere you have only a single sentence or a fragment)
Material Design writing guidelines
- `step` must be greater than or equal to `width` * 2. | |
- `step` must be greater than or equal to `width` * 2 |
- RGB, one byte per component. | ||
- Each output pixel is encoded as [B, G, R]. | ||
- \`step\` must be greater than or equal to \`width\` * 3. | ||
- \`bgra8\`, \`8UC3\`: |
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.
8UC3
is equivalent to bgr8
, not bgra8
description: `Raw image data. | ||
|
||
The requirements for each \`encoding\` value are: | ||
- \`yuv422\`, \`uyvy\`: |
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.
what order are these in? I'd probably order them either alphabetically, by bit depth/number of channels/etc., or by popularity
- The order of the four letters after \`bayer_\` determine the layout, so for \`bayer_wxyz8\` the pattern is: | ||
\`\`\`plaintext | ||
w | x | ||
- | - |
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.
- | - | |
- + - |
\`\`\` | ||
- \`step\` must be greater than or equal to \`width\`. | ||
- \`mono8\`, \`8UC1\`: | ||
- 8-bit brightness values from 0 (black) to 255 (white). |
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.
we don't specify the 0-255 range for other 8-bit formats -- intentional choice?
- 8-bit brightness values from 0 (black) to 255 (white). | ||
- \`step\` must be greater than or equal to \`width\`. | ||
- \`mono16\`, \`16UC1\`: | ||
- 16-bit abstract per-pixel values. Rendering of these values is controlled in [Image panel color mode settings](https://docs.foxglove.dev/docs/visualization/panels/image#general). |
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 don't know that we often refer to panel docs/settings inside schema comments, this might be the first time. Maybe we should leave it out and remove the language around "abstract values" / "brightness values"?
- 8-bit brightness values from 0 (black) to 255 (white). | ||
- \`step\` must be greater than or equal to \`width\`. | ||
- \`mono16\`, \`16UC1\`: | ||
- 16-bit abstract per-pixel values. Rendering of these values is controlled in [Image panel color mode settings](https://docs.foxglove.dev/docs/visualization/panels/image#general). |
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.
endianness?
Changelog
Docs
https://github.com/foxglove/docs/pull/628
Description
Documents our supported raw image encodings in (slightly more) detail.
Fixes: FG-6974