From acba38c2889d210097b76cad1bb0fba9e027a620 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 27 Sep 2017 17:53:34 -0700 Subject: [PATCH] Move the png open and destroy functions into a class The open_png() function used to open the png file but didn't close it; and this caused the leak of fd. However, we cannot close the file inside open_png() because the png file needs to remain open until the outer function finishes parsing the file and destroys the png struct. This CL addresses this issue by implementing a PngReader class to handle the creation/destruction of the png struct. Bug: 67010912 Test: Run graphic tests; also run locale tests and check fd. Change-Id: I9a803b3cd8c16f16a9ffe8f0acc7fe0f42e95eb0 --- minui/resources.cpp | 545 ++++++++++++++++++++++---------------------- 1 file changed, 269 insertions(+), 276 deletions(-) diff --git a/minui/resources.cpp b/minui/resources.cpp index 756f29d2..837f5ebc 100644 --- a/minui/resources.cpp +++ b/minui/resources.cpp @@ -25,10 +25,12 @@ #include #include +#include #include #include #include +#include #include #include @@ -46,89 +48,126 @@ static GRSurface* malloc_surface(size_t data_size) { return surface; } -static int open_png(const char* name, png_structp* png_ptr, png_infop* info_ptr, - png_uint_32* width, png_uint_32* height, png_byte* channels) { - char resPath[256]; - unsigned char header[8]; - int result = 0; - int color_type, bit_depth; - size_t bytesRead; +// This class handles the png file parsing. It also holds the ownership of the png pointer and the +// opened file pointer. Both will be destroyed/closed when this object goes out of scope. +class PngHandler { + public: + PngHandler(const std::string& name); - snprintf(resPath, sizeof(resPath)-1, "/res/images/%s.png", name); - resPath[sizeof(resPath)-1] = '\0'; - FILE* fp = fopen(resPath, "rbe"); - if (fp == NULL) { - result = -1; - goto exit; - } + ~PngHandler(); - bytesRead = fread(header, 1, sizeof(header), fp); - if (bytesRead != sizeof(header)) { - result = -2; - goto exit; - } + png_uint_32 width() const { + return width_; + } - if (png_sig_cmp(header, 0, sizeof(header))) { - result = -3; - goto exit; - } + png_uint_32 height() const { + return height_; + } - *png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL); - if (!*png_ptr) { - result = -4; - goto exit; - } + png_byte channels() const { + return channels_; + } - *info_ptr = png_create_info_struct(*png_ptr); - if (!*info_ptr) { - result = -5; - goto exit; - } + png_structp png_ptr() const { + return png_ptr_; + } - if (setjmp(png_jmpbuf(*png_ptr))) { - result = -6; - goto exit; - } + png_infop info_ptr() const { + return info_ptr_; + } - png_init_io(*png_ptr, fp); - png_set_sig_bytes(*png_ptr, sizeof(header)); - png_read_info(*png_ptr, *info_ptr); + int error_code() const { + return error_code_; + }; - png_get_IHDR(*png_ptr, *info_ptr, width, height, &bit_depth, - &color_type, NULL, NULL, NULL); + operator bool() const { + return error_code_ == 0; + } - *channels = png_get_channels(*png_ptr, *info_ptr); + private: + png_structp png_ptr_{ nullptr }; + png_infop info_ptr_{ nullptr }; + png_uint_32 width_; + png_uint_32 height_; + png_byte channels_; - if (bit_depth == 8 && *channels == 3 && color_type == PNG_COLOR_TYPE_RGB) { - // 8-bit RGB images: great, nothing to do. - } else if (bit_depth <= 8 && *channels == 1 && color_type == PNG_COLOR_TYPE_GRAY) { - // 1-, 2-, 4-, or 8-bit gray images: expand to 8-bit gray. - png_set_expand_gray_1_2_4_to_8(*png_ptr); - } else if (bit_depth <= 8 && *channels == 1 && color_type == PNG_COLOR_TYPE_PALETTE) { - // paletted images: expand to 8-bit RGB. Note that we DON'T - // currently expand the tRNS chunk (if any) to an alpha - // channel, because minui doesn't support alpha channels in - // general. - png_set_palette_to_rgb(*png_ptr); - *channels = 3; - } else { - fprintf(stderr, "minui doesn't support PNG depth %d channels %d color_type %d\n", - bit_depth, *channels, color_type); - result = -7; - goto exit; - } + // The |error_code_| is set to a negative value if an error occurs when opening the png file. + int error_code_; + // After initialization, we'll keep the file pointer open before destruction of PngHandler. + std::unique_ptr png_fp_; +}; - return result; +PngHandler::PngHandler(const std::string& name) : error_code_(0), png_fp_(nullptr, fclose) { + std::string res_path = android::base::StringPrintf("/res/images/%s.png", name.c_str()); + png_fp_.reset(fopen(res_path.c_str(), "rbe")); + if (!png_fp_) { + error_code_ = -1; + return; + } - exit: - if (result < 0) { - png_destroy_read_struct(png_ptr, info_ptr, NULL); - } - if (fp != NULL) { - fclose(fp); - } + unsigned char header[8]; + size_t bytesRead = fread(header, 1, sizeof(header), png_fp_.get()); + if (bytesRead != sizeof(header)) { + error_code_ = -2; + return; + } - return result; + if (png_sig_cmp(header, 0, sizeof(header))) { + error_code_ = -3; + return; + } + + png_ptr_ = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr); + if (!png_ptr_) { + error_code_ = -4; + return; + } + + info_ptr_ = png_create_info_struct(png_ptr_); + if (!info_ptr_) { + error_code_ = -5; + return; + } + + if (setjmp(png_jmpbuf(png_ptr_))) { + error_code_ = -6; + return; + } + + png_init_io(png_ptr_, png_fp_.get()); + png_set_sig_bytes(png_ptr_, sizeof(header)); + png_read_info(png_ptr_, info_ptr_); + + int color_type; + int bit_depth; + png_get_IHDR(png_ptr_, info_ptr_, &width_, &height_, &bit_depth, &color_type, nullptr, nullptr, + nullptr); + + channels_ = png_get_channels(png_ptr_, info_ptr_); + + if (bit_depth == 8 && channels_ == 3 && color_type == PNG_COLOR_TYPE_RGB) { + // 8-bit RGB images: great, nothing to do. + } else if (bit_depth <= 8 && channels_ == 1 && color_type == PNG_COLOR_TYPE_GRAY) { + // 1-, 2-, 4-, or 8-bit gray images: expand to 8-bit gray. + png_set_expand_gray_1_2_4_to_8(png_ptr_); + } else if (bit_depth <= 8 && channels_ == 1 && color_type == PNG_COLOR_TYPE_PALETTE) { + // paletted images: expand to 8-bit RGB. Note that we DON'T + // currently expand the tRNS chunk (if any) to an alpha + // channel, because minui doesn't support alpha channels in + // general. + png_set_palette_to_rgb(png_ptr_); + channels_ = 3; + } else { + fprintf(stderr, "minui doesn't support PNG depth %d channels %d color_type %d\n", bit_depth, + channels_, color_type); + error_code_ = -7; + } +} + +PngHandler::~PngHandler() { + if (png_ptr_) { + png_destroy_read_struct(&png_ptr_, &info_ptr_, nullptr); + } } // "display" surfaces are transformed into the framebuffer's required @@ -198,178 +237,152 @@ static void transform_rgb_to_draw(unsigned char* input_row, } int res_create_display_surface(const char* name, GRSurface** pSurface) { - GRSurface* surface = NULL; - int result = 0; - png_structp png_ptr = NULL; - png_infop info_ptr = NULL; - png_uint_32 width, height; - png_byte channels; - unsigned char* p_row; - unsigned int y; + *pSurface = nullptr; - *pSurface = NULL; + PngHandler png_handler(name); + if (!png_handler) return png_handler.error_code(); - result = open_png(name, &png_ptr, &info_ptr, &width, &height, &channels); - if (result < 0) return result; + png_structp png_ptr = png_handler.png_ptr(); + png_uint_32 width = png_handler.width(); + png_uint_32 height = png_handler.height(); - surface = init_display_surface(width, height); - if (surface == NULL) { - result = -8; - goto exit; - } + GRSurface* surface = init_display_surface(width, height); + if (!surface) { + return -8; + } #if defined(RECOVERY_ABGR) || defined(RECOVERY_BGRA) - png_set_bgr(png_ptr); + png_set_bgr(png_ptr); #endif - p_row = static_cast(malloc(width * 4)); - for (y = 0; y < height; ++y) { - png_read_row(png_ptr, p_row, NULL); - transform_rgb_to_draw(p_row, surface->data + y * surface->row_bytes, channels, width); - } - free(p_row); + for (png_uint_32 y = 0; y < height; ++y) { + std::vector p_row(width * 4); + png_read_row(png_ptr, p_row.data(), nullptr); + transform_rgb_to_draw(p_row.data(), surface->data + y * surface->row_bytes, + png_handler.channels(), width); + } - *pSurface = surface; + *pSurface = surface; - exit: - png_destroy_read_struct(&png_ptr, &info_ptr, NULL); - if (result < 0 && surface != NULL) free(surface); - return result; + return 0; } int res_create_multi_display_surface(const char* name, int* frames, int* fps, - GRSurface*** pSurface) { - GRSurface** surface = NULL; - int result = 0; - png_structp png_ptr = NULL; - png_infop info_ptr = NULL; - png_uint_32 width, height; - png_byte channels; - png_textp text; - int num_text; - unsigned char* p_row; - unsigned int y; + GRSurface*** pSurface) { + *pSurface = nullptr; + *frames = -1; - *pSurface = NULL; - *frames = -1; + PngHandler png_handler(name); + if (!png_handler) return png_handler.error_code(); - result = open_png(name, &png_ptr, &info_ptr, &width, &height, &channels); - if (result < 0) return result; + png_structp png_ptr = png_handler.png_ptr(); + png_uint_32 width = png_handler.width(); + png_uint_32 height = png_handler.height(); - *frames = 1; - *fps = 20; - if (png_get_text(png_ptr, info_ptr, &text, &num_text)) { - for (int i = 0; i < num_text; ++i) { - if (text[i].key && strcmp(text[i].key, "Frames") == 0 && text[i].text) { - *frames = atoi(text[i].text); - } else if (text[i].key && strcmp(text[i].key, "FPS") == 0 && text[i].text) { - *fps = atoi(text[i].text); - } - } - printf(" found frames = %d\n", *frames); - printf(" found fps = %d\n", *fps); + *frames = 1; + *fps = 20; + png_textp text; + int num_text; + if (png_get_text(png_ptr, png_handler.info_ptr(), &text, &num_text)) { + for (int i = 0; i < num_text; ++i) { + if (text[i].key && strcmp(text[i].key, "Frames") == 0 && text[i].text) { + *frames = atoi(text[i].text); + } else if (text[i].key && strcmp(text[i].key, "FPS") == 0 && text[i].text) { + *fps = atoi(text[i].text); + } } + printf(" found frames = %d\n", *frames); + printf(" found fps = %d\n", *fps); + } - if (*frames <= 0 || *fps <= 0) { - printf("bad number of frames (%d) and/or FPS (%d)\n", *frames, *fps); - result = -10; - goto exit; - } + int result = 0; + GRSurface** surface = nullptr; + if (*frames <= 0 || *fps <= 0) { + printf("bad number of frames (%d) and/or FPS (%d)\n", *frames, *fps); + result = -10; + goto exit; + } - if (height % *frames != 0) { - printf("bad height (%d) for frame count (%d)\n", height, *frames); - result = -9; - goto exit; - } + if (height % *frames != 0) { + printf("bad height (%d) for frame count (%d)\n", height, *frames); + result = -9; + goto exit; + } - surface = static_cast(calloc(*frames, sizeof(GRSurface*))); - if (surface == NULL) { - result = -8; - goto exit; - } - for (int i = 0; i < *frames; ++i) { - surface[i] = init_display_surface(width, height / *frames); - if (surface[i] == NULL) { - result = -8; - goto exit; - } + surface = static_cast(calloc(*frames, sizeof(GRSurface*))); + if (!surface) { + result = -8; + goto exit; + } + for (int i = 0; i < *frames; ++i) { + surface[i] = init_display_surface(width, height / *frames); + if (!surface[i]) { + result = -8; + goto exit; } + } #if defined(RECOVERY_ABGR) || defined(RECOVERY_BGRA) - png_set_bgr(png_ptr); + png_set_bgr(png_ptr); #endif - p_row = static_cast(malloc(width * 4)); - for (y = 0; y < height; ++y) { - png_read_row(png_ptr, p_row, NULL); - int frame = y % *frames; - unsigned char* out_row = surface[frame]->data + - (y / *frames) * surface[frame]->row_bytes; - transform_rgb_to_draw(p_row, out_row, channels, width); - } - free(p_row); + for (png_uint_32 y = 0; y < height; ++y) { + std::vector p_row(width * 4); + png_read_row(png_ptr, p_row.data(), nullptr); + int frame = y % *frames; + unsigned char* out_row = surface[frame]->data + (y / *frames) * surface[frame]->row_bytes; + transform_rgb_to_draw(p_row.data(), out_row, png_handler.channels(), width); + } - *pSurface = surface; + *pSurface = surface; exit: - png_destroy_read_struct(&png_ptr, &info_ptr, NULL); - - if (result < 0) { - if (surface) { - for (int i = 0; i < *frames; ++i) { - free(surface[i]); - } - free(surface); - } + if (result < 0) { + if (surface) { + for (int i = 0; i < *frames; ++i) { + free(surface[i]); + } + free(surface); } - return result; + } + return result; } int res_create_alpha_surface(const char* name, GRSurface** pSurface) { - GRSurface* surface = NULL; - int result = 0; - png_structp png_ptr = NULL; - png_infop info_ptr = NULL; - png_uint_32 width, height; - png_byte channels; + *pSurface = nullptr; - *pSurface = NULL; + PngHandler png_handler(name); + if (!png_handler) return png_handler.error_code(); - result = open_png(name, &png_ptr, &info_ptr, &width, &height, &channels); - if (result < 0) return result; + if (png_handler.channels() != 1) { + return -7; + } - if (channels != 1) { - result = -7; - goto exit; - } + png_structp png_ptr = png_handler.png_ptr(); + png_uint_32 width = png_handler.width(); + png_uint_32 height = png_handler.height(); - surface = malloc_surface(width * height); - if (surface == NULL) { - result = -8; - goto exit; - } - surface->width = width; - surface->height = height; - surface->row_bytes = width; - surface->pixel_bytes = 1; + GRSurface* surface = malloc_surface(width * height); + if (!surface) { + return -8; + } + surface->width = width; + surface->height = height; + surface->row_bytes = width; + surface->pixel_bytes = 1; #if defined(RECOVERY_ABGR) || defined(RECOVERY_BGRA) - png_set_bgr(png_ptr); + png_set_bgr(png_ptr); #endif - unsigned char* p_row; - unsigned int y; - for (y = 0; y < height; ++y) { - p_row = surface->data + y * surface->row_bytes; - png_read_row(png_ptr, p_row, NULL); - } + for (png_uint_32 y = 0; y < height; ++y) { + unsigned char* p_row = surface->data + y * surface->row_bytes; + png_read_row(png_ptr, p_row, nullptr); + } - *pSurface = surface; + *pSurface = surface; - exit: - png_destroy_read_struct(&png_ptr, &info_ptr, NULL); - if (result < 0 && surface != NULL) free(surface); - return result; + return 0; } // This function tests if a locale string stored in PNG (prefix) matches @@ -397,109 +410,89 @@ bool matches_locale(const std::string& prefix, const std::string& locale) { } std::vector get_locales_in_png(const std::string& png_name) { - png_structp png_ptr = nullptr; - png_infop info_ptr = nullptr; - png_uint_32 width, height; - png_byte channels; - - int status = open_png(png_name.c_str(), &png_ptr, &info_ptr, &width, &height, &channels); - if (status < 0) { - printf("Failed to open %s\n", png_name.c_str()); + PngHandler png_handler(png_name); + if (!png_handler) { + printf("Failed to open %s, error: %d\n", png_name.c_str(), png_handler.error_code()); return {}; } - if (channels != 1) { - printf("Expect input png to have 1 data channel, this file has %d\n", channels); - png_destroy_read_struct(&png_ptr, &info_ptr, nullptr); + if (png_handler.channels() != 1) { + printf("Expect input png to have 1 data channel, this file has %d\n", png_handler.channels()); return {}; } std::vector result; - std::vector row(width); - for (png_uint_32 y = 0; y < height; ++y) { - png_read_row(png_ptr, row.data(), nullptr); + std::vector row(png_handler.width()); + for (png_uint_32 y = 0; y < png_handler.height(); ++y) { + png_read_row(png_handler.png_ptr(), row.data(), nullptr); int h = (row[3] << 8) | row[2]; std::string loc(reinterpret_cast(&row[5])); if (!loc.empty()) { result.push_back(loc); } for (int i = 0; i < h; ++i, ++y) { - png_read_row(png_ptr, row.data(), NULL); + png_read_row(png_handler.png_ptr(), row.data(), nullptr); } } - png_destroy_read_struct(&png_ptr, &info_ptr, nullptr); return result; } int res_create_localized_alpha_surface(const char* name, const char* locale, GRSurface** pSurface) { - GRSurface* surface = NULL; - int result = 0; - png_structp png_ptr = NULL; - png_infop info_ptr = NULL; - png_uint_32 width, height; - png_byte channels; - png_uint_32 y; - std::vector row; + *pSurface = nullptr; + if (locale == nullptr) { + return 0; + } - *pSurface = NULL; + PngHandler png_handler(name); + if (!png_handler) return png_handler.error_code(); - if (locale == NULL) { - return result; + if (png_handler.channels() != 1) { + return -7; + } + + png_structp png_ptr = png_handler.png_ptr(); + png_uint_32 width = png_handler.width(); + png_uint_32 height = png_handler.height(); + + for (png_uint_32 y = 0; y < height; ++y) { + std::vector row(width); + png_read_row(png_ptr, row.data(), nullptr); + int w = (row[1] << 8) | row[0]; + int h = (row[3] << 8) | row[2]; + __unused int len = row[4]; + char* loc = reinterpret_cast(&row[5]); + + if (y + 1 + h >= height || matches_locale(loc, locale)) { + printf(" %20s: %s (%d x %d @ %d)\n", name, loc, w, h, y); + + GRSurface* surface = malloc_surface(w * h); + if (!surface) { + return -8; + } + surface->width = w; + surface->height = h; + surface->row_bytes = w; + surface->pixel_bytes = 1; + + for (int i = 0; i < h; ++i, ++y) { + png_read_row(png_ptr, row.data(), nullptr); + memcpy(surface->data + i * w, row.data(), w); + } + + *pSurface = surface; + break; } - result = open_png(name, &png_ptr, &info_ptr, &width, &height, &channels); - if (result < 0) return result; - - if (channels != 1) { - result = -7; - goto exit; + for (int i = 0; i < h; ++i, ++y) { + png_read_row(png_ptr, row.data(), nullptr); } + } - row.resize(width); - for (y = 0; y < height; ++y) { - png_read_row(png_ptr, row.data(), NULL); - int w = (row[1] << 8) | row[0]; - int h = (row[3] << 8) | row[2]; - __unused int len = row[4]; - char* loc = reinterpret_cast(&row[5]); - - if (y+1+h >= height || matches_locale(loc, locale)) { - printf(" %20s: %s (%d x %d @ %d)\n", name, loc, w, h, y); - - surface = malloc_surface(w*h); - if (surface == NULL) { - result = -8; - goto exit; - } - surface->width = w; - surface->height = h; - surface->row_bytes = w; - surface->pixel_bytes = 1; - - int i; - for (i = 0; i < h; ++i, ++y) { - png_read_row(png_ptr, row.data(), NULL); - memcpy(surface->data + i*w, row.data(), w); - } - - *pSurface = surface; - break; - } else { - int i; - for (i = 0; i < h; ++i, ++y) { - png_read_row(png_ptr, row.data(), NULL); - } - } - } - -exit: - png_destroy_read_struct(&png_ptr, &info_ptr, NULL); - if (result < 0 && surface != NULL) free(surface); - return result; + return 0; } void res_free_surface(GRSurface* surface) { - free(surface); + free(surface); }