Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One shot REMB magic check #1226

Closed

Conversation

Nemirtingas
Copy link
Contributor

@Nemirtingas Nemirtingas commented Jul 6, 2024

Proof of concept:
Test app:

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

bool is_little_endian()
{
    uint16_t value = 0x0001;
    return *((uint8_t*)(&value)) == 0x01;
}

unsigned int to_endian_agnostic(unsigned int value)
{
    return ((uint8_t*)&value)[0] | (((uint8_t*)&value)[1] << 8) | (((uint8_t*)&value)[2] << 16) | (((uint8_t*)&value)[3] << 24);
}

int main (int argc, char *argv[])
{
    static unsigned int z = to_endian_agnostic('BMER');

    union
    {
      unsigned char c[4];
      unsigned int i;
    } y;

    y.c[0] = 'R';
    y.c[1] = 'E';
    y.c[2] = 'M';
    y.c[3] = 'B';

    printf("Hello ! %08x, %08x: little endian: %ld\n", y.i, z, is_little_endian());

    if (y.i == z) {
        printf("REMB\n");
    }

    return 0;
}

Build:

aarch64-linux-gnu-g++ -g main.cpp -o a_aarch64.out

# wget 'https://releases.linaro.org/components/toolchain/binaries/7.5-2019.12/armeb-linux-gnueabi/gcc-linaro-7.5.0-2019.12-x86_64_armeb-linux-gnueabi.tar.xz'
# wget 'https://releases.linaro.org/components/toolchain/binaries/7.5-2019.12/armeb-linux-gnueabi/runtime-gcc-linaro-7.5.0-2019.12-armeb-linux-gnueabi.tar.xz'
# wget 'https://releases.linaro.org/components/toolchain/binaries/7.5-2019.12/armeb-linux-gnueabi/sysroot-glibc-linaro-2.25-2019.12-armeb-linux-gnueabi.tar.xz'
./gcc-linaro-7.5.0-2019.12-x86_64_armeb-linux-gnueabi/bin/armeb-linux-gnueabi-g++ main.cpp -g -o a_armeb.out
g++ -g main.cpp -o a.out

Test:
x86: ./a.out

Hello ! 424d4552, 424d4552: little endian: 1
REMB

aarch64: qemu-aarch64 -L /usr/aarch64-linux-gnu ./a_aarch64.out

Hello ! 424d4552, 424d4552: little endian: 1
REMB

arm big endian: qemu-armeb -L $(pwd)/sysroot-glibc-linaro-2.25-2019.12-armeb-linux-gnueabi/ ./a_armeb.out

Hello ! 52454d42, 52454d42: little endian: 0
REMB

@Nemirtingas
Copy link
Contributor Author

Probably could change the _id field to uint32 ?

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit like reinventing the wheel. If you want to precompute the id field as uint32_t in network byte order, it should be done with htonl().

However, I don't think changing this comparison would not make any difference performance-wise. A good compiler might optimize it already, and it doesn't look like a critical code path. Am I missing something?

@@ -17,11 +17,18 @@

#if RTC_ENABLE_MEDIA

static inline uint32_t makeEndianAgnosticValue(uint32_t value) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording "endian agnostic" sounds a bit strange. This function returns the value as little endian, irrelevant of the architecture.

Copy link
Contributor Author

@Nemirtingas Nemirtingas Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does return a value big or little endian, and not always little endian, as you can see in the proof of concept tests.

Copy link
Owner

@paullouisageneau paullouisageneau Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proof of concept prints whether the current architecture is little endian, but to_endian_agnostic() always returns the value as little endian, irrelevant of the architecture (If the architecture is little endian, it has no effect, if the architecture is big endian, it inverts the order).

Copy link
Contributor Author

@Nemirtingas Nemirtingas Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not according to my tests:

bool is_little_endian()
{
    uint16_t value = 0x0001;
    return *((uint8_t*)(&value)) == 0x01;
}

unsigned int to_endian_agnostic(unsigned int value)
{
    return ((uint8_t*)&value)[0] | (((uint8_t*)&value)[1] << 8) | (((uint8_t*)&value)[2] << 16) | (((uint8_t*)&value)[3] << 24);
}

int main (int argc, char *argv[])
{
    unsigned int z1 = to_endian_agnostic(0x424d4552);
    unsigned int z2 = to_endian_agnostic(0x52454d42);
    unsigned int z3 = 0x424d4552;
    unsigned int z4 = 0x52454d42;
    bool a = is_little_endian();
    printf("z1 bytes: ");
    for (int i = 0; i < 4; ++i) {
        printf("%d ", (uint32_t)((uint8_t*)&z1)[i]);
    }
    printf("\n");
    printf("z2 bytes: ");
    for (int i = 0; i < 4; ++i) {
        printf("%d ", (uint32_t)((uint8_t*)&z2)[i]);
    }
    printf("\n");
    printf("z3 bytes: ");
    for (int i = 0; i < 4; ++i) {
        printf("%d ", (uint32_t)((uint8_t*)&z3)[i]);
    }
    printf("\n");
    printf("z4 bytes: ");
    for (int i = 0; i < 4; ++i) {
        printf("%d ", (uint32_t)((uint8_t*)&z4)[i]);
    }
    printf("\n");

    printf("z1 to_endian_agnostic(0x424d4552): %08x(REMB), %08x(z1): little endian: %ld\n", y.i, z1, a);
    printf("z2 to_endian_agnostic(0x52454d42): %08x(REMB), %08x(z2): little endian: %ld\n", y.i, z2, a);
    printf("z3 0x424d4552                    : %08x(REMB), %08x(z3): little endian: %ld\n", y.i, z3, a);
    printf("z4 0x52454d42                    : %08x(REMB), %08x(z4): little endian: %ld\n", y.i, z4, a);
    
    return 0;
}
Native
z1 bytes: 52 45 4d 42
z2 bytes: 42 4d 45 52
z3 bytes: 52 45 4d 42
z4 bytes: 42 4d 45 52
z1 to_endian_agnostic(0x424d4552): 424d4552(REMB), 424d4552(z1): little endian: 1
z2 to_endian_agnostic(0x52454d42): 424d4552(REMB), 52454d42(z2): little endian: 1
z3 0x424d4552                    : 424d4552(REMB), 424d4552(z3): little endian: 1
z4 0x52454d42                    : 424d4552(REMB), 52454d42(z4): little endian: 1

Aarch64
z1 bytes: 52 45 4d 42
z2 bytes: 42 4d 45 52
z3 bytes: 52 45 4d 42
z4 bytes: 42 4d 45 52
z1 to_endian_agnostic(0x424d4552): 424d4552(REMB), 424d4552(z1): little endian: 1
z2 to_endian_agnostic(0x52454d42): 424d4552(REMB), 52454d42(z2): little endian: 1
z3 0x424d4552                    : 424d4552(REMB), 424d4552(z3): little endian: 1
z4 0x52454d42                    : 424d4552(REMB), 52454d42(z4): little endian: 1

arm big endian
z1 bytes: 52 45 4d 42
z2 bytes: 42 4d 45 52
z3 bytes: 42 4d 45 52
z4 bytes: 52 45 4d 42
z1 to_endian_agnostic(0x424d4552): 52454d42(REMB), 52454d42(z1): little endian: 0
z2 to_endian_agnostic(0x52454d42): 52454d42(REMB), 424d4552(z2): little endian: 0
z3 0x424d4552                    : 52454d42(REMB), 424d4552(z3): little endian: 0
z4 0x52454d42                    : 52454d42(REMB), 52454d42(z4): little endian: 0

Anyway, this is irrelevant as this way to do will not be validated.

Copy link
Contributor Author

@Nemirtingas Nemirtingas Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread, yes it always returns the value as little endian. Sorry, my bad. Should be to_little_endian

namespace rtc {

RembHandler::RembHandler(std::function<void(unsigned int)> onRemb) : mOnRemb(onRemb) {}

void RembHandler::incoming(message_vector &messages, [[maybe_unused]] const message_callback &send) {
static uint32_t rembValue = makeEndianAgnosticValue(0x424d4552); // BMER
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a custom function, the htonl function converts to network byte order (big endian).

Suggested change
static uint32_t rembValue = makeEndianAgnosticValue(0x424d4552); // BMER
static const uint32_t rembValue = htonl(0x52454d42); // REMB

@Nemirtingas
Copy link
Contributor Author

I'm not sure a compiler would optimize this sort of thing. On the other hand, I don't think either it will make a big difference in performances. I just tend to avoid this kind of test in my code and though it would fit here. As there is still no endianness const expression, htonl or memcmp would do the trick I guess ?

@Nemirtingas
Copy link
Contributor Author

Change will make no difference, consider using htonl or memcmp.

@Nemirtingas Nemirtingas closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants