[Libreoffice-commits] online.git: common/Png.hpp

[Ashod Nakashian (via logerrit)]

 common/Png.hpp |   66 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 23 deletions(-)

New commits:
commit f7bd2c7e1df5c2f1e17ae2b8e1658f9013704648
Author:     Ashod Nakashian <ashod.nakashian at collabora.co.uk>
AuthorDate: Sat Sep 21 21:10:18 2019 -0400
Commit:     Ashod Nakashian <ashnakash at gmail.com>
CommitDate: Sun Sep 22 20:24:36 2019 +0200

    wsd: fix variable might be clobbered by ‘longjmp’ or ‘vfork’
    
    As the comment details, this avoids having C++ objects
    in the same frame as setjmp, which may reset their
    contents without the dtor getting called.
    
    Change-Id: I851ae8bffb4356d465a25dfc815a1fecb489fa30
    Reviewed-on: https://gerrit.libreoffice.org/79338
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/common/Png.hpp b/common/Png.hpp
index d4e1c2cf5..d629158c2 100644
--- a/common/Png.hpp
+++ b/common/Png.hpp
@@ -111,13 +111,17 @@ unpremultiply_data (png_structp /*png*/, png_row_infop row_info, png_bytep data)
     }
 }
 
-// Sadly, older libpng headers don't use const for the pixmap pointer parameter to
-// png_write_row(), so can't use const here for pixmap.
-inline
-bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
-                          int width, int height,
-                          int bufferWidth, int bufferHeight,
-                          std::vector<char>& output, LibreOfficeKitTileMode mode)
+/// This function uses setjmp which may clobbers non-trivial objects.
+/// So we can't use logging or create complex C++ objects in this frame.
+/// Specifically, logging uses std::string objects, and GCC gives the following:
+/// error: variable ‘__capacity’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
+/// In practice, this is bogus, since __capacity is has a trivial type, but this error
+/// shows up with the sanitizers. But technically we shouldn't mix C++ objects with setjmp.
+/// Sadly, older libpng headers don't use const for the pixmap pointer parameter to
+/// png_write_row(), so can't use const here for pixmap.
+inline bool impl_encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
+                                      int width, int height, int bufferWidth, int bufferHeight,
+                                      std::vector<char>& output, LibreOfficeKitTileMode mode)
 {
     if (bufferWidth < width || bufferHeight < height)
     {
@@ -148,7 +152,8 @@ bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
     auto initialSize = output.size();
 #endif
 
-    png_set_IHDR(png_ptr, info_ptr, width, height, 8, PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
+    png_set_IHDR(png_ptr, info_ptr, width, height, 8, PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE,
+                 PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
 
     png_set_write_fn(png_ptr, &output, user_write_fn, user_flush_fn);
     png_set_write_status_fn(png_ptr, user_write_status_fn);
@@ -160,8 +165,6 @@ bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
         png_set_write_user_transform_fn (png_ptr, unpremultiply_data);
     }
 
-    auto a = std::chrono::steady_clock::now();
-
     for (int y = 0; y < height; ++y)
     {
         size_t position = ((startY + y) * bufferWidth * 4) + (startX * 4);
@@ -170,19 +173,6 @@ bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
 
     png_write_end(png_ptr, info_ptr);
 
-    auto b = std::chrono::steady_clock::now();
-
-    std::chrono::milliseconds::rep duration = std::chrono::duration_cast<std::chrono::milliseconds>(b-a).count();
-
-    static std::chrono::milliseconds::rep totalDuration = 0;
-    static int nCalls = 0;
-
-    totalDuration += duration;
-    nCalls += 1;
-    LOG_TRC("PNG compression took " << duration << " ms (" << output.size()
-                                    << " bytes). Average after " << std::to_string(nCalls)
-                                    << " calls: " << (totalDuration / static_cast<double>(nCalls)));
-
     png_destroy_write_struct(&png_ptr, &info_ptr);
 
 #ifdef IOS
@@ -198,6 +188,36 @@ bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY,
     return true;
 }
 
+/// Sadly, older libpng headers don't use const for the pixmap pointer parameter to
+/// png_write_row(), so can't use const here for pixmap.
+inline bool encodeSubBufferToPNG(unsigned char* pixmap, size_t startX, size_t startY, int width,
+                                 int height, int bufferWidth, int bufferHeight,
+                                 std::vector<char>& output, LibreOfficeKitTileMode mode)
+{
+    const auto start = std::chrono::steady_clock::now();
+
+    const bool res = impl_encodeSubBufferToPNG(pixmap, startX, startY, width, height, bufferWidth,
+                                               bufferHeight, output, mode);
+    if (Log::traceEnabled())
+    {
+        const auto end = std::chrono::steady_clock::now();
+
+        std::chrono::milliseconds::rep duration
+            = std::chrono::duration_cast<std::chrono::milliseconds>(end - start).count();
+
+        static std::chrono::milliseconds::rep totalDuration = 0;
+        static int nCalls = 0;
+
+        totalDuration += duration;
+        ++nCalls;
+        LOG_TRC("PNG compression took "
+                << duration << " ms (" << output.size() << " bytes). Average after " << nCalls
+                << " calls: " << (totalDuration / static_cast<double>(nCalls)));
+    }
+
+    return res;
+}
+
 inline
 bool encodeBufferToPNG(unsigned char* pixmap, int width, int height,
                        std::vector<char>& output, LibreOfficeKitTileMode mode)