Details

Reviewers

Ante Lausic (alausic)
Sybren A. Stüvel (sybren)
Bastien Montagne (mont29)
Ray Molenkamp (LazyDodo)
Lukas Stockner (lukasstockner97)
Sebastian Parborg (zeddb)

Maniphest Tasks

T63736: Unify blend file thumbnail extraction
T91841: Support thumbnail extraction for Zstd compressed blend files

Summary

Thumbnail extraction now shares code between Linux/Windows,
allowing thumbnails from Zstd compressed blend files to be extracted.

The main logic is placed in blendthumb_extract.cc and is built as static
library. For windows there is DLL which is registered during blender
install and which then reads and generates thumbnails.

For other platforms there is blender-thumbnailer executable file which
takes blend file as an input and generates PNG file. As a result
Python script blender-thumbnailer.py is no longer needed.

The thumbnail extractor shares the same code-path as Blenders file
reading, so there is no need to duplicate any file reading logic.
This means reading compressed blend files is supported (broken since
the recent move Zstd compression - D5799).

This resolves T63736.

Contributors:

@Ante Lausic (alausic) original patch.
@Ray Molenkamp (LazyDodo) windows fixes/support.
@Campbell Barton (campbellbarton) general fixes/update.
@Lukas Stockner (lukasstockner97) Zstd support.

Diff Detail

Repository

rB Blender

Branch

T (branched from master)

Build Status

Buildable 17486
Build 17486: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fix compilation error after D5799 changes (thanks, Windows, for defining min as a macro...). Also rebased.

Harbormaster completed remote builds in B15143: Diff 38231.Jun 11 2021, 10:30 PM

Campbell Barton (campbellbarton) added reviewers: Sybren A. Stüvel (sybren), Bastien Montagne (mont29).Jul 30 2021, 3:26 PM

Campbell Barton (campbellbarton) added a reviewer: Ray Molenkamp (LazyDodo).

Campbell Barton (campbellbarton) added 4 blocking reviewer(s): Sybren A. Stüvel (sybren), Bastien Montagne (mont29), Ray Molenkamp (LazyDodo), Campbell Barton (campbellbarton).Jul 30 2021, 4:16 PM

This revision now requires review to proceed.Jul 30 2021, 4:16 PM

Campbell Barton (campbellbarton) accepted this revision.Jul 30 2021, 4:19 PM

Campbell Barton (campbellbarton) added a reviewer: Sebastian Parborg (zeddb).Aug 16 2021, 7:20 AM

Can confirm it works on windows. thumbnail dll went from 50k to 400k, not super thrilled about that, but it's much better than earlier in the patch where it linked half of blender so I guess that's a win.

@Campbell Barton (campbellbarton) anything specific you wanted me to test/checkout?

In D6408#317160, @Sebastian Parborg (zeddb) wrote:

@Campbell Barton (campbellbarton) anything specific you wanted me to test/checkout?

Just that this doesn't cause any problems on macOS, although this change should only impact Linux/Windows.

In D6408#317170, @Campbell Barton (campbellbarton) wrote:

In D6408#317160, @Sebastian Parborg (zeddb) wrote:

@Campbell Barton (campbellbarton) anything specific you wanted me to test/checkout?

Just that this doesn't cause any problems on macOS, although this change should only impact Linux/Windows.

Ah, you got the wrong guy then. You want @Sebastián Barschkis (sebbas)

Tom Edwards (artfunkel) mentioned this in D12290: Add ZSTD support to Windows thumbnail provider.Aug 22 2021, 11:42 AM

The patch description needs updating. It still refers to using LZ4 compression, which got me confused for a minute.

The build fails on my Kubuntu 20.04 LTS machine:

: && /usr/bin/g++-9 -Wuninitialized -Wredundant-decls -Wall -Wno-invalid-offsetof -Wno-sign-compare -Wlogical-op -Winit-self -Wmissing-include-dirs -Wno-div-by-zero -Wtype-limits -Werror=return-type -Wno-char-subscripts -Wno-unknown-pragmas -Wpointer-arith -Wunused-parameter -Wwrite-strings -Wundef -Wformat-signedness -Wrestrict -Wno-suggest-override -Wuninitialized -Wundef -Wmissing-declarations -Wimplicit-fallthrough=5  -fuse-ld=gold -msse -pipe -fPIC -funsigned-char -fno-strict-aliasing -ffp-contract=off -msse2 -D_GLIBCXX_USE_CXX11_ABI=0 -fmacro-prefix-map="/home/sybren/workspace/blender-git/blender/"="" -fmacro-prefix-map="/home/sybren/workspace/blender-git/build_linux/"="" -Wno-maybe-uninitialized -O2 -DNDEBUG -static-libstdc++ -no-pie source/blender/blendthumb/CMakeFiles/blender-thumbnailer.dir/src/blendthumb_extract.cc.o source/blender/blendthumb/CMakeFiles/blender-thumbnailer.dir/src/blendthumb_png.cc.o source/blender/blendthumb/CMakeFiles/blender-thumbnailer.dir/src/blender_thumbnailer.cc.o -o bin/blender-thumbnailer  lib/libbf_blenlib.a  lib/libbf_intern_eigen.a  /usr/lib/gcc/x86_64-linux-gnu/9/libgomp.a  lib/libbf_intern_guardedalloc.a  lib/libbf_intern_numaapi.a  lib/libextern_wcwidth.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/freetype/lib/libfreetype.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/zlib/lib/libz.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/zstd/lib/libzstd.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/tbb/lib/libtbb.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/gmp/lib/libgmpxx.a  /home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/gmp/lib/libgmp.a  lib/libbf_intern_libc_compat.a && :
/home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/zstd/lib/libzstd.a(pool.c.o):pool.c:function POOL_free: error: undefined reference to 'pthread_join'
/home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/zstd/lib/libzstd.a(pool.c.o):pool.c:function POOL_create_advanced: error: undefined reference to 'pthread_create'
/home/sybren/workspace/blender-git/lib/linux_centos7_x86_64/zstd/lib/libzstd.a(pool.c.o):pool.c:function POOL_resize: error: undefined reference to 'pthread_create'
collect2: error: ld returned 1 exit status
[32/1849] Building C object source/blender/windowmanager/CMakeFiles/bf_windowmanager.dir/intern/wm_event_system.c.o
ninja: build stopped: subcommand failed.

The code also needs a pass to mark any local variable or parameter that isn't changing value with const.

The biggest issue with the code I feel is the use of std::string for non-string data. C++ has a rich type system, and there are plenty of ways to gracefully handle binary data (like blender::Vector<uint8_t>).

source/blender/blendthumb/src/blendthumb_extract.cc
128–141	This nesting is too deep, needs refactoring into one or more separate functions.
169	This function does too many different things. It's better to split up into smaller functions.

This revision now requires changes to proceed.Sep 10 2021, 3:30 PM

@Sybren A. Stüvel (sybren) could you check if this resolves the linking error? P2431

NOTE: we might consider linking pthread to blenlib, so each blenlib user doesn't run into this problem.

Yup, P2431 fixes the linker error.

Dalai Felinto (dfelinto) added a project: BF Blender (3.0).Sep 30 2021, 3:26 PM

Dalai Felinto (dfelinto) mentioned this in T91841: Support thumbnail extraction for Zstd compressed blend files.

Dalai Felinto (dfelinto) added a task: T91841: Support thumbnail extraction for Zstd compressed blend files.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 1 2021, 2:32 AM

Campbell Barton (campbellbarton) added a task: T63736: Unify blend file thumbnail extraction.

Campbell Barton (campbellbarton) retitled this revision from Unify blend file thumbnail extraction to Thumbnails: refactor extraction to use one code-path for all platforms.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) mentioned this in rB4485dc483cf0: Cleanup: use C-style comments, nullptr for C++.Oct 1 2021, 2:46 AM

Link against pthreads, rebase on master.

arc refused to apply this, but manually applying did the trick, still works on windows, no issues.

Replace std::string and blender::StringRef with blender::Vector & blender::Span.
Use optional types instead of empty vectors to signify failure.

@Ray Molenkamp (LazyDodo) I had to update the patch manually, getting SSL errors downloading/uploading with arc at the moment which I need to look into.

Quiet clang-tidy warnings, correct version check.

Updated with arc.

Harbormaster completed remote builds in B17484: Diff 42768.Oct 1 2021, 7:27 AM

Avoid overly verbose array syntax (blender::Span<char>({..}) can be written as {..} in some cases).
Avoid reallocations, calculate the space needed ahead of time.

Harbormaster completed remote builds in B17485: Diff 42769.Oct 1 2021, 7:54 AM

Correct comment.

Harbormaster completed remote builds in B17486: Diff 42770.Oct 1 2021, 7:56 AM

Replace reference to blender-thumbnailer.py with blender-thumbnailer in code comments.

Harbormaster completed remote builds in B17487: Diff 42771.Oct 1 2021, 7:58 AM

Add back early-exit behavior which stops reading the thumbnail when any block that isn't a TEST / REND is reached.
Move file seek fallback into it's own function, reuse the same buffer when reading to seek (instead of new allocations each time).
Return an error when seek fails.
Read into pre-allocated buffers instead of creating arrays (avoid allocations for very small arrays).
Use sizeof instead of repeating header size.

Harbormaster completed remote builds in B17535: Diff 42875.Oct 4 2021, 9:04 AM

Campbell Barton (campbellbarton) mentioned this in rBfd592538d983: Cleanup: move BLI_vfontdata.h to BKE_vfontdata.h.Oct 6 2021, 1:56 AM

Remove freetype dependancy (committed to master rBfd592538d983bec51e331f1d073c39582d43520f)
Return an error if writing the file fails.
Close file on failure.
Use BLI_fopen for writing the file since it's used for reading and has better compatibility for file paths (if this is ever used on Windows).

Harbormaster completed remote builds in B17587: Diff 42951.Oct 6 2021, 2:00 AM

Quickly check the code itself, looks fine to me, besides the trivial note below.

source/blender/blendthumb/src/blender_thumbnailer.cc
91	This is not closing `dst_file`

Campbell Barton (campbellbarton) added inline comments.Oct 18 2021, 2:39 PM

source/blender/blendthumb/src/blender_thumbnailer.cc
91	This is in fact being closed, `blendthumb_create_png_data_from_thumb` takes ownership and closes this file in the case of an error. Since this reads like a bug, I'll note this in a comment.

Campbell Barton (campbellbarton) marked an inline comment as not done.Oct 19 2021, 12:56 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blendthumb/src/blender_thumbnailer.cc
91	Correction, you were right (the logic described above is done for `src_file`, where `BLI_filereader_new_file` takes ownership).

Fix file handle recource leak.
Use uint8_t instead of char for binary data.

Harbormaster completed remote builds in B17971: Diff 43529.Oct 19 2021, 1:01 AM

Campbell Barton (campbellbarton) marked 9 inline comments as done.Oct 19 2021, 1:09 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blendthumb/src/blendthumb_extract.cc
37	Using fized size char arrays now.
101	Using `blender::Array<uint8_t>`.
128–141	Split `file_seek` into a separate function.

Quiet warnings on WIN32, resolve build error on macOS.

Harbormaster completed remote builds in B17974: Diff 43534.Oct 19 2021, 2:11 AM

Sybren A. Stüvel (sybren) accepted this revision.Oct 19 2021, 10:56 AM

Fix: Build issues on windows

This fixes the following issues:

missing namespace in the usage of std::max in blendthumb_win32.cc
Windows.h redefines min and max macros
blenlib's stack trace code requires version.lib and dbghelp.lib to be linked
version.lib tries to implicitly drag in the release CRT, which leads to a linker warning for debug builds

Harbormaster completed remote builds in B18006: Diff 43580.Oct 19 2021, 3:40 PM

Removing sebbas since he isn't available at the moment.

This revision is now accepted and ready to land.Oct 20 2021, 1:05 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 1:08 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 1:14 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 1:25 AM

Campbell Barton (campbellbarton) mentioned this in rBef9269bd62f3: Thumbnails: refactor extraction to use one code-path for all platforms.Oct 20 2021, 1:33 AM

This differential can be closed with https://developer.blender.org/rBef9269bd62f31e39d39cc59cd34354b53eae6661

Closing, as this has been commited.

Thumbnails: refactor extraction to use one code-path for all platforms
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42770

release/bin/blender-thumbnailer.py

source/blender/CMakeLists.txt

source/blender/blendthumb/CMakeLists.txt

source/blender/blendthumb/src/BlendThumb.def

source/blender/blendthumb/src/BlendThumb.rc

source/blender/blendthumb/src/BlenderThumb.cpp

source/blender/blendthumb/src/Dll.cpp

source/blender/blendthumb/src/blender_thumbnailer.cc

source/blender/blendthumb/src/blendthumb.hh

source/blender/blendthumb/src/blendthumb_extract.cc

source/blender/blendthumb/src/blendthumb_png.cc

source/blender/blendthumb/src/blendthumb_win32.cc

source/blender/blendthumb/src/blendthumb_win32.def

source/blender/blendthumb/src/blendthumb_win32.rc

source/blender/blendthumb/src/blendthumb_win32_dll.cc

source/creator/CMakeLists.txt

Thumbnails: refactor extraction to use one code-path for all platformsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42770

release/bin/blender-thumbnailer.py

source/blender/CMakeLists.txt

source/blender/blendthumb/CMakeLists.txt

source/blender/blendthumb/src/BlendThumb.def

source/blender/blendthumb/src/BlendThumb.rc

source/blender/blendthumb/src/BlenderThumb.cpp

source/blender/blendthumb/src/Dll.cpp

source/blender/blendthumb/src/blender_thumbnailer.cc

source/blender/blendthumb/src/blendthumb.hh

source/blender/blendthumb/src/blendthumb_extract.cc

source/blender/blendthumb/src/blendthumb_png.cc

source/blender/blendthumb/src/blendthumb_win32.cc

source/blender/blendthumb/src/blendthumb_win32.def

source/blender/blendthumb/src/blendthumb_win32.rc

source/blender/blendthumb/src/blendthumb_win32_dll.cc

source/creator/CMakeLists.txt

Thumbnails: refactor extraction to use one code-path for all platforms
ClosedPublic
Actions

Revision Contents
Changeset List