From aa5f06f45df59d7dce99d60036b6612e6d8840f4 Mon Sep 17 00:00:00 2001 From: Rodolfo Ribeiro Gomes Date: Thu, 8 Sep 2022 10:09:45 -0300 Subject: [PATCH 1/5] refactor(trgt_magickpp): avoid direct allocation prefer std::vector rather raw pointers --- .../modules/mod_magickpp/trgt_magickpp.cpp | 34 +++++-------------- .../src/modules/mod_magickpp/trgt_magickpp.h | 10 +++--- 2 files changed, 13 insertions(+), 31 deletions(-) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp index bdb1027e6e5..84be476ade7 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp @@ -199,9 +199,6 @@ magickpp_trgt::~magickpp_trgt() synfig::error("unknown exception"); } - if (buffer1) delete [] buffer1; - if (buffer2) delete [] buffer2; - if (color_buffer) delete [] color_buffer; //exceptionInfo = MagickCore::DestroyExceptionInfo(exceptionInfo); MagickCore::DestroyExceptionInfo(exceptionInfo); } @@ -221,24 +218,11 @@ magickpp_trgt::init(synfig::ProgressCallback*) start_pointer = nullptr; - buffer1 = new unsigned char[4*width*height]; - if (!buffer1) - return false; + std::size_t buffer_size = static_cast(4) * width * height; + buffer1.resize(buffer_size); + buffer2.resize(buffer_size); - buffer2 = new unsigned char[4*width*height]; - if (!buffer2) - { - delete [] buffer1; - return false; - } - - color_buffer = new Color[width]; - if (!color_buffer) - { - delete [] buffer1; - delete [] buffer2; - return false; - } + color_buffer.resize(width); return true; } @@ -255,10 +239,10 @@ magickpp_trgt::end_frame() bool magickpp_trgt::start_frame(synfig::ProgressCallback */*callback*/) { - if (start_pointer == buffer1) - start_pointer = buffer_pointer = buffer2; + if (start_pointer == buffer1.data()) + start_pointer = buffer_pointer = buffer2.data(); else - start_pointer = buffer_pointer = buffer1; + start_pointer = buffer_pointer = buffer1.data(); previous_buffer_pointer = start_pointer; @@ -269,14 +253,14 @@ magickpp_trgt::start_frame(synfig::ProgressCallback */*callback*/) Color* magickpp_trgt::start_scanline(int /*scanline*/) { - return color_buffer; + return color_buffer.data(); } bool magickpp_trgt::end_scanline() { if (previous_buffer_pointer) - color_to_pixelformat(previous_buffer_pointer, color_buffer, PF_RGB|PF_A, 0, width); + color_to_pixelformat(previous_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); if (!transparent) for (int i = 0; i < width; i++) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h index ef1794c0af2..5a979eeceb1 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h @@ -56,10 +56,11 @@ class magickpp_trgt : public synfig::Target_Scanline int width, height; synfig::String filename; - unsigned char *buffer1, *start_pointer, *buffer_pointer; - unsigned char *buffer2, *previous_buffer_pointer; + std::vector buffer1, buffer2; + unsigned char *start_pointer, *buffer_pointer; + unsigned char *previous_buffer_pointer; bool transparent; - synfig::Color *color_buffer; + std::vector color_buffer; std::vector images; synfig::String sequence_separator; @@ -69,13 +70,10 @@ class magickpp_trgt : public synfig::Target_Scanline width(), height(), filename(filename), - buffer1(nullptr), start_pointer(nullptr), buffer_pointer(nullptr), - buffer2(nullptr), previous_buffer_pointer(nullptr), transparent(), - color_buffer(nullptr), sequence_separator(params.sequence_separator) { } virtual ~magickpp_trgt(); From cb9b89b1b371b39bbb6c7d9d98ec44e796d78f77 Mon Sep 17 00:00:00 2001 From: Rodolfo Ribeiro Gomes Date: Thu, 8 Sep 2022 10:32:48 -0300 Subject: [PATCH 2/5] refactor(trgt_magickpp): rename pointer variables --- .../modules/mod_magickpp/trgt_magickpp.cpp | 37 ++++++++++--------- .../src/modules/mod_magickpp/trgt_magickpp.h | 9 +++-- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp index 84be476ade7..9e95691ec55 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp @@ -216,7 +216,7 @@ magickpp_trgt::init(synfig::ProgressCallback*) width = desc.get_w(); height = desc.get_h(); - start_pointer = nullptr; + buffer_pointer = nullptr; std::size_t buffer_size = static_cast(4) * width * height; buffer1.resize(buffer_size); @@ -230,7 +230,7 @@ magickpp_trgt::init(synfig::ProgressCallback*) void magickpp_trgt::end_frame() { - Magick::Image image(width, height, "RGBA", Magick::CharPixel, start_pointer); + Magick::Image image(width, height, "RGBA", Magick::CharPixel, buffer_pointer); if (transparent && images.begin() != images.end()) (images.end()-1)->gifDisposeMethod(Magick::BackgroundDispose); images.push_back(image); @@ -239,14 +239,15 @@ magickpp_trgt::end_frame() bool magickpp_trgt::start_frame(synfig::ProgressCallback */*callback*/) { - if (start_pointer == buffer1.data()) - start_pointer = buffer_pointer = buffer2.data(); + if (buffer_pointer == buffer1.data()) + buffer_pointer = current_row_buffer_pointer = buffer2.data(); else - start_pointer = buffer_pointer = buffer1.data(); + buffer_pointer = current_row_buffer_pointer = buffer1.data(); - previous_buffer_pointer = start_pointer; + previous_row_buffer_pointer = buffer_pointer; transparent = false; + return true; } @@ -259,23 +260,25 @@ magickpp_trgt::start_scanline(int /*scanline*/) bool magickpp_trgt::end_scanline() { - if (previous_buffer_pointer) - color_to_pixelformat(previous_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); - - if (!transparent) - for (int i = 0; i < width; i++) - if (previous_buffer_pointer && // this isn't the first frame - buffer_pointer[i*4 + 3] < 128 && // our pixel is transparent - !(previous_buffer_pointer[i*4 + 3] < 128)) // the previous frame's pixel wasn't + if (previous_row_buffer_pointer) + color_to_pixelformat(previous_row_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); + + if (!transparent) { + for (int i = 0; i < width; i++) { + if (previous_row_buffer_pointer && // this isn't the first frame + current_row_buffer_pointer[i*4 + 3] < 128 && // our pixel is transparent + !(previous_row_buffer_pointer[i*4 + 3] < 128)) // the previous frame's pixel wasn't { transparent = true; break; } + } + } - buffer_pointer += 4 * width; + current_row_buffer_pointer += 4 * width; - if (previous_buffer_pointer) - previous_buffer_pointer += 4 * width; + if (previous_row_buffer_pointer) + previous_row_buffer_pointer += 4 * width; return true; } diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h index 5a979eeceb1..590c9973566 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h @@ -57,8 +57,9 @@ class magickpp_trgt : public synfig::Target_Scanline synfig::String filename; std::vector buffer1, buffer2; - unsigned char *start_pointer, *buffer_pointer; - unsigned char *previous_buffer_pointer; + unsigned char* buffer_pointer; + unsigned char* current_row_buffer_pointer; + unsigned char* previous_row_buffer_pointer; bool transparent; std::vector color_buffer; std::vector images; @@ -70,9 +71,9 @@ class magickpp_trgt : public synfig::Target_Scanline width(), height(), filename(filename), - start_pointer(nullptr), buffer_pointer(nullptr), - previous_buffer_pointer(nullptr), + current_row_buffer_pointer(nullptr), + previous_row_buffer_pointer(nullptr), transparent(), sequence_separator(params.sequence_separator) { } From 630762f291e60747817d6c5c13cf53c210a1e1e4 Mon Sep 17 00:00:00 2001 From: Rodolfo Ribeiro Gomes Date: Thu, 8 Sep 2022 10:37:01 -0300 Subject: [PATCH 3/5] refactor(trgt_magickpp): no need to start loop if it is first frame --- synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp index 9e95691ec55..b6175fe320d 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp @@ -263,10 +263,9 @@ magickpp_trgt::end_scanline() if (previous_row_buffer_pointer) color_to_pixelformat(previous_row_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); - if (!transparent) { + if (!transparent && previous_row_buffer_pointer) { for (int i = 0; i < width; i++) { - if (previous_row_buffer_pointer && // this isn't the first frame - current_row_buffer_pointer[i*4 + 3] < 128 && // our pixel is transparent + if (current_row_buffer_pointer[i*4 + 3] < 128 && // our pixel is transparent !(previous_row_buffer_pointer[i*4 + 3] < 128)) // the previous frame's pixel wasn't { transparent = true; From 9784b2d67a102495f2d1b3bdb1ff918f39e7dafc Mon Sep 17 00:00:00 2001 From: Rodolfo Ribeiro Gomes Date: Thu, 8 Sep 2022 10:38:27 -0300 Subject: [PATCH 4/5] fix(trgt_magickpp): the previous row pointer was always the current one --- synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp index b6175fe320d..d76b43441cb 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp @@ -239,13 +239,13 @@ magickpp_trgt::end_frame() bool magickpp_trgt::start_frame(synfig::ProgressCallback */*callback*/) { + previous_row_buffer_pointer = buffer_pointer; + if (buffer_pointer == buffer1.data()) buffer_pointer = current_row_buffer_pointer = buffer2.data(); else buffer_pointer = current_row_buffer_pointer = buffer1.data(); - previous_row_buffer_pointer = buffer_pointer; - transparent = false; return true; @@ -260,8 +260,7 @@ magickpp_trgt::start_scanline(int /*scanline*/) bool magickpp_trgt::end_scanline() { - if (previous_row_buffer_pointer) - color_to_pixelformat(previous_row_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); + color_to_pixelformat(current_row_buffer_pointer, color_buffer.data(), PF_RGB|PF_A, 0, width); if (!transparent && previous_row_buffer_pointer) { for (int i = 0; i < width; i++) { From ee9d66f1255a20dedc42e6e45051b247e2b972e1 Mon Sep 17 00:00:00 2001 From: Rodolfo Ribeiro Gomes Date: Thu, 8 Sep 2022 16:52:04 -0300 Subject: [PATCH 5/5] refactor(trgt_magickpp): check if it is rendering a GIF file avoid unnecessary extra buffer if it isn't GIF Is it really needed? Check later --- .../src/modules/mod_magickpp/trgt_magickpp.cpp | 18 ++++++++++++------ .../src/modules/mod_magickpp/trgt_magickpp.h | 2 ++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp index d76b43441cb..71d3e8c8984 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.cpp @@ -98,7 +98,7 @@ magickpp_trgt::~magickpp_trgt() if (multiple_images) { // check whether this file format supports multiple-image files - Magick::Image image(*(images.begin())); + Magick::Image image(images.front()); image.fileName(filename); try { @@ -218,9 +218,14 @@ magickpp_trgt::init(synfig::ProgressCallback*) buffer_pointer = nullptr; + std::string extension = filename_extension(filename); + std::transform(extension.begin(), extension.end(), extension.begin(), [](unsigned char c) { return std::tolower(c); }); + is_gif = extension == ".gif"; + std::size_t buffer_size = static_cast(4) * width * height; buffer1.resize(buffer_size); - buffer2.resize(buffer_size); + if (is_gif) + buffer2.resize(buffer_size); color_buffer.resize(width); @@ -231,17 +236,18 @@ void magickpp_trgt::end_frame() { Magick::Image image(width, height, "RGBA", Magick::CharPixel, buffer_pointer); - if (transparent && images.begin() != images.end()) - (images.end()-1)->gifDisposeMethod(Magick::BackgroundDispose); + if (is_gif && transparent && images.size() > 1) + images.back().gifDisposeMethod(Magick::BackgroundDispose); images.push_back(image); } bool magickpp_trgt::start_frame(synfig::ProgressCallback */*callback*/) { - previous_row_buffer_pointer = buffer_pointer; + if (is_gif) + previous_row_buffer_pointer = buffer_pointer; - if (buffer_pointer == buffer1.data()) + if (is_gif && buffer_pointer == buffer1.data()) buffer_pointer = current_row_buffer_pointer = buffer2.data(); else buffer_pointer = current_row_buffer_pointer = buffer1.data(); diff --git a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h index 590c9973566..523ebc615a3 100644 --- a/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h +++ b/synfig-core/src/modules/mod_magickpp/trgt_magickpp.h @@ -61,6 +61,7 @@ class magickpp_trgt : public synfig::Target_Scanline unsigned char* current_row_buffer_pointer; unsigned char* previous_row_buffer_pointer; bool transparent; + bool is_gif; std::vector color_buffer; std::vector images; synfig::String sequence_separator; @@ -75,6 +76,7 @@ class magickpp_trgt : public synfig::Target_Scanline current_row_buffer_pointer(nullptr), previous_row_buffer_pointer(nullptr), transparent(), + is_gif(false), sequence_separator(params.sequence_separator) { } virtual ~magickpp_trgt();