macOS/QuickLook: support rich thumbnail in Finder #107072

Open
Ankit Meel wants to merge 4 commits from ankitm/blender:ankitm/2ql into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
6 changed files with 290 additions and 3 deletions
Showing only changes of commit 27651493d9 - Show all commits

View File

@ -0,0 +1,50 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleDevelopmentRegion</key>
<string>en</string>
<key>CFBundleName</key>
<string>blender-thumbnailer</string>
<key>CFBundleDisplayName</key>
<string>blender_thumbnailer</string>
<key>CFBundleExecutable</key>
<string>blender-thumbnailer</string>
<key>CFBundleIdentifier</key>
<string>org.blenderfoundation.blender.thumbnailer</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundlePackageType</key>
<string>XPC!</string>
<key>CFBundleSupportedPlatforms</key>
<array>
<string>MacOSX</string>
</array>
<key>CFBundleShortVersionString</key>
<string>${MACOSX_BUNDLE_SHORT_VERSION_STRING}</string>
<key>CFBundleVersion</key>
<string>${MACOSX_BUNDLE_LONG_VERSION_STRING}</string>
<key>CFBundleGetInfoString</key>
<string>${MACOSX_BUNDLE_LONG_VERSION_STRING}, Blender Foundation</string>
<key>LSMinimumSystemVersion</key>
<string>10.15</string>
<key>NSExtension</key>
<dict>
<key>NSExtensionAttributes</key>
<dict>
<key>QLSupportedContentTypes</key>
<array>
<!-- The supported file UTIs. Not inherited from parent bundle. -->
<string>org.blenderfoundation.blender.file</string>
</array>
<key>QLThumbnailMinimumDimension</key>
<integer>0</integer>
</dict>
<key>NSExtensionPointIdentifier</key>
<string>com.apple.quicklook.thumbnail</string>
<key>NSExtensionPrincipalClass</key>
<!-- Must be the same as the class implementing the reply method. -->
<string>ThumbnailProvider</string>
</dict>
ankitm marked this conversation as resolved Outdated

Is this required, and common for these kinds of thumbnails generators? I don't see it for any other apps, but maybe I just happen to not have any apps that do this.

Is this required, and common for these kinds of thumbnails generators? I don't see it for any other apps, but maybe I just happen to not have any apps that do this.

Not required and not sure about commonness. But since there's no other way to disable it, I added it.

I could remove the key from plist and let the system decide.

Not required and not sure about commonness. But since there's no other way to disable it, I added it. I could remove the key from plist and let the system decide.

I'd leave it out and let the system decide.

I'd leave it out and let the system decide.
</dict>
</plist>

View File

@ -0,0 +1,12 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<!-- Extension must be codesigned even locally. -->
<key>com.apple.security.app-sandbox</key>
<true/>
<!-- XCode adds this by default to a QuickLook extension. -->
<key>com.apple.security.files.user-selected.read-only</key>
<true/>
</dict>
</plist>

View File

@ -41,7 +41,30 @@ if(WIN32)
set_target_properties(BlendThumb PROPERTIES LINK_FLAGS_DEBUG "/NODEFAULTLIB:msvcrt")
set_target_properties(BlendThumb PROPERTIES VS_GLOBAL_VcpkgEnabled "false")
ankitm marked this conversation as resolved Outdated

Code is more clearly mutually exclusive if you use:

if(WIN32)
..
elseif(APPLE)
...
elseif(UNIX)
...
endif()
Code is more clearly mutually exclusive if you use: ``` if(WIN32) .. elseif(APPLE) ... elseif(UNIX) ... endif() ```
else()
elseif(APPLE)
# -----------------------------------------------------------------------------
# Build `blender-thumbnailer.appex` app extension.
set(SRC_APPEX
src/thumbnail_provider.mm
src/thumbnail_provider.h
)
add_executable(blender-thumbnailer MACOSX_BUNDLE ${SRC} ${SRC_APPEX})
# It needs to be codesigned (ad-hoc in this case) even on developer machine to generate thumbnails.
# Command taken from XCode build process.
add_custom_command(
TARGET blender-thumbnailer POST_BUILD
COMMAND codesign --deep --force --sign - --entitlements "${CMAKE_SOURCE_DIR}/release/darwin/thumbnailer_entitlements.plist"
--timestamp=none $<TARGET_BUNDLE_DIR:blender-thumbnailer>
)
setup_platform_linker_flags(blender-thumbnailer)
target_link_libraries(blender-thumbnailer
bf_blenlib
# Avoid linker error about undefined _main symbol.
"-e _NSExtensionMain"
"-framework QuickLookThumbnailing"
)
elseif(UNIX)
# -----------------------------------------------------------------------------
# Build `blender-thumbnailer` executable

View File

@ -0,0 +1,14 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include <Foundation/Foundation.h>
ankitm marked this conversation as resolved
Review

Similarly to the .mm file, #import should be used here

Similarly to the `.mm` file, `#import` should be used here
#import <QuickLookThumbnailing/QuickLookThumbnailing.h>
NS_ASSUME_NONNULL_BEGIN
@interface ThumbnailProvider : QLThumbnailProvider
@end
NS_ASSUME_NONNULL_END

View File

@ -0,0 +1,175 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include <AppKit/NSImage.h>
#include <CoreGraphics/CGDataProvider.h>
ankitm marked this conversation as resolved
Review

CoreGraphics headers can be removed since they're not used anymore, also Objective-C headers should use #import instead of #include for correctness

CoreGraphics headers can be removed since they're not used anymore, also Objective-C headers should use `#import` instead of `#include` for correctness
#include <CoreGraphics/CoreGraphics.h>
#include <Foundation/Foundation.h>
#include "BLI_fileops.h"
#include "BLI_filereader.h"
#include "BLI_utility_mixins.hh"
#include "blendthumb.hh"
#include "thumbnail_provider.h"
/**
* This section intends to list the important steps for creating a thumbnail extension.
* qlgenerator has been deprecated and removed in platforms we support. App extensions are the way
* forward. But there's little guidance on how to do it outside Xcode.
*
* The process of thumbnail generation goes something like this:
* 1. If an app is launched, or is registered with lsregister, its plugins also get registered.
* 2. When a file thumbnail in Finder or QuickLook is requested, the system looks for a plugin
* that supports the file type UTI.
* 3. The plugin is launched in a sandboxed environment and should call the handler with a reply.
*
* # Plugin Info.plist
* The Info.plist file should be properly configured with supported content type.
*
* # Codesigning
* The plugin should be codesigned with entitlements at least for sandbox and read-only/
* read-write (for access to the given file). It's needed to even run the plugin locally.
* com.apple.security.get-task-allow entitlement is required for debugging.
*
* # Registering the plugin
* The plugin should be registered with lsregister. Either by calling lsregister or by launching
* the parent app.
* /System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Support/lsregister \
-dump | grep blender-thumbnailer
*
* # Debugging
* Since read-only entitlement is there, creating files to log is not possible. So NSLog and
* viewing it in Console.app (after triggering a thumbnail) is the way to go. Interesting processes
* are: qlmanage, quicklookd, kernel, blender-thumbnailer, secinitd,
* com.apple.quicklook.ThumbnailsAgent
*
* LLDB/ Xcode etc., debuggers can be used to get extra logs than CLI invocation but breakpoints
* still are a pain point. /usr/bin/qlmanage is the target executable. Other args to qlmanage
* follow.
*
* # Troubleshooting
* - The appex shouldn't have any quarantine flag.
xattr -rl bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex
* - Is it registered with lsregister and there isn't a conflict with another plugin taking
* precedence? lsregister -dump | grep blender-thumbnailer.appex
* - For RBSLaunchRequest error: is the executable executable? chmod u+x
bin/Blender.app/Contents/PlugIns/blender-thumbnailer.appex/Contents/MacOS/blender-thumbnailer
* - Is it codesigned and sandboxed?
* codesign --display --verbose --entitlements - --xml \
bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex codesign --deep --force --sign - \
--entitlements ../blender/release/darwin/thumbnailer_entitlements.plist --timestamp=none \
bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex
* - Sometimes blender-thumbnailer running in background can be killed.
* - qlmanage -r && killall Finder
* - The code cannot attempt to do anything outside sandbox like writing to blend.
*
ankitm marked this conversation as resolved Outdated

A bit of a codestyle nitpick, but an explicit private: specifier would help readability here

A bit of a codestyle nitpick, but an explicit `private:` specifier would help readability here
* # Triggering a thumbnail
* - qlmanage -t -s 512 -o /tmp/ /path/to/file.blend
*
* # External resources
* https://developer.apple.com/library/archive/documentation/UserExperience/Conceptual/Quicklook_Programming_Guide/Introduction/Introduction.html#//apple_ref/doc/uid/TP40005020-CH1-SW1
*/
class FileDescriptorRAII : blender::NonCopyable, blender::NonMovable {
private:
int src_fd = -1;
public:
explicit FileDescriptorRAII(const char *file_path)
{
src_fd = BLI_open(file_path, O_BINARY | O_RDONLY, 0);
}
~FileDescriptorRAII()
{
if (good()) {
int ok = close(src_fd);
if (!ok) {
NSLog(@"Blender Thumbnailer Error: Failed to close the blend file.");
}
}
}
bool good()
{
return src_fd > 0;
}
int get()
{
return src_fd;
}
};
static NSError *create_nserror_from_string(NSString *errorStr)
{
NSLog(@"Blender Thumbnailer Error: %@", errorStr);
return [NSError errorWithDomain:@"org.blenderfoundation.blender.thumbnailer"
code:-1
userInfo:@{NSLocalizedDescriptionKey : errorStr}];
}
static NSImage *generate_nsimage_for_file(const char *src_blend_path, NSError **error)
{
/* Open source file `src_blend`. */
Brainzman marked this conversation as resolved Outdated

For safety, this entire function should be wrapped in an @autoreleasepool since we're calling Cocoa code which might call autorelease internally

For safety, this entire function should be wrapped in an `@autoreleasepool` since we're calling Cocoa code which might call `autorelease` internally

I thought the external pool would take effect.
removed now

I thought the external pool would take effect. removed now

The external pool does take effect yeah, but since we do not have a global pool to fallback to like in a normal Obj-C app, it's safer and simpler to wrap all Cocoa code in pools to prevent leaks later down the line when refactoring.
It also leads to way easier maintenance since you do not have to worry about figuring out which function is/isn't wrapped in a pool, and lets you safely call any Obj-C function from C/C++.

The external pool does take effect yeah, but since we do not have a global pool to fallback to like in a normal Obj-C app, it's safer and simpler to wrap all Cocoa code in pools to prevent leaks later down the line when refactoring. It also leads to way easier maintenance since you do not have to worry about figuring out which function is/isn't wrapped in a pool, and lets you safely call any Obj-C function from C/C++.
FileDescriptorRAII src_file_fd = FileDescriptorRAII(src_blend_path);
if (!src_file_fd.good()) {
*error = create_nserror_from_string(@"Failed to open blend");
return nil;
}
FileReader *file_content = BLI_filereader_new_file(src_file_fd.get());
if (file_content == nullptr) {
*error = create_nserror_from_string(@"Failed to read from blend");
return nil;
}
/* Extract thumbnail from file. */
Thumbnail thumb;
eThumbStatus err = blendthumb_create_thumb_from_file(file_content, &thumb);
ankitm marked this conversation as resolved Outdated

This can be heavily simplified by using the [NSimage initWithData] method, removing the need for CoreGraphics altogether. In which case, the Cocoa calling code should be wrapped in an @autoreleasepool block.

This can be heavily simplified by using the `[NSimage initWithData]` method, removing the need for CoreGraphics altogether. In which case, the Cocoa calling code should be wrapped in an `@autoreleasepool` block.
if (err != BT_OK) {
*error = create_nserror_from_string(@"Failed to create thumbnail from file");
return nil;
}
std::optional<blender::Vector<uint8_t>> png_buf_opt = blendthumb_create_png_data_from_thumb(
&thumb);
if (!png_buf_opt) {
*error = create_nserror_from_string(@"Failed to create png data from thumbnail");
return nil;
}
NSData *ns_data = [NSData dataWithBytes:png_buf_opt->data() length:png_buf_opt->size()];
NSImage *ns_image = [[NSImage alloc] initWithData:ns_data];
return ns_image;
}
@implementation ThumbnailProvider
- (void)provideThumbnailForFileRequest:(QLFileThumbnailRequest *)request
completionHandler:(void (^)(QLThumbnailReply *_Nullable reply,
NSError *_Nullable error))handler
{
NSLog(@"Generating thumbnail for %@", request.fileURL.path);
Brainzman marked this conversation as resolved
Review

Do we want to keep these logging statements or are they just for debugging purposes?

Do we want to keep these logging statements or are they just for debugging purposes?
Review

This is the only output generated by the tool visible in console.app and other reviewers signed off on it..
i doubt there's any downside to it

This is the only output generated by the tool visible in console.app and other reviewers signed off on it.. i doubt there's any downside to it
Review

Alrighty makes sense

Alrighty makes sense
@autoreleasepool {
NSError *error = nil;
NSImage *ns_image = generate_nsimage_for_file(request.fileURL.path.UTF8String, &error);
if (ns_image == nil) {
handler(nil, error);
return;
}
handler([QLThumbnailReply replyWithContextSize:request.maximumSize
currentContextDrawingBlock:^BOOL {
[ns_image drawInRect:NSMakeRect(0,
0,
request.maximumSize.width,
ankitm marked this conversation as resolved Outdated

ns_image should be sent a release message here for correct MRR alloc/release balance. The object won't be directly de-allocated since the currentContextDrawingBlock inline block owns a strong reference to it via capture.

`ns_image` should be sent a `release` message here for correct MRR alloc/release balance. The object won't be directly de-allocated since the `currentContextDrawingBlock` inline block owns a strong reference to it via capture.

Great catch. Verified by running qlmanage 20k times that the new code stays in 5-7 MB memory use. Old code increased steadily.

Great catch. Verified by running qlmanage 20k times that the new code stays in 5-7 MB memory use. Old code increased steadily.
request.maximumSize.height)];
// Release the ns_image that was strongly captured by the block.
[ns_image release];
return YES;
}],
nil);
}
NSLog(@"Thumbnail generation succcessfully completed");
}
@end

View File

@ -319,6 +319,7 @@ if(WITH_PYTHON_MODULE)
if(APPLE)
set_target_properties(blender PROPERTIES MACOSX_BUNDLE TRUE)
set_target_properties(blender-thumbnailer PROPERTIES MACOSX_BUNDLE TRUE)
endif()
if(WIN32)
@ -1575,6 +1576,16 @@ elseif(APPLE)
MACOSX_BUNDLE_LONG_VERSION_STRING "${BLENDER_VERSION}.${BLENDER_VERSION_PATCH} ${BLENDER_DATE}"
)
if(WITH_BLENDER_THUMBNAILER)
set(OSX_THUMBNAILER_SOURCEDIR ${OSX_APP_SOURCEDIR}/Contents/PlugIns/blender-thumbnailer.appex)
Review

For the consistency with the release/darwin/Blender.app/Contents/Plugins/ should this be ${OSX_APP_SOURCEDIR}/Contents/PlugIns/blender-thumbnailer.appex (Plugins vs. PlugIns) ?

For the consistency with the `release/darwin/Blender.app/Contents/Plugins/` should this be `${OSX_APP_SOURCEDIR}/Contents/PlugIns/blender-thumbnailer.appex` (`Plugins` vs. `PlugIns`) ?
Review

Made it the other way around: PlugIns everywhere. Default macOS apps like Safari do it too. I never paid attention so I'm guessing Xcode had made the PlugIns path and I manually and mistakenly made Plugins folder in git.

Made it the other way around: `PlugIns` everywhere. Default macOS apps like Safari do it too. I never paid attention so I'm guessing Xcode had made the `PlugIns` path and I manually and mistakenly made `Plugins` folder in git.
set_target_properties(blender-thumbnailer PROPERTIES
BUNDLE_EXTENSION appex
MACOSX_BUNDLE_INFO_PLIST ${OSX_THUMBNAILER_SOURCEDIR}/Contents/Info.plist
MACOSX_BUNDLE_SHORT_VERSION_STRING "${BLENDER_VERSION}.${BLENDER_VERSION_PATCH}"
MACOSX_BUNDLE_LONG_VERSION_STRING "${BLENDER_VERSION}.${BLENDER_VERSION_PATCH} ${BLENDER_DATE}"
)
endif()
# Gather the date in finder-style.
execute_process(
COMMAND date "+%m/%d/%Y/%H:%M"
@ -1609,9 +1620,11 @@ elseif(APPLE)
)
if(WITH_BLENDER_THUMBNAILER)
# Install TARGET breaks the signature so copy it instead. It's ad-hoc codesigned by the build step.
install(
TARGETS blender-thumbnailer
DESTINATION "./Blender.app/Contents/MacOS"
FILES $<TARGET_BUNDLE_DIR:blender-thumbnailer>
DESTINATION "./Blender.app/Contents/PlugIns"
PERMISSIONS OWNER_EXECUTE OWNER_READ OWNER_WRITE GROUP_EXECUTE GROUP_READ WORLD_EXECUTE WORLD_READ
)
endif()