topology: pre-processor: Introduce extends/overrides keywords for classes #268

ranj063 · 2024-05-22T17:07:30Z

Introduce a new keyword "Subtreecopy" for extending existing conf nodes
with additional nodes. This feature is useful for extending previous
pipeline class definitions with the addition of one or more widgets
without having to duplicate everything in the new class definition.

For example: Consider a pipeline class definition as below. Note that
only the widgets & routes are shown here.

Class.Pipeline.mixout-gain-dai-copier-playback {
	Object.Widget {
		mixout."1" {}
		dai-copier."1" {}
		gain."1" {}
		pipeline."1" {}
	}

	Object.Base {
		!route [
			{
				source mixout.$index.1
				sink	gain.$index.1
			}
		]
	}
}

If we want to extend this pipeline with the addition of an eqiir/eqfir,
we can create a Subtreecopy node with type extend as follows:

Subtreecopy.mixout-gain-eqiir-eqfir-dai-copier-playback {
	source "Class.Pipeline.mixout-gain-dai-copier-playback"
	target "Class.Pipeline"
	type extend

	Object.Widget {
		eqiir.1 {}
		eqfir.1 {}
	}

	Object.Base {
		!route [
			{
				source gain.$index.1
				sink   eqiir.$index.1
			}
			{
				source eqiir.$index.1
				sink   eqfir.$index.1
			}
		]
	}
}

But if we want to modify an existing pipeline class while modifying the
order of widgets and/or inserting new widgets, we should use the type
"override" instead. This allows for adding new widgets to the list of
widgets in the base class definition while also allowing overriding the
routes to allow inserting the new widgets and reordering the widgets in
the base class. For example, if we want to add a drc widget between the
gain and the eqiir modules in the above class, we can do the following:

Subtreecopy.mixout-efx-dai-copier-playback {
	source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback"
	target "Class.Pipeline"
	type override

	Object.Widget {
		drc.1 {}
	}

	Object.Base {
		!route [
			{
				source mixout.$index.1
				sink	gain.$index.1
			}
			{
				source gain.$index.1
				sink	drc.$index.1
			}
			{
				source	drc.$index.1
				sink	eqiir.$index.1
			}
			{
				source	eqiir.$index.1
				sink	eqfir.$index.1
			}
		]
	}
}

Note that the routes array contains all the routes in the new subtreecopy definition.

This subtreecopy feature can also be used to copy normal config blocks
without having the need to duplicate information.

For example, if all the widgets in a pipeline support the same audio
formats, we can define it as follows:

Class.Pipeline.dai-copier-eqiir-module-copier-capture {
	formats {
		num_input_audio_formats 2
		num_output_audio_formats 2

		Object.Base.input_audio_format [
			{
				in_bit_depth		32
				in_valid_bit_depth	32
			}
			{
				in_channels		4
				in_bit_depth		32
				in_valid_bit_depth	32
				in_ch_cfg	$CHANNEL_CONFIG_3_POINT_1
				in_ch_map	$CHANNEL_MAP_3_POINT_1
			}
		]

		Object.Base.output_audio_format [
			{
				out_bit_depth		32
				out_valid_bit_depth	32
			}
			{
				out_channels		4
				out_bit_depth		32
				out_valid_bit_depth	32
				out_ch_cfg	$CHANNEL_CONFIG_3_POINT_1
				out_ch_map	$CHANNEL_MAP_3_POINT_1
			}
		]
	}

	Object.Widget {
		dai-copier."1" {
			type dai_out
			num_output_pins 1

			Subtreecopy.formats {
				source "Class.Pipeline.dai-copier-eqiir-module-copier-capture.formats"
				type override
			}
		}

		eqiir."1" {
			Subtreecopy.formats {
				source "Class.Pipeline.dai-copier-eqiir-module-copier-capture.formats"
				type override
			}

			Object.Control.bytes."1" {
				IncludeByKey.DMIC0_DAI_EQIIR {
					"passthrough"		"include/components/eqiir/passthrough.conf"
					"highpass_40hz_0db"	"include/components/eqiir/highpass_40hz_0db_48khz.conf"
					"highpass_40hz_20db"	"include/components/eqiir/highpass_40hz_20db_48khz.conf"
				}
			}
		}

		module-copier."2" {
			Subtreecopy.formats {
				source "Class.Pipeline.dai-copier-eqiir-module-copier-capture.formats"
				type override
			}
		}

		pipeline."1" {
			priority	0
			lp_mode		0
		}
	}
}

The subtreecopy node in each widget ensures that the pipeline formats
are applied to all widgets without the need for duplicating them for
each widget.

Link: thesofproject/sof#9082

ranj063 · 2024-05-22T17:08:16Z

@plbossart @lrgirdwo @jsarha FYI

plbossart · 2024-05-22T17:12:50Z

Thanks @ranj063
This feature would simplify the life of maintainers and 3rd parties, we have way too much duplication of entire pipelines when we add an additional processing element. The use of tried-and-tested templates that can be simply extended in a nested manner is probably the best thing since we moved over from M4 to topology2!

perexg · 2024-05-22T17:26:55Z

Could we just work with whole configuration blocks? This feature may be useful for all config parts. Like:

SubtreeCopy.AnUniqueId {
  type override
  source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback"
  target "Class.Pipeline.mixout-efx-dai-copier-playback"
  tree {
    Object.Widget {
	drc.1 {}
    }

    Object.Base {
         !route [
            ...
         ]
    } 
  }
}

ranj063 · 2024-05-22T17:44:01Z

Could we just work with whole configuration blocks? This feature may be useful for all config parts. Like:

SubtreeCopy.AnUniqueId {
  type override
  source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback"
  target "Class.Pipeline.mixout-efx-dai-copier-playback"
  tree {
    Object.Widget {
	drc.1 {}
    }

    Object.Base {
         !route [
            ...
         ]
    } 
  }
}

@perexg since we'd like to keep the new sub class definitions in separate conf files, the uniqueid might be problematic. But maybe if I make it an array like below, it might be OK.
Subtreecopy [
{
....
}
]

Also, can I retain both the extends/overrides type for the subtreecopy to achieve both pipeline extensions and pipeline widget insertion+reordering for Pipeline classes. For other configs, this could work as a generic config block copy.

perexg · 2024-05-22T18:13:31Z

@perexg since we'd like to keep the new sub class definitions in separate conf files, the uniqueid might be problematic. But maybe if I make it an array like below, it might be OK. Subtreecopy [ { .... } ]

Unique ID may be like "Pipeline-mixout-efx-dai-copier-playback" so I don't see a problem here. Using [] (arrays) will assign IDs from the index range (0 ... ) automatically.

Also, can I retain both the extends/overrides type for the subtreecopy to achieve both pipeline extensions and pipeline widget insertion+reordering for Pipeline classes. For other configs, this could work as a generic config block copy.

The merge type for the new tree update may be specified using a field - I proposed type in my example.

plbossart · 2024-05-22T18:36:29Z

Not sure I follow what the uniqueID is, it looks redundant with the target in the suggested example?

SubtreeCopy.Pipeline-mixout-efx-dai-copier-playback {
  type override
  source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback"
  target "Class.Pipeline.mixout-efx-dai-copier-playback"

ranj063 · 2024-05-22T23:40:37Z

@perexg I have modified the PR to define the Subtreecopy keyword now. Please let me know what you think of this. Thanks!

perexg · 2024-05-23T10:39:57Z

Not sure I follow what the uniqueID is, it looks redundant with the target in the suggested example?
SubtreeCopy.Pipeline-mixout-efx-dai-copier-playback {
  type override
  source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback"
  target "Class.Pipeline.mixout-efx-dai-copier-playback"

It's just up to the user to create a unique ID scheme. You can compose any string. If you use one key for multiple configs, the last value is used:

a.a "1"
a.a "2"

The result is that a.a = "2" when the configuration tree is parsed.

perexg · 2024-05-23T10:56:06Z

topology/pre-processor.c

+			return ret;
+		}
+
+		ret = snd_config_merge(tmp, n, override);


Why is this required? Only source_tree should be merged to the target_tree here.

Probably a missing lookup for the tree field before.

perexg · 2024-05-23T10:58:00Z

topology/pre-processor.c

+	snd_config_t *subtrees;
+	int ret;
+
+	ret = snd_config_search(curr, "Subtreecopy", &subtrees);


We have already special blocks like CombineArrays, thus using upper cases in the middle is fine. I would prefer SubtreeCopy or SubTreeCopy here or even TreeCopy or ConfigCopy (may be a good alternative).

perexg · 2024-05-23T11:04:12Z

topology/pre-processor.c

+		/* get the type of copy ex: override/extend */
+		ret = snd_config_search(n, "type", &type_cfg);
+		if (ret < 0) {
+			SNDERR("failed to get type for subtree %s\n", id);


I would keep the type field as optional with a default (override ?).

perexg · 2024-05-23T11:07:23Z

Ok, I would remove all class.pipeline specific code. It is not required IMHO. See:

Class.Pipeline.mixout-efx-dai-copier-playback {
       Object.Base {
         !route [
            ...
         ]
    }
}

SubtreeCopy.add-to-mixout-efx {
  type override
  source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback.Object.Widget"
  target "Class.Pipeline.mixout-efx-dai-copier-playback.Object.Widget"
}

perexg · 2024-05-23T11:09:59Z

topology/pre-processor.c

+
+			ret = snd_config_search(top, target_id, &target_tree);
+			if (ret < 0) {
+				SNDERR("failed to get target tree %s\n", target_id);


You should create an empty compound for the given target path (multi id) here.

@perexg I think I have addressed all your comments except this one. Im not clear why I need to create an empty compound here. I'm creating a new conf block with the source tree and the subtree merged with/without override and then I merge the merged tree to the target tree.

But the target subtree in the top tree may not exist yet and it's not an error. In this case, an empty tree with target_id path should be created to work with it in the merge operation. Probably the easiest implementation is to use snd_config_load_string with "<target_id> {}" string as source and merge the result to top and do new snd_config_search for target_id / target_tree.

ack, fixed now

perexg · 2024-05-24T09:21:39Z

BTW: I like your nested examples. They show the power for the subtree copying.

perexg · 2024-05-24T09:23:20Z

topology/pre-processor.c

+		}
+
+		/* delete the source/target/type nodes */
+		ret = snd_config_delete(source_cfg);


snd_config_delete(subtrees) does this job for the whole processed SubTreeCopy block. I think that this step is extra.

perexg · 2024-05-24T09:27:16Z

topology/pre-processor.c

+			return ret;
+
+		if (type) {
+			ret = snd_config_delete(type_cfg);


I would recheck all error paths and the resource deallocation in this function. There are many resource leaks IMHO.

The only 2 resources allocated are for the temporary target ID and the temporary node while merging the source tree. I think I have freed both of them in the error paths.

perexg · 2024-05-24T12:23:07Z

The tree field removal means data mixup (what if "source" id is a valid id in the copied sub-tree for example?). My proposal was to have SubTreeCopy configuration separated.

ranj063 · 2024-05-28T16:38:19Z

The tree field removal means data mixup (what if "source" id is a valid id in the copied sub-tree for example?). My proposal was to have SubTreeCopy configuration separated.

@perexg Sorry for the delay, it was a long weekend in the US. I removed the target/source/type fields from the SUbTreeCOpy node explicitly so that if those fields are valid in the subtree being copied, they will be retained.

perexg · 2024-05-31T11:53:26Z

The tree field removal means data mixup (what if "source" id is a valid id in the copied sub-tree for example?). My proposal was to have SubTreeCopy configuration separated.

@perexg Sorry for the delay, it was a long weekend in the US. I removed the target/source/type fields from the SUbTreeCOpy node explicitly so that if those fields are valid in the subtree being copied, they will be retained.

Yes, but you cannot override them later then. In my eyes, it would be better to keep the source subtree completely separated (although it requires a new tab level).

ranj063 · 2024-05-31T15:40:11Z

Yes, but you cannot override them later then. In my eyes, it would be better to keep the source subtree completely separated (although it requires a new tab level).

@perexg I'm not sure I'm following your comment. Could you please elaborate what you're thinking here?

perexg · 2024-06-10T08:46:27Z

Do not mix SubTreeCopy configuration with the added/modified tree.

SubTreeCopy.### {
  type ###
  source ###
  target ###
  tree {
    ### changes here ###
  }
}

Vs. your current implementation:

SubTreeCopy.### {
  type ###
  source ###
  target ###
  ### changes here ###
}

ranj063 · 2024-06-10T18:35:00Z

Do not mix SubTreeCopy configuration with the added/modified tree.
SubTreeCopy.### {
  type ###
  source ###
  target ###
  tree {
    ### changes here ###
  }
}
Vs. your current implementation:
SubTreeCopy.### {
  type ###
  source ###
  target ###
  ### changes here ###
}

Thanks @perexg. Updated now

ranj063 · 2024-06-19T16:20:23Z

@perexg can I please bother you to have another look at this PR? I've addressed all your comments.

perexg

Please, do the last little change. Thank you.

perexg · 2024-06-21T14:35:15Z

topology/pre-processor.c

+		/* get the subtree to be merged. create an empty tree if it dosen't exist */
+		ret = snd_config_search(n, "tree", &subtree_cfg);
+		if (ret < 0) {
+			char *s;


It's too overkill. Just assign subtree_cfg = NULL here. The function snd_config_merge does nothing when src == NULL.

@perexg sorry for the delay. I was out on vacation. Fixed now.

@perexg Could I please request to have a look at the PR again?

Introduce a new kyword "SubTreeCopy" for extneding existing conf nodes with additional nodes. This feature is useful for extending previous pipeline class definitions with the addition of one or more widgets without having to duplicate everything in the new class definition. For example: Consider a pipeline class definition as below. Note that only the widgets & routes are shown here. Class.Pipeline.mixout-gain-dai-copier-playback { Object.Widget { mixout."1" {} dai-copier."1" {} gain."1" {} pipeline."1" {} } Object.Base { !route [ { source mixout.$index.1 sink gain.$index.1 } ] } } If we want to extend this pipeline with the addition of an eqiir/eqfir, we can create a SubTreeCopy node with type extend as follows: Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback { SubTreeCopy.baseclass { source "Class.Pipeline.mixout-gain-dai-copier-playback" type extend tree { Object.Widget { eqiir.1 {} eqfir.1 {} } Object.Base { !route [ { source gain.$index.1 sink eqiir.$index.1 } { source eqiir.$index.1 sink eqfir.$index.1 } ] } } } } Note that the target is left undefined, which means that the newly created subtree will be merged to the parent node that contains the "SubTreeCopy" node i.e. in this case Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback". But if we want to modify an existing pipeline class while modifying the order of widgets and/or inserting new widgets, we should use the type "override" instead. This allows for adding new widgets to the list of widgets in the base class definition while also allowing overriding the routes to allow inserting the new widgets and reordering the widgets in the base class. For example, if we want to add a drc widget between the gain and the eqiir modules in the above class, we can do the following: Class.Pipeline.mixout-efx-dai-copier-playback { # This copy will override all widgets/routes in the base class SubTreeCopy.baseclass { source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback" target "Class.Pipeline" type override tree { Object.Widget { drc.1 {} } Object.Base { !route [ { source mixout.$index.1 sink gain.$index.1 } { source gain.$index.1 sink drc.$index.1 } { source drc.$index.1 sink eqiir.$index.1 } { source eqiir.$index.1 sink eqfir.$index.1 } ] } } } # Explicitly copy the widgets from the base class now SubTreeCopy.widgets { source "Class.Pipeline.mixout-gain-eqiir-eqfir-dai-copier-playback.Object.Widget" target "Class.Pipeline.mixout-efx-dai-copier-playback.Object.Widget" } } Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 · 2024-08-06T15:43:18Z

@perexg Could you please have a look at this again? This will help us update our SOF topologies. Thanks!

ranj063 force-pushed the pipeline-extn-2 branch from 9b5c5fa to 3113646 Compare May 22, 2024 23:39

perexg reviewed May 23, 2024

View reviewed changes

ranj063 force-pushed the pipeline-extn-2 branch from 3113646 to d1fefac Compare May 23, 2024 20:04

perexg requested changes May 24, 2024

View reviewed changes

ranj063 force-pushed the pipeline-extn-2 branch from d1fefac to 922a9f4 Compare May 28, 2024 16:37

ranj063 force-pushed the pipeline-extn-2 branch from 922a9f4 to 938a26d Compare June 10, 2024 18:34

perexg requested changes Jun 21, 2024

View reviewed changes

ranj063 force-pushed the pipeline-extn-2 branch from 938a26d to ffc63f7 Compare July 12, 2024 17:25

perexg closed this in 6e3fc04 Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topology: pre-processor: Introduce extends/overrides keywords for classes #268

topology: pre-processor: Introduce extends/overrides keywords for classes #268

ranj063 commented May 22, 2024 •

edited

Loading

ranj063 commented May 22, 2024

plbossart commented May 22, 2024

perexg commented May 22, 2024

ranj063 commented May 22, 2024

perexg commented May 22, 2024

plbossart commented May 22, 2024

ranj063 commented May 22, 2024 •

edited

Loading

perexg commented May 23, 2024

perexg May 23, 2024

perexg May 23, 2024

perexg May 23, 2024

perexg May 23, 2024 •

edited

Loading

perexg commented May 23, 2024

perexg May 23, 2024

ranj063 May 23, 2024

perexg May 24, 2024

ranj063 May 28, 2024

perexg commented May 24, 2024

perexg May 24, 2024

perexg May 24, 2024

ranj063 May 28, 2024

perexg commented May 24, 2024

ranj063 commented May 28, 2024

perexg commented May 31, 2024

ranj063 commented May 31, 2024

perexg commented Jun 10, 2024

ranj063 commented Jun 10, 2024

ranj063 commented Jun 19, 2024

perexg left a comment

perexg Jun 21, 2024

ranj063 Jul 12, 2024

ranj063 Jul 22, 2024

ranj063 commented Aug 6, 2024

topology: pre-processor: Introduce extends/overrides keywords for classes #268

topology: pre-processor: Introduce extends/overrides keywords for classes #268

Conversation

ranj063 commented May 22, 2024 • edited Loading

ranj063 commented May 22, 2024

plbossart commented May 22, 2024

perexg commented May 22, 2024

ranj063 commented May 22, 2024

perexg commented May 22, 2024

plbossart commented May 22, 2024

ranj063 commented May 22, 2024 • edited Loading

perexg commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perexg May 23, 2024 • edited Loading

Choose a reason for hiding this comment

perexg commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perexg commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perexg commented May 24, 2024

ranj063 commented May 28, 2024

perexg commented May 31, 2024

ranj063 commented May 31, 2024

perexg commented Jun 10, 2024

ranj063 commented Jun 10, 2024

ranj063 commented Jun 19, 2024

perexg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Aug 6, 2024

ranj063 commented May 22, 2024 •

edited

Loading

ranj063 commented May 22, 2024 •

edited

Loading

perexg May 23, 2024 •

edited

Loading