Enable image loading from xopz format #3782
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The new image loading code may have affected backwards compatibility, so I will need to double check that images are still saved in PNG format. |
|
Rebased and added a test that checks image saving backwards compatibility. |
bhennion
reviewed
Mar 13, 2022
Technius
force-pushed
the
image-prep
branch
2 times, most recently
from
8056178
to
02ca862
Compare
|
Updated the commits:
I think the MacOS failures are due to the gdk pixbuf loaders not being available during the unit tests. I don't have a MacOS device, so I need some help from someone who does. |
That makes sense to me. For me the gdk pixbuf loaders only become available through the .app bundle created via the |
A first observation: If I use the following Compare with the output of |
|
Also compare with what the |
|
I remember we also did something similar with the AppImage. One solution is to write a helper script to set up the MacOS test environment (e.g., create the pixbuf loader file, set environment vars, etc.), and then source the script before running the tests. |
|
Like this? diff --git a/mac-setup/setup-gdk-pixbuf.sh b/mac-setup/setup-gdk-pixbuf.sh
new file mode 100755
index 00000000..a0f1752a
--- /dev/null
+++ b/mac-setup/setup-gdk-pixbuf.sh
@@ -0,0 +1,10 @@
+#!/bin/bash
+
+if [ $# -eq 0 ]; then
+ echo 'Please provide the path of the custom gdk pixbuf file to be created'
+ exit 1
+fi
+
+# Remove all lines starting with # and replace the relative paths with absolute paths
+gdk-pixbuf-query-loaders | sed '/^#/d' | sed 's:lib/:/Users/xournal-dev/gtk/inst/lib/:g' > "$1"
+export GDK_PIXBUF_MODULE_FILE="$1"
\ No newline at end of file
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 0b9fd91a..a52879cb 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -16,6 +16,13 @@ if(MACOSX)
# could also be used instead of the following lines.
set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib")
set(CMAKE_BUILD_WITH_INSTALL_RPATH ON)
+
+ add_custom_target (setup-gdk-pixbuf
+ COMMAND source "${PROJECT_SOURCE_DIR}/src/mac-setup/setup-gdk-pixbuf.sh" "loaders.cache"
+ COMMENT "Make gdk-pixbuf find the loaders on MacOS"
+ )
+
endif()
# For Windows: Prevent overriding the parent project's compiler/linker settings
|
|
Thanks! I will try the script. |
|
@Technius Do you also plan to load background images from the xopz format? (I mean the images you can have through the menu journal/paper background/image) Edit: This is now done in #3503 |
|
@bhennion Thanks. I will take a look. |
Technius
force-pushed
the
image-prep
branch
3 times, most recently
from
608b64d
to
6dda29a
Compare
bhennion
approved these changes
Apr 5, 2022
| if (!g_file_load_contents(file, nullptr, &contents, &length, nullptr, &err)) { | ||
| g_error_free(err); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we use std::ifstream instead of GFile here? Maybe in a follow up PR?
There was a problem hiding this comment.
Yeah, let's do that in a follow up.
Technius
force-pushed
the
image-prep
branch
2 times, most recently
from
4261c76
to
9aef532
Compare
|
Thanks for the look over. Diff from previous version is below. DiffI wish GitHub had diffs between force pushes. That would be so useful... diff --git a/src/core/control/xojfile/LoadHandler.cpp b/src/core/control/xojfile/LoadHandler.cpp
index c454984bc..650714fe1 100644
--- a/src/core/control/xojfile/LoadHandler.cpp
+++ b/src/core/control/xojfile/LoadHandler.cpp
@@ -372,7 +372,7 @@ void LoadHandler::parseBgPixmap() {
if (!readResult) {
return;
}
- std::string imgData = *readResult;
+ std::string& imgData = *readResult;
GBytes* attachment = g_bytes_new_take(imgData.data(), imgData.size());
GInputStream* inputStream = g_memory_input_stream_new_from_bytes(attachment);
@@ -442,7 +442,7 @@ void LoadHandler::parseBgPdf() {
if (!readResult) {
return;
}
- std::string pdfBytes = readResult.value();
+ std::string& pdfBytes = readResult.value();
doc.readPdf(pdfFilename, false, attachToDocument, pdfBytes.data(), pdfBytes.size());
if (!doc.getLastErrorMsg().empty()) {
@@ -710,7 +710,7 @@ void LoadHandler::parseAttachment() {
if (!readResult) {
return;
}
- std::string imgData = *readResult;
+ std::string& imgData = *readResult;
switch (this->pos) {
case PARSER_POS_IN_IMAGE: {
diff --git a/src/core/model/Image.cpp b/src/core/model/Image.cpp
index 289988391..acf8ad0f1 100644
--- a/src/core/model/Image.cpp
+++ b/src/core/model/Image.cpp
@@ -5,7 +5,6 @@
#include <cairo.h>
-#include "util/pixbuf-utils.h"
#include "util/serializing/ObjectInputStream.h"
#include "util/serializing/ObjectOutputStream.h"
@@ -92,7 +91,7 @@ void Image::setImage(std::string&& data) {
void Image::setImage(GdkPixbuf* img) {
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
- setImage(f_pixbuf_to_cairo_surface(img));
+ setImage(gdk_cairo_surface_create_from_pixbuf(img, 0, nullptr));
#pragma GCC diagnostic pop
}
@@ -130,8 +129,19 @@ auto Image::getImage() const -> cairo_surface_t* {
g_assert(pixbuf != nullptr);
this->imageSize = {gdk_pixbuf_get_width(pixbuf), gdk_pixbuf_get_height(pixbuf)};
- this->image = f_pixbuf_to_cairo_surface(gdk_pixbuf_loader_get_pixbuf(loader));
+ // TODO: pass in window once this code is refactored into ImageView
+ this->image = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, gdk_pixbuf_get_height(pixbuf),
+ gdk_pixbuf_get_width(pixbuf));
g_assert(this->image != nullptr);
+
+ // Paint the pixbuf on to the surface
+ // NOTE: we do this manually instead of using gdk_cairo_surface_create_from_pixbuf
+ // since this does not work in CLI mode.
+ cairo_t* cr = cairo_create(this->image);
+ gdk_cairo_set_source_pixbuf(cr, pixbuf, 0, 0);
+ cairo_paint(cr);
+ cairo_destroy(cr);
+
g_object_unref(loader);
}
diff --git a/src/core/model/Image.h b/src/core/model/Image.h
index 56409c3f5..807e38c0b 100644
--- a/src/core/model/Image.h
+++ b/src/core/model/Image.h
@@ -36,14 +36,11 @@ public:
/// Set the image data by moving the data.
void setImage(std::string&& data);
- /// Set the image data by rendering the surface to PNG and copying the PNG data.
- ///
- /// \deprecated Pass the raw image data instead.
- [[deprecated]] void setImage(cairo_surface_t* image);
-
/// Set the image data by copying the data from the provided pixbuf.
///
/// \deprecated Pass the raw image data instead.
+ ///
+ /// FIXME: remove this method. Currently, it is used by Control::clipboardPasteImage.
[[deprecated]] void setImage(GdkPixbuf* img);
/// Returns the internal surface that contains the rendered image data.
@@ -82,6 +79,13 @@ private:
static cairo_status_t cairoReadFunction(const Image* image, unsigned char* data, unsigned int length);
private:
+ /// Set the image data by rendering the surface to PNG and copying the PNG data.
+ ///
+ /// \deprecated Pass the raw image data instead.
+ ///
+ /// FIXME: remove this when setImage(GdkPixbuf*) is removed.
+ [[deprecated]] void setImage(cairo_surface_t* image);
+
/// Temporary surface used as a render buffer.
mutable cairo_surface_t* image = nullptr;
|
|
@Technius I haven't found time to study the code in detail. It seems to work fine. I would suggest though to make it more explicit/clear in the PR description and/or the commit message of a706f87, what the PR does (what new documents can be loaded and how you created the test documents) and what it doesn't (e.g. saving in the new format). |
In summary, this commit allows images of formats other than PNG (e.g. JPEG) to be used for Image elements. Specifically, before this commit, the Image element stored the image data in PNG format. Callers of the Image element API had to decode the source image and convert to PNG before creating an Image or modifying Image data. Now, the Image element stores (1) the raw image data and (2) the file format of the image. This has the following benefits: * The image can be decoded on demand (e.g., when it needs to be rendered). * Image data (de)serialization (e.g., to clipboard) is just a copy instead of converting to/from PNG. * In the future, images can be stored in their original formats instead of PNG, which avoids xopp file size blowup. * Vector graphics are preserved instead of rasterized, which improves image quality and decreases xopp file size. While images of arbitrary format can now be _loaded_ from an xopp file, in order to preserve backwards compatibility, images are still converted to PNG format when saving.
The default GDK pixbuf loader file uses relative paths, which point to the wrong location when running tests in the CI build environment. This commit ensures that a pixbuf cache with absolute paths is created and used when running tests on MacOS. Co-Authored-By: Roland Lötscher <40485433+rolandlo@users.noreply.github.com>
Thanks for the catch. I attached a detailed commit message. |
Febbe
reviewed
Jun 14, 2022
Comment on lines
+92
to
+95
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
| setImage(gdk_cairo_surface_create_from_pixbuf(img, 0, nullptr)); | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
Since when gdk_cairo_surface_create_from_pixbuf is deprecated? Also, MSVC does not detect that pragma.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enables embedded image loading (of arbitrary image formats) for the packaged file format.
Related: #937.