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

accept buffers as input #53

Open
florianbepunkt opened this issue Jul 12, 2021 · 17 comments · May be fixed by #74
Open

accept buffers as input #53

florianbepunkt opened this issue Jul 12, 2021 · 17 comments · May be fixed by #74

Comments

@florianbepunkt
Copy link

I'm trying to integrate odiff into a third-party lib. It would be great if the compare function would also accept buffers instead of paths. This would save the round trip to write a buffer to disk in some use cases.

@eWert-Online
Copy link
Collaborator

This is definetly on my TODO list for things i want to implement, as I also need this for OSnap.
This does need to wait until #47 is merged though.

@web-padawan
Copy link

It would be great to have this feature. I'm personally interested in it because I would like to use odiff instead of pixelmatch in @web/test-runner-visual-regression.

@eWert-Online
Copy link
Collaborator

I am currently on my way to implement this.
#57 would make this a lot easier, as we would have a unified interface across all formats to implement the buffer IO into.
Sadly I had to take a detour to fix some Windows issues (which hopefully get resolved in the next few days).

@web-padawan
Copy link

web-padawan commented Sep 23, 2021

Thanks for letting me know. For now I've tried to write files as a workaround and it works, here is an example.
If possible, it would be great to also get the diff image as a buffer, so that we could write it separately.

@rosczja
Copy link

rosczja commented Dec 9, 2022

Hello, any news on this one? We would like to integrate this into our puppeteer workflow, where we are currently using pixelmatch.

@dmtrKovalenko
Copy link
Owner

dmtrKovalenko commented Dec 9, 2022

Unfortunately no news, need to sit and implement a feature from scratch, if you have interest in trying this out – feel free to open a PR

@rosczja
Copy link

rosczja commented Dec 9, 2022

Maybe I will get around checking this out. Any info to point me in the right direction?

@dmtrKovalenko
Copy link
Owner

dmtrKovalenko commented Dec 9, 2022

The idea is simple but implementation may be a bit tricky especially for C part. We need to accept stdin buffer with an encoded image, then decode it in memory and everything else odiff already has implemented.

(there is no reason to support raw images because there would be not so much benefit)

@rosczja
Copy link

rosczja commented Dec 9, 2022

Ok, yes sounds simple. But I am afraid I have no knowledge of OCAML or Reason. I will therefore probably not be able to help out.

@dmtrKovalenko
Copy link
Owner

I’ve been thinking about this but in fact I can not understand benefit of using especially buffer in your case.

You are getting encoded image from puppeteer it means that browser already made some work on encoding it. Why not to write the image in file system and after use odiff? You will be able to also provide screenshots as test artifacts after.

@rosczja
Copy link

rosczja commented Dec 12, 2022

We would save IO time I guess. But at least we could try to use the filesystem.

@dmtrKovalenko
Copy link
Owner

dmtrKovalenko commented Dec 12, 2022

You still want to produce the base as artifact I assume

@web-padawan
Copy link

web-padawan commented Dec 12, 2022

As mentioned above, I'm also interested in this feature. You can find our plugin source code here:

I'm not sure about potential IO gains but supporting Buffer would make things simpler for us.

@eWert-Online
Copy link
Collaborator

eWert-Online commented Dec 12, 2022

@dmtrKovalenko
I am also using a buffer io for pngs in osnap.

https://github.com/eWert-Online/OSnap/blob/master/lib/OSnap_Diff/ReadPng.c

I would be able to work on "upstreaming" it if you are interested.
I don't know if (and how well) the other io libraries are supporting buffers as input though.

@dmtrKovalenko
Copy link
Owner

Wow that is really cool, we can leverage this as well, thanks Torben!

@eWert-Online eWert-Online linked a pull request Dec 21, 2022 that will close this issue
@jasan-s
Copy link

jasan-s commented Jul 9, 2023

any progress on this?

@dmtrKovalenko
Copy link
Owner

@jasan-s there is a pr partially open but a bit abandoned. Any help or new PR is welcoem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants