LV2 State Extension path mapping duplicated feature

[swesterfeld]

To convert the state of an instance to a string, this code is used as a first step

        LilvState *state = lilv_state_new_from_instance(
                lv2_plugin(), m_ppInstances[0], &g_lv2_urid_map,
                nullptr, nullptr, nullptr, nullptr,
                qtractor_lv2_get_port_value, this,
                LV2_STATE_IS_POD | LV2_STATE_IS_PORTABLE, m_lv2_features);

If the LV2 plugin stores paths, you probably want the plugin to use the LV2_STATE__mapPath feature to translate the paths during generating the state. So you pass nullptr for all the dirs, but provide m_lv2_features so that the plugin uses your path translation.

However, lilv_state_new_from_instance unconditionally adds its own LV2_STATE__mapPath feature like this:

  LV2_State_Map_Path  pmap          = {state, abstract_path, absolute_path};
  LV2_Feature         pmap_feature  = {LV2_STATE__mapPath, &pmap};
  LV2_State_Make_Path pmake         = {state, make_path};
  LV2_Feature         pmake_feature = {LV2_STATE__makePath, &pmake};
  LV2_State_Free_Path pfree         = {NULL, lilv_free_path};
  LV2_Feature         pfree_feature = {LV2_STATE__freePath, &pfree};
  features = sfeatures = add_features(
    features, &pmap_feature, save_dir ? &pmake_feature : NULL, &pfree_feature);

So now there are two instances of the LV2_STATE__mapPath feature, and unless there is a rule about which the plugin should use in this case (which I am not aware of), it might use the first one or the second one, which means that maybe it would use your version or not.

Not sure how to fix this; maybe it could also be fixed in lilv.

[rncbc]

yes, it probably uses the first it encounters in given m_lv2_features, as qtractor does have its own LV2_STATE__mapPath hook; it doesn't use any of the other though.

do you have any specific concern on this or wish to propose a fix anyhow?

[swesterfeld]

No, its not simply a matter of lilv using the first one. It passes the array with both LV2_STATE__mapPath features to the LV2 plugin that is being saved. For my SpectMorph plugin, it would use the second one:

https://github.com/swesterfeld/spectmorph/blob/22b34433df7f079206e5a5874ada381cd61e1f5b/lv2/smlv2plugin.cc#L296

which means that the "lilv" implementation and not your implementation gets used. I don't consider this a SpectMorph bug, because really I wouldn't expect to get the same feature passed twice, unless the LV2 spec would clarify on this. So as it stands the qtractor saving doesn't work for SpectMorph and even if I added a workaround for that (which I don't think is the right thing to do) there may be other plugins which likewise get confused here.

[swesterfeld]

Here is another piece of code that is taking the "wrong" mapPath:

https://github.com/DISTRHO/DPF/blob/63dfb7610bc37dee69f4a303f3e3362529d95f24/distrho/src/DistrhoPluginLV2.cpp#L970

I think my concern is that you cannot really convince all plugin developers to agree on the one true strategy to deal with duplicated features. So it is either qtractor cannot use lilv_state_new_from_instance or the function needs to be altered in lilv.

[rncbc]

yeah you're certainly right; it all actually depends on how the lilv::add_features(() works, whether to append or prepend the lilv local map_features etc. it even might have changed across lilv releases because i'm sure this wrong doing wasn't there a few releases back.

guess that it means that qtractor must have its own lilv_state_new_from_instance() and scrap the provided one; lilv should check if the caller already provides its own features (same URI) before duplicating them--but that's probably just me.

thanks for the heads up.

[rncbc]

my take for the fix in lilv side:

diff --git a/src/state.c b/src/state.c
index c8e8f14..9c284fb 100644
--- a/src/state.c
+++ b/src/state.c
@@ -378,6 +378,12 @@ add_features(const LV2_Feature* const* features,
 {
   size_t n_features = 0;
   for (; features && features[n_features]; ++n_features) {
+    if (map && strcmp(features[n_features]->URI, LV2_STATE__mapPath) == 0)
+      map = NULL;
+    if (make && strcmp(features[n_features]->URI, LV2_STATE__makePath) == 0)
+      make = NULL;
+    if (free && strcmp(features[n_features]->URI, LV2_STATE__freePath) == 0)
+      free = NULL;
   }
 
   const LV2_Feature** ret =

suppose this would solve the duplicates for good. :) what do you think?

[swesterfeld]

Yes, I think your fix is good. On Linux/macOS it should just work in any situation. On Windows, unfortunately there is this issue: lv2/lilv#14 which means that if you mix lilv path allocation with host freePath implementation (or the other way around) you're in trouble. But I think that is solvable on the host side with a two easy rules:

1. if your host provides mapPath, always also provide freePath
2. if your host provides mapPath for lilv_state_new_from_instance, set all directories to NULL

So yes, I think your solution is appropriate to fix the problem.

[rncbc]

thanks,

an MR has been submitted to lilv upstream:
https://gitlab.com/lv2/lilv/-/merge_requests/2

ps. maybe you (and others) may help and up-vote the GL ticket? please?

[rncbc]

Here is another piece of code that is taking the "wrong" mapPath:

https://github.com/DISTRHO/DPF/blob/63dfb7610bc37dee69f4a303f3e3362529d95f24/distrho/src/DistrhoPluginLV2.cpp#L970

and here's the code that mitigates and most probably resolves to the right feature being picked up:

	const LV2_State_Map_Path* mapPath = nullptr;
	const LV2_State_Free_Path* freePath = nullptr;
	for (int i=0; features[i] != nullptr; ++i)
	{
		if (mapPath == nullptr
			&& std::strcmp(features[i]->URI, LV2_STATE__mapPath) == 0)
			mapPath = (const LV2_State_Map_Path*)features[i]->data;
		else
		if (freePath == nullptr
			&& std::strcmp(features[i]->URI, LV2_STATE__freePath) == 0)
			freePath = (const LV2_State_Free_Path*)features[i]->data;
	}

[rncbc]

No, its not simply a matter of lilv using the first one. It passes the array with both LV2_STATE__mapPath features to the LV2 plugin that is being saved. For my SpectMorph plugin, it would use the second one:

https://github.com/swesterfeld/spectmorph/blob/22b34433df7f079206e5a5874ada381cd61e1f5b/lv2/smlv2plugin.cc#L296

in your case this would suffice:

  LV2_State_Map_Path *map_path = nullptr;
#ifdef LV2_STATE__freePath
  LV2_State_Free_Path *free_path = nullptr;
#endif
  for (int i = 0; features[i]; i++)
    {
      if (!map_path && !strcmp (features[i]->URI, LV2_STATE__mapPath))
        {
          map_path = (LV2_State_Map_Path *)features[i]->data;
        }
#ifdef LV2_STATE__freePath
      else if (!free_path && !strcmp (features[i]->URI, LV2_STATE__freePath))
        {
          free_path = (LV2_State_Free_Path *)features[i]->data;
        }
#endif
    }