[Bug] Different behaviour between TS and AS

[LinusU]

I'm currently in the process of porting my BMP decoding library from JS to AS, and I have a version that is compiling with both tsc and asc at the moment. Everything works when using the output from tsc, but when using the output from asc I'm running into a strange decoding artifact.

The code is in this branch: https://github.com/LinusU/decode-bmp/tree/wasm

For some reason, the decoded file comes out as this when using the output from asc:

The tsc version correctly decodes the image like this:

I'm using the branch with the changes in #316, so potentially something in the DataView is wrong. But since the metadata (magic byte, width, height, color depth, header size) is correct, and that is the only part read with DataView, I think that the problem is elsewhere.

I'll continue to investigate to pin down the exactly where the problem is, but wanted to open early to hopefully get some help getting in the right direction ☺️

[MaxGraey]

Hard to tell without fork. Could you share this as fiddle on WebAssembly Studio (with manually copy DataView)?

Also this should use >>> probably

Also DataView still untested and could contain issues

[MaxGraey]

I tried you branch and all tests are passed. Could you explain how reproduce this issue?

[LinusU]

I tried you branch and all tests are passed. Could you explain how reproduce this issue?

Sorry, should have included that from the beginning, missed it...

# Clone the repo
git clone https://github.com/LinusU/decode-bmp.git
cd decode-bmp

#Switch branch
git checkout wasm

# Install deps
npm install

# Build with tsc and asc
npm run prepare

# Test tsc
./node_modules/.bin/mocha -b

# Test asc
USE_WASM=1 ./node_modules/.bin/mocha -b

USE_WASM=1 will make index.js load the .wasm file instead of the .js file

[MaxGraey]

Got it. So it seems stride shifted on 18 pixels. Hmm

[dcodeIO]

When looking at that file I also notice a couple red, green and blue pixels at the bottom left, which might actually be header bytes, possibly indicating that the entire output buffer is shifted by some offset.

[MaxGraey]

So check what is actual value for headerSize in wasm version.

[LinusU]

headerSize is 54 in wasm version, the same as in the JS version

edit: (totally unrelated: is there a better way to check this than changing the function to return headerSize and the console.loging that just after the call to the function from JS? 😂)

[LinusU]

Looking at the subarray implementation, I'm trying to understand why it doesn't simply construct a new Uint8Array with just the byteOffset and byteLength changed, isn't that how it should work?

Current implementation:

export abstract class TypedArray<T,V> {
  // ...

  @inline
  subarray(begin: i32 = 0, end: i32 = i32.MAX_VALUE): TypedArray<T,V> {
    var length = this.length;
    if (begin < 0) begin = max(length + begin, 0);
    else begin = min(begin, length);
    if (end < 0) end = max(length + end, begin);
    else end = max(min(end, length), begin);
    var slice = memory.allocate(offsetof<this>());
    store<usize>(slice, this.buffer, offsetof<this>("buffer"));
    store<i32>(slice, begin << alignof<T>(), offsetof<this>("byteOffset"));
    store<i32>(slice, end << alignof<T>(), offsetof<this>("byteLength"));
    return changetype<this>(slice);
  }

  // ...
}

edit: I now understand how this works, hmmmmm 🤔 it's a bit annoying that we cannot overload the Uint8Array constructor, I was actually running into that problem earlier, when I saw that only new Uint8Array(length) was implemented...

[MaxGraey]

Try this branch: https://github.com/MaxGraey/assemblyscript/tree/fix-subarray
now in master

[LinusU]

Tried with f41c5d9 which is #316 rebased on top of latest master, and the same error is still present in the first test case.

I think that it fixed three crashes in the other test cases though, so it seems like a step in the right direction 👍

[LinusU]

    var slice = memory.allocate(offsetof<this>());
    store<usize>(slice, this.buffer, offsetof<this>("buffer"));
    store<i32>(slice, begin << alignof<T>(), offsetof<this>("byteOffset"));
    store<i32>(slice, (end - begin) << alignof<T>(), offsetof<this>("byteLength"));
    return changetype<this>(slice);

I'm probably missing something here, but why do we bitshift begin by alignof<T>()? alignof<T>() should be 1 since this is an Uint8Array, and 54 << 1 is 108. The data was off by 18 pixels, and 18 pixels with 24 bits per pixels is 54 bytes 🤔

[LinusU]

Haha! I've tracked it down!

The problem is when running subarray on a subarray 😄

  const bitmapData = source.subarray(headerSize, source.byteLength)
  return bitmapData[0]

☝️ this returns 0, which is what is at offset 54

  const bitmapData = source.subarray(headerSize, source.byteLength)
  const test = bitmapData.subarray(0, 10368)
  return test[0]

☝️ this returns 66, which is what is at offset 0 in the original buffer!

Seems like the problem is that the existing byteOffset isn't being taken into account! Submitting a PR now ☺️

edit: #329

[LinusU]

@dcodeIO unfortunately, it's not that easy 😄

➜  decode-bmp git:(wasm) ✗ USE_WASM=1 mocha -b


  ✓ decodes w3c_home.bmp
  ✓ decodes w3c_home_2.bmp
  1) decodes w3c_home_256.bmp

  2 passing (19ms)
  1 failing

  1) decodes w3c_home_256.bmp:
     Error: abort: <yet unknown> at <yet unknown>:40:63
      at abort (node_modules/assemblyscript/lib/loader/index.js:35:11)
      at ~lib/internal/typedarray/TypedArray<u8,u32>#__get (wasm-function[24]:20)
      at src/index/Bitmap#get (wasm-function[25]:228)
      at src/index/decodePaletteBmp (wasm-function[27]:230)
      at src/index/decodeBmp (wasm-function[29]:324)
      at Object.wrap [as decodeBmp] (node_modules/assemblyscript/lib/loader/index.js:198:12)
      at decodeBmp (index.js:14:32)
      at Context.it (test.js:32:20)

I don't have time to investigate more at the moment, but can hopefully get to it tonight, would you mind keeping this issue open? ❤️