Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Optimize gc cycles and introduce flag for reduced memory usage #95

Merged
merged 4 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ export type ODiffOptions = Partial<{
antialiasing: boolean;
/** If `true` reason: "pixel-diff" output will contain the set of line indexes containing different pixels */
captureDiffLines: boolean;
/** If `true` odiff will use less memory but will be slower with larger images */
reduceRamUsage: boolean;
/** An array of regions to ignore in the diff. */
ignoreRegions: Array<{
x1: number;
Expand Down
46 changes: 27 additions & 19 deletions bin/Color.ml
Original file line number Diff line number Diff line change
@@ -1,23 +1,31 @@
let ofHexString s =
match String.length s with
| (4 | 7) as len -> (
let short = len = 4 in
let r' =
match short with true -> String.sub s 1 1 | false -> String.sub s 1 2
in
let g' =
match short with true -> String.sub s 2 1 | false -> String.sub s 3 2
in
let b' =
match short with true -> String.sub s 3 1 | false -> String.sub s 5 2
in
let r = int_of_string_opt ("0x" ^ r') in
let g = int_of_string_opt ("0x" ^ g') in
let b = int_of_string_opt ("0x" ^ b') in
| (4 | 7) as len ->
(let short = len = 4 in
let r' =
match short with true -> String.sub s 1 1 | false -> String.sub s 1 2
in
let g' =
match short with true -> String.sub s 2 1 | false -> String.sub s 3 2
in
let b' =
match short with true -> String.sub s 3 1 | false -> String.sub s 5 2
in
let r = int_of_string_opt ("0x" ^ r') in
let g = int_of_string_opt ("0x" ^ g') in
let b = int_of_string_opt ("0x" ^ b') in

match (r, g, b) with
| Some r, Some g, Some b when short ->
Some ((16 * r) + r, (16 * g) + g, (16 * b) + b)
| Some r, Some g, Some b -> Some (r, g, b)
| _ -> None)
match (r, g, b) with
| Some r, Some g, Some b when short ->
Some ((16 * r) + r, (16 * g) + g, (16 * b) + b)
| Some r, Some g, Some b -> Some (r, g, b)
| _ -> None)
|> Option.map (fun (r, g, b) ->
(* Create rgba pixel value right after parsing *)
let r = (r land 255) lsl 0 in
let g = (g land 255) lsl 8 in
let b = (b land 255) lsl 16 in
let a = 255 lsl 24 in

Int32.of_int (a lor b lor g lor r))
| _ -> None
26 changes: 22 additions & 4 deletions bin/Main.ml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,23 @@ type 'output diffResult = { exitCode : int; diff : 'output option }
(* Arguments must remain positional for the cmd parser lib that we use *)
let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange
diffColorHex toEmitStdoutParsableString antialiasing ignoreRegions diffLines
=
disableGcOptimizations =
(*
We do not need to actually maintain memory size of the allocated RAM by odiff, so we are
increasing the minor memory size to avoid most of the possible deallocations. For sure it is
not possible be sure that it won't be run in OCaml because we allocate the Stack and Queue

By default set the minor heap size to 256mb on 64bit machine
*)
if not disableGcOptimizations then
Gc.set
{
(Gc.get ()) with
Gc.minor_heap_size = 64_000_000;
Gc.stack_limit = 2_048_000;
Gc.window_size = 25;
};

let module IO1 = (val getIOModule img1Path) in
let module IO2 = (val getIOModule img2Path) in
let module Diff = MakeDiff (IO1) (IO2) in
Expand All @@ -24,9 +40,9 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange
Diff.diff img1 img2 ~outputDiffMask ~threshold ~failOnLayoutChange
~antialiasing ~ignoreRegions ~diffLines
~diffPixel:
(Color.ofHexString diffColorHex |> function
| Some col -> col
| None -> (255, 0, 0))
(match Color.ofHexString diffColorHex with
| Some c -> c
| None -> redPixel)
()
|> Print.printDiffResult toEmitStdoutParsableString
|> function
Expand All @@ -43,4 +59,6 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange
(match diff with
| Some output when outputDiffMask -> IO1.freeImage output
| _ -> ());

(*Gc.print_stat stdout;*)
exit exitCode
9 changes: 8 additions & 1 deletion bin/ODiffBin.ml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ let diffLines =
"With this flag enabled, output result in case of different images \
will output lines for all the different pixels"

let disableGcOptimizations =
value & flag
& info [ "reduce-ram-usage" ]
~doc:
"With this flag enabled odiff will use less memory, but will be slower \
in some cases."

let ignoreRegions =
value
& opt
Expand All @@ -76,7 +83,7 @@ let cmd =
in
( const Main.main $ base $ comp $ diffPath $ threshold $ diffMask
$ failOnLayout $ diffColor $ parsableOutput $ antialiasing $ ignoreRegions
$ diffLines,
$ diffLines $ disableGcOptimizations,
Term.info "odiff" ~version:"3.0.0" ~doc:"Find difference between 2 images."
~exits:
(Term.exit_info 0 ~doc:"on image match"
Expand Down
2 changes: 2 additions & 0 deletions bin/node-bindings/odiff.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export type ODiffOptions = Partial<{
antialiasing: boolean;
/** If `true` reason: "pixel-diff" output will contain the set of line indexes containing different pixels */
captureDiffLines: boolean;
/** If `true` odiff will use less memory but will be slower with larger images */
reduceRamUsage: boolean;
/** An array of regions to ignore in the diff. */
ignoreRegions: Array<{
x1: number;
Expand Down
4 changes: 4 additions & 0 deletions bin/node-bindings/odiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ function optionsToArgs(options) {
setFlag("output-diff-lines", value);
break;

case "reduceRamUsage":
setFlag("reduce-ram-usage", value);
break;

case "ignoreRegions": {
const regions = value
.map(
Expand Down
34 changes: 14 additions & 20 deletions src/Diff.ml
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
let redPixel = (255, 0, 0)
(* Decimal representation of the RGBA in32 pixel red pixel *)
let redPixel = Int32.of_int 4278190335

(* Decimal representation of the RGBA in32 pixel green pixel *)
let maxYIQPossibleDelta = 35215.

type 'a diffVariant = Layout | Pixel of ('a * int * float * int Stack.t)
Expand All @@ -18,18 +21,21 @@ module MakeDiff (IO1 : ImageIO.ImageIO) (IO2 : ImageIO.ImageIO) = struct

let compare (base : IO1.t ImageIO.img) (comp : IO2.t ImageIO.img)
?(antialiasing = false) ?(outputDiffMask = false) ?(diffLines = false)
?(diffPixel : int * int * int = redPixel) ?(threshold = 0.1)
?(ignoreRegions = []) () =
?diffPixel ?(threshold = 0.1) ?(ignoreRegions = []) () =
let maxDelta = maxYIQPossibleDelta *. (threshold ** 2.) in
let diffPixel = match diffPixel with Some x -> x | None -> redPixel in
let diffOutput =
match outputDiffMask with
| true -> IO1.makeSameAsLayout base
| false -> base
in
let diffPixelQueue = Queue.create () in

let diffCount = ref 0 in
let diffLinesStack = Stack.create () in
let countDifference x y =
diffPixelQueue |> Queue.push (x, y);
incr diffCount;
IO1.setImgColor ~x ~y diffPixel diffOutput;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmtrKovalenko
Setting the diff color directly can break the antialiasing detection in some cases.

When diffMask is off, we are changing the pixel color on the base image directly, which we might use later to determine antialiased pixels.
(Remember, that we look at the surrounding 8 pixels to detect antialiasing). So we do look at "past" pixels, which with this change might not be the original color anymore.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the output image is a totally new buffer copied from the original image and not the same source.

This queue is having a lot of memory overhead for every run

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think on line 30 its the same buffer we use for comparison. We only create a new buffer, when "outputDiffMask" is true.
I might be wrong though. Its been a long time since i looked at this code 😄

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks need to check this again

if
diffLines
&& (diffLinesStack |> Stack.is_empty || diffLinesStack |> Stack.top < y)
Expand Down Expand Up @@ -74,26 +80,14 @@ module MakeDiff (IO1 : ImageIO.ImageIO) (IO2 : ImageIO.ImageIO) = struct
else incr x
done;

let diffCount = diffPixelQueue |> Queue.length in
(if diffCount > 0 then
let r, g, b = diffPixel in
let a = (255 land 255) lsl 24 in
let b = (b land 255) lsl 16 in
let g = (g land 255) lsl 8 in
let r = (r land 255) lsl 0 in
let diffPixel = Int32.of_int (a lor b lor g lor r) in
diffPixelQueue
|> Queue.iter (fun (x, y) ->
diffOutput |> IO1.setImgColor ~x ~y diffPixel));

let diffPercentage =
100.0 *. Float.of_int diffCount
100.0 *. Float.of_int !diffCount
/. (Float.of_int base.width *. Float.of_int base.height)
in
(diffOutput, diffCount, diffPercentage, diffLinesStack)
(diffOutput, !diffCount, diffPercentage, diffLinesStack)

let diff (base : IO1.t ImageIO.img) (comp : IO2.t ImageIO.img) ~outputDiffMask
?(threshold = 0.1) ?(diffPixel = redPixel) ?(failOnLayoutChange = true)
?(threshold = 0.1) ~diffPixel ?(failOnLayoutChange = true)
?(antialiasing = false) ?(diffLines = false) ?(ignoreRegions = []) () =
if
failOnLayoutChange = true
Expand Down
11 changes: 7 additions & 4 deletions test/Test_Core.ml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ let _ =
PNG_Diff.compare img1 img2 ~outputDiffMask:false ~antialiasing:true
()
in
(expect.int diffPixels).toBe 38;
(expect.float diffPercentage).toBeCloseTo 0.095);
(expect.int diffPixels).toBe 46;
(expect.float diffPercentage).toBeCloseTo 0.115);
test "tests different sized AA images" (fun { expect; _ } ->
let img1 = loadImage "test/test-images/aa/antialiasing-on.png" in
let img2 =
Expand Down Expand Up @@ -58,14 +58,17 @@ let _ =

let _ =
describe "CORE: Diff Color" (fun { test; _ } ->
test "creates diff output image with custom diff color"
test "creates diff output image with custom green diff color"
(fun { expect; _ } ->
let img1 = Png.IO.loadImage "test/test-images/png/orange.png" in
let img2 =
Png.IO.loadImage "test/test-images/png/orange_changed.png"
in
let diffOutput, _, _, _ =
PNG_Diff.compare img1 img2 ~diffPixel:(0, 255, 0) ()
PNG_Diff.compare img1 img2
~diffPixel:
(Int32.of_int 4278255360 (*int32 representation of #00ff00*))
()
in
let originalDiff =
Png.IO.loadImage "test/test-images/png/orange_diff_green.png"
Expand Down
8 changes: 2 additions & 6 deletions test/Test_IO_PNG.ml
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,7 @@ let _ =

test "Correctly handles different encodings of transparency"
(fun { expect; _ } ->
let img1 =
loadImage "test/test-images/png/extreme-alpha.png"
in
let img2 =
loadImage "test/test-images/png/extreme-alpha-1.png"
in
let img1 = loadImage "test/test-images/png/extreme-alpha.png" in
let img2 = loadImage "test/test-images/png/extreme-alpha-1.png" in
let _, diffPixels, _, _ = Diff.compare img1 img2 () in
(expect.int diffPixels).toBe 0))
54 changes: 34 additions & 20 deletions test/node-binding.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,37 @@ const BINARY_PATH = path.resolve(

console.log(`Testing binary ${BINARY_PATH}`);

const options = {
__binaryPath: BINARY_PATH,
}

test("Outputs correct parsed result when images different", async (t) => {
const { reason, diffCount, diffPercentage } = await compare(
path.join(IMAGES_PATH, "donkey.png"),
path.join(IMAGES_PATH, "donkey-2.png"),
path.join(IMAGES_PATH, "diff.png"),
options
);

t.is(reason, "pixel-diff");
t.is(diffCount, 101841);
t.is(diffPercentage, 2.65077570347);
})

test("Correctly works with reduceRamUsage", async (t) => {
const { reason, diffCount, diffPercentage } = await compare(
path.join(IMAGES_PATH, "donkey.png"),
path.join(IMAGES_PATH, "donkey-2.png"),
path.join(IMAGES_PATH, "diff.png"),
{
__binaryPath: BINARY_PATH,
...options,
reduceRamUsage: true,
}
);

t.is(reason, "pixel-diff");
t.is(diffCount, 109861);
t.is(diffPercentage, 2.85952484323);
t.is(diffCount, 101841);
t.is(diffPercentage, 2.65077570347);
});

test("Correctly parses threshold", async (t) => {
Expand All @@ -37,14 +55,14 @@ test("Correctly parses threshold", async (t) => {
path.join(IMAGES_PATH, "donkey-2.png"),
path.join(IMAGES_PATH, "diff.png"),
{
threshold: 0.6,
__binaryPath: BINARY_PATH,
...options,
threshold: 0.5,
}
);

t.is(reason, "pixel-diff");
t.is(diffCount, 50332);
t.is(diffPercentage, 1.31007003768);
t.is(diffCount, 65357);
t.is(diffPercentage, 1.70114931758);
});

test("Correctly parses antialiasing", async (t) => {
Expand All @@ -53,14 +71,14 @@ test("Correctly parses antialiasing", async (t) => {
path.join(IMAGES_PATH, "donkey-2.png"),
path.join(IMAGES_PATH, "diff.png"),
{
...options,
antialiasing: true,
__binaryPath: BINARY_PATH,
}
);

t.is(reason, "pixel-diff");
t.is(diffCount, 108208);
t.is(diffPercentage, 2.8164996153);
t.is(diffCount, 101499);
t.is(diffPercentage, 2.64187393218);
});

test("Correctly parses ignore regions", async (t) => {
Expand All @@ -69,6 +87,7 @@ test("Correctly parses ignore regions", async (t) => {
path.join(IMAGES_PATH, "donkey-2.png"),
path.join(IMAGES_PATH, "diff.png"),
{
...options,
ignoreRegions: [
{
x1: 749,
Expand All @@ -83,7 +102,6 @@ test("Correctly parses ignore regions", async (t) => {
y2: 1334,
},
],
__binaryPath: BINARY_PATH,
}
);

Expand All @@ -95,9 +113,7 @@ test("Outputs correct parsed result when images different for cypress image", as
path.join(IMAGES_PATH, "www.cypress.io.png"),
path.join(IMAGES_PATH, "www.cypress.io-1.png"),
path.join(IMAGES_PATH, "diff.png"),
{
__binaryPath: BINARY_PATH,
}
options
);

t.is(reason, "pixel-diff");
Expand All @@ -110,9 +126,7 @@ test("Correctly handles same images", async (t) => {
path.join(IMAGES_PATH, "donkey.png"),
path.join(IMAGES_PATH, "donkey.png"),
path.join(IMAGES_PATH, "diff.png"),
{
__binaryPath: BINARY_PATH,
}
options
);

t.is(match, true);
Expand All @@ -125,12 +139,12 @@ test("Correctly outputs diff lines", async (t) => {
path.join(IMAGES_PATH, "diff.png"),
{
captureDiffLines: true,
__binaryPath: BINARY_PATH,
...options
}
);

t.is(match, false);
t.is(diffLines.length, 411);
t.is(diffLines.length, 402);
});

test("Returns meaningful error if file does not exist and noFailOnFsErrors", async (t) => {
Expand All @@ -139,8 +153,8 @@ test("Returns meaningful error if file does not exist and noFailOnFsErrors", asy
path.join(IMAGES_PATH, "not-existing.png"),
path.join(IMAGES_PATH, "diff.png"),
{
...options,
noFailOnFsErrors: true,
__binaryPath: BINARY_PATH,
}
);

Expand Down
Loading