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

Image width/height/channel method helper seems broken #6357

Closed
teh-cmc opened this issue May 16, 2024 · 2 comments · Fixed by #6360
Closed

Image width/height/channel method helper seems broken #6357

teh-cmc opened this issue May 16, 2024 · 2 comments · Fixed by #6360
Assignees
Labels
🔺 re_renderer affects re_renderer itself 🦟 regression A thing that used to work in an earlier release
Milestone

Comments

@teh-cmc
Copy link
Member

teh-cmc commented May 16, 2024

    dbg!((
        &tensor.shape,
        tensor.shape_short(),
        [height, width, channel]
    ));

outputs:

[crates/re_viewer_context/src/gpu_bridge/tensor_to_gpu.rs:556:5] (&tensor.shape, tensor.shape_short(), [height, width, channel]) = (
    [
        width=1,
        height=1,
        depth=4,
    ],
    [
        depth=4,
    ],
    [
        4,
        1,
        1,
    ],
)

which then results in:

[2024-05-16T12:58:07Z WARN  re_space_view_spatial::mesh_cache] Failed to load mesh "/world/65v1/comps/sprite": Only 3 and 4 channel u8 tensor data is supported currently for mesh textures, got 1.

Looks like a logic issue in:

    pub fn image_height_width_channels(&self) -> Option<[u64; 3]> {
        let shape_short = self.shape_short();
        match &self.buffer {
            // In the case of NV12, return the shape of the RGB image, not the tensor size.
            TensorBuffer::Nv12(_) => {
                // NV12 encodes a color image in 1.5 "channels" -> 1 luma (per pixel) + (1U+1V) / 4 pixels.
                match shape_short {
                    [h, w] => Some([h.size * 2 / 3, w.size, 3]),
                    _ => None,
                }
            }
            // In the case of YUY2, return the shape of the RGB image, not the tensor size.
            TensorBuffer::Yuy2(_) => {
                // YUY2 encodes a color image in 2 "channels" -> 1 luma (per pixel) + (1U + 1V) (per 2 pixels).
                match shape_short {
                    [h, w] => Some([h.size, w.size / 2, 3]),
                    _ => None,
                }
            }
            TensorBuffer::Jpeg(_)
            | TensorBuffer::U8(_)
            | TensorBuffer::U16(_)
            | TensorBuffer::U32(_)
            | TensorBuffer::U64(_)
            | TensorBuffer::I8(_)
            | TensorBuffer::I16(_)
            | TensorBuffer::I32(_)
            | TensorBuffer::I64(_)
            | TensorBuffer::F16(_)
            | TensorBuffer::F32(_)
            | TensorBuffer::F64(_) => {
                match shape_short.len() {
                    1 => {
                        // Special case: Nx1(x1x1x …) tensors are treated as Nx1 gray images.
                        // Special case: Nx1(x1x1x …) tensors are treated as Nx1 gray images.
                        if self.shape.len() >= 2 {
                            Some([shape_short[0].size, 1, 1])
                        } else {
                            None
                        }
                    }
                    2 => Some([shape_short[0].size, shape_short[1].size, 1]),
                    3 => {
                        let channels = shape_short[2].size;
                        if matches!(channels, 1 | 3 | 4) {
                            // mono, rgb, rgba
                            Some([shape_short[0].size, shape_short[1].size, channels])
                        } else {
                            None
                        }
                    }
                    _ => None,
                }
            }
        }
    }
@teh-cmc teh-cmc added 🪳 bug Something isn't working 🔺 re_renderer affects re_renderer itself labels May 16, 2024
@emilk
Copy link
Member

emilk commented May 16, 2024

The problem is that shape_short ignores leading dim=1 since #5579

@emilk emilk added this to the 0.16 milestone May 16, 2024
@emilk
Copy link
Member

emilk commented May 16, 2024

The usage of shape_short in Nv12 and Yuy2 is also sus

@emilk emilk self-assigned this May 16, 2024
@teh-cmc teh-cmc added 🦟 regression A thing that used to work in an earlier release and removed 🪳 bug Something isn't working labels May 16, 2024
emilk added a commit that referenced this issue May 16, 2024
- Fixes #6357

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6360?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6360?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6360)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Jeremy Leibs <jeremy@rerun.io>
emilk added a commit that referenced this issue May 16, 2024
- Fixes #6357

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6360?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6360?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6360)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Jeremy Leibs <jeremy@rerun.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔺 re_renderer affects re_renderer itself 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants