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

Add glReadPixels and image saving capabilties #75

Merged
merged 7 commits into from
Jan 3, 2019

Conversation

OhadRau
Copy link
Contributor

@OhadRau OhadRau commented Dec 28, 2018

Currently, this PR adds the following functions:

  • Glfw.glReadPixels: (int, int, int, int, texturePixelDataFormat, glType, 'buffer) => unit - Read pixels from the current framebuffer into the buffer given
  • Image.create: (~width: int, ~height: int, ~numChannels: int, ~channelSize: int) => Image.t - Create an empty image object (provides a pixel buffer and allows for saving)
  • Image.destroy : (Image.t) => unit - Destroy an image object, freeing the memory from its buffer
  • Image.getBuffer: (Image.t) => Image.pixelBuffer - Get a pointer to the image's buffer, for use with Glfw.glReadPixels
  • Image.save: (Image.t, string) => unit - Save an image object as a TGA file with the given name. In the browser, this will generate a file and trigger a download for it.

Below is an example of how these can be used to capture a window's contents:

let captureWindow(window, filename) {
  /* Get window dimensions */
  let {width, height} = getFramebufferSize(window);
  /* Create an image of the correct size. Note: JS only supports RGBA in most
     browsers, so we need 4 color channels */
  let image = Image.create(~width, ~height, ~numChannels=4, ~channelSize=1);
  /* Get a reference to the image's buffer so we can write data to it */
  let buffer = Image.getBuffer(image);
  /* Copy all pixels from (0, 0) to (width, height) from the framebuffer into the
     image's buffer. Copy all 4 channels (RGBA), where each channel is 1 byte. */
  glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buffer);
  /* Save the image to the file */
  Image.save(image, filename);
  /* Destroy it, since buffers aren't garbage collected */
  Image.destroy(image);
}

(All the stuff below this line is outdated)


This PR adds Reglfw.Glfw.captureWindow: (Window.t, string) => unit which takes the contents of the current window and saves it to a TGA file. Currently, the stub for this method is located in glfw_wrapper.cpp but that may not be the best logical place for it. It also isn't implemented in JS. This could be added using the GL.readPixels() call in the browser (note: this gives us far less control over the desired format that the real glReadPixels()) and outputting the TGA file to a data URI, I just haven't put in the work for that yet.

The primary motivation for this PR is for testing apps written using reason-glfw, since it allows us to capture the window at a certain frame (see the example, where it captures it every 60 frames). This can be used for UI regression tests as discussed in revery-ui/revery#156. For example, we provide a reference image for frame X and then take a screenshot at frame X when the program runs using this API. The two images are then diffed in a shell script (we can use magick compare for this), and if they don't match they're uploaded to some image host where we can compare the two.

Note for some further discussion on this API: I was originally going to implement raw calls to glReadPixels but eventually decided against this for the following reasons:

  • We would need an image buffer to work with, so we'd have to expose some malloc functionality to OCaml. The approach I thought would work best would be to create an image object, but I thought that might be confusing because we wouldn't be able to utilize any of stb_image for that.
  • glReadPixels is pretty different in JS, since it doesn't take any parameters and doesn't write into a buffer of any kind. It would also be weird to break this process up into multiple functions because they would have differing levels of JS compatibility along the way.
  • It would provide a lot more room for user error, since the user would have to pick an image format/type for glReadPixels and then find a matching format to use for the TGA format.
  • One other minor thing is that the API for glReadPixels seems to vary a lot between GL versions (not sure if this is normal). For example, in OpenGL 2 it supported a GL_ALPHA format which seems to be removed in OpenGL 3/4.

@OhadRau OhadRau mentioned this pull request Dec 28, 2018
@bryphe
Copy link
Member

bryphe commented Dec 29, 2018

Thanks for your work on this @OhadRau !

The two images are then diffed in a shell script (we can use magick compare for this), and if they don't match they're uploaded to some image host where we can compare the two.

Great idea! We don't have to write our own image diffing algorithm 🎉

We would need an image buffer to work with, so we'd have to expose some malloc functionality to OCaml. The approach I thought would work best would be to create an image object, but I thought that might be confusing because we wouldn't be able to utilize any of stb_image for that.

@cryza was doing some work / had some ideas here (exposing buffers from the OCaml side). One possibility I could see would be adding an Image.create call that takes width/height/pixel data format and allocates the appropriate buffer for the environment (malloc in native, maybe UInt8Array in JS, etc).

glReadPixels is pretty different in JS, since it doesn't take any parameters and doesn't write into a buffer of any kind.

I didn't understand this part - I was looking at this readPixels link, which shows:

// WebGL1: 
void gl.readPixels(x, y, width, height, format, type, pixels); 
  • it seems similiar to the native variant, just a different type of buffer to write in. Is this what you were referring to? Want to make sure I'm not misunderstanding.

It would provide a lot more room for user error, since the user would have to pick an image format/type for glReadPixels and then find a matching format to use for the TGA format.

If we went the Image.create route, I wonder if we could encapsulate this conversion to TGA (or perhaps PNG/GIF/etc) in the image code we have, separate from GLFW/GL. For example - have an Image.save("/path/to/img", ImageFormat.TGA)

I'm open to creating a separate function if it makes sense! But want to make sure I understand fully why it's not feasible to use glReadPixels across the platforms.

One other challenge that will affect us next is that, currently, I'm not actually able to run OpenGL apps on any of our Azure CI machines (#76). I believe that, because they are VMs, they may not have the necessary OpenGL drivers - so they fail in getting a window context. Need to experiment with installing Mesa to see where it gets. Having the JS functionality would be helpful in the interim , because we could at least potentially run the browser version to get the image compare results. (Or, we could just use travis to generate the screenshot / run the compare)

@OhadRau
Copy link
Contributor Author

OhadRau commented Dec 29, 2018

Thanks for the feedback. I'll try to clarify a few of the things you brought up.

One possibility I could see would be adding an Image.create call that takes width/height/pixel data format and allocates the appropriate buffer for the environment (malloc in native, maybe UInt8Array in JS, etc).

Yeah, so this is basically what I was doing at first. The problem I saw was that the current ReglfwImageInfo struct wouldn't perfectly model all the OpenGL supported formats/types (for example, if we used UNSIGNED_SHORT_5_6_5 or one of the various _REV settings). But now that I look at the implementation in JS, it looks like the format/type is a much more limited subset than what's supported in any native OpenGL version, so maybe our best bet is to just expose all the types that are compatible with everything?

I didn't understand this part - I was looking at this readPixels link, which shows:

Huh.. I have no idea what I was looking at then. This should work fine I guess. I'll bring back in some of the original code and polish this up later today.

@OhadRau
Copy link
Contributor Author

OhadRau commented Dec 29, 2018

Based on this, looks like the common subset of these two APIs would be:

type glPixelFormat = GL_RGB | GL_RGBA;
type glPixelType =
  | GL_UNSIGNED_BYTE
  | GL_UNSIGNED_SHORT_5_6_5
  | GL_UNSIGNED_SHORT_4_4_4_4
  | GL_UNSIGNED_SHORT_5_5_5_1
  | GL_FLOAT

@bryphe Should I try to support all the available modes and throw an error if it doesn't exist, or would you suggest just using this subset?

https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glReadPixels.xhtml
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/readPixels

@OhadRau
Copy link
Contributor Author

OhadRau commented Dec 30, 2018

Actually, bumped into another issue. The WebGL version of glReadPixels expects the buffer to be a certain data type. This presents an issue, since we create the buffer in caml_createImage but don't know whether the user wants a GL_FLOAT buffer or a GL_UNSIGNED_BYTE if they request 4 channels of 1 byte each. We also can't instantiate the buffer inside of caml_glReadPixels since we need a reference to the image buffer. For now, I put a big comment explaining this issue in the JS version of caml_createImage and am choosing the default of GL_UNSIGNED_BYTE here. There's a few hacks we could probably employ to get around this restriction, but that would require assuming that the user is only ever writing to a buffer from Image.create.

@OhadRau
Copy link
Contributor Author

OhadRau commented Dec 30, 2018

Update on where we are -- I've reimplemented basically everything from the previous commit but using glReadPixels and the Image module as the primitives (captureWindow can now be implemented in Reason!). I haven't finished the JS bindings yet because I'm not super familiar with how to use data URIs & generate file download links. If someone feels comfortable with that kind of stuff I'd trust them to take it over for me, but otherwise I can just do a little bit of research and try to figure it out tomorrow.

@OhadRau
Copy link
Contributor Author

OhadRau commented Dec 31, 2018

Ok, I'm making a lot of progress (I think) on the JS end but I'm still getting weird WebGL errors. Specifically I get glReadPixels: format and type incompatible with the current read framebuffer in Chrome and variations of that same error in Firefox and Edge. After inspecting the current scope for the call and putting it manually in the JS console it seems to work without a hitch so I honestly have no idea what the cause is. I'm gonna push what I have now but I'm kind of stuck here so I would appreciate help from someone more versed in JS/WebGL.

AHHHH I just found the issue as soon as I posted this -- none of the major browsers support GL_RGB for glReadPixels. Super annoying, but at least we can run the same code on both the browser and native.

I got file downloads working as well, but at least in Firefox the endianness that the DataView writes is wrong for Uint16s. Shouldn't be a hard fix, I just want to make sure that this is the same behavior in all browsers and that I didn't do something stupid to make that happen.

@OhadRau OhadRau changed the title Add captureWindow Add glReadPixels and image saving capabilties Dec 31, 2018
@OhadRau
Copy link
Contributor Author

OhadRau commented Jan 2, 2019

@bryphe Not sure if you've had a chance to look over this since your last comment. I've addressed most of the stuff you brought up and redesigned most of this code to be more general. Looks like the failing test is due to some issue in GLM compiling on Windows, I don't think that had to do with my code (but I could be wrong). Anyways, let me know if there's anything here you'd like me to change or add. I edited the main PR description to reflect the new API if you want a quick intro to what this will look like.

@bryphe
Copy link
Member

bryphe commented Jan 3, 2019

@OhadRau - sorry for the slow response on my end! Really impressive work - that's awesome that you found a way to implement use glReadPixels on all platforms (and implemented it across JS + Native!)

It's amazing that we have a cross-platform way to create images, call glReadPixels against it, and save it to a file. Thanks for your tenacity getting there, and sorry I wasn't more help for the challenges on the way - there certainly was a lot of stuff to figure out (picking a common subset of the pixel type... figuring out how to handle > 5 parameters via bytecode/native hooks... implementing the image 'write' gesture in a cross-platform way... figure out the right data representations for images).

Very happy with the way the API turned out, and I like the direction the Image API is moving 💯

@bryphe
Copy link
Member

bryphe commented Jan 3, 2019

Just going to test it on Windows, but the code looks great to me.

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.

Works great on Windows - tested both the native strategy and chrome! 😍 It's really neat that the Image.save "just works" as you'd expect in all these places - testament to your work. I'm just thinking now of the cool apps I could build with this...

I'll merge this to get around the CI issue, and see if that reason-gl-matrix issue comes back up. Sorry about the issues with the build there. But it definitely doesn't look related to your code. Thanks again for all your work on this, @OhadRau !

@bryphe bryphe merged commit b1d99b9 into revery-ui:master Jan 3, 2019
@OhadRau
Copy link
Contributor Author

OhadRau commented Jan 3, 2019

Awesome! Thanks for reviewing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants