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.

Sign up for GitHub

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

Bindings: UnknownSchema added data property #1799

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bindings: UnknownSchema added data property #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/opentimelineio/unknownSchema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ class UnknownSchema : public SerializableObject
return _original_schema_version;
}

AnyDictionary& data() noexcept
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the intention to provide mutable access? Reading the original issue it talks about accessing dictionaries presumably for reading. A reference opens the door to lifetime issues where the reference to the AnyDictionary can outlive the UnknownSchema object.

I think it would be better to return a copy of the AnyDictionary, even though that incurs overhead, and if we want mutability provide a method to supply a new dictionary.

Alternatively, we could change _data to be a shared pointer, but treating the AnyDictionary as a value type would give safer concurrency, if we also added a mutex during the copy and replace.

We don't have much consideration for concurrency in otio at the moment, but it would make sense to me to think about it moving forward.

Does anyone (@jminor ?) have a sense of whether these dictionaries might tend to be massive (dozens or hundreds of entries), or are they more on the order of a few entries?

If the former, I think I'd bias to a shared_ptr solution and ignore concurrency inside the object; if the latter, return/set by value pattern with a mutex in anticipation of concurrency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@meshula Would a const & do the trick? Or you want a copy? And if its a copy should we call it data_copy() or something to disambiguate that you're not getting direct access to the child dictionary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding size: a typical OTIO object should be small, with possibly dozens of JSON key/values in metadata. However, given that this class, UnknownSchema, is meant for user defined schema types, they really could be anything. This is the place where we're encouraging developers to add interesting new stuff, so we should be open-ended about what might be in there.

That leads me to support your suggestion that the API be defensive about UnknownSchema by providing read-only access. That will address the desired workflow (making UnknownSchema introspectable) without opening the door to meddling with the contents.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool :) @ssteinbach const& doesn't protect against object lifetime issues, or concurrent modification, which I'm worried about. It would work if we had a notion of a holder on the container that also blocked writing while the container is held, but we haven't got that :]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there's a way to modify the contents of an UnknownSchema object via our API, except via read_from which fully replaces the contents, so I suspect concurrency is not an issue. If a developer wants to modify these things, then they can register a schema and get a real object instead of UnknownSchema.

Would lifetime issues be handled by returning a copy as suggested earlier in this thread?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, a copy solves lifetime issues :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, cool. @natchar could you alter data() to return a copy of _data?

Copy link
Author

Choose a reason for hiding this comment

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

If it is returning a copy of data, perhaps instead of it being a property, it should be a function definition so that the call is unknownSchema.data().

Maybe I'm missing something, but if I do change the function signature to AnyDictionary data() const noexcept that should return a copy of _data. However, in the python bindings we use AnyDictionaryProxy for AnyDictionary and running the test will yield Underlying C++ AnyDictionary has been destroyed. Is there another change I need to make to AnyDictionaryProxy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's reporting that the copy has been destroyed, right? So I guess we need to look at the binding, and make sure that's it's doing what we expect, and then remove the warning... Any other thoughts?

{
return _data;
}

bool read_from(Reader&) override;
void write_to(Writer&) const override;

Expand Down
3 changes: 3 additions & 0 deletions src/py-opentimelineio/opentimelineio-bindings/otio_serializableObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ static void define_bases1(py::module m) {
.def_property_readonly("is_unknown_schema", &SerializableObject::is_unknown_schema);

py::class_<UnknownSchema, SerializableObject, managing_ptr<UnknownSchema>>(m, "UnknownSchema")
.def_property_readonly("data", [](UnknownSchema* schema) {
auto ptr = schema->data().get_or_create_mutation_stamp();
return (AnyDictionaryProxy*)(ptr); }, py::return_value_policy::take_ownership)
.def_property_readonly("original_schema_name", &UnknownSchema::original_schema_name)
.def_property_readonly("original_schema_version", &UnknownSchema::original_schema_version);

Expand Down
11 changes: 11 additions & 0 deletions tests/test_unknown_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ def test_is_unknown_schema(self):
unknown = self.orig.media_reference.metadata["stuff"]
self.assertTrue(unknown.is_unknown_schema)

def test_unknown_to_dict(self):
unknown = self.orig.media_reference.metadata["stuff"]
self.assertTrue(unknown.is_unknown_schema)
self.assertEqual(
unknown.data,
{
"some_data": 895,
"howlongami": otio.opentime.RationalTime(rate=30, value=100)
}
)


if __name__ == '__main__':
unittest.main()
Loading