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 support to decode JPEG, GIF and BMP formats, and more pixel formats #74

Merged
merged 6 commits into from
May 6, 2024

Conversation

martincapello
Copy link
Contributor

@martincapello martincapello commented Apr 26, 2024

Add support to decode data in JPEG, GIF, or BMP formats. When decoding animated GIFs only the first frame is taken into account.
Also this PR adds support to decoded images in more pixel formats: 24bpp, 32bpp without alpha, and 8bpp indexed with or without alpha.

This changes will be needed for a future commit (which I'm currently working on) in laf to make the drag&drop of images from Chrome on Windows behave in a similar fashion as Chrome on macOS. Because figured out that the dragged data from Chrome contains a CFSTR_FILEDESCRIPTOR and a CFSTR_FILECONTENTS, which means that we can get the content of the file where the image is temporarily stored.

@dacap
Copy link
Owner

dacap commented Apr 29, 2024

@martincapello although I like this PR, it feels like something is missing. The idea of the clip library is to include code that is used inside the clip library (it's not a general purpose library like laf, the clip library has just one goal that is copying/pasting data from/to the clipboard).

I think we can add these functions if we use them here, so I'd suggest:

  1. Detect an use case where a JPEG, BMP, and GIF is reported as clipboard content (find other program that copy this kind of image in the clipboard, probably just copying an image of this type in Chrome or Edge?)
  2. Add the code to detect this clipboard format and call the respective function (just like the PNG format)

@martincapello
Copy link
Contributor Author

@martincapello although I like this PR, it feels like something is missing. The idea of the clip library is to include code that is used inside the clip library (it's not a general purpose library like laf, the clip library has just one goal that is copying/pasting data from/to the clipboard).

I think we can add these functions if we use them here, so I'd suggest:

  1. Detect an use case where a JPEG, BMP, and GIF is reported as clipboard content (find other program that copy this kind of image in the clipboard, probably just copying an image of this type in Chrome or Edge?)

I think this is not gonna be possible. Tried in Chrome and Edge and when an image is copied to the clipboard (whatever format had the source file) it creates the "PNG" data on the clipboard. But this is not the case when a drag & drop operation takes place. In this case the IDataObject that contains the dragged data from Chrome in Windows has the following formats:

DragContext                 (this is for internal use by windows)
DragImageBits               (image used to show the dragged stuff, this could be a scaled down version of the original, so we can't actually use it)
chromium/x-renderer-taint   (not sure what is this for, couldn't find any documentation about this)
FileGroupDescriptorW        (contains a filename of the contents in FileContents might not match the name at the image's URL, this is not a path, just a filename including its extension, for instance "download.jpg")
FileContents                (contains the bytes of the file mentioned in the FileGroupDescriptorW)
text/x-moz-url              (url)
UniformResourceLocatorW     (url)
UniformResourceLocator      (url)
HTML Format                 (HTML text)
text/html                   (HTML text)

Reference about clipboard formats on Windows: https://www.codeproject.com/Reference/1091137/Windows-Clipboard-Formats

Hence, there is no "JPG", "GIF", nor "BMP" data reported on the IDataObject either. The format is inferred from the filename in FileGroupDescriptorW (by using the file extension) and then try to decode the FileContents with the appropriate decoder.
Given these facts, there is a big chance that the added functions won't be used for copy & paste operations at all.

  1. Add the code to detect this clipboard format and call the respective function (just like the PNG format)

As I just said, I didn't find a case where the additional formats are used because they are not clipboard formats actually nor used by Chrome or Edge. I just need to use the decoders because I need to decode the file contents. So, it was (is) actually pretty convenient to have the code in clip because it has a decoder already and then I was able to avoid duplicating several code.

So, let me know what do you prefer to do with these changes. Should I move the code to laf and then copy the parts I need?

@dacap
Copy link
Owner

dacap commented Apr 29, 2024

I think we have several options:

  1. From the linked page (https://www.codeproject.com/Reference/1091137/Windows-Clipboard-Formats) I see that we can use the formats from the "Common Image Formats" ("PNG", "image/png", "JPG", "JPEG", "image/jpeg", "GIF", "image/gif", "BMP", "image/bmp"), and call each function depending on the clipboard format.

  2. We could just publish the decode() function (with a better public name like decode_with_wic()) and receive the CLSIDs of the WIC impl we want to use (clip would use the PNG ones, and laf could use the function with other decoder CLDIDs).

  3. There is a tricky thing when a GIF file is dropped, Aseprite could just read the GIF (or JPG, BMP, PNG, etc.) with its own decoder and get the full animation (instead of the first animation frame). So in some way laf could decode the drop image in the WIC decoders (as the default impl) and offer a way to leave the decode operation of the dropped file content to the client (e.g. Aseprite decoders).

@martincapello
Copy link
Contributor Author

I think we have several options:

  1. From the linked page (https://www.codeproject.com/Reference/1091137/Windows-Clipboard-Formats) I see that we can use the formats from the "Common Image Formats" ("PNG", "image/png", "JPG", "JPEG", "image/jpeg", "GIF", "image/gif", "BMP", "image/bmp"), and call each function depending on the clipboard format.

Oh right! Forgot about those, despite I couldn't find an actual use case it seems that they are actually a thing that might be used, then I believe we should add support (I will start adding this).

  1. We could just publish the decode() function (with a better public name like decode_with_wic()) and receive the CLSIDs of the WIC impl we want to use (clip would use the PNG ones, and laf could use the function with other decoder CLDIDs).

I like the idea of making the decode_with_wic public, but I would also like to keep the current read methods. Let me know if this makes sense to you, otherwise I think I prefer to keep it private and have only the read methods for now.

  1. There is a tricky thing when a GIF file is dropped, Aseprite could just read the GIF (or JPG, BMP, PNG, etc.) with its own decoder and get the full animation (instead of the first animation frame). So in some way laf could decode the drop image in the WIC decoders (as the default impl) and offer a way to leave the decode operation of the dropped file content to the client (e.g. Aseprite decoders).

This is a good idea, been able to customize which decoder laf should use for each case makes a lot of sense. I will see how to do it.

@martincapello
Copy link
Contributor Author

martincapello commented May 3, 2024

There is something we have to figure it out about decoding GIFs...currently all the read methods proposed here return just an image. If we want that the decoding of a GIF returns the full set of frames, I guess that the read method should return a clip::image sequence along with timing data, right? This would imply that from Laf, the DragDataProvider interface must have some method to get the animation, because currently it supports getPaths, getImage and getUrl.
Then, I'm wondering if it wouldn't it be better for GIFs to just return one single frame (or drop GIF support?) for now, to avoid adding more complexity to the current laf PR.

"JPG", "JPEG", "image/jpeg", "image/png", "BMP", "image/bmp",
"GIF", and "image/gif"
@dacap
Copy link
Owner

dacap commented May 3, 2024

I'm wondering if it wouldn't it be better for GIFs to just return one single frame (or drop GIF support?) for now, to avoid adding more complexity to the current laf PR.

We could leave the GIF support in clip library to load just one frame (or drop it completely) there is no difference here for me.

What I was thinking: 1) minimal clip support for these clipboard support (read just one image), 2) same minimal support in laf library to load these types in case of drag-and-drop, 3) full support to load GIF files in Aseprite (using the path or the file content and the Aseprite GIF decoder).

@martincapello
Copy link
Contributor Author

We could leave the GIF support in clip library to load just one frame (or drop it completely) there is no difference here for me.

What I was thinking: 1) minimal clip support for these clipboard support (read just one image),

Perfect, this is what this PR is all about right now.

  1. same minimal support in laf library to load these types in case of drag-and-drop,

Good, this is what I'm currently doing in laf.

  1. full support to load GIF files in Aseprite (using the path or the file content and the Aseprite GIF decoder).

About this...the only way to achieve it with the current DragDataProvider interface is by using getUrl() and download the gif from there. If we would want to use the file contents, then we would need to add a new method to DragDataProvider to get the file contents in some way. But I believe we can discuss this later.

…ette when converting 8bpp images

This avoids crashing the program when converting a decoded malformed BMP (i.e. references palette indexes that don't exist).
@dacap dacap self-assigned this May 6, 2024
@dacap
Copy link
Owner

dacap commented May 6, 2024

It looks good so far although I've made some changes:

  1. Avoid a std::vector in decode function b047b27
  2. Avoid std::vector and the whole initialization code to call constructors fa93604

This last change is the most important one as it reduces the final binary size by 30KB (creating a global std::vector and doing all the ctors calls + push_back adds a lot of code in the initialization). Here the comparison:

As in this PR:

-rwxr-xr-x 1 David 197121 135K May  6 10:12 examples/copy.exe
-rwxr-xr-x 1 David 197121 133K May  6 10:12 examples/hello_world.exe
-rwxr-xr-x 1 David 197121 137K May  6 10:12 examples/int_format.exe
-rwxr-xr-x 1 David 197121 133K May  6 10:12 examples/paste.exe
-rwxr-xr-x 1 David 197121 129K May  6 10:12 examples/put_image.exe
-rwxr-xr-x 1 David 197121 143K May  6 10:12 examples/show_image.exe

With fa93604:

-rwxr-xr-x 1 David 197121 105K May  6 10:11 examples/copy.exe
-rwxr-xr-x 1 David 197121 104K May  6 10:11 examples/hello_world.exe
-rwxr-xr-x 1 David 197121 108K May  6 10:11 examples/int_format.exe
-rwxr-xr-x 1 David 197121 104K May  6 10:11 examples/paste.exe
-rwxr-xr-x 1 David 197121  99K May  6 10:11 examples/put_image.exe
-rwxr-xr-x 1 David 197121 114K May  6 10:11 examples/show_image.exe

Please check the dacap/improve-decoders-support branch with your laf changes and if it works I can merge this to main.

@martincapello
Copy link
Contributor Author

I've pushed your commits since it works as well

@dacap dacap merged commit 7fa83b2 into dacap:main May 6, 2024
20 checks passed
@dacap
Copy link
Owner

dacap commented May 6, 2024

Just merged 👍

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