equinor / segyio

Fast Python library for SEGY files.
Other
476 stars 214 forks source link

`bswap24vec` related question. #469

Closed aleshaleksey closed 4 years ago

aleshaleksey commented 4 years ago

This is not so much a problem as a "Hmm, that's funny. Maybe I'll learn something if I ask a question" kind of issue.

So there are a bunch of functions: bswap64vec, bswap32vec, bswap24vec and bswap16vec. in "lib/src/segy.c" (approximately lines 1778), which is fine. I was looking at them and noticed that bswap24vec was a little bit (no pun intended) odd. Obviously in "normal" computing 24-bit (3-byte) numerical types are something of a unicorn, so that explains why the bswap24vec function is different from the rest of the bswap functions (aka 24-bit (3-byte) numerical types are not particularly "standard").

However, here we have the use of sizeof(int_whatever_t) which returns the number of bytes for int_whatever_t in most of the functions, (which would be 2, 4 and 8 for int16_t, int32_t and int64_t respectively according to C Playground), but a straight up "24" is used in bswap24vec which is the bit count and not the byte count.

So in ( char* xs = begin; xs != end; xs += sizeof(int32_t) I believe the pointer is being incremented by 4 bytes each time, while in (char* xs = begin; xs != end; xs += 24) it is being incremented by 24? Is this Ok? Does bswap24vec end up skipping a bunch of bytes? If not, would someone be able to point me to what I am misunderstanding (yes, everything, of course, but...).

static int bswap32vec( void* vec, long long len ) {
    char* begin = (char*) vec;
    char* end = (char*) begin + len * sizeof(int32_t);

    for( char* xs = begin; xs != end; xs += sizeof(int32_t) ) {
        uint32_t v;
        memcpy( &v, xs, sizeof(int32_t) );
        v = bswap32( v );
        memcpy( xs, &v, sizeof(int32_t) );
    }

    return SEGY_OK;
}

static int bswap24vec( void* vec, long long len ) {
    char* begin = (char*) vec;
    char* end = (char*) begin + len * 24;

    for (char* xs = begin; xs != end; xs += 24) {
        uint8_t bits[3];
        memcpy(bits, xs, sizeof(bits));
        uint8_t tmp = bits[0];
        bits[0] = bits[2];
        bits[2] = tmp;
        memcpy(xs, bits, sizeof(bits));
    }

    return SEGY_OK;
}

Again, I would like to apologise for wasting your time if I am misunderstanding the nature of the code here.

jokva commented 4 years ago

You've stumbled upon a bug, which apparently is not caught by our tests :---)

It shouldn't be 24, it should be 3. Sharp edges and all. Unfortunately I don't have to to fix it for a couple of weeks, but I'll get to designing a test for it, and fix the program.

Thanks!

jokva commented 4 years ago

Thanks for the report! I've added (mostly flipped the switch) libsegyio support for int24, and fixed the bswap bug you found.