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

API: Window.takeScreenshot #185

Closed
bryphe opened this issue Jan 4, 2019 · 8 comments
Closed

API: Window.takeScreenshot #185

bryphe opened this issue Jan 4, 2019 · 8 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@bryphe
Copy link
Member

bryphe commented Jan 4, 2019

Building off the excellent work that @OhadRau did in bryphe/reason-glfw#72 - exposing the glReadPixels API + screenshots in all the platforms, I think it makes sense to have this be a first-class API on our Window object.

Proposal

We add a Window.takeScreenshot(w: Window.t, fileName: string) API.

This would:

  • At the end of the next render frame, call glReadPixels into a new Image, and call Image.save. These would be synchronous operations

This code that @OhadRau added in the reason-glfw example basically does all of this already - just about wrapping it up in an API:

  let captureWindow(w, filename) {
    let size = glfwGetFramebufferSize(w);
    let image = Image.create(~width=size.width, ~height=size.height,
                             ~numChannels=4, ~channelSize=1);
    let buffer = Image.getBuffer(image);
    glReadPixels(0, 0, size.width, size.height, GL_RGBA, GL_UNSIGNED_BYTE, buffer);
    Image.save(image, filename);
    Image.destroy(image);
  };

Use Cases

  • For Oni 2 - I'd love to have this to make it easy to snap a screenshot of the editor! IE, bound a key press to 'take screenshot', and have a configurable place for screenshots to be stored
  • For Revery - this would be a simple API for Automated Testing: Image Snapshot Tests #156 to use for automated testing

NOTE: The proposed implementation would save the image synchronously, so it wouldn't be something used in performance-critical code. But for the use cases outlined above - that's not an issue.

@bryphe bryphe added enhancement New feature or request help wanted Extra attention is needed labels Jan 4, 2019
@OhadRau
Copy link
Collaborator

OhadRau commented Jan 4, 2019

Guess I'll take this over since I'm already pretty familiar with the API. Want anything specific for the example code?

@bryphe
Copy link
Member Author

bryphe commented Jan 4, 2019

Awesome, thanks @OhadRau !

Regarding example - good question. Hmm, perhaps we could add a Take Screenshot button somewhere. Or maybe just a minimal Screenshot.re example with a Take Screenshot button?

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 4, 2019

So uh.... looks like I missed something with my TGA encoder.

image

Pretty sure I know why this happens but I'll have to go fix that before we can add this to Revery.

@bryphe
Copy link
Member Author

bryphe commented Jan 4, 2019

Nice catch! That's interesting that it's inverted - and looks like the red/blue color channels are inverted for the background. But strange that the logo doesn't seem impacted by the color switch...

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 5, 2019

Yep. Just the type of bug that's a pain to catch while testing with a cube lol. The upside down part is related to how glReadPixels returns the framebuffer upside down. Thought it would be right because I basically copied the TGA encoding from an example I saw online, but it seems like their code had this same issue. Not entirely sure how the color relates to this all yet

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 5, 2019

@bryphe So I finally actually sat down and started working on this. The issue has to do with little-endianness causing the color bits to be reversed. Kind of an architectural question for reason-glfw, but would you say to swap these bits inside of glReadPixels or inside of Image.save? Or should I write a separate function for that maybe?

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 7, 2019

Everything is working well for me now on native with this, but note that reason-fontkit is still depending on an old reason-glfw so this is gonna be blocked on that. However I'm actually having some trouble getting this to give the right result in browser--seems to always give an all white image when I'm working in revery, even though it looked okay on the reason-glfw example when I tested it in the browser. Really confused about what's causing that to happen, so I'll have to debug this a little bit. Not sure about any ETA since I start classes again tomorrow, so no need to push ahead a reason-fontkit release just to get the version bump in. @bryphe

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 7, 2019

Ran through this in the Chrome debugger and I think the issue that's causing this is https://stackoverflow.com/questions/7156971/webgl-readpixels-is-always-returning-0-0-0-0. I want to avoid turning on preserveDrawingBuffer, but unfortunately the method suggested in the second answer doesn't work here. This may also be another issue (like out of bounds dimensions passed to glReadPixels, but I specifically altered the function to use w.width/w.height instead of the framebuffer width/framebuffer height).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants