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

CLAP 1.2.3 #432

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

CLAP 1.2.3 #432

wants to merge 12 commits into from

Conversation

abique
Copy link
Contributor

@abique abique commented Nov 18, 2024

No description provided.

@abique abique force-pushed the next branch 2 times, most recently from 773e346 to 069d784 Compare November 18, 2024 19:32
baconpaul and others added 3 commits November 18, 2024 20:35
* Add a description of the expectation of request_callback timing

Without making a requirement, indicate the intent of the timing.

* Add an apostrophe

* Add host can starve feedback from alex

* more review feedback

* notjusthosts
jatinchowdhury18 and others added 2 commits November 18, 2024 20:46
comment refered to a previous version of the ambisonic extension.
@abique abique marked this pull request as ready for review November 18, 2024 19:46
@abique abique self-assigned this Nov 18, 2024
Copy link
Collaborator

@baconpaul baconpaul left a comment

Choose a reason for hiding this comment

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

Added some comments on the location extension.

include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
// of scratch memory.
//
// [main-thread & being-activated]
bool(CLAP_ABI *reserve)(const clap_host_t *host, uint32_t scratch_size_bytes);
Copy link

@p--b p--b Nov 19, 2024

Choose a reason for hiding this comment

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

question: Would you accept a PR that added a concurrent_buffers parameter as discussed in #432?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so.

It'd need to explain what happens if the plugins performs more access to the buffer than initially requested.
It'd need to state that even if concurrent_buffers == 1, you may still get a thread local pointer, so if you want the same pointer on all threads, you need to fetch it before calling into the thread pool.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p--b ETA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p--b will you make that PR?

Copy link

Choose a reason for hiding this comment

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

Hey, apologies for the delay. Yes, I can have something to you in the next 24 hours.

Copy link

Choose a reason for hiding this comment

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

#436 :)

@baconpaul
Copy link
Collaborator

LGTM also.

Copy link
Contributor

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

Some questions and suggestions regarding the location extension

include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
include/clap/ext/draft/location.h Outdated Show resolved Hide resolved
Comment on lines +22 to +23
// The first device within a track group has the index of
// the last track or track group within this group + 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically the first device, or should this be the first child (whether that is track group, track, or device?).

What is the purpose of setting the index to that value? Is it to tell the plugin the range of location elements that are children (direct or not) of the parent element? That should probably be made clear.

Is the first child required to immediately follow its parent element in the clap_plugin_location_element_t *path array in set_location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it could be simplified.
We could say that index starts at 0 for tracks and then 0 again for devices.

This is the model:

track_group
 `- track0 -|
 `- track1 -| -> (+) -> device0 -> device1 -> ... 
 `- ...    -|

// Represents a nested device chain (serial).
// Its parent must be a device.
// It contains other devices.
CLAP_PLUGIN_LOCATION_NESTED_DEVICE_CHAIN = 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why nested device chains & devices function differently from how track groups & tracks function? That is, why do device chains need to be a child of a device rather than function as a "device group" which can have child devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A device chain is a serial list of devices.
A device can contain multiple device chains, which maybe use for parallel processing.
There are similarities between tracks and device chain. Though there's a different semantic, which the plugin can use for display. Think Fabfilter Pro Q3, with sidechain.

// such is expected to be of kind CLAP_PLUGIN_LOCATION_DEVICE.
// [main-thread]
void(CLAP_ABI *set_location)(clap_plugin_t *plugin,
clap_plugin_location_element_t *path,
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this array is a flattened tree structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is an array, which describes the path toward the current plugin instance.

Comment on lines +17 to +18
// Represents a document/project/session.
CLAP_PLUGIN_LOCATION_PROJECT = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be required to be the first element in the array passed to set_location, if it is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should, because if your host is just a device chain, it doesn't have a track or a project.

Comment on lines +43 to +45
// Index within the parent element.
// Set to 0 if irrelevant.
uint32_t index;
Copy link
Contributor

@messmerd messmerd Nov 24, 2024

Choose a reason for hiding this comment

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

So index does not refer to the array index of this element's parent element within the array passed to set_location?

I could be completely wrong, but it seems like a simpler way of flattening the tree would be to specify that the order of elements in set_location's path array is the same order that elements would be visited in a pre-order depth-first traversal of the tree. Then each index would be the array index of the element's direct parent. And since the root element does not have a parent, its index could be the index of the plugin device itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The index here, refers to the position within the track/device_chain/...

@messmerd
Copy link
Contributor

How much information should a host provide to a plugin through the location extension? Should it provide all the information it can, or only information about the plugin itself?

@abique
Copy link
Contributor Author

abique commented Dec 2, 2024

How much information should a host provide to a plugin through the location extension? Should it provide all the information it can, or only information about the plugin itself?

I'm not sure I understood well your question.

Think that you have a very large project, all you need to know your location is:
/project/track_group[0]/track[3]/device[6]

Here from the root track group, take the third track, then the 6th device and here we are.

Does that clarify the question?

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

Successfully merging this pull request may close these issues.

9 participants