felixc / rexiv2

Rust library for read/write access to media-file metadata (Exif, XMP, and IPTC)
GNU General Public License v3.0
79 stars 17 forks source link

Provide a wrapper for gexiv2::gexiv2_initialize #30

Closed mineo closed 5 years ago

mineo commented 5 years ago

gexiv2_initialize is required to be called in multi-threaded applications, because otherwise the program might crash with multiple threads having stack traces ending in:

(gdb) bt 0 0x00007ffff7cb6f04 in memcmp_avx2_movbe () at /usr/lib/libc.so.6 1 0x00007ffff758be98 in std::_Rb_tree<std::cxx11::basic_string<char, std::char_traits, std::allocator >, std::pair<std::cxx11::basic_string<char, std::char_traits, std::allocator > const, std::cxx11::basic_string<char, std::char_traits, std::allocator > >, std::_Select1st<std::pair<std::cxx11::basic_string<char, std::char_traits, std::allocator > const, std::cxx11::basic_string<char, std::char_traits, std::allocator > > >, std::less<std::cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<std::cxx11::basic_string<char, std::char_traits, std::allocator > const, std::cxx11::basic_string<char, std::char_traits, std::allocator > > > >::_M_get_insert_hint_unique_pos(std::_Rb_tree_const_iterator<std::pair<std::cxx11::basic_string<char, std::char_traits, std::allocator > const, std::cxx11::basic_string<char, std::char_traits, std::allocator > > >, std::cxx11::basic_string<char, std::char_traits, std::allocator > const&) () at /usr/lib/libexiv2.so.26 2 0x00007ffff76b727a in () at /usr/lib/libexiv2.so.26 3 0x00007ffff76b7759 in () at /usr/lib/libexiv2.so.26 4 0x00007ffff7691d8d in () at /usr/lib/libexiv2.so.26 5 0x00007ffff7680c85 in TXMPMeta<std::cxx11::basic_string<char, std::char_traits, std::allocator > >::RegisterNamespace(char const, char const) () at /usr/lib/libexiv2.so.26 6 0x00007ffff767c320 in Exiv2::XmpParser::initialize(void ()(void, bool), void*) () at /usr/lib/libexiv2.so.26 7 0x00007ffff767ed8a in Exiv2::XmpParser::decode(Exiv2::XmpData&, std::cxx11::basic_string<char, std::char_traits, std::allocator > const&) () at /usr/lib/libexiv2.so.26 8 0x00007ffff75e5642 in Exiv2::JpegBase::readMetadata() () at /usr/lib/libexiv2.so.26 9 0x00007ffff7d794cf in () at /usr/lib/libgexiv2.so.2 10 0x00007ffff7d79879 in gexiv2_metadata_open_path () at /usr/lib/libgexiv2.so.2

This calls https://felixcrux.com/files/doc/gexiv2_sys/fn.gexiv2_initialize.html, which in turn calls https://gitlab.gnome.org/GNOME/gexiv2/blob/bde3a816807ad998f80127b8df0263b036fdc732/gexiv2/gexiv2-startup.h#L23-33, which calls http://www.exiv2.org/doc/classExiv2_1_1XmpParser.html#aea661a7039adb5a748eb7639c8ce9294, which mentions in its documentation:

Calling this method is usually not needed, as encode() and decode() will initialize the XMP Toolkit if necessary.

The initialize() function itself still is not thread-safe and needs to be called in a thread-safe manner (e.g., on program startup), but if used with suitable additional locking parameters, any subsequent registration of namespaces will be thread-safe.

felixc commented 5 years ago

Great catch; and thank you very much for the patch!

I wonder if there's a way to do this automatically, e.g. whenever someone creates a Metadata for the first time... but that can be an enhancement for another time!

If you'd like to, you should feel free to add yourself to the CONTRIBUTORS.md file, or I can go ahead and merge this as-is and add you myself if you'd prefer?

Thanks again for the contribution!

mineo commented 5 years ago

I've added myself to CONTRIBUTORS.md

I wasn't sure about the costs of moving the initialization into Metadatas various constructors, since I assume there's at least some sort of synchronization going on with std::sync::Once, which might add up for a large amount of calls.

felixc commented 5 years ago

Thank you again for this improvement! Merged!

felixc commented 5 years ago

Hey; just wanted to mention that I've just published a new version that includes this: https://github.com/felixc/rexiv2/releases/tag/v0.7.0