Neopallium / jpeg2k

Safe wrapper for openjpeg-sys.
MIT License
6 stars 7 forks source link

Stream from slice is totally broken #2

Closed s3bk closed 2 years ago

s3bk commented 2 years ago

There is a small misunderstanding going on ...

C assumes the end of the stream is signaled by -1 Rust uses 0.

Result: It never returns.

Will submit a PR soon.

Neopallium commented 2 years ago

I looked into the openjp2 code. It looks like the read function returns OPJ_SIZE_T which is size_t an unsigned integer. The test for end of stream casts -1 to OPJ_SIZE_T: https://github.com/uclouvain/openjpeg/blob/6a29f5a9e3a1e2dbf1e3df22b7e449bc1db20b5c/src/lib/openjp2/cio.c#L327

I think that is the same as usize::MAX. So the Rust streams need to return usize::MAX instead of 0.

s3bk commented 2 years ago

Yep. I have it working and will make the PR tomorrow.

s3bk commented 2 years ago

Do you know anything about opj_decoder_set_strict_mode ? I got a missing symbol when compiling to wasi.

Neopallium commented 2 years ago

Yes. That was added recently to Openjpeg. It allows support for decoding truncated jp2 images, which is used for progressive downloading (showing low-res image before the full image has downloaded).

Looks like it hasn't been included in a Openjpeg release yet.

Neopallium commented 2 years ago

My fork of openjpeg-sys has support for strict mode: https://github.com/Neopallium/openjpeg-sys

s3bk commented 2 years ago

I am using your fork. It compiles and runs fine for x86-64. However the wasi build has this one unresolved symbol.

s3bk commented 2 years ago

Would you mind if I make a PR with wasi support that feature-gates it?

Neopallium commented 2 years ago

Yes. That would be cool.

Neopallium commented 2 years ago

If you open the PR, I can try helping debugging the wasm build issue.

Neopallium commented 2 years ago

The problem might be with the build.rs script for openjpeg-sys.

Neopallium commented 2 years ago

I was able to compile the examples as wasm32-wasi using this command:

CC="${WASI_SDK_PATH}/bin/clang -D_WASI_EMULATED_PROCESS_CLOCKS -lwasi-emulated-process-clocks --sysroot=${WASI_SDK_PATH}/share/wasi-sysroot" cargo build --no-default-features --features image --target=wasm32-wasi --examples

Using wasi-sdk version 14 from here: https://github.com/WebAssembly/wasi-sdk/releases

Would be nice to have an easier wasi feature gate for compiling.

I didn't get any linking errors about the .._set_strict_mode symbol.

s3bk commented 2 years ago

It compiles fine. But you can't run it because the symbol is not defined. It turns into an import that you have to provide. I suppose one could define a no-op for it.

Neopallium commented 2 years ago

How are you running the examples? I am trying with wasmtime and just get Error: File not found: "test.j2k" errors.

Neopallium commented 2 years ago
wasmtime ./target/wasm32-wasi/debug/examples/dump_jp2.wasm ./tests/test.j2k
Neopallium commented 2 years ago

Got it to run:

wasmtime --dir=./ ./target/wasm32-wasi/debug/examples/dump_jp2.wasm ./tests/test.j2k

Had to give it access to the current folder.

s3bk commented 2 years ago

Maybe I compiled it with the wrong openjpeg code.

s3bk commented 2 years ago

struct Data {
    wasi: WasiCtx,
    api: ApiData,
}

    let mut linker = Linker::new(&engine);
    wasmtime_wasi::add_to_linker(&mut linker, |d: &mut Data| &mut d.wasi).unwrap();
    let wasi = WasiCtxBuilder::new()
        .inherit_stderr()
        .inherit_stdout()
        .inherit_args().unwrap()
        .build();

    let api = ApiData::default();
    let mut store = Store::new(&engine, Data { wasi, api });

    let (api, _instance) = Api::instantiate(&mut store, &module, &mut linker, |d: &mut Data| &mut d.api)
        .unwrap();

    api.jp2k(&mut store, data);
Neopallium commented 2 years ago

It might be pulling in your systems openjpeg?

Also try:

cargo clean
cargo update
s3bk commented 2 years ago

and the following api.wit file

record image {
    buffer: list<u8>,
    width: u32,
    height: u32,
    bands: u32,
}

jp2k: function(data: list<u8>) -> expected<image, list<u8>>
jbig2: function(data: list<u8>) -> expected<image, list<u8>>
s3bk commented 2 years ago

.. and the following code using the jpeg2k crate

wit_bindgen_rust::export!("../api.wit");

fn jpeg2k_image(data: &[u8]) -> Result<api::Image, jpeg2k::error::Error> {
    let image = jpeg2k::Image::from_bytes(data)?;
    let bands = image.num_components();
    let jpeg2k::ImageData { width, height, data, .. } = image.get_pixels(None)?;

    Ok(api::Image {
        buffer: data, width, height, bands
    })
}

pub struct Api;
impl api::Api for Api {
    fn jp2k(data: Vec<u8>,) -> Result<api::Image, Vec<u8>> {
        jpeg2k_image(&data).map_err(|e| e.to_string().into())
    }
    fn jbig2(data: Vec<u8>) -> Result<api::Image, Vec<u8>> {
        let mut ctx = jbig2::Context::new(&data)?;
        let jbig2::Image { buf, width, height } = ctx.get_page()?;
        Ok(api::Image { buffer: buf, width, height, bands: 1 })
    }
}
Neopallium commented 2 years ago

That last block of code with the jpeg2k_image(.....) rust code. Does it build directly against the jpeg2k crate?

s3bk commented 2 years ago

against a local copy

Neopallium commented 2 years ago

A short term fix would be to disable all references to ..._set_strict_mode in jpeg2k and openjpeg-sys. Using both as a local copy.

Try that to just see if it compiles and runs.

s3bk commented 2 years ago

It looks like the vendor submodule is incorrect locally..

s3bk commented 2 years ago

Yes, I disabled that yesterday and deployed it. I am now trying to make it so it does not depend on a local copy.

Neopallium commented 2 years ago

My local openjpeg-sys vender folder is at commit:

commit 3db5f0e48cb841e5394929714e9c1b3ac7211049 (HEAD -> partial_decode_mode, origin/partial_decode_mode)
Author: Robert G. Jakabosky <rjakabosky+neopallium@neoawareness.com>
Date:   Sun Jan 30 00:18:32 2022 +0800
cd openjpeg-sys/vendor/
git log | head
s3bk commented 2 years ago

Mine is not..

Neopallium commented 2 years ago

also check in your jpeg2k folder:

cargo tree | grep openjpeg
├── openjpeg-sys v1.0.6 (https://github.com/Neopallium/openjpeg-sys#ef4dba07)
Neopallium commented 2 years ago

If the commit hash isn't ef4dba07, then try cargo update and check again.

s3bk commented 2 years ago

The submodule is the problem. All dependencies are local. I am trying to figure out how to change it.

Neopallium commented 2 years ago

Git submodules are a pain.

I think the command might be:

git submodule update
or
git submodule sync
Neopallium commented 2 years ago

I need to open a PR on the original openjpeg-sys repo for the .._set_strict_mode change. That repo has already updated their vendor folder, so it just needs the ffi symbol.

Neopallium commented 2 years ago

Your local openjpeg-sys might be forked from the original https://github.com/kornelski/openjpeg-sys

I opened a PR on that repo to add the strict mode support.

s3bk commented 2 years ago

Thanks!