Skip to content

Draft: Add buffers as input #74

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Draft: Add buffers as input #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
424d537
WIP: add buffers as input
eWert-Online Dec 21, 2022
19517f9
read buffer input from stdin
eWert-Online Dec 23, 2022
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
85 changes: 74 additions & 11 deletions bin/Main.re
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,49 @@
open Odiff.ImageIO;
open Odiff.Diff;

let getIOModule = filename =>
let getTypeFromFilename = filename =>
Filename.extension(filename)
|> (
fun
| ".png" => ((module ODiffIO.Png.IO): (module ImageIO))
| ".png" => `png
| ".jpg"
| ".jpeg" => ((module ODiffIO.Jpg.IO): (module ImageIO))
| ".bmp" => ((module ODiffIO.Bmp.IO): (module ImageIO))
| ".tiff" => ((module ODiffIO.Tiff.IO): (module ImageIO))
| ".jpeg" => `jpg
| ".bmp" => `bmp
| ".tiff" => `tiff
| f => failwith("This format is not supported: " ++ f)
);

let getIOModule =
fun
| `png => ((module ODiffIO.Png.IO): (module ImageIO))
| `jpg => ((module ODiffIO.Jpg.IO): (module ImageIO))
| `bmp => ((module ODiffIO.Bmp.IO): (module ImageIO))
| `tiff => ((module ODiffIO.Tiff.IO): (module ImageIO));

type diffResult('output) = {
exitCode: int,
diff: option('output),
};

let readFromStdin = () => {
/* We use 65536 because that is the size of OCaml's IO buffers. */
let chunk_size = 65536;
let buffer = Buffer.create(chunk_size);
let rec loop = () => {
Buffer.add_channel(buffer, stdin, chunk_size);
loop();
};
try(loop()) {
| End_of_file => Buffer.contents(buffer)
};
Comment on lines +29 to +38
Copy link
Collaborator Author

@eWert-Online eWert-Online Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We are reading from stdin until there is no more data provided.
If we wanted to support both base and compare image as buffer inputs, we would need to implement a check for some kind of separator in here.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that is basically why I didn't implement this from scratch. Looks incredibly tough to correctly read stdin and avoid memory copying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean the performance is not bad. The only issue is knowing when the first image ends and the second one starts.
I will need to see what I can come up with to solve this problem.

};

let main =
(
img1Path,
img2Path,
img1,
img2,
img1Type,
img2Type,
diffPath,
threshold,
outputDiffMask,
Expand All @@ -31,13 +53,54 @@ let main =
antialiasing,
ignoreRegions,
) => {
module IO1 = (val getIOModule(img1Path));
module IO2 = (val getIOModule(img2Path));
let img1Type =
switch (img1Type) {
| `auto when img1 == "_" =>
Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be named stdin instead of auto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

auto is the one which automatically gets the type from the filename (the currently published behaviour). The explicit types (png, jpg, ...) are the types given with the stdin input.
I think we can get rid of this though, if we are reading in the format from the magic number.

failwith("--base-type has to be not auto, when using buffer as input")
| `auto => getTypeFromFilename(img1)
| `bmp => `bmp
| `jpg => `jpg
| `png => `png
| `tiff => `tiff
};

let img2Type =
switch (img2Type) {
| `auto when img2 == "_" =>
failwith(
"--compare-type has to be not auto, when using buffer as input",
)
| `auto => getTypeFromFilename(img2)
| `bmp => `bmp
| `jpg => `jpg
| `png => `png
| `tiff => `tiff
};

module IO1 = (val getIOModule(img1Type));
module IO2 = (val getIOModule(img2Type));

module Diff = MakeDiff(IO1, IO2);

let img1 = IO1.loadImage(img1Path);
let img2 = IO2.loadImage(img2Path);
let img1 =
switch (img1) {
| "_" =>
if (!stdoutParsableString) {
print_endline("Please provide the buffer for the base image:");
};
readFromStdin() |> IO1.loadImageFromBuffer;
| path => IO1.loadImageFromPath(path)
};

let img2 =
switch (img2) {
| "_" =>
if (!stdoutParsableString) {
print_endline("Please provide the buffer for the compare image:");
};
readFromStdin() |> IO2.loadImageFromBuffer;
| path => IO2.loadImageFromPath(path)
};

let {diff, exitCode} =
Diff.diff(
Expand Down
68 changes: 63 additions & 5 deletions bin/ODiffBin.re
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,21 @@
open Cmdliner;

let supported_formats = [
("jpg", `jpg),
("png", `png),
("bmp", `bmp),
("tiff", `tiff),
];

let underscore_or = ((parse, print)) => {
let parse = s =>
switch (s) {
| "_" => `Ok(s)
| s => parse(s)
};
(parse, print);
};

let diffPath =
Arg.(
value
Expand All @@ -10,15 +26,55 @@ let diffPath =
let base =
Arg.(
value
& pos(0, file, "")
& info([], ~docv="BASE", ~doc="Path to base image")
& pos(0, underscore_or(non_dir_file), "_")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are now able to add either a file or an _ (underscore) to accept a raw image buffer.
I explicitly added the non_dir_file converter, so directories are not a valid input.

& info(
[],
~docv="BASE",
~doc=
"Path to base image (or \"_\" when you want to provide the base image from stdin)",
)
);

let baseType =
Arg.(
value
& opt(enum([("auto", `auto), ...supported_formats]), `auto)
& info(
["base-type"],
~docv="FORMAT",
~doc=
Printf.sprintf(
"The type of the base image (required to be not auto when a buffer is used as input).\nSupported values are: auto,%s",
supported_formats |> List.map(fst) |> String.concat(","),
),
)
Comment on lines +38 to +50
Copy link
Collaborator Author

@eWert-Online eWert-Online Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You are now able to explicitly set the type of the provided input. This is required for buffer inputs.
We could also try to read the type from the images header, but I think this would be too much work (and it would hurt the performance).

);

let comp =
Arg.(
value
& pos(1, file, "")
& info([], ~docv="COMPARING", ~doc="Path to comparing image")
& pos(1, underscore_or(non_dir_file), "_")
& info(
[],
~docv="COMPARING",
~doc=
"Path to comparing image (or \"_\" when you want to provide the comparing image from stdin)",
)
);

let compType =
Arg.(
value
& opt(enum([("auto", `auto), ...supported_formats]), `auto)
& info(
["compare-type"],
~docv="FORMAT",
~doc=
Printf.sprintf(
"The type of the comparing image (required to be not auto when a buffer is used as input).\nSupported values are: auto,%s",
supported_formats |> List.map(fst) |> String.concat(","),
),
)
);

let threshold = {
Expand Down Expand Up @@ -113,14 +169,16 @@ let cmd = {
let man = [
`S(Manpage.s_description),
`P("$(tname) is the fastest pixel-by-pixel image comparison tool."),
`P("Supported image types: .png, .jpg, .jpeg, .bitmap"),
`P("Supported image types: .png, .jpg, .jpeg, .bmp, .tiff"),
];

(
Term.(
const(Main.main)
$ base
$ comp
$ baseType
$ compType
$ diffPath
$ threshold
$ diffMask
Expand Down
14 changes: 9 additions & 5 deletions bin/node-bindings/odiff.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
export type ODiffOptions = Partial<{
/** The image type of the base image. This has to be set to the corresponding image format when using a buffer as input */
baseImageType?: 'filepath' | 'jpg' | 'png' | 'bmp' | 'tiff' = 'filepath';
/** The image type of the compare image. This has to be set to the corresponding image format when using a buffer as input */
compareImageType?: 'filepath' | 'jpg' | 'png' | 'bmp' | 'tiff' = 'filepath';
Comment on lines +2 to +5
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really like, that you are currently able to provide a buffer as input and at the same time add "filepath" as the image type.
Maybe this can be prevented with some typescript magic, but I am really not experienced in ts.

Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way too quickly read codec info from buffer? I understand why this is done looks like it may hurt performance to add a lookup over all the codecs.

I think this can be done by simply duplicating overrides like this

declare function compare(
  baseImage: Buffer,
  compareImage: buffer,
  baseCodec: "..",
  compareCodec: ".."
)

declare function compare(
  baseImage: string,
  compareImage: string,
)

and then in function check the type of baseImage && compareImage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can of course try to read in the first 4 bytes and identify the magic number. Here is a list of the valid magic numbers for different image formats: https://gist.github.com/leommoore/f9e57ba2aa4bf197ebc5.

TIFF and JPG do seem to have multiple valid ones, But I think it would be simple (and probably reasonably fast) to check it.

/** Color used to highlight different pixels in the output (in hex format e.g. #cd2cc9). */
diffColor: string;
/** Output full diff image. */
Expand All @@ -21,24 +25,24 @@ export type ODiffOptions = Partial<{
}>;

declare function compare(
basePath: string,
comparePath: string,
baseImage: string | Buffer,
compareImage: string | Buffer,
diffPath: string,
options?: ODiffOptions
): Promise<
| { match: true }
| { match: false; reason: "layout-diff" }
| { match: false; reason: 'layout-diff' }
| {
match: false;
reason: "pixel-diff";
reason: 'pixel-diff';
/** Amount of different pixels */
diffCount: number;
/** Percentage of different pixels in the whole image */
diffPercentage: number;
}
| {
match: false;
reason: "file-not-exists";
reason: 'file-not-exists';
/** Errored file path */
file: string;
}
Expand Down
54 changes: 49 additions & 5 deletions bin/node-bindings/odiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @ts-check
const path = require("path");
const { execFile } = require("child_process");
var stream = require('stream');

function optionsToArgs(options) {
let argArray = ["--parsable-stdout"];
Expand All @@ -26,6 +27,18 @@ function optionsToArgs(options) {
const [option, value] = optionEntry;

switch (option) {
case "baseImageType":
if(value !== "filepath") {
setArgWithValue("base-type", value);
}
break;

case "compareImageType":
if(value !== "filepath") {
setArgWithValue("compare-type", value);
}
break;

case "failOnLayoutDiff":
setFlag("fail-on-layout", value);
break;
Expand Down Expand Up @@ -90,7 +103,7 @@ function parsePixelDiffStdout(stdout) {
const CMD_BIN_HELPER_MSG =
"Usage: odiff [OPTION]... [BASE] [COMPARING] [DIFF]\nTry `odiff --help' for more information.\n";

async function compare(basePath, comparePath, diffOutput, options = {}) {
async function compare(baseImage, compareImage, diffOutput, options = {}) {
return new Promise((resolve, reject) => {
let producedStdout, producedStdError;

Expand All @@ -99,9 +112,23 @@ async function compare(basePath, comparePath, diffOutput, options = {}) {
? options.__binaryPath
: path.join(__dirname, "bin", "odiff");

execFile(
let baseImageArg = baseImage;
let baseImageIsBuffer = false;
if(options.baseImageType && options.baseImageType !== "filepath") {
baseImageArg = "_";
baseImageIsBuffer = true;
}

let compareImageArg = compareImage;
let compareImageIsBuffer = false;
if(options.compareImageType && options.compareImageType !== "filepath") {
compareImageArg = "_";
compareImageIsBuffer = true;
}

const cp = execFile(
binaryPath,
[basePath, comparePath, diffOutput, ...optionsToArgs(options)],
[baseImageArg, compareImageArg, diffOutput, ...optionsToArgs(options)],
(_, stdout, stderr) => {
producedStdout = stdout;
producedStdError = stderr;
Expand All @@ -128,7 +155,7 @@ async function compare(basePath, comparePath, diffOutput, options = {}) {
).replace(CMD_BIN_HELPER_MSG, "");

const noFileOrDirectoryMatches = originalErrorMessage.match(
/no\n\s*`(.*)'\sfile or\n\s*directory/
/no\n\s*`(.*)'\sfile/
);

if (options.noFailOnFsErrors && noFileOrDirectoryMatches[1]) {
Expand All @@ -153,7 +180,24 @@ async function compare(basePath, comparePath, diffOutput, options = {}) {
);
break;
}
});
})

if(baseImageIsBuffer) {
cp.stdin?.write(baseImage, 'binary');
cp.stdin?.end();
}

if(compareImageIsBuffer) {
cp.stdin?.write(compareImage, 'binary');
cp.stdin?.end();
}

// const stdinStream = new stream.Readable();
// if(compareImageIsBuffer) {
// stdinStream.push(compareImage, 'binary');
// stdinStream.push("\n");
// stdinStream.pipe(cp?.stdin, { end: false });
// }
});
}

Expand Down
6 changes: 5 additions & 1 deletion io/bmp/Bmp.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@ type data = Array1.t(int32, int32_elt, c_layout);
module IO: Odiff.ImageIO.ImageIO = {
type t = data;

let loadImage = (filename): Odiff.ImageIO.img(t) => {
let loadImageFromPath = (filename): Odiff.ImageIO.img(t) => {
let (width, height, data) = ReadBmp.load(filename);

{width, height, image: data};
};

let loadImageFromBuffer = (buffer): Odiff.ImageIO.img(t) => {
failwith("Not implemented");
};

[@inline]
let readDirectPixel = (~x: int, ~y: int, img: Odiff.ImageIO.img(t)) => {
let image: data = img.image;
Expand Down
6 changes: 5 additions & 1 deletion io/jpg/Jpg.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module IO = {
buffer,
};

let loadImage = (filename): Odiff.ImageIO.img(t) => {
let loadImageFromPath = (filename): Odiff.ImageIO.img(t) => {
let (width, height, data, buffer) = ReadJpg.read_jpeg_image(filename);

{
Expand All @@ -22,6 +22,10 @@ module IO = {
};
};

let loadImageFromBuffer = (buffer): Odiff.ImageIO.img(t) => {
failwith("Not implemented");
};

let readDirectPixel = (~x, ~y, img: Odiff.ImageIO.img(t)) => {
Array1.unsafe_get(img.image.data, y * img.width + x);
};
Expand Down
Loading