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

State store function allows null and duplicate keys #23

Closed
sadko4u opened this issue Jun 18, 2019 · 28 comments
Closed

State store function allows null and duplicate keys #23

sadko4u opened this issue Jun 18, 2019 · 28 comments

Comments

@sadko4u
Copy link

sadko4u commented Jun 18, 2019

The LV2_State_Store_Function does not analyze the type of the value.
Trying to submit an Atom:Object to it results into corrupted *.ttl file.

Code snippet:

LV2_Atom_Forge forge;
LV2_State_Store_Function hStore;
LV2_URID_Map *map;
const char *path = "/home/sadko/eclipse/lsp-plugins/res/test/3d/devel-room.obj";
                
    LV2_State_Status lv2_save_state(
        LV2_Handle                 instance,
        LV2_State_Store_Function   hStore,
        LV2_State_Handle           handle,
        uint32_t                   flags,
        const LV2_Feature *const * features)
    {

                LV2_Atom *msg       = lv2_atom_forge_object(&forge, &frame, 0, map->map(map->handle, "http://lsp-plug.in/types/lv2/types#KVT"));
                lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/selected"));
                lv2_atom_forge_int(&forge, int32_t(0));
                lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/objects"));
                lv2_atom_forge_int(&forge, int32_t(0));
                lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/object/0/enabled"));
                lv2_atom_forge_float(&forge, 1.0f);
                lv2_atom_forge_pop(&forge, &frame);
                hStore(handle, 0, &msg[1], msg->size, forge.Object, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);

                hStore(handle, map->map(map->handle, "http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn"), path, strlen(path) + 1, forge.Path, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);
    }

The expected result:

	state:state [
		a <http://lsp-plug.in/types/lv2/types#KVT> [
			<http://lsp-plug.in/kvt/scene/selected> "0"^^xsd:int ;
			<http://lsp-plug.in/kvt/scene/objects> "0"^^xsd:int ;
			<http://lsp-plug.in/kvt/scene/object/0/enabled> "1.0"^^xsd:float
		] ;
		<http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/sadko/eclipse/lsp-plugins/res/test/3d/devel-room.obj>
	] .

The actual result:

	state:state [
		a <http://lsp-plug.in/types/lv2/types#KVT> ;
		<http://lsp-plug.in/kvt/scene/selected> "0"^^xsd:int ;
		<http://lsp-plug.in/kvt/scene/objects> "0"^^xsd:int ;
		<http://lsp-plug.in/kvt/scene/object/0/enabled> "1.0"^^xsd:float
	] .

<http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/sadko/eclipse/lsp-plugins/res/test/3d/devel-room.obj> .

According to specification:

Atoms should be used anywhere values of various types must be stored or transmitted.

Actually, we can not submit something more complicated than plain data types.

@drobilla
Copy link
Collaborator

You are passing a zero (null) key for the object. This doesn't make sense, but I realize now that lilv does not check for that correctly. I will fix it to return an error in this case (no idea why you get this really odd result specifically, but that doesn't really matter).

Also your "expected" output isn't syntactically valid. The blank node with the selected, objects, etc properties has no predicate. The bug here is that lilv does anything at all other than report an error.

You don't need to (i.e. can't) construct an object atom for (almost) the whole state like this anyway. The way to store a bunch of properties for your state is to call the callback once for every property. Each call to the store callback corresponds to a property on your state object. This means you don't have to screw around with the forge as much. Drop the object construction and you should get sensible output.

Building complex objects with a forge is only necessary when you have values for state properties that are complex objects. They should work fine, though, all POD atoms can round-trip to a serialization and back.

@sadko4u
Copy link
Author

sadko4u commented Jun 20, 2019

The problem is that I can not save properties which will have generated URIs because there is no way to get the full list of URIs when deserializing the state. That's why I was forced to use objects and, surprise, found that they don't work, too. Then I rewrote the whole bunch of code to save keys as a long set of strings separated by the ':' character.
But it's actually a bad trick. We currently can not control the whole contents of the plugin's LV2:State from the side of plugin when reading the state. And that's sad.
I've created the topic: lv2/lv2#37

@drobilla
Copy link
Collaborator

You seem to be working with the misconception that the state API lets you save and load a single complete description (object) of the state. This is not how it works. You don't create one on save, and you don't get one on restore (one never even exists, actually, it's not implemented as an atom in lilv).

The state extension lets you save properties explicitly, and load those properties explicitly. So do that. If you want to save and restore some complete object for whatever reason, then save and restore a property with that value.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

@drobilla , I understand how the LV2:State works. You don't understand what I want to finally get.
Consider you have a hash table where the key is string and value is some primitive type. How can I store this hash table using LV2:State interface?
Alright, I can generate a special URI for each key and store it. But how then I will restore the whole hash table from the LV2 State? I can not predict the URIs of properties that I need to fetch from the LV2:State explicitly. And LV2:State does not provide interface for iterating all stored properties instead of fetching them by URID.
The solution could be easier if I could generate an LV2 Object with fixed URI and store all key-value pairs as properties of this object. But the LV2:State interface does not properly handle object as a value that can be stored in the TTL file.
So actually, current solution that really works is to store a long string containing all keys, parse it at the State deserialization stage, generate URIs and then fetch all these URIs into my hash table.
This is not beautiful and, moreover, very tricky solution that I don't like but currently I have no other way to deal with it.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

Maybe I'm wrong in syntax but why I can not finally get the following TTL structure:

....
state:state [
		<http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT> [
                        a <http://lsp-plug.in/types/lv2/types#KVT>;
			<http://lsp-plug.in/kvt/scene/selected> "0"^^xsd:int ;
			<http://lsp-plug.in/kvt/scene/objects> "0"^^xsd:int ;
			<http://lsp-plug.in/kvt/scene/object/0/enabled> "1.0"^^xsd:float
		] ;
		<http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/sadko/eclipse/lsp-plugins/res/test/3d/devel-room.obj>
	] .

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

This code snippet:

            lv2_atom_forge_set_buffer(&forge, buf, sizeof(buf));
            lv2_atom_forge_object(&forge, &frame, 0, map->map(map->handle, "http://lsp-plug.in/types/lv2/types#KVT"));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/selected"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/objects"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/object/0/enabled"));
            lv2_atom_forge_float(&forge, 1.0f);

            LV2_Atom *msg           = reinterpret_cast<LV2_Atom *>(buf);

            lv2_atom_forge_pop(&forge, &frame);
            store(handle, map->map(map->handle, "http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT"),
                    buf, lv2_atom_total_size(msg),
                    forge.Property, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);

Gives the following result:

        state:state [
                <http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT> [
                        a atom:Property ;
                        rdf:value """UAAAAAcAAAAAAAAA3wEAAN4BAAAAAAAABAAAACQAAAAAAAAAAAAAAN0BAAAAAAAABAAAACQAAAAA
AAAAAAAAAGkBAAAAAAAABAAAAAgAAAAAAIA/AAAAAA=="""^^xsd:base64Binary
                ] .

Which actually is bad.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

This code snippet:

            lv2_atom_forge_set_buffer(&forge, buf, sizeof(buf));
            lv2_atom_forge_object(&forge, &frame, 0, map->map(map->handle, "http://lsp-plug.in/types/lv2/types#KVT"));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/selected"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/objects"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/object/0/enabled"));
            lv2_atom_forge_float(&forge, 1.0f);

            LV2_Atom *msg           = reinterpret_cast<LV2_Atom *>(buf);

            lv2_atom_forge_pop(&forge, &frame);
            store(handle, map->map(map->handle, "http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT"),
                    &buf[1], msg->size,
                    forge.Object, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);

Yields to corrupted TTL file:

<>
        a pset:Preset ;
        lv2:appliesTo <http://lsp-plug.in/plugins/lv2/room_builder_mono> ;
        state:state [
                <http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/vsadovnikov/eclipse/lsp-plugins/res/test/3d/devel-room.obj>rdf:value "AA=="^^xsd:base64Binary
        ] .

rdf:value "AA=="^^xsd:base64Binary .

rdf:value "AA=="^^xsd:base64Binary .

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

This snippet:

            lv2_atom_forge_set_buffer(&forge, buf, sizeof(buf));
            lv2_atom_forge_object(&forge, &frame, 0, map->map(map->handle, "http://lsp-plug.in/types/lv2/types#KVT"));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/selected"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/objects"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/object/0/enabled"));
            lv2_atom_forge_float(&forge, 1.0f);

            LV2_Atom *msg           = reinterpret_cast<LV2_Atom *>(buf);

            lv2_atom_forge_pop(&forge, &frame);
            store(handle, map->map(map->handle, "http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT"),
                    msg, lv2_atom_total_size(msg),
                    forge.Object, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);

Also yields to corrupted TTL:

<>
        a pset:Preset ;
        lv2:appliesTo <http://lsp-plug.in/plugins/lv2/room_builder_mono> ;
        state:state [
                <http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/vsadovnikov/eclipse/lsp-plugins/res/test/3d/devel-room.obj><http://opensoundcontrol.org/spec-1_0Packet>
                a atom:Objectrdf:value """BAAAACQAAAAAAAAAAAAAAGgBAAAAAAAABAAAACQAAAAAAAAAAAAAAGkBAAAAAAAABAAAAAgAAAAA
AIA/AAAAACEAAAAAAAAAd2F2ZS1hbXBsaXR1ZGUtem9vbQAAAAAAQQAAAAAAAAABAAAAAQAAAKYI
AAD0fwAAoDReCgAAAAAAAAAAAAAAADASXgoAAAAAAAAAAAAAAABAAAAAAAAAAAECAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAGddAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAADwIl4K
AAAAAGhdAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAACgIl4KAAAAAHVdAAAA
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAwK14KAAAAAHRdAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAABAAA="""^^xsd:base64Binary
        ] .

@drobilla
Copy link
Collaborator

drobilla commented Jun 21, 2019

You pass the body of the atom, not the whole thing with a header (which is why you can pass a float directly, for example). Pass LV2_ATOM_BODY_CONST(msg) (or just msg + 1 assuming it's an LV2_Atom*), msg->type and msg->size to store.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

You pass the body of the atom, not the whole thing with a header (which is why you can pass a float directly, for example). Pass LV2_ATOM_BODY_CONST(msg) (or just msg + 1 assuming it's an LV2_Atom*), msg->type and msg->size to store.

I've submitted the code snippet for the case you've described here:
#23 (comment)

It's not working.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

Aaah, my failure. This snippet seems to be working:

            lv2_atom_forge_set_buffer(&forge, buf, sizeof(buf));
            lv2_atom_forge_object(&forge, &frame, 0, map->map(map->handle, "http://lsp-plug.in/types/lv2/types#KVT"));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/selected"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/objects"));
            lv2_atom_forge_int(&forge, int32_t(0));
            lv2_atom_forge_key(&forge, map->map(map->handle, "http://lsp-plug.in/kvt/scene/object/0/enabled"));
            lv2_atom_forge_float(&forge, 1.0f);

            LV2_Atom *msg           = reinterpret_cast<LV2_Atom *>(buf);

            lv2_atom_forge_pop(&forge, &frame);
            store(handle, map->map(map->handle, "http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT"),
                    &msg[1], msg->size,
                    msg->type, LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE);

Here's what I've expected:

<>
        a pset:Preset ;
        lv2:appliesTo <http://lsp-plug.in/plugins/lv2/room_builder_mono> ;
        state:state [
                <http://lsp-plug.in/plugins/lv2/room_builder_mono/ports#ifn> <file:///home/vsadovnikov/eclipse/lsp-plugins/res/test/3d/devel-room.obj> ;
                <http://lsp-plug.in/plugins/lv2/room_builder_mono#KVT> [
                        a <http://lsp-plug.in/types/lv2/types#KVT> ;
                        <http://lsp-plug.in/kvt/scene/selected> "0"^^xsd:int ;
                        <http://lsp-plug.in/kvt/scene/objects> "0"^^xsd:int ;
                        <http://lsp-plug.in/kvt/scene/object/0/enabled> "1.0"^^xsd:float
                ]
        ] .

@drobilla
Copy link
Collaborator

That looks reasonable.

Isn't lack of type safety fun?! Good C++ bindings would make this way less of a nightmare for C++ plugins, but unfortunately those aren't here yet...

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

That looks reasonable.

Isn't lack of type safety fun?! Good C++ bindings would make this way less of a nightmare for C++ plugins, but unfortunately those aren't here yet...

These C++ bindings are not necessary and, moreover, they will be inefficient. The lack of good examples is the main reason of failures. AFAIK nobody before me saved Atom objects into TTL files.

@drobilla
Copy link
Collaborator

These C++ bindings are not necessary

This thread suggests otherwise.

they will be inefficient

Not necessarily, and in the case of state, a bit of wrapper overhead (if there was any) would be insignificant anyway.

The lack of good examples is the main reason of failures. AFAIK nobody before me saved Atom objects into TTL files.

Sure. I don't know about "the main reason", but an official example that does this would be good.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

When restoring the object's state from the LV2:State TTL file, the object becomes duplicated:
https://pastebin.com/igVTp833

Here's the part of the log (You can see duplications of 'plain/text' and 'Test text' in binary dump):
https://pastebin.com/KL1RDRQz

The related code:
https://github.com/sadko4u/lsp-plugins/blob/dcb26b35829f1e983d2c33ec01381e1663204a90/include/container/lv2/wrapper.h#L1659-L1783

@drobilla
Copy link
Collaborator

1bce600

@drobilla
Copy link
Collaborator

Constructing URIs like this is pretty questionable, you'd probably be better off with a list if that's what your state is really like.

Anyway, there is way too much going on here for me to tell, but I'd guess that your code is constructing it that way. You can use sratom to print atoms which might help clarify what is going on.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

Constructing URIs like this is pretty questionable, you'd probably be better off with a list if that's what your state is really like.

Sadly, but I can not predict how many 3D objects there are in a 3D scene. And each object has it's own set of properties. And if some other plugin will use Key-Value Tree storage, then I actually have no information about what keys can be used. The only thing I can say that these keys will be compatible to OSC address.

Anyway, there is way too much going on here for me to tell, but I'd guess that your code is constructing it that way. You can use sratom to print atoms which might help clarify what is going on.

In a binary dump which is taken by retrieve() is visible that Atom object is twice duplicated. The dump is logged directly after the buffer has been retrieved.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

I have no expirience with sratom, maybe you'll reproduce the case easier by trying to parse the original *.ttl file.

@drobilla
Copy link
Collaborator

This is very definitely not a problem with parsing the file.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

I think I can also ask @x42 because I'm trying all things in Ardour 5.12 and it is reproducible there.

@drobilla
Copy link
Collaborator

Loading that preset does not produce the "duplication" for me.

@sadko4u
Copy link
Author

sadko4u commented Jun 21, 2019

Can not reproduce in jalv.gtk. In Ardour still reproducible.

@x42
Copy link
Contributor

x42 commented Jun 22, 2019 via email

@drobilla
Copy link
Collaborator

I am skeptical that Ardour is actually loading the same data. Are you positive?

@sadko4u
Copy link
Author

sadko4u commented Jun 22, 2019

@x42 do I right understand that Ardour is using system lilv library if it is possible?
Currently I can not reproduce the problem on my home computer, the binary dump seems to be OK and I see only one BLOB object there:

Contents of the atom object::
00000000: 00 00 00 00 3c 00 00 00 68 01 00 00 00 00 00 00    ....<...h.......
00000010: 48 00 00 00 07 00 00 00 00 00 00 00 3d 00 00 00    H...........=...
00000020: 3f 00 00 00 00 00 00 00 0a 00 00 00 01 00 00 00    ?...............
00000030: 54 65 73 74 20 74 65 78 74 00 00 00 00 00 00 00    Test text.......
00000040: 3e 00 00 00 00 00 00 00 0b 00 00 00 29 00 00 00    >...........)...
00000050: 74 65 78 74 2f 70 6c 61 69 6e 00 00 00 00 00 00    text/plain......
00000060: 6a 01 00 00 00 00 00 00 04 00 00 00 08 00 00 00    j...............
00000070: d0 cc 4c be 00 00 00 00 6b 01 00 00 00 00 00 00    ..L.....k.......
00000080: 04 00 00 00 08 00 00 00 c8 cc 4c 3e 00 00 00 00    ..........L>....
00000090: 6c 01 00 00 00 00 00 00 04 00 00 00 08 00 00 00    l...............
000000a0: c3 cc cc bd 00 00 00 00 6d 01 00 00 00 00 00 00    ........m.......
000000b0: 04 00 00 00 08 00 00 00 00 00 00 00 00 00 00 00    ................
000000c0: 6e 01 00 00 00 00 00 00 04 00 00 00 08 00 00 00    n...............
000000d0: 00 00 80 3f 00 00 00 00 6f 01 00 00 00 00 00 00    ...?....o.......

I can check the behaviour on another machine where I've found this. But it's not earlier than monday, so we need to wait a bit.

@sadko4u
Copy link
Author

sadko4u commented Jun 24, 2019

Reproduced the problem. It's not a problem of Ardour or lilv. I forgot that for test purposes I've added unconditional save of a BLOB object to the code, so on each save I got additional one object in TTL file:

                        <http://lsp-plug.in/kvt/TEST_BLOB> [
                                a <http://lsp-plug.in/types/lv2/types#Blob> ;
                                <http://lsp-plug.in/types/lv2/Blob#ContentType> "text/plain" ;
                                <http://lsp-plug.in/types/lv2/Blob#Content> "VGVzdCB0ZXh0AA=="^^xsd:base64Binary
                        ] , [
                                a <http://lsp-plug.in/types/lv2/types#Blob> ;
                                <http://lsp-plug.in/types/lv2/Blob#ContentType> "text/plain" ;
                                <http://lsp-plug.in/types/lv2/Blob#Content> "VGVzdCB0ZXh0AA=="^^xsd:base64Binary
                        ]

Sorry, my mistake

@drobilla drobilla changed the title LV2_State_Store_Function does not properly handle Objects State store function allows null and duplicate keys Jun 24, 2019
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

No branches or pull requests

3 participants