Allow to overwrite the i2s_stream_set_clk in the esp audio library for being able to implement custom i2s output modules. (AUD-4706)

[masterxq]

Is your feature request related to a problem? Please describe. Hello *, I wrote a custom i2s module for SPDIF output. This works fine and does not conflict with the rest of the ADF. But there is a problem with esp_err_t i2s_stream_set_clk(audio_element_handle_t i2s_stream, int rate, int bits, int ch) as this is statically called in the esp audio library I have troubles with overwrite them when the custom output module is selected.

If the name of the method in the custom module is the same as in i2s_stream.c there is no issue if the original i2s stream module is not in use. It's ugly (method name prefix not matching the rest of the module) but can be done. But if both modules are needed depending on the selected output modules the linker will fail because it defined twice.

If the name of the method for the custom module differs (eg: esp_err_t i2s_spdif_out_stream_set_clk(audio_element_handle_t i2s_stream, int rate, int bits, int ch) the linker will fail with:

/builds/adf/esp-adf-libs-source/esp_audio/src/media_ctrl_task.c:956: undefined reference to 'i2s_stream_set_clk'

The only Option the user has is to wrap the original i2s_stream_set_clk and write a wrapper that selects on the currently used output module the correct set_clk_method. This is very hacky and it's hard to write the wrapper without having all implementation details. Specially if both modules are active.

Describe the solution you'd like

It is super hard to suggest an implementation without all details, but the problem exists only in case of set clk method because all other methods (eg open close process...) are part of the public interface of audio element. So a generic implementation could touch the public interface of the audio element:

typedef struct {
    el_io_func          open;             /*!< Open callback function */
    ctrl_func           seek;             /*!< Seek callback function */
    process_func        process;          /*!< Process callback function */
    el_io_func          close;            /*!< Close callback function */
    el_io_func          destroy;          /*!< Destroy callback function */
    stream_func         read;             /*!< Read callback function */
    stream_func         write;            /*!< Write callback function */
    format_func         format;           /*!< Set audio format callback function (channels, rate, bits) */ 
    /*^^^^!!!!!!!!!!!!THIS IS NEW!!!!!!!!!!!!^^^^*/ 
    int                 buffer_len;       /*!< Buffer length use for an Element */
    int                 task_stack;       /*!< Element task stack */
    int                 task_prio;        /*!< Element task priority (based on freeRTOS priority) */
    int                 task_core;        /*!< Element task running in core (0 or 1) */
    int                 out_rb_size;      /*!< Output ringbuffer size */
    void                *data;            /*!< User context */
    const char          *tag;             /*!< Element tag */
    bool                stack_in_ext;     /*!< Try to allocate stack in external memory */
    int                 multi_in_rb_num;  /*!< The number of multiple input ringbuffer */
    int                 multi_out_rb_num; /*!< The number of multiple output ringbuffer */
} audio_element_cfg_t;

However not all audio elements requires this functionality. e.g. it is not necessary for a decoder. Additionally, it will increment the memory usage of the interface struct for each audio element by 4 bytes, and it will add additionally complexity.

As I think I use the esp audio library in an unintended way it could also be a possibility to make this to an intended functionality. Changes in esp_audio.h:

/* Set output format callback function type */
typedef esp_err_t (*set_format_func)(audio_element_handle_t i2s_stream, int rate, int bits, int ch);

/**
 * @brief Add audio output stream to specific esp_audio instance
 *
 * @param[in] handle       The esp_audio instance
 * @param[in] out_stream   The audio stream element instance
 * @param[in] set_format   A callback function that sets the specified format
 *
 * @return
 *      - ESP_ERR_AUDIO_NO_ERROR: on success
 *      - ESP_ERR_AUDIO_INVALID_PARAMETER: invalid arguments
 *      - ESP_ERR_AUDIO_MEMORY_LACK: allocate memory fail
 */
audio_err_t esp_audio_output_generic_stream_add(esp_audio_handle_t handle, audio_element_handle_t out_stream, set_format_func set_format_cb);

Internally in esp audio this saves this function pointer and call this it instead of making a static call on i2s_stream_set_clk. As the problem only exists for esp audio, this may be the better solution, as it fixes the problem where it occurs. It could be an option to pack the set_format_func into a struct and parse a pointer to such a struct when calling esp_audio_output_generic_stream_add. It would be more solid if additionally customization would be needed. Because could just be added to the struct. eg:

typedef esp_err_t (*set_format_func)(audio_element_handle_t i2s_stream, int rate, int bits, int ch);
typedef struct custom_output_cfg_t
{
  set_format_func set_format_cb;
} custom_output_cfg_t;

extern audio_err_t esp_audio_output_generic_stream_add(esp_audio_handle_t handle, audio_element_handle_t out_stream, custom_output_cfg_t custom_cfg);

Additional context

No idea what else could be needed, but if more information are needed I will supply them.

What really is needed is the option the add custom modules without hacking the library code, what in this case is not even possible as the source code is not public available.

In every case, thanks for your work on this, even it still has some small issues it's a really impressive library.

[TempoTian]

In theory, esp_audio should not have any hardcode call of other API like i2s_stream_set_clk except audio element and pipeline. But it has been used long time and will have compatiable issue if change common API directly. I think the most straight forward way is providing an API to let use to overwrite clock settings, if not provided use i2s_stream_set_clk . The API maybe like: esp_audio_set_output_fmt_cb

[masterxq]

I'm not completely sure I understand what you plan to do. But currently one problem is that the linker will fail no function called i2s_stream_set_clk is provided. Ok, it would be possible to have a custom weak dummy implementation, which provides the function if the original module is disabled or is overwritten by the original function when the original module will be linked. And call esp_audio_set_output_fmt_cb after adding the custom module to esp_audio to overwrite the weak/original implementation. So it can be done ^^ but this would at least not be my first idea how I have to implement a custom i2s module :)

It would be fine if the static call would additionally be removed from esp_audio. Currently, I have no idea how I would implement that it defaults to i2s_stream_set_clk when it is not part of an API without break the linker if no function with the name i2s_stream_set_clk is provided. Ofc my knowledge is limited. And I'm happy with the solution if the static call can completely be removed.

Thanks for reading!

[masterxq]

One more question. I'm not sure if esp_audio supports multiple output modules at all. If multiple output modules are added to esp_audio, would it be possible to have a different format callback for each output?

Just for science, as I think this is something that at least I don't need, I can recreate esp_audio with another i2s module if another output is selected.

[TempoTian]

Audio elment support multiple outputs，add an extra output ringfifo to the element before I2S maybe can resolve your issue, but there are many corner-cases need to care. To fullsupport it, need ADF 3.0, add an audio render module to support multiple input and multiple output then it's easy to fullfill the meets.