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

'vImageSepConvolve_Planar8' is only available on macOS 11.0 or newer #13

Closed
kunitoki opened this issue Nov 10, 2023 · 22 comments
Closed

Comments

@kunitoki
Copy link
Contributor

As the title says, if targeting older osx, it's not possible to compile

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

Thanks for reporting.

I couldn't easily find versioning information on Accelerate functions. Did you see any Apple information about this, or is it just the experience you made compiling?

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

Arc - 2023-11-10 31@2x

Ah right, I always forget they are specified this way. Also the header specifies

API_AVAILABLE(macos(11.0), ios(14.0), watchos(7.0), tvos(14.0));

I've was always confused about if that was when the function was added/became available to the SDK or where it's currently available.

Assuming the latter, I'll pick an alternate implementation for < 11.0, just not sure how to properly test...

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

I'm targeting 10.13 (High Sierra) and it seems to compile fine. I'll look into adding a runtime check.

Did you run into problems compiling?

@asimilon
Copy link

just not sure how to properly test...

I will need to confirm, but I think I have 10.9 or 10.10 as lowest versions of macOS I still keep on my old Mac Pro just for this kind of scenario. If I can be of any assistance hit me up.

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

Thanks! I'll ping the issue when I get something to try!

@kunitoki
Copy link
Contributor Author

I'm getting other compilation issues as well, and yes, i've added Accelerate.framework
image

@kunitoki
Copy link
Contributor Author

Those seems to be macOS 14.0+

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

Looks like I have some work to do here! Sorry about that, was just working hard on finding the best performing solutions, now I need to work on compatibility, it seems!

@sudara
Copy link
Owner

sudara commented Nov 10, 2023

Probably the route to go is have things fall back to the included Gin implementation at runtime where needed (until i find time to benchmark alternatives).

@kunitoki
Copy link
Contributor Author

I've tried to add (JUCE_MAC && MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_VERSION_13_3) for those methods but it seems like this doesn't work:

    static void argb (const juce::Image& srcImage, juce::Image& dstImage, size_t radius)
    {
        jassert (srcImage.getFormat() == juce::Image::PixelFormat::ARGB);
#if (JUCE_MAC && MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_VERSION_13_3) || JUCE_IOS
        auto kernel = createFloatKernel (radius);

        const auto w = (unsigned int) srcImage.getWidth();
        const auto h = (unsigned int) srcImage.getHeight();
        juce::Image::BitmapData srcData (srcImage, juce::Image::BitmapData::readWrite);
        juce::Image::BitmapData dstData (dstImage, juce::Image::BitmapData::readWrite);

        // vImageSepConvolve isn't happy operating in-place
        vImage_Buffer src = { srcData.getLinePointer (0), h, w, (size_t) srcData.lineStride };
        vImage_Buffer dst = { dstData.getLinePointer (0), h, w, (size_t) dstData.lineStride };
        vImageSepConvolve_ARGB8888 (&src, &dst, nullptr, 0, 0, kernel.data(), (unsigned int) kernel.size(), kernel.data(), (unsigned int) kernel.size(), 0, Pixel_8888 { 0, 0, 0, 0 }, kvImageEdgeExtend);
#else
        stackBlur::ginRGBA (img, radius); // There seems to be missing a non in-place blur in gin
#endif

@kunitoki
Copy link
Contributor Author

Probably the route to go is have things fall back to the included Gin implementation at runtime where needed (until i find time to benchmark alternatives).

Or provide a good simd convolution alternative.

@sudara
Copy link
Owner

sudara commented Nov 11, 2023

Or provide a good simd convolution alternative.

There are couple people interested in building SIMD variations of the vector implementation, so I'm betting this will happen!

@sudara
Copy link
Owner

sudara commented Nov 11, 2023

Ok, I have a draft implementation to make all the vImage stuff conditional at both compile and run time #16

I'm going to have to get #6 done first before merging, to ensure it's happy on at least macOS 11/12/13 and Windows. Don't want any more breakage!

@sudara
Copy link
Owner

sudara commented Nov 12, 2023

@kunitoki You should be good to go, I merged the compatibility changes in #16.

I still would like to see some evidence that the runtime checks are needed (i.e. if i can compile vImageSepConvolve on one machine, will it work on earlier targets before the API was available?) but I went the conservative route for now.

#19 and #20 are both followups for the compatibility work. Off to ADC now though!

@sudara sudara closed this as completed Nov 12, 2023
@kmolcard
Copy link
Contributor

kmolcard commented Dec 4, 2023

Hello, I'm trying to compile on 10.14.6 mac Intel Xcode 11.3.1 and got the following compile error:
vImage.h:44:9: error: use of undeclared identifier 'vImageSepConvolve_Planar8'

Just to be sure, can you confirm you only want to target macOS 11+?

@sudara
Copy link
Owner

sudara commented Dec 4, 2023

@kmolcard I opened a new issue for this, it should run happily on 10.14 (when compiled with something newer) but it looks like we'd need to put in a few more conditionals for it to actually compile there. I'll track the issue here: #37

@kmolcard
Copy link
Contributor

kmolcard commented Dec 4, 2023

thanks for the reply, definitely willing to help you on this if you are interesed

@sudara
Copy link
Owner

sudara commented Dec 4, 2023

Sure! You'll just have to navigate the mess that is implementations.h and do some preprocessor juggling so MELATONIN_BLUR_VIMAGE is only set for > 11.0.

Maybe one note we should make somewhere is.. by compiling on older OS, I believe this will exclude newer macs from taking advantage of the newer/faster APIs since the code paths that run those APIs won't be available in the binary.

Also related: I'm working on a better/faster fallback implementation for older OSes/machines.

@sudara sudara reopened this Dec 25, 2023
@kunitoki
Copy link
Contributor Author

kunitoki commented Dec 25, 2023

image
Still getting these warnings on ee269d3 it compiles tho, so it might be correctly not using these in reality, but maybe they should be protected by macros / availability guards

@sudara
Copy link
Owner

sudara commented Dec 25, 2023

Thx — I meant to ask, is this on a Projucer project, exported to xcode?

@kunitoki
Copy link
Contributor Author

Yes

@sudara sudara mentioned this issue Jan 11, 2024
@sudara
Copy link
Owner

sudara commented Jan 11, 2024

#53 should handle those warnings still showing up (despite being guarded at runtime)

@sudara sudara closed this as completed Jan 11, 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

No branches or pull requests

4 participants