Fixed Image.save_jpg() does not write any file #66929
Conversation
modules/jpg/image_loader_jpegd.cpp
Outdated
| ERR_FAIL_COND_V(p_img.is_null() || p_img->is_empty(), ERR_INVALID_PARAMETER); | ||
| Ref<Image> image = p_img; | ||
| if (image->get_format() != Image::FORMAT_RGB8) { | ||
| image->convert(Image::FORMAT_RGB8); |
There was a problem hiding this comment.
I see that _jpgd_buffer_save_func above does this check instead:
if (image->get_format() != Image::FORMAT_RGB8) {
image->convert(Image::FORMAT_ETC2_RGB8);
}
Sounds like it's wrong and yours is better? If so should be fixed above too.
I don't know if the intention from #62122 was to do the implementation using ImageLoaderJPGOSFile as done here, or just getting a buffer from the file and calling the buffer version.
There was a problem hiding this comment.
I added a printf before and inside the if and it seems the image is already RGB8 and the conversion doesn't happen.
If i change the if to
if (image->get_format() != Image::FORMAT_ETC2_RGB8) { image->convert(Image::FORMAT_ETC2_RGB8); }
it gives an error
ERROR: Cannot convert to <-> from compressed formats. Use compress() and decompress() instead.
There was a problem hiding this comment.
Changed the _jpgd_buffer_save_func to use FORMAT_RGB8.
Changed the include path to jpgd.h and jpge.h.
Merged duplicate code into one function.
Can still change the _jpgd_save_func to use the buffer from the _jpgd_buffer_save_func if need be
|
It's not directly related to your fix but it's also a wrong change from #62122, so maybe you can include this in your commit if it ever requires an amend (otherwise I can include it in a future cleanup if I remember): diff --git a/modules/jpg/image_loader_jpegd.cpp b/modules/jpg/image_loader_jpegd.cpp
index ce20ac9060..8446e063a5 100644
--- a/modules/jpg/image_loader_jpegd.cpp
+++ b/modules/jpg/image_loader_jpegd.cpp
@@ -33,8 +33,8 @@
#include "core/os/os.h"
#include "core/string/print_string.h"
-#include "thirdparty/jpeg-compressor/jpgd.h"
-#include "thirdparty/jpeg-compressor/jpge.h"
+#include <jpgd.h>
+#include <jpge.h>
#include <string.h>
Error jpeg_load_image_from_buffer(Image *p_image, const uint8_t *p_buffer, int p_buffer_len) {
|
MladoniSzabi
force-pushed
the
save_jpeg_file
branch
from
8cdcb08 to
2d597c9
Compare
The function that was supposed to implement the saving in image_loader_jpegd was just returning OK without doing anything. Copied the code from _jpgd_buffer_save_func to _jpgd_save_func but changed the ImageLoaderJPGOSBufferto a ImageLoaderJPGOSFile to save to a file instead of memory. Changed the image format from FORMAT_ETC2_RGB8 to FORMAT_RGB8 since the first one was creating a weird greyscale interlaced image.
MladoniSzabi
force-pushed
the
save_jpeg_file
branch
from
2d597c9 to
a5c4df7
Compare
|
Is there something else I need to do on this PR? |
Code looks good from a quick glance, but I'll let someone else review just to make sure. Akien is currently not available for PR reviews. |
|
Thanks! |
The function that was supposed to implement the saving in image_loader_jpegd was returning OK without doing anything. Copied the code from _jpgd_buffer_save_func to _jpgd_save_func but changed the ImageLoaderJPGOSBufferto a ImageLoaderJPGOSFile to save to a file instead of memory. Changed the image format from FORMAT_ETC2_RGB8 to FORMAT_RGB8 since the first one was creating a weird greyscale interlaced image.
I am not sure how to merge the two functions or make the code more dry
Resolves #66650