Add support for getting list of supported codecs #16
Conversation
SanchayanMaity
changed the title
SanchayanMaity
force-pushed
the
get_caps
branch
from
5da7025
to
312b133
Compare
SanchayanMaity
force-pushed
the
get_caps
branch
from
312b133
to
e21d175
Compare
SanchayanMaity
force-pushed
the
get_caps
branch
2 times, most recently
from
bd9242a
to
7f81f55
Compare
|
Two notes:
|
…decs Right now, there is no way to expose a list of codecs from tinycompress. While one could theoretically call is_codec_supported multiple times in an application to check against a list of codecs, an API would make it easy to enumerate supported codecs. The IOCTL SNDRV_COMPRESS_GET_CAPS after all already exists. The use case is Pipewire where we would like the compressed sink Pipewire node to advertise only codecs supported by the underlying hardware. Signed-off-by: Sanchayan Maity <sanchayan@asymptotic.io>
SanchayanMaity
force-pushed
the
get_caps
branch
from
7f81f55
to
fef7b85
Compare
Assuming this is the consensus made the changes. |
Need the supported codecs API which is yet to be merged upstream. alsa-project/tinycompress#16
|
Any chance of progressing on this? The PipeWire support has been merged upstream and is included in the 0.3.65 release. It could use this to detect the supported codecs properly. |
| * | ||
| * format of name is : | ||
| * hw:<card>,<device> for real hw compress node | ||
| * <plugin_libname>:<custom string> for virtual compress node |
There was a problem hiding this comment.
what is a 'virtual compress node'?
What is a 'node' in general? That's not a terminology we use in this area. please explain.
| * <plugin_libname>:<custom string> for virtual compress node | ||
| * | ||
| * @name: name of the compress node | ||
| * @flags: stream flags |
There was a problem hiding this comment.
what is the definition of those flags?
| * @name: name of the compress node | ||
| * @flags: stream flags | ||
| * @codecs: Pointer to an array which will hold the list of codecs | ||
| * @size: Size in bytes of the codecs array |
There was a problem hiding this comment.
still weird. Why not add a callback to get the number of codecs first? How would userspace guessestimate what to add here?
There was a problem hiding this comment.
There is a missing description. If codecs == NULL the size should be returned by the API. I don't think that we need two exported functions / callbacks for this simple job.
| if (!compress) | ||
| return ret; | ||
|
|
||
| if ((name[0] == 'h') || (name[1] == 'w') || (name[2] == ':')) { |
There was a problem hiding this comment.
check is name != NULL first and string size > 3 ?
There was a problem hiding this comment.
It's an undefined input so the crash is ok. The assert may be added. The size checking is not required, because the '\0' termination character is matched correctly.
There was a problem hiding this comment.
"the crash is ok" ? not following, sorry.
|
|
||
| if (num_codecs > codecs_buf_sz) | ||
| return oops(&bad_compress, EINVAL, | ||
| "Input not big enough to hold %d available codecs", num_codecs); |
There was a problem hiding this comment.
why not return that number of codecs earlier with a separate API, that would help applications know what buffer size they need to provide?
Of course you still want to check the size.
Right now, there is no way to expose a list of codecs from
tinycompress. While one could theoretically callis_codec_supportedmultiple times in an application to check against a list of codecs, an API would make it easy to enumerate supported codecs. The IOCTLSNDRV_COMPRESS_GET_CAPSafter all already exists.Our use case is Pipewire where we would like the compressed sink node to advertise only codecs supported by the underlying hardware. So far we were just advertising the complete list of codecs and let node linking fail at runtime based on result from
is_codec_supported.Pipewire MR: https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1431