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

Fix glReadPixels #78

Merged
merged 2 commits into from
Jan 5, 2019
Merged

Fix glReadPixels #78

merged 2 commits into from
Jan 5, 2019

Conversation

OhadRau
Copy link
Contributor

@OhadRau OhadRau commented Jan 5, 2019

Addresses the issue I brought up in revery-ui/revery#185. Again, this is one of those places where I feel like I sort of had to employ a hack, but it works on my test inputs (easiest way to verify that this is working is to change the glClearColor to red or blue).

Basic description of what I changed:

  • I was previously setting the image descriptor byte based on what some example code was doing. Not sure why, but this swapped the y-axis so now I just set the image descriptor byte to 0 (which should work according to the TGA spec).
  • I was previously copying in the raw pixel array as returned by glReadPixels. This would break if you used GL_UNSIGNED_BYTE on a little-endian system, so instead I'm swapping the red/blue channels in the glReadPixels stubs (both native & js).

bryphe
bryphe reviewed Jan 5, 2019
View reviewed changes
src/gl_stubs.js
var u32view = new Uint32Array(marker);
u32view[0] = 0x12345678;
var u8view = new Uint8Array(marker);
if (u8view[0] == 0x78 && type == joo_global_object.gl.UNSIGNED_BYTE) {
Copy link
Member

Choose a reason for hiding this comment

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

Wow, thanks for implementing the logic to determine endianness! Surprising that the JS engine doesn't handle this for us. Thanks for figuring this out 👍

bryphe
bryphe reviewed Jan 5, 2019
View reviewed changes
src/gl_wrapper.cpp
int numChannels = format == GL_RGBA ? 4 : 3;
for (int i = 0; i < width * height * numChannels; i += numChannels) {
uint8_t tmp = *((uint8_t *) data + i);
*((uint8_t *) data + i) = *((uint8_t *) data + i + 2);
Copy link
Member

Choose a reason for hiding this comment

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

Fun 😄

bryphe
bryphe approved these changes Jan 5, 2019
View reviewed changes
Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Looks great, @OhadRau ! Thank you for the in-depth investigation on both the WebGL and native fronts, finding the issue, and implementing the swap.

@bryphe
Copy link
Member

bryphe commented Jan 5, 2019

Not sure why, but this swapped the y-axis so now I just set the image descriptor byte to 0 (which should work according to the TGA spec).

How'd you figure this out? Did you have to dive into the TGA spec? Nice find!

@OhadRau
Copy link
Contributor Author

OhadRau commented Jan 5, 2019

Lol it's actually a weird edge case I bumped into when implementing the original PR. TypedArrays respect system endianess whereas DataViews don't, which was kind of confusing to figure out. I tried to keep the C and JS as close as possible here to make sure both platforms are correct though.

@OhadRau
Copy link
Contributor Author

OhadRau commented Jan 5, 2019 •
edited
Loading

Yeah, I used this http://www.paulbourke.net/dataformats/tga/. Only the second type (unmapped RGB) matters for us luckily.

Anyways I just tested this in Revery and it worked as expected. I'm wondering if there's any way to compile this in big endian on ARM so that I can test that, but it seems like such an edge case for a GUI lib that it might not be worth bothering with.

Actually I think the width and height for Image.save will be backwards right now for big endian, so I'll try to address that before this gets merged

@bryphe
Copy link
Member

bryphe commented Jan 5, 2019

Anyways I just tested this in Revery and it worked as expected. I'm wondering if there's any way to compile this in big endian on ARM so that I can test that, but it seems like such an edge case for a GUI lib that it might not be worth bothering with.

Great! I'm on-board with this change as-is. We haven't tested ARM support at all across the board, so I suspect this wouldn't be the only issue.

@bryphe
Copy link
Member

bryphe commented Jan 5, 2019

I'll publish a new NPM release once this is in so we can pick it up easily in revery 👍

@OhadRau
Copy link
Contributor Author

OhadRau commented Jan 5, 2019 •
edited
Loading

Oh cool. In that case I guess we can merge this in now

@bryphe
Copy link
Member

bryphe commented Jan 5, 2019 •
edited
Loading

@OhadRau - sounds good - merging now! I'll publish 3.2.1010 shortly.

Thanks for fixing this! Learned a lot from your work here 👍

@bryphe bryphe merged commit 371f18c into revery-ui:master Jan 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryphe bryphe bryphe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OhadRau @bryphe