[0.4.x] libvisual: Improve AltiVec detection for PPC/PPC64 Linux

[hartwork]

This is from https://src.fedoraproject.org/rpms/libvisual/blob/rawhide/f/libvisual-0.4.0-better-altivec-detection.patch , started out at https://bugzilla.redhat.com/show_bug.cgi?id=435771#c21 originally.

Thanks to @dwmw2!

[kaixiong]

@hartwork, interesting patch. I was hoping for something more 'fundamental' (like cpuid on x86/x64) than relying on the kernel. Seems like that's what everyone does.

Anyway, I went and found the Altivec detection code in ffmpeg's libavutil here. The Linux-specific code also reads from auxv, but it's a bit cleaner due to

• initialization is used for declaration
• use of /proc/self/auxv rather than /proc/%d/auxv
• use of while loop to read from /proc/self/auxv multiple times instead of goto more

I'd prefer to adapt the code from there.

[hartwork]

• initialization is used for declaration

@kaixiong copuld you elaborate what you mean by that?

• use of /proc/self/auxv rather than /proc/%d/auxv

I can fix that, if you like.

• use of while loop to read from /proc/self/auxv multiple times instead of goto more

I agree that there is likely a way to resolve some gotos and maybe end up with an easier to understand version. I must admit though, that I consider the current patch good enough and that the time to adjust and re-test this code I can better use elsewhere, because of the limited value. I would vote that either you take over this branch or that we only fix /proc/self/auxv and then merge. What do you think?

[kaixiong]

• initialization is used for declaration

@kaixiong copuld you elaborate what you mean by that?

@hartwork, I meant declarations like these:

int fd = open("/proc/self/auxv", O_RDONLY);

instead of:

int fd;
/* ... */
fd = open("/proc/self/auxv", O_RDONLY);

• use of /proc/self/auxv rather than /proc/%d/auxv

I can fix that, if you like.

• use of while loop to read from /proc/self/auxv multiple times instead of goto more

I agree that there is likely a way to resolve some gotos and maybe end up with an easier to understand version. I must admit though, that I consider the current patch good enough and that the time to adjust and re-test this code I can better use elsewhere, because of the limited value. I would vote that either you take over this branch or that we only fix /proc/self/auxv and then merge. What do you think?

The ffmpeg code uses a while loop only for that part. The remaining gotos for cleanup is not really important to eliminate.

I can take over but I don't really have a way to test at the moment. Here is how the section would look if adapted

    int fd = open ("/proc/self/auxv", O_RDONLY);

    if (fd < 0)
        return;

    ssize_t count;
    while ((count = read (fd, buf, sizeof(buf))) > 0) {
        for (int i = 0; i < count / sizeof (*buf); i += 2) {
            if (buf[i] == AT_HWCAP) {
                if (buf[i + 1] & PPC_FEATURE_HAS_ALTIVEC) {
                    cpu_caps.hasAltVec = 1;
                    goto out_close;
                }
            } else if (buf[i] == AT_NULL) {
                goto out_close;
            }
        }
    }

out_close:
    close (fd);

If testing is too much work then let's keep the patch as-is for 0.4.x.

[hartwork]

@hartwork, I meant declarations like these:

int fd = open("/proc/self/auxv", O_RDONLY);

instead of:

int fd;
/* ... */
fd = open("/proc/self/auxv", O_RDONLY);

@kaixiong I see, so essentially what C89 forced us to do. I prefer the first version most of the time. You too?

I can take over but I don't really have a way to test at the moment. Here is how the section would look if adapted

    int fd = open ("/proc/self/auxv", O_RDONLY);

    if (fd < 0)
        return;

    ssize_t count;
    while ((count = read (fd, buf, sizeof(buf))) > 0) {
        for (int i = 0; i < count / sizeof (*buf); i += 2) {
            if (buf[i] == AT_HWCAP) {
                if (buf[i + 1] & PPC_FEATURE_HAS_ALTIVEC) {
                    cpu_caps.hasAltVec = 1;
                    goto out_close;
                }
            } else if (buf[i] == AT_NULL) {
                goto out_close;
            }
        }
    }

out_close:
    close (fd);

I agree it's simpler, yes.

If testing is too much work then let's keep the patch as-is for 0.4.x.

Decoupling is great, thanks!