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.

Sign up for GitHub

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

Jump to bottom

Make rectangle_fs.wgsl compile on chrome despite angle/mesa bug (#3931) #5074

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Make rectangle_fs.wgsl compile on chrome despite angle/mesa bug (#3931) #5074

merged 3 commits into from
Feb 7, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Feb 6, 2024 •
edited by github-actions bot
Loading

What

When we added support for YUY2 format in (#4877) we we re-triggered the issue originally originally reported in #3931 and now #5073

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@jleibs jleibs marked this pull request as ready for review February 6, 2024 23:13
@jleibs jleibs added 🔺 re_renderer affects re_renderer itself 🦟 regression A thing that used to work in an earlier release 🚜 refactor Change the code, not the functionality labels Feb 6, 2024
@jleibs jleibs changed the title Code-golf the shader down some more to work around the crash again Code-golf shader down to work around crash on chrome (#3931) to avoid a suspected angle/mesa bug #3948 Feb 6, 2024
@jleibs jleibs changed the title Code-golf shader down to work around crash on chrome (#3931) to avoid a suspected angle/mesa bug #3948 Code-golf shader down to work around a suspected angle/mesa bug (#3948) to avoid crash on chrome (#3931) Feb 6, 2024
@jleibs jleibs changed the title Code-golf shader down to work around a suspected angle/mesa bug (#3948) to avoid crash on chrome (#3931) Code-golf shader down to work around a suspected angle/mesa bug to avoid crash on chrome (#3931) Feb 6, 2024
@jleibs jleibs changed the title Code-golf shader down to work around a suspected angle/mesa bug to avoid crash on chrome (#3931) Code-golf shader down to work around a suspected angle/mesa bug and avoid crash on chrome (#3931) Feb 6, 2024
@jleibs jleibs changed the title Code-golf shader down to work around a suspected angle/mesa bug and avoid crash on chrome (#3931) Code-golf rectangle_fs.wgsl to work around a suspected angle/mesa bug and avoid crash on chrome (#3931) Feb 6, 2024
@jleibs jleibs added this to the 0.13 milestone Feb 7, 2024
emilk
emilk approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

So sad that this is necessary, but the resulting code is not all bad, so 👍

crates/re_renderer/shader/rectangle_fs.wgsl Show resolved Hide resolved
crates/re_renderer/shader/decodings.wgsl
/// Loads an RGBA texel from a texture holding an NV12 encoded image at the given screen space coordinates.
fn decode_nv12(texture: texture_2d<u32>, coords: vec2i) -> vec4f {
/// Loads an RGBA texel from a texture holding an NV12 or YUY2 encoded image at the given screen space coordinates.
fn decode_nv12_or_yuy2(sample_type: u32, texture: texture_2d<u32>, coords: vec2i) -> vec4f {
Copy link
Member

Choose a reason for hiding this comment

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

it surprises me that combining these two into one function did much difference

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah -- this was the final tweak that got things to work :-(

Wumpf
Wumpf approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

what Emil said. Disastrous but will do - you once again managed to not regress us, but we won't be able to hold this up for much longer.

We did not report this so far since we weren't able to break this down well enough - when building an application from javascript that uses the translated glsl shader everything works. But we have to get to the bottom of this

crates/re_renderer/shader/decodings.wgsl
let y = f32(textureLoad(texture, vec2u(coords), 0).r);
let u = f32(textureLoad(texture, vec2u(u32(uv_col), uv_offset + uv_row), 0).r);
let v = f32(textureLoad(texture, vec2u((u32(uv_col) + 1u), uv_offset + uv_row), 0).r);
// WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING! WARNING!
Copy link
Member

Choose a reason for hiding this comment

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

are you sure these are enough WARNING!? 😆

@emilk
Copy link
Member

emilk commented Feb 7, 2024

Let's make sure we test this new shader properly before merging!

@jleibs
Copy link
Member Author

jleibs commented Feb 7, 2024 •
edited
Loading

Let's make sure we test this new shader properly before merging!

Confirmed both nv12/yuy2 working on both native and web.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2024

Size changes

Name main 5074/merge Change
dna.rrd 645.12 kiB 552.96 kiB -14.29%
plots.rrd 163.84 kiB 1054.72 kiB +543.75%

@jleibs jleibs merged commit bff8898 into main Feb 7, 2024
40 checks passed
@jleibs jleibs deleted the jleibs/codegolf_nv12 branch February 7, 2024 14:32
@teh-cmc teh-cmc changed the title Code-golf rectangle_fs.wgsl to work around a suspected angle/mesa bug and avoid crash on chrome (#3931) Make rectangle_fs.wgsl compile on chrome despite angle/mesa bug (#3931) Feb 7, 2024
@Wumpf Wumpf added exclude from changelog PRs with this won't show up in CHANGELOG.md and removed include in changelog labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilk emilk emilk approved these changes

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🔺 re_renderer affects re_renderer itself 🚜 refactor Change the code, not the functionality 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jleibs @emilk @Wumpf