Fix incorrect slice read in blocks #474
Conversation
targets a TypeError "TypeError: Cannot read properties of undefined (reading 'offset')" that was happening when: - a slice read was equal to a block size - reading last part of the data The fix removes the "one-off" bug: - top is the first byte of the next block that we don't want to read. - The last block we want to read (blockIdHigh) is found by knowing the last byte we want to read and divide it by the block size. - Moved blockedSource declaration inside each test for better isolation. (BlockedSource is stateful)
| } | ||
| const blockIdLow = Math.floor(slice.offset / this.blockSize); | ||
| const blockIdHigh = Math.floor(top / this.blockSize); | ||
| const blockIdHigh = Math.floor((top - 1) / this.blockSize); |
There was a problem hiding this comment.
The PR mention the bug was occurring when reading the last block and the blockSize is equals to slice.length.
For what I see, this change may also affect the behavior of reading the first block.
In a situation where slice.offset is 0 and blockSize is equals to slice.length, for the previous implementation, blockIdHigh would have a value of 1, with the new implementation it will have a value of 0.
There was a problem hiding this comment.
that's a great observation! It is a bug as well. It was documented with a test at https://github.com/geotiffjs/geotiff.js/pull/474/files#diff-492ba43e11ce778a6c52157c7795c3a45b40213fb11b00df31dbb469614b4bd1R1325-R1335
In case you describe, our data would fit exactly in one block - blockId 0.
There was a problem hiding this comment.
I've generalized this case in the description as reading a block that is equal to a block size (so originally the code would also fail if you try to read offset 2, blockSize 2 and slice length 2.. etc.)
|
Is there anything else needed to merge this PR? |
|
What else is needed? |
| const blockedSource = new BlockedSource(null, { blockSize: 2 }); | ||
| blockedSource.source = { fileSize: null, fetch: async () => [{ data: new Uint8Array(2).buffer, offset: 0 }] }; | ||
| const data = await blockedSource.fetch([{ offset: 0, length: 2 }]); | ||
| expect(data[0].byteLength).to.equal(2); |
There was a problem hiding this comment.
What is this it testing? To me, this kind of looks like
new Uint8Array(2).byteLength === 2There was a problem hiding this comment.
Maybe I should reword the title to Fetches all data in a single block?
new Uint8Array(2).byteLength === 2
it is like so, but jus as a coincidence. It equals 2 not because we defined a fetch like so, but because we did blockedSource.fetch([{...length: 2 }]).
(it also fails without the fix from the PR)
There was a problem hiding this comment.
Ahh okay makes sense, thank you! Yeah I think Fetches all data in a single block would make it a bit more clear, but it's not a huge deal either way.
What
Addresses: #374
targets a TypeError "TypeError: Cannot read properties of undefined (reading 'offset')" that was happening when:
Details
The fix removes the "one-off" bug:
Test improvements: