rxing-core / rxing

cRustacean Crossing
https://crates.io/crates/rxing
Apache License 2.0
194 stars 19 forks source link

Join forces with zxing-cpp? #37

Closed axxel closed 7 months ago

axxel commented 9 months ago

I saw that you decided to port some of my improvements to the QRCode reader over to your rust port. I take that as a compliment, so thank you :). I also noticed that the development of this port seems to have effectively stalled around 6 months ago? I just released version 2.2 including support for rMQR codes, in case you are interested.

That said: I wanted to ask you whether you'd consider bringing your rust experience to the table and instead of reimplementing the whole library and keep on maintaining that, why not write a rust wrapper for zxing-cpp and contributing that to my project? It is obviously not nearly as much fun but also not nearly as much work ;).

There are already Android, iOS, C, Python, Flutter, WASM and WinRT wrappers available. I'd be curious to hear your thoughts. If you decide to stick to your port and want to dive in even more, I'd recommend to look at the zxing-cpp implementation of all the linear barcodes. That code has not much in common with the original Java version anymore and is around 10-20x faster.

hschimke commented 9 months ago

An interesting proposal. Let me think about it a bit.

You've done really amazing work on zxing-cpp, by the way!

SteveCookTU commented 8 months ago

This is a great idea!

The only downside is that cpp -> rust is pretty limited currently. (For example cpp array types aren't even implemented yet)

The easiest path around this is a 3 step binding of rust -> c -> cpp and vice-versa. The autocxx crate does this under the hood iirc but makes it look like it directly communicates with cpp. I could be wrong thinking that though.

axxel commented 8 months ago

Googling a bit let me come to the conclusion that either a "manual" wrapper of the existing C-API via ffi with some some proper/save/native rust layer on top or maybe cxx.rs (was that what you meant by "autocxx"?).

SteveCookTU commented 8 months ago

Googling a bit let me come to the conclusion that either a "manual" wrapper of the existing C-API via ffi with some some proper/save/native rust layer on top or maybe cxx.rs (was that what you meant by "autocxx"?).

Sorry forgot to link the repo. autocxx wraps around cxx and uses a bindgen to automatically create the interfaces needed to call CPP. I haven't really dug much into the regular cxx crate since autocxx just made things simple for my use case.

SteveCookTU commented 8 months ago

I have created a repo to demonstrate wrapping zxing-cpp in Rust. It was very quickly thrown together so it may look a little messy being all in one file.

https://github.com/SteveCookTU/zxing-cpp2rs

I have also only built it on Windows so you may need to play around with it when testing on another OS. If anything, -std:c++17 in the build.rs will need to be changed to -stc=c++17 or something along those lines. Wasn't sure if the latter was compatible for MSVC

hschimke commented 7 months ago

I’m pleased to see the work on the zxing-cpp rust wrapper progressing. For now I’m going to close this thread as “wont-fix” because I think the two libraries can exist in harmony for a while longer. In my opinion there is value in having a “pure-rust” implementation, though the current iteration of rxing is not as mature as zxing-cpp, the value of having options seems high at the moment.

@axxel If I had realized how much work you had been putting into zxing-xpp i probably would have ported it instead of the java version, originally.

axxel commented 7 months ago

Thanks for sharing your thoughts.

In my opinion there is value in having a “pure-rust” implementation

Can you explain were you see that value? This was exactly what I questioned in the first place. The fact that one option now has support for Telepen while the other has support for DXFilmEdge is in fact not of value to a Rust user who just want to get the job done.

But I totally get that abandoning ones work is not an attractive proposal. So it looks like we end up competing after all... ;)

hschimke commented 7 months ago

I had a couple thoughts on this:

  1. Ease of debugging
  2. Compile time optimizations
  3. Environments where interoperability isn't available (such as WASM)
  4. As you said, abandoning a project isn't always fun

But, that's all my bias, so I asked Reddit about their opinion. They were pretty split, really. The primary opinion is that they would use the best available option: that is, pick the C++ library with a wrapper that was battle tested over the new rust library (in this case, zxing-cpp is more mature and tested). The other side of that, however, was there were a lot of people who preferred the portability of all-rust implementations, the availability of WASM, and the ability to directly click through to the source of a library to see what it was doing and/or write patches for it. In-language auditable memory safety was also a popular point people brought up, since rust treats all FFI code as unsafe. At least two people said: "I'd rather use C++"

Send me a mail -> henry@rxing.org and I'll forward you the thread, I thought it was pretty interesting actually.

axxel commented 7 months ago

I had a couple thoughts on this:

1. Ease of debugging

Well, if you have a 1:1 mapping of c++ to Rust code, so that they both have the same bugs, that is technically true. But in experience only few people go as far as debug this code base. Mostly I just receive mediocre quality bug reports. And in general it is indeed sensible for people to not spent time first understanding the code when I can most certainly find any issue faster, having written most of it.

2. Compile time optimizations

I don't understand what you mean here. Are you arguing that Rust is fundamentally more performant than C++?

3. Environments where interoperability isn't available (such as WASM)

Well, I'd argue that someone interested in a WASM port/wrapper for zxing-cpp, they can simply use one of the two that are already provided.

4. As you said, abandoning a project isn't always fun

Absolutely.

Send me a mail -> henry@rxing.org and I'll forward you the thread, I thought it was pretty interesting actually.

I did. Thanks for the offer.

hschimke commented 7 months ago

Well, if you have a 1:1 mapping of c++ to Rust code, so that they both have the same bugs, that is technically true. But in experience only few people go as far as debug this code base. Mostly I just receive mediocre quality bug reports. And in general it is indeed sensible for people to not spent time first understanding the code when I can most certainly find any issue faster, having written most of it.

Fair enough, I've received a handful of great PRs, but yes, most people just provide vague bug reports and then ghost me in terms of validation.

Are you arguing that Rust is fundamentally more performant than C++?

Oh no! Not at all. Rust is at best as performant as C++. I am just referring to optimizations carried out on rust code by the compiler and linker. I'm not saying that rust will be more performant, I'm saying that in certain situations having all the code built at once offers a few possibilities for improvement overall. Same goes for profile driven optimizations, which should allow for some clever optimization when building all at once. These are almost certainly never going to be very big, and being realistic, right now zxing-cpp is more efficient than rxing even with those potential benefits.

Well, I'd argue that someone interested in a WASM port/wrapper for zxing-cpp, they can simply use one of the two that are already provided.

In some cases, absolutely. But, if they are building a larger project in rust and simply interacting with it through wasm, this would allow them to build that project in rust instead of c++. If they are just using the NPM package that has a wrapper for the wasm then I agree fully, I only think this would matter if we were talking about a larger project in which the library was only one part.

axxel commented 7 months ago

optimization...

Understood. In this particular case, I'm convinced that there is virtually nothing to gain. Potentially my choices regarding the passing of strings via the C-API is desasterous in terms of performance because of the extra memcopies involved (which I opted for to make the C-API as clean and consistent as I was able to). But those copies of a few hundred bytes is completely dwarfed by the image processing work that needs to happens before each of those string copies.

WASM...

I understand that argument now (also after reading through the reddit thread). That is actually a real blocker for someone with this particular feature request. Or you have to tie two separate WASM assemblies together, which would also somehow work, I assume.

Some thoughts I have after reading that reddit discussion: