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

Adds support for reading OTIOZ #85

Merged
merged 10 commits into from
Nov 15, 2024
Merged

Adds support for reading OTIOZ #85

merged 10 commits into from
Nov 15, 2024

Conversation

jminor
Copy link
Member

@jminor jminor commented Nov 10, 2024

This PR adds support for reading the embedded content.otio within an OTIOZ file.

The file type is inferred from the filename extension.

% raven filename.otioz

To accomplish this, Raven now depends on the minizip-ng submodule. If I'm understanding the minizip-ng API correctly, Raven seeks into the OTIOZ and decompresses only the content.otio into memory without attempting to do anything with the media in the OTIOZ.

Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
@jminor jminor mentioned this pull request Nov 10, 2024
@jminor
Copy link
Member Author

jminor commented Nov 10, 2024

Hmmm... it looks like I'll need some assistance getting CMake to build/find universal builds of some dependencies. Or do we need those at all? I'm guessing libssl and libcrypto are for decrypting password protected ZIP files (which OTIOZ doesn't support), so we might be able to just disable those two. I'm not sure about liblzma or libzstd.

[ 98%] Linking CXX executable raven
ld: warning: ignoring file '/opt/homebrew/Cellar/xz/5.6.3/lib/liblzma.5.dylib': found architecture 'arm64', required architecture 'x86_64'
ld: warning: ignoring file '/opt/homebrew/Cellar/zstd/1.5.6/lib/libzstd.1.5.6.dylib': found architecture 'arm64', required architecture 'x86_64'
ld: warning: ignoring file '/opt/homebrew/Cellar/openssl@3/3.3.2/lib/libssl.3.dylib': found architecture 'arm64', required architecture 'x86_64'
ld: warning: ignoring file '/opt/homebrew/Cellar/openssl@3/3.3.2/lib/libcrypto.3.dylib': found architecture 'arm64', required architecture 'x86_64'
Undefined symbols for architecture x86_64:
  "_ERR_get_error", referenced from:
      _mz_crypt_sha_begin in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_sha_update in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_sha_end in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_aes_encrypt_final in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_aes_decrypt_final in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_aes_set_key in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_hmac_init in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      ...
  "_EVP_CIPHER_CTX_ctrl", referenced from:
      _mz_crypt_aes_encrypt_final in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)
      _mz_crypt_aes_decrypt_final in libminizip.a[x86_64][14](mz_crypt_openssl.c.o)

@jminor
Copy link
Member Author

jminor commented Nov 10, 2024

and Windows is unhappy about this:

D:\a\raven\raven\libs\minizip-ng\third_party\zstd\build\VS2010\libzstd-dll\libzstd-dll.rc(4): fatal error RC1015: cannot open include file 'zstd.h'. [D:\a\raven\raven\build_win64\_deps\zstd-build\lib\libzstd_shared.vcxproj]

@ThomasWilshaw
Copy link
Contributor

I'm also getting issues buiding on Windows with various unresolved external symbol errors e.g.:

app.obj : error LNK2019: unresolved external symbol mz_zip_reader_open_file referenced in function "class opentimelineio::v1_0::Timeline * __cdecl LoadOTIOZFile(class std::basic_string<char,struct std::char_traits<char>,class std::alloc ator<char> >)" (?LoadOTIOZFile@@YAPEAVTimeline@v1_0@opentimelineio@@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) [D:\src\raven\build\raven.vcxproj]

@darbyjohnston
Copy link
Contributor

I would disable all of the options, leaving just ZIP compression + file parsing. Which is hopefully enough for supporting .otioz? If so we should probably mention that in the docs:

https://opentimelineio.readthedocs.io/en/latest/tutorials/otio-filebundles.html

-DMZ_BZIP2=OFF
-DMZ_LZMA=OFF
-DMZ_ZSTD=OFF
-DMZ_LIBCOMP=OFF
-DMZ_FETCH_LIBS=OFF
-DMZ_PKCRYPT=OFF
-DMZ_WZAES=OFF
-DMZ_OPENSSL=OFF
-DMZ_BCRYPT=OFF
-DMZ_LIBBSD=OFF
-DMZ_ICONV=OFF

@jminor
Copy link
Member Author

jminor commented Nov 11, 2024

Okay great. By turning off all those options, now the macOS build works, but Windows is still having trouble linking...

@ThomasWilshaw
Copy link
Contributor

ThomasWilshaw commented Nov 11, 2024

A small bit of progress, if I add the line set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) everything compiles fine and things sort of work (on Windows this is). The only example otioz file I have doesn't open and gives this message:

LOG: Invalid OTIOZ: "C:\Users\Tom\Downloads\aws-picchu-edit-4096x2048.otioz": Unable to open entry: -109

but that does imply the library is at least working. I don't know why that worked or if that is a sensible thing to do but if nothing else it might indicate the problem? I'm not at all good with CMake.

I also noticed the drag and drop code only excepts .otio files so that will need updating (app.cpp:is_valid_file()).

@ThomasWilshaw
Copy link
Contributor

With a bit of fiddling I've managed to get this all working on Windows. In CMakeLists.txt I had to set:

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
set(MZ_ZLIB ON)

and remove your changes from the last commit (I'm not sure if that step is essential or not).

I also found a bug in the effects drawing code, InvisibleButtons must have x and y sizes greater than 0 and with the larger OTIO file being rendered this wasn't the case. I added a catch for that and everything seems to be working correctly:

image

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f4a7feb..e215b82 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -66,17 +66,8 @@ if(NOT EMSCRIPTEN AND NOT WIN32)
   add_subdirectory("libs/glfw")
 endif()
 
-set(MZ_BZIP2 OFF)
-set(MZ_LZMA OFF)
-set(MZ_ZSTD OFF)
-set(MZ_LIBCOMP OFF)
-set(MZ_FETCH_LIBS OFF)
-set(MZ_PKCRYPT OFF)
-set(MZ_WZAES OFF)
-set(MZ_OPENSSL OFF)
-set(MZ_BCRYPT OFF)
-set(MZ_LIBBSD OFF)
-set(MZ_ICONV OFF)
+set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
+set(MZ_ZLIB ON)
 add_subdirectory("libs/minizip-ng")
 
 if(NOT EMSCRIPTEN)
diff --git a/timeline.cpp b/timeline.cpp
index ccddf0e..5d07e90 100644
--- a/timeline.cpp
+++ b/timeline.cpp
@@ -393,6 +393,11 @@ void DrawEffects(
     auto old_pos = ImGui::GetCursorPos();
     ImGui::SetCursorPos(render_pos);
 
+    // ImGui::InvisibleButton size x and y must be greater than 0.0.
+    // See definition of ImGui::InvisibleButton in imgui_widgets.cpp:
+    if (size.x <= 0.0 || size.y <= 0.0) {
+        return;
+    }
     ImGui::PushID(item);
     ImGui::BeginGroup();
 

@ThomasWilshaw
Copy link
Contributor

Also it's possible these two issues are relevant for the Windows build issues:
zlib-ng/minizip-ng#475
msys2/MINGW-packages#14029

Apologies if I'm posting too much

@jminor
Copy link
Member Author

jminor commented Nov 13, 2024

Oh, this is great progress - all of this is 100% welcome and encouraged :)

Is the linking issue a static vs dynamic link issue? I've attempted to keep everything statically linked into Raven for ease of installation. On macOS I checked the result with otool -L raven to be sure it only references system libraries. Do you know how to check that on Windows?

Could you make a separate PR for the InvisibleButton fix?

Signed-off-by: Joshua Minor <[email protected]>
@jminor
Copy link
Member Author

jminor commented Nov 14, 2024

Thanks @ThomasWilshaw - I think that works. Can you verify that this actually runs and reads an OTIOZ on Windows?

I also addressed your note about checking for otioz via drag and drop.

@ThomasWilshaw
Copy link
Contributor

Haha almost, I had to set set(MZ_FETCH_LIBS ON) and set(BUILD_SHARED_LIBS OFF) and it all seems to work bar the InvisibleButton crash (I'll open a PR for that shortly). I ran the dependancy checker on Windows and got the following:

D:\src\raven\build\Debug>dumpbin /dependents raven.exe
Microsoft (R) COFF/PE Dumper Version 14.29.30148.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file raven.exe

File Type: EXECUTABLE IMAGE

  Image has the following dependencies:

    d3d11.dll
    D3DCOMPILER_47.dll
    dwmapi.dll
    KERNEL32.dll
    USER32.dll
    GDI32.dll
    SHELL32.dll
    ole32.dll
    MSVCP140D.dll
    IMM32.dll
    VCRUNTIME140D.dll
    VCRUNTIME140_1D.dll
    ucrtbased.dll

  Summary

        1000 .00cfg
      23E000 .data
        6000 .idata
       3F000 .pdata
      279000 .rdata
        D000 .reloc
        1000 .rsrc
      528000 .text
        1000 .tls

Which looks correct I think. Do OTIOZ files only ever use deflate compression or might we need some of the other compression formats?

@jminor
Copy link
Member Author

jminor commented Nov 14, 2024

Thanks for checking that. I added those two more settings just now.

Would you mind also checking if the pre-compiled build result for Windows runs? https://github.com/OpenTimelineIO/raven/actions/runs/11840470183

@ThomasWilshaw thanks so much for helping with this!

CMakeLists.txt Outdated
set(MZ_LZMA OFF)
set(MZ_ZSTD OFF)
set(MZ_LIBCOMP OFF)
set(MZ_FETCH_LIBS OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line needs to be removed as you set it to ON earlier

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this line fixes the build locally for me and you can see in the GH Acions build log ZLIB is disabled so I suspect it's the same issue there.

Signed-off-by: Joshua Minor <[email protected]>
@ThomasWilshaw
Copy link
Contributor

Works like a charm :)

@jminor jminor merged commit 2039019 into OpenTimelineIO:main Nov 15, 2024
6 checks passed
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.

3 participants