[Feat]: Adding screenshot feature #63
Conversation
|
Unfortunately you're hitting a bug in ocaml-gif. You can see this more clearly if you run the following: Which gives you a stack trace. I'm not sure what the problem is, whether its a logic bug in the LZW compression (most likely) or a lack of sanity check on the inputs to say they're not quite what is expected. I did try running your test with a 256 colour palette, given that is probably a better tested option, and we still get the same error. Looking at the ocaml-gif code, which I've not looked at for six months, so I'm a bit rust, but I suspect we might need to first compact the data so that the all the 6 bit values are just taking 6 bits (i.e., call Lzw.flatten_codes, which you can see in the test_lzw.ml in I also fear that there's another issue. Currently the So, a bit of a mess: the ocaml-gif writing has clearly not been as well tested as I thought. When I updated the library back in August I mostly was focussed on the read side of things, and just did a few tests for write, but clearly not enough. So, what are the next steps here:
Then you have two options:
My heart says you should do the first option, but my head says you should do the second option :) Whilst I'd love to see you fix ocaml-gif, there's a lot to get your head around in the LZW code, and only a week left in the Outreachy contribution phase, and so I think it best if for now you switch to the other library which is probably going to actually work out the box and let you get this PR working this week. This means the infrastructure around screenshots will be in place and we'll have something working, which is better than running out of time and having just a broken PR. |
|
Thank you soooo much for the detailed explanation. That really helps make sense of what’s going on. I actually really appreciate the chance to learn more about the internals here, even if it might be a bit challenging. Even my heart says to go with the first option. So, I think I’d like to stick with ocaml-gif for now and see if I can figure out what’s going wrong in the LZW compression. It’ll be a bit of a challenge, but it sounds like a great learning opportunity, and I’d love to try and contribute a fix if I can under your guidance. I’ll definitely go and open the issue on the ocaml-gif repo and link this PR as you suggested. If I get completely stuck or time starts running too short, then we can pivot to the other library — but for now, I’m excited to see if I can make this work. Thanks again for your support @mdales! It means a lot! I've learnt a lot of things in the past couple of weeks. From being completely new to the language to being able to make contributions to Claudius. It has been an amazing for sure! |
|
Actually I've been also trying to understand ocaml-gif from the last couple of days. I'd be very happy if we can figure out this! |
|
Okay, good luck! Do remember though that you'll need to submit your contributions https://www.outreachy.org/outreachy-june-2025-internship-cohort/communities/ocaml/claudius/contributions/ here by the 15th of April, and have your application with a proposed timeline for your project submitted here also: https://www.outreachy.org/eligibility/ - whilst I like that you want to follow the problem, just don't lose track of that deadline! |
|
As noted, I fixed the original issue causing the out of bounds, which was just a silly mistake on my behalf I think. However, the GIF files saved are still not valid for some reason, so there is more work to do here! One thing to note in your test, you do need to pack the 6 bit data before encoding it (I think :) currently your test is really compressing 8 bit data where the top two bits are just always 0. Again, this is down to my fault for not documenting the GIF library properly ahead of making the original ticket, apologies. I think you want something like this: let save_screenshot (fb : Framebuffer.t) (palette : Palette.t) =
let width = Array.length fb.data.(0) in
let height = Array.length fb.data in
Printf.printf "Framebuffer dimensions: %d x %d\n" width height; (*Debug*)
let size = width * height in
let colors = palette
|> color_table_of_palette
|> pad_palette_to_power_of_two
in
let color_depth =
let len = Array.length colors in
let rec bits_needed n b = if n <= 1 then b else bits_needed (n / 2) (b + 1) in
min 8 (max 2 (bits_needed (len-1) 1))
in
let nulls_replaced = ref 0 in
let pixels =
List.init size (fun idx ->
let x = idx mod width in
let y = idx / width in
let v = fb.data.(y).(x) in
if v < 0 || v > 255 then
failwith (Printf.sprintf "Framebuffer value %d out of byte range at (%d,%d)" v x y);
if v = 0 then incr nulls_replaced;
let max_index = Palette.size palette - 1 in
let safe_v =
if v = 0 then 1
else if v > max_index then max_index
else v in
(Z.of_int safe_v, color_depth)
)
in
Printf.printf "Replaced %d null pixels with index 1\n%!" !nulls_replaced;
let palette_len = Array.length colors in
let max_allowed_color_depth =
let rec bits_needed n b = if n <= 1 then b else bits_needed (n / 2) (b + 1) in
bits_needed (palette_len - 1) 1
in
if color_depth > max_allowed_color_depth then
failwith (Printf.sprintf
"Color depth %d exceeds max allowed for palette size %d (max: %d)"
color_depth palette_len max_allowed_color_depth);
Printf.printf "Color depth: %d\n" color_depth; (*Debug*)
Printf.printf "Pixels length: %d, expected: %d\n" (List.length pixels) (width * height);
Printf.printf "Palette size: %d, color depth: %d\n" (Array.length colors) color_depth;
let flattened = Lzw.flatten_codes color_depth pixels in
Printf.printf "Uncommpressed size: %d, flattened size: %d\n" (List.length pixels) (Bytes.length flattened);
let compressed = Lzw.encode flattened color_depth in
Printf.printf "Uncompressed size: %d, Compressed size: %d\n%!" (List.length pixels) (Bytes.length compressed);
|
|
I am so sorry for this @mdales Even I was myself trying to find out what was going wrong and try to find a fix for it. Really sorry for taking this much time, but understanding the entire compression process and how it is working was really taking me alot of time to fully understand it deeply as it was very new for me. I really feel bad as I was not able to make this on time :( |
Yes, exactly. I was trying all possible ways to find out why the index out of bounds error was arising. It was sort of in that direction to find if things work in that way.
But even though it was not documented, it was really fun trying to understand it. Thankyou very much for taking out time and sovling this error as I probabily would have needed a little bit of more time to do it on my own and as the deadlines are approaching, I really appreciate your help here! I will fix the remaining issues in the pr and clean up all the debug statements and report back to you! Thank you! |
|
@pawaskar-shreya no need to apologise - I should have tested ocaml-gif more before suggesting it as an approach. The GIF's work for 256 colours:
I had to convert that with imagemagick, as Preview on the Mac didn't like the image, but imagemagick seems to like it fine: The issue with the 64 colour palette I think we just make a problem for me to fix on ocaml-gif, and for now let's focus on getting this into shape even if it just works for 256 colours. |
|
@mdales I am getting the error even now. I have pulled the changes from the upstream. So, the changes are applied. But it does not seem to be working as expected. Theres a stack overflow error. Am I doing something wrong here?
|
I mentioned above that there was still an error I'd not fixed when saving images that aren't 256 colour and I'd not got to that yet, so for now just make your test use a palette of 256 colours, and I'll try work out the 64 bit colour thing as time allows me, but you can still make progress just using a 256 colour test.
Yeah - not sure where you picked up the |
|
I created claudiusFX/ocaml-gif#6 for the bit depth saving problem |
|
I fixed the bug, try updating now. I did also find an error in the sample code I gave you: when you flatten the pixel data you need to flatten it to 8 bits per byte, not to the colour depth. See the test code I added here: https://github.com/claudiusFX/ocaml-gif/blob/main/test/test_gif.ml#L56-L73 At some point Image.t needs a better interface that deals with all the compression for you: claudiusFX/ocaml-gif#8 |
|
Hey @mdales, first of all really sorry, I really could not communicate and answer to your reviews and suggestions on time. I was trying to manage the contributions along with my college assessment tests. Those are scheduled 7th april to 10th april. I promised to look into ocaml-gif to make it work for our screenshot feature but most of the error solving was done by you. Again really sorry for that. I have done all the changes as you told me to do, but since you fixed most of the errors, it's actually working perfectly right now. I will push the changes in a while.
Ahhhh, now I see my mistake! That was the reason for the crash.
Can I replicate this on wsl? Actually I am able to open the screenshot as is and it is opening fine for me. |
No need to apologise - your tests are important, good luck to them. I just did the fixes to ocaml-gif because ultimately I'd recommended a library that was broken (by me!) and so wanted to unblock you. If you'd gone with your suggestion of the other graphics library you'd not have had these issues!
Again, no apologies needed.
Excellent. There's still work to do here, but that can wait until after your test tomorrow!
Yes. I do think it's shown me that I should restructure that code to use the tail-recursion trick in OCaml, which would mean it doesn't need so many stack frames.
I suspect the GIF library is making valid GIFs (imagemagick likes them, and clearly WSL tools too), so this seems to be a Mac problem, so this is on my for now. I'll file another issue over on the other repo for that. For now, get your tests out the way, and then it'd be great to land this feature, as I'm excited about getting it in. A question for when you have time: what do you think to scaling the images to match the scaling on screen? |
|
I am just about to push the changes and the feature seems to work for a good number of examples, but I remember you mentioning that it might not work for the day2 example and I tested it and it did not work! Is there any way we can make this wrok? |
That would be even better. It would enhance the visuals of the generated art. Right now we are storing the images considering the scaling to be 1. But I can totally see why you recommended this!!!!
The day4 example scaled to 2 looks really beautiful 🤩🤩 |
The feature is almost on the way! Implemented your suggestions and pushing the changes for now, so I can work on any more suggesstions tomorrow after your review once I am done with the test! |
pawaskar-shreya
force-pushed
the
feat/screenshot
branch
from
9c6e868 to
84633a0
Compare
|
@mdales I have pushed the changes. The feature works well with a lot of examples with few exceptions like the day2 example. For some reason github is not showing the review of the screenshot attached of the image that we just got through our feature here. |
Oh, that's because day2 has a palette size of 1024 entries, and the biggest GIF palette size is 256. I think down the line we'll want to perhaps also offer PNG for that use case, but that is an outlier in all the examples (and I was cheating - the generative art prompt for day 2 was "no palette" and given you MUST use a palette in Claudius I just went with a really big one so it looked like no palette :D) - for now I think we'll get a lot of use with the GIF output, and as you noted, it leads to save animated gifs one day nicely. |
|
Well done! I'll take a look at the updates tomorrow! |
Ahhh, that explains it!
Yess, I remember your suggestion of adding an abstraction there to be able to have both the gif and png options available to us. That will help solve this!
That's a very hacky way to get to the no palette look. I would love to try this myself also!
Yesss, that was the very reason I was so determined to make ocaml-gif work for the screenshot feature, it would just make things even better for us in the future eventually. |
pawaskar-shreya
force-pushed
the
feat/screenshot
branch
from
1188736 to
188bf60
Compare
|
Good afternoon @mdales! I found the suggestion for saving imgs with scaling very fascinating and so I have tried to add that feature also in the current pr. I was looking for some scaling algorithms that we can use in our use case and found the Nearest-Neighbor Scaling to be perfect for our use case! For every pixel in the scaled image, we copy the value of the nearest pixel from the original image. I found that it is it preserving the crisp pixel boundaries which seems to be perfect for pixel art and retro graphics that we are doing! So what we are trying to do is:
As it can be seen here that I am using a scale of 7 on the day4 example and the dimensions are scaled as can be seen in the lower left corner
|
|
Also @mdales, the builds here seem to be failing as there is no ocaml-gif vendor here which I was using in my local. How do we plan to make this libs accessible for us in the future, so that the builds don't fail? There's also bdfparser lib right that we are planning to use for fonts. We would also have to make it accessible, right?
Ohhh also, the tests went really good and I am very happy that now I can focus all my attention here on our contributions and proposal drafting 😊😊 |
What work are we expecting to be done here? I would like to know if I can do it!
@mdales, do you think I should give this a go?
Also this one. Should I add it in my project proposal or we plan to try to implement it now? |
mdales
left a comment
mdales
left a comment
There was a problem hiding this comment.
Overall this is looking great. I'll ponder the vendor problem you mentioned, but for now here's some minor comments to address - but otherwise happy to try get this in!
| let palette = Screen.palette s in | ||
| let scale = Screen.scale s in | ||
| Screenshot.save_screenshot ~scale prev_buffer palette; | ||
| false, { input with keys = KeyCodeSet.add key input.keys } |
There was a problem hiding this comment.
This last line is duplicated, which means if someone has to update it they now. have to do so in two places. Can you restructure this to avoid that?
There was a problem hiding this comment.
yes I will make this change.
| timestamped filename like "screenshot_MMDDYY_HHMMSS.gif" for uniqueness. | ||
| The output image is scaled by the given [scale] factor using nearest-neighbor scaling. *) | ||
|
|
||
| val color_table_of_palette : Palette.t -> Giflib.ColorTable.t |
There was a problem hiding this comment.
I don't strongly object to this, but my sense is that by having this in the mli file expose that we use ocaml-gif internally, and so if we wanted to move to another image library, then we'd still have to support this API, which isn't ideal?
I could understand it if we were exposing the API to help with tests, but currently we're not.
I guess the thing about APIs is that once you make them, you have to support them, and so whilst there's nothing incorrect about having this, I don't feel it's necessary and so not worth the support overhead.
Does that make sense?
There was a problem hiding this comment.
I guess the thing about APIs is that once you make them, you have to support them, and so whilst there's nothing incorrect about having this, I don't feel it's necessary and so not worth the support overhead.
Yeaa, that makes sense. I'll remove it from there!
| open Claudius | ||
| open Screenshot | ||
|
|
||
| let () = |
There was a problem hiding this comment.
Can you make this use ounit2 like the other test files? That way we get things like progress reports in the tests.
| if i < len then colors.(i) else (0, 0, 0) | ||
| ) | ||
|
|
||
| let save_screenshot ~(scale : int) (fb : Framebuffer.t) (palette : Palette.t) = |
There was a problem hiding this comment.
You can simplify this interface if you pass in the screen object: that contains the palette and the scale, plus the dimensions of the screen which means you don't need to look at the internal workings of Framebuffer on the next two lines.
| min 8 (max 2 (bits_needed (len - 1) 1)) | ||
| in | ||
|
|
||
| let nulls_replaced = ref 0 in |
There was a problem hiding this comment.
What is this being used for? I can see it being incremented, but then its never used.
There was a problem hiding this comment.
No it was being used for debugging to check how many 0s were replaced to see if there was any problem in that step. But now it is of no use. I will remove this part
| let src_x = x / scale in | ||
| let src_y = y / scale in | ||
| let v = fb.data.(src_y).(src_x) in | ||
| if v < 0 || v > 255 then |
There was a problem hiding this comment.
The 255 here should be dependant on the size of the palette?
| failwith (Printf.sprintf "Framebuffer value %d out of byte range at (%d,%d)" v src_x src_y); | ||
| if v = 0 then incr nulls_replaced; | ||
| let max_index = Palette.size palette - 1 in | ||
| let safe_v = |
There was a problem hiding this comment.
I don't quite understand what this block is doing. You have the above failwith if there's data out of range, so I'm not sure why this bit is needed? Apologies if I've missed something!
There was a problem hiding this comment.
Actually, the failwith was to ensure that the framebuffer value is within the valid GIF range (0–255), while the other part was to limit to max_index to ensure that it stays within the bounds of the actual palette, which might be smaller than 256 as you seem to point it out in one of the above comment. But then we can also simplify the logic by either relying on limiting alone or keeping the failwith for validation.
| let y = idx / scaled_width in | ||
| let src_x = x / scale in | ||
| let src_y = y / scale in | ||
| let v = fb.data.(src_y).(src_x) in |
There was a problem hiding this comment.
Rather than assume the internals of the Framebuffer, we should either use Framebuffer.read_pixel or you could earlier do Framebuffer.to_array.
Not now - let's get this in with those comments addressed, and that can be a follow on ticket later. What you have done here is already useful! |
|
hello @mdales! I have done all the changes. Could you please tell me if there's any more changes that need to be done? |
mdales
left a comment
mdales
left a comment
There was a problem hiding this comment.
This is looking great @pawaskar-shreya - I've asked for one late change, apologies, but hopefully an easy fix and you can see my reasoning, and I spotted how we can vendor the GIF library to get this to pass CI!
| | `Quit -> (true, input) | ||
| | `Key_down -> | ||
| let key = PlatformKey.of_backend_keycode (Sdl.Event.(get e keyboard_keycode)) in | ||
| if key = Key.F2 then ( |
There was a problem hiding this comment.
Sorry to be a pain, but can you move this to be at the start of the loop, around like 91 perhaps, and just check the input_state?
Two reasons for this:
Firstly. it'll mean we don't clash with @Vanisha1606 who is changing how keyboard events work.
Secondly though, having this here doesn't scale well and obfuscates the event code. This section should just be the mechanics of getting key and mouse events, and then we should somewhere else decide what to do with them. For instance, we have #61 by @Vanshikaxxa also in the wings, which also needs to make a key check to add a debug overlay, and who knows what else we'll later, so I think what you've done here will only get bigger over time as we add more things, so deserves to be in a distinct block of code, not lost in with the event reading.
Does that make sense?
There was a problem hiding this comment.
Yess, that's true. It'll be hard for us to maintain it later. I'll make this change @mdales .
|
@mdales just leaving this update here so you can check whenever you get time When I was testing for the final changes that you suggested and I moved out the screenshot on F2 press section out of Key_down, I noticed something strange happening. There were 2 to 3 simultaneous screenshots being captured. I initially thought I might be calling save_screenshot multiple times, maybe due to not having cleaned up the code while testing. But then later I realized that a long press of F2 was being continuously recorded and hence triggering simultaneous screenshots. So I decided to handle this by introducing a flag to ensure we only capture a single screenshot per key press. But the code started to grow bigger and clutter the base.ml file. So I thought it would be better to move this into the screenshot module and wrap the core screenshot functionality in a separate function. That’s when I split the responsibilities: the lower-level capture_screenshot function does the actual encoding and writing, while the higher-level save_screenshot handles the key checking and flag control. But that's when I started facing circular dependency issue. Screenshot.ml needed to access the pressed keys (which lived in base.ml), but base.ml also depended on screenshot.ml to call the screenshot function. So, I read up a bit on OCaml’s compilation model and module dependency resolution, and realized this wasn’t going to work directly as the compiler needs a clear top-down dependency order. So, to break the cycle, I changed the design so that save_screenshot takes the list of pressed keys (keys : Key.t list) as an argument. That way, base.ml stays in control of input, and screenshot.ml stays focused on handling the save. But then as save_screenshot was sort of acting like a wrapper around capture_screenshot and we were not able to throw the exception for palette size greater than 256 as this check was done in capture_screenshot and capture_screenshot wasn't triggered in save_screenshot until F2 was pressed. This was causing the buils to fail after we had pinned the ocaml-gif. So, now there's a palette size check in save_screenshot and builds are successful!! So kudos to you to suggest to move the action on events section out of the events recording section. If you wouldn't you suggested this change then this bug would have stayed hidden. My theory of why this thing of multiple screenshots was happening: This taught me a very important lesson! This ticket has been the best ticket I have worked on. Although I have only worked on two tickets 😅😅 What a great learning experience this was! Have a great weekend! |
mdales
left a comment
mdales
left a comment
There was a problem hiding this comment.
Well done, and a good catch regarding the keypresses. This logic can be simplified once we have #13 added, which lets you see the down and up events on keys, so you could then just check for the F2 key down event - that ticket, being worked on by @Vanisha1606, was motivated by this exact problem :)
And well done on the restructuring - pulling things out of base.ml is in general a good thing I think in terms of making code easier to test and see what is going on, so moving all the logic out there was a win.
|
Thankyou sooo much @mdales for approving the pr. It was soooo wonderful
working on this and all the valuable input that you gave lead to refining
the logic and working of the screenshot feature.
Tracking control change feature would be a great addition. It would surely
help us catching bugs like this.
The past few weeks have been totally wonderful and productive. It was so
great working with the team under your guidance!
Great learnings!
…On Sat, 12 Apr 2025, 1:37 pm Michael Dales, ***@***.***> wrote:
Merged #63 <#63> into main.
—
Reply to this email directly, view it on GitHub
<#63 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQIF662NKJF3QT5YAKKQAY32ZDCUBAVCNFSM6AAAAAB2RZK5MCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJXGIZTGOBXGE3TMNI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
2 participants
Attempting to fix #39
To be able to use the ocaml-gif for screenshot feature, I have vendored it into Claudius.
I have added screenshot modules for implementing the feature. To ensure uniqueness for every screenshot saved, I am using the name for screenshot in the format "screenshot_ddmmyy_hhmmss"
The current pr has a lot of debug statements and I will clean those up after getting a review from you @mdales, but currently I am getting an index out of bounds error. Can you please help me with why I am getting this error.
Also, on running dune runtest, I am getting these other bunch of error: