From 21c6d691d8ee1ae8a5678c79ab7c453a71009e4a Mon Sep 17 00:00:00 2001 From: Jeroen van Rijn Date: Wed, 6 Oct 2021 20:10:37 +0200 Subject: [PATCH] Add additional error checking to helpers. --- core/compress/common.odin | 2 + core/image/common.odin | 1 + core/image/png/example.odin | 155 +++++++++++++------------- core/image/png/helpers.odin | 72 ++++++------ core/image/png/png.odin | 71 ++++++++---- tests/core/image/test_core_image.odin | 18 +-- 6 files changed, 176 insertions(+), 143 deletions(-) diff --git a/core/compress/common.odin b/core/compress/common.odin index 683679566..5951f8bd0 100644 --- a/core/compress/common.odin +++ b/core/compress/common.odin @@ -11,6 +11,7 @@ package compress import "core:io" import "core:image" import "core:bytes" +import "core:mem" /* These settings bound how much compression algorithms will allocate for their output buffer. @@ -47,6 +48,7 @@ when size_of(uintptr) == 8 { Error :: union { General_Error, + mem.Allocator_Error, Deflate_Error, ZLIB_Error, GZIP_Error, diff --git a/core/image/common.odin b/core/image/common.odin index f30febc26..5de4bb84e 100644 --- a/core/image/common.odin +++ b/core/image/common.odin @@ -131,6 +131,7 @@ Error :: enum { Unknown_Interlace_Method, Requested_Channel_Not_Present, Post_Processing_Error, + Invalid_Chunk_Length, } /* diff --git a/core/image/png/example.odin b/core/image/png/example.odin index e27ea9671..b047f5c6a 100644 --- a/core/image/png/example.odin +++ b/core/image/png/example.odin @@ -41,7 +41,7 @@ main :: proc() { demo :: proc() { file: string - options := image.Options{} // {.return_metadata}; + options := image.Options{.return_metadata} err: compress.Error img: ^image.Image @@ -53,23 +53,26 @@ demo :: proc() { if err != nil { fmt.printf("Trying to read PNG file %v returned %v\n", file, err) } else { - v: ^Info - fmt.printf("Image: %vx%vx%v, %v-bit.\n", img.width, img.height, img.channels, img.depth) - if img.metadata_ptr != nil && img.metadata_type == Info { - v = (^Info)(img.metadata_ptr) - // Handle ancillary chunks as you wish. - // We provide helper functions for a few types. - for c in v.chunks { - #partial switch c.header.type { - case .tIME: - t, _ := core_time(c) + assert(img.metadata_ptr != nil && img.metadata_type == Info) + + v := (^Info)(img.metadata_ptr) + + // Handle ancillary chunks as you wish. + // We provide helper functions for a few types. + for c in v.chunks { + #partial switch c.header.type { + case .tIME: + if t, t_ok := core_time(c); t_ok { fmt.printf("[tIME]: %v\n", t) - case .gAMA: - fmt.printf("[gAMA]: %v\n", gamma(c)) - case .pHYs: - phys := phys(c) + } + case .gAMA: + if gama, gama_ok := gamma(c); gama_ok { + fmt.printf("[gAMA]: %v\n", gama) + } + case .pHYs: + if phys, phys_ok := phys(c); phys_ok { if phys.unit == .Meter { xm := f32(img.width) / f32(phys.ppu_x) ym := f32(img.height) / f32(phys.ppu_y) @@ -80,73 +83,71 @@ demo :: proc() { } else { fmt.printf("[pHYs] x: %v, y: %v pixels per unknown unit.\n", phys.ppu_x, phys.ppu_y) } - case .iTXt, .zTXt, .tEXt: - res, ok_text := text(c) - if ok_text { - if c.header.type == .iTXt { - fmt.printf("[iTXt] %v (%v:%v): %v\n", res.keyword, res.language, res.keyword_localized, res.text) - } else { - fmt.printf("[tEXt/zTXt] %v: %v\n", res.keyword, res.text) - } - } - defer text_destroy(res) - case .bKGD: - fmt.printf("[bKGD] %v\n", img.background) - case .eXIf: - res, ok_exif := exif(c) - if ok_exif { - /* - Other than checking the signature and byte order, we don't handle Exif data. - If you wish to interpret it, pass it to an Exif parser. - */ - fmt.printf("[eXIf] %v\n", res) - } - case .PLTE: - plte, plte_ok := plte(c) - if plte_ok { - fmt.printf("[PLTE] %v\n", plte) - } else { - fmt.printf("[PLTE] Error\n") - } - case .hIST: - res, ok_hist := hist(c) - if ok_hist { - fmt.printf("[hIST] %v\n", res) - } - case .cHRM: - res, ok_chrm := chrm(c) - if ok_chrm { - fmt.printf("[cHRM] %v\n", res) - } - case .sPLT: - res, ok_splt := splt(c) - if ok_splt { - fmt.printf("[sPLT] %v\n", res) - } - splt_destroy(res) - case .sBIT: - if res, ok_sbit := sbit(c); ok_sbit { - fmt.printf("[sBIT] %v\n", res) - } - case .iCCP: - res, ok_iccp := iccp(c) - if ok_iccp { - fmt.printf("[iCCP] %v\n", res) - } - iccp_destroy(res) - case .sRGB: - if res, ok_srgb := srgb(c); ok_srgb { - fmt.printf("[sRGB] Rendering intent: %v\n", res) - } - case: - type := c.header.type - name := chunk_type_to_name(&type) - fmt.printf("[%v]: %v\n", name, c.data) } + case .iTXt, .zTXt, .tEXt: + res, ok_text := text(c) + if ok_text { + if c.header.type == .iTXt { + fmt.printf("[iTXt] %v (%v:%v): %v\n", res.keyword, res.language, res.keyword_localized, res.text) + } else { + fmt.printf("[tEXt/zTXt] %v: %v\n", res.keyword, res.text) + } + } + defer text_destroy(res) + case .bKGD: + fmt.printf("[bKGD] %v\n", img.background) + case .eXIf: + if res, ok_exif := exif(c); ok_exif { + /* + Other than checking the signature and byte order, we don't handle Exif data. + If you wish to interpret it, pass it to an Exif parser. + */ + fmt.printf("[eXIf] %v\n", res) + } + case .PLTE: + if plte, plte_ok := plte(c); plte_ok { + fmt.printf("[PLTE] %v\n", plte) + } else { + fmt.printf("[PLTE] Error\n") + } + case .hIST: + if res, ok_hist := hist(c); ok_hist { + fmt.printf("[hIST] %v\n", res) + } + case .cHRM: + if res, ok_chrm := chrm(c); ok_chrm { + fmt.printf("[cHRM] %v\n", res) + } + case .sPLT: + res, ok_splt := splt(c) + if ok_splt { + fmt.printf("[sPLT] %v\n", res) + } + splt_destroy(res) + case .sBIT: + if res, ok_sbit := sbit(c); ok_sbit { + fmt.printf("[sBIT] %v\n", res) + } + case .iCCP: + res, ok_iccp := iccp(c) + if ok_iccp { + fmt.printf("[iCCP] %v\n", res) + } + iccp_destroy(res) + case .sRGB: + if res, ok_srgb := srgb(c); ok_srgb { + fmt.printf("[sRGB] Rendering intent: %v\n", res) + } + case: + type := c.header.type + name := chunk_type_to_name(&type) + fmt.printf("[%v]: %v\n", name, c.data) } } } + fmt.printf("Done parsing metadata.\n") + if err == nil && .do_not_decompress_image not_in options && .info not_in options { if ok := write_image_as_ppm("out.ppm", img); ok { fmt.println("Saved decoded image.") diff --git a/core/image/png/helpers.odin b/core/image/png/helpers.odin index 4d8cebedf..1b3749f76 100644 --- a/core/image/png/helpers.odin +++ b/core/image/png/helpers.odin @@ -34,15 +34,17 @@ destroy :: proc(img: ^Image) { } bytes.buffer_destroy(&img.pixels) + + assert(img.metadata_ptr != nil && img.metadata_type == Info) + v := (^Info)(img.metadata_ptr) + + for chunk in &v.chunks { + delete(chunk.data) + } + delete(v.chunks) + // Clean up Info. free(img.metadata_ptr) - - /* - We don't need to do anything for the individual chunks. - They're allocated on the temp allocator, as is info.chunks - - See read_chunk. - */ free(img) } @@ -50,46 +52,50 @@ destroy :: proc(img: ^Image) { Chunk helpers */ -gamma :: proc(c: Chunk) -> f32 { - assert(c.header.type == .gAMA) - res := (^gAMA)(raw_data(c.data))^ - when true { - // Returns the wrong result on old backend - // Fixed for -llvm-api - return f32(res.gamma_100k) / 100_000.0 - } else { - return f32(u32(res.gamma_100k)) / 100_000.0 +gamma :: proc(c: Chunk) -> (res: f32, ok: bool) { + if c.header.type != .gAMA || len(c.data) != size_of(gAMA) { + return {}, false } + gama := (^gAMA)(raw_data(c.data))^ + return f32(gama.gamma_100k) / 100_000.0, true } INCHES_PER_METER :: 1000.0 / 25.4 -phys :: proc(c: Chunk) -> pHYs { - assert(c.header.type == .pHYs) - res := (^pHYs)(raw_data(c.data))^ - return res +phys :: proc(c: Chunk) -> (res: pHYs, ok: bool) { + if c.header.type != .pHYs || len(c.data) != size_of(pHYs) { + return {}, false + } + + return (^pHYs)(raw_data(c.data))^, true } phys_to_dpi :: proc(p: pHYs) -> (x_dpi, y_dpi: f32) { return f32(p.ppu_x) / INCHES_PER_METER, f32(p.ppu_y) / INCHES_PER_METER } -time :: proc(c: Chunk) -> tIME { - assert(c.header.type == .tIME) - res := (^tIME)(raw_data(c.data))^ - return res +time :: proc(c: Chunk) -> (res: tIME, ok: bool) { + if c.header.type != .tIME || len(c.data) != size_of(tIME) { + return {}, false + } + + return (^tIME)(raw_data(c.data))^, true } core_time :: proc(c: Chunk) -> (t: coretime.Time, ok: bool) { - png_time := time(c) - using png_time - return coretime.datetime_to_time( - int(year), int(month), int(day), - int(hour), int(minute), int(second), - ) + if png_time, png_ok := time(c); png_ok { + using png_time + return coretime.datetime_to_time( + int(year), int(month), int(day), + int(hour), int(minute), int(second), + ) + } else { + return {}, false + } } text :: proc(c: Chunk) -> (res: Text, ok: bool) { + assert(len(c.data) == int(c.header.length)) #partial switch c.header.type { case .tEXt: ok = true @@ -228,9 +234,7 @@ iccp_destroy :: proc(i: iCCP) { } srgb :: proc(c: Chunk) -> (res: sRGB, ok: bool) { - ok = true - - if c.header.type != .sRGB || len(c.data) != 1 { + if c.header.type != .sRGB || len(c.data) != size_of(sRGB_Rendering_Intent) { return {}, false } @@ -238,7 +242,7 @@ srgb :: proc(c: Chunk) -> (res: sRGB, ok: bool) { if res.intent > max(sRGB_Rendering_Intent) { ok = false; return } - return + return res, true } plte :: proc(c: Chunk) -> (res: PLTE, ok: bool) { diff --git a/core/image/png/png.odin b/core/image/png/png.odin index 6e9102aaa..204f98b66 100644 --- a/core/image/png/png.odin +++ b/core/image/png/png.odin @@ -100,13 +100,13 @@ Chunk_Type :: enum u32be { } IHDR :: struct #packed { - width: u32be, - height: u32be, - bit_depth: u8, - color_type: Color_Type, + width: u32be, + height: u32be, + bit_depth: u8, + color_type: Color_Type, compression_method: u8, - filter_method: u8, - interlace_method: Interlace_Method, + filter_method: u8, + interlace_method: Interlace_Method, } IHDR_SIZE :: size_of(IHDR) #assert (IHDR_SIZE == 13) @@ -135,22 +135,22 @@ PLTE_Entry :: [3]u8 PLTE :: struct #packed { entries: [256]PLTE_Entry, - used: u16, + used: u16, } hIST :: struct #packed { entries: [256]u16, - used: u16, + used: u16, } sPLT :: struct #packed { - name: string, - depth: u8, + name: string, + depth: u8, entries: union { [][4]u8, [][4]u16, }, - used: u16, + used: u16, } // Other chunks @@ -223,14 +223,14 @@ Exif :: struct { } iCCP :: struct { - name: string, + name: string, profile: []u8, } sRGB_Rendering_Intent :: enum u8 { - Perceptual = 0, + Perceptual = 0, Relative_colorimetric = 1, - Saturation = 2, + Saturation = 2, Absolute_colorimetric = 3, } @@ -274,6 +274,35 @@ read_chunk :: proc(ctx: ^$C) -> (chunk: Chunk, err: Error) { return chunk, nil } +copy_chunk :: proc(src: Chunk, allocator := context.allocator) -> (dest: Chunk, err: Error) { + if int(src.header.length) != len(src.data) { + return {}, .Invalid_Chunk_Length + } + + dest.header = src.header + dest.crc = src.crc + dest.data = make([]u8, dest.header.length, allocator) or_return + + copy(dest.data[:], src.data[:]) + return +} + +append_chunk :: proc(list: ^[dynamic]Chunk, src: Chunk, allocator := context.allocator) -> (err: Error) { + if int(src.header.length) != len(src.data) { + return .Invalid_Chunk_Length + } + + c := copy_chunk(src, allocator) or_return + length := len(list) + append(list, c) + if len(list) != length + 1 { + // Resize during append failed. + return .Out_Of_Memory + } + + return +} + read_header :: proc(ctx: ^$C) -> (IHDR, Error) { c, e := read_chunk(ctx) if e != nil { @@ -422,8 +451,6 @@ load_from_context :: proc(ctx: ^$C, options := Options{}, allocator := context.a header: IHDR - info.chunks.allocator = context.temp_allocator - // State to ensure correct chunk ordering. seen_ihdr := false; first := true seen_plte := false @@ -518,7 +545,7 @@ load_from_context :: proc(ctx: ^$C, options := Options{}, allocator := context.a } if .return_metadata in options { - append(&info.chunks, c) + append_chunk(&info.chunks, c) or_return } case .IDAT: // If we only want image metadata and don't want the pixel data, we can early out. @@ -563,7 +590,7 @@ load_from_context :: proc(ctx: ^$C, options := Options{}, allocator := context.a c = read_chunk(ctx) or_return seen_bkgd = true if .return_metadata in options { - append(&info.chunks, c) + append_chunk(&info.chunks, c) or_return } ct := transmute(u8)info.header.color_type @@ -599,7 +626,7 @@ load_from_context :: proc(ctx: ^$C, options := Options{}, allocator := context.a } if .return_metadata in options { - append(&info.chunks, c) + append_chunk(&info.chunks, c) or_return } /* @@ -626,16 +653,14 @@ load_from_context :: proc(ctx: ^$C, options := Options{}, allocator := context.a /* iPhone PNG bastardization that doesn't adhere to spec with broken IDAT chunk. We're not going to add support for it. If you have the misfortunte of coming - across one of these files, use a utility to defry it.s + across one of these files, use a utility to defry it. */ return img, E_PNG.PNG_Does_Not_Adhere_to_Spec case: // Unhandled type c = read_chunk(ctx) or_return - if .return_metadata in options { - // NOTE: Chunk cata is currently allocated on the temp allocator. - append(&info.chunks, c) + append_chunk(&info.chunks, c) or_return } first = false diff --git a/tests/core/image/test_core_image.odin b/tests/core/image/test_core_image.odin index 339faff58..14f7db074 100644 --- a/tests/core/image/test_core_image.odin +++ b/tests/core/image/test_core_image.odin @@ -1514,10 +1514,10 @@ run_png_suite :: proc(t: ^testing.T, suite: []PNG_Test) -> (subtotal: int) { case .gAMA: switch(file.file) { case "pp0n2c16", "pp0n6a08": - gamma := png.gamma(c) + gamma, gamma_ok := png.gamma(c) expected_gamma := f32(1.0) error = fmt.tprintf("%v test %v gAMA is %v, expected %v.", file.file, count, gamma, expected_gamma) - expect(t, gamma == expected_gamma, error) + expect(t, gamma == expected_gamma && gamma_ok, error) } case .PLTE: switch(file.file) { @@ -1557,25 +1557,25 @@ run_png_suite :: proc(t: ^testing.T, suite: []PNG_Test) -> (subtotal: int) { expect(t, expected_chrm == chrm && chrm_ok, error) } case .pHYs: - phys := png.phys(c) + phys, phys_ok := png.phys(c) phys_err := "%v test %v cHRM is %v, expected %v." switch (file.file) { case "cdfn2c08": expected_phys := png.pHYs{ppu_x = 1, ppu_y = 4, unit = .Unknown} error = fmt.tprintf(phys_err, file.file, count, phys, expected_phys) - expect(t, expected_phys == phys, error) + expect(t, expected_phys == phys && phys_ok, error) case "cdhn2c08": expected_phys := png.pHYs{ppu_x = 4, ppu_y = 1, unit = .Unknown} error = fmt.tprintf(phys_err, file.file, count, phys, expected_phys) - expect(t, expected_phys == phys, error) + expect(t, expected_phys == phys && phys_ok, error) case "cdsn2c08": expected_phys := png.pHYs{ppu_x = 1, ppu_y = 1, unit = .Unknown} error = fmt.tprintf(phys_err, file.file, count, phys, expected_phys) - expect(t, expected_phys == phys, error) + expect(t, expected_phys == phys && phys_ok, error) case "cdun2c08": expected_phys := png.pHYs{ppu_x = 1000, ppu_y = 1000, unit = .Meter} error = fmt.tprintf(phys_err, file.file, count, phys, expected_phys) - expect(t, expected_phys == phys, error) + expect(t, expected_phys == phys && phys_ok, error) } case .hIST: hist, hist_ok := png.hist(c) @@ -1589,7 +1589,7 @@ run_png_suite :: proc(t: ^testing.T, suite: []PNG_Test) -> (subtotal: int) { expect(t, hist.used == 256 && hist_ok, error) } case .tIME: - png_time := png.time(c) + png_time, png_time_ok := png.time(c) time_err := "%v test %v tIME was %v, expected %v." expected_time: png.tIME @@ -1610,7 +1610,7 @@ run_png_suite :: proc(t: ^testing.T, suite: []PNG_Test) -> (subtotal: int) { } error = fmt.tprintf(time_err, file.file, count, png_time, expected_time) - expect(t, png_time == expected_time, error) + expect(t, png_time == expected_time && png_time_ok, error) error = fmt.tprintf(time_core_err, file.file, count, core_time, expected_core) expect(t, core_time == expected_core && core_time_ok, error)