Implemented AVRCP metadata and notification register commands #1078
Conversation
|
|
| memset(&arg, 0, sizeof(btc_avrc_args_t)); | ||
|
|
||
| arg.md_cmd.tl = tl; | ||
| arg.md_cmd.attr_list = attr_list; |
There was a problem hiding this comment.
I suggest to transfer data value of attr_list(copy into btc_avrc_args_t) and not just the data pointer, thus application can use local variable and do not need to worry about managing the storage of attribute_list
There was a problem hiding this comment.
Ah, I see.
There are currently maximum of 7 attributes to use and I just realised that I
should have created a bit mask instead.
Are you ok with that solution?
| { | ||
| // todo: critical section protection | ||
| if (bt_av_sink_data_callback) { | ||
| bt_av_sink_data_callback(data, len); |
There was a problem hiding this comment.
For SBC codec, the sample frequency information can be provided in Codec Specific Information Element through ESP_A2D_AUDIO_CFG_EVT, so there is no need to transfer this parameter in the data callback function.
There was a problem hiding this comment.
I understand, however A2DP official spec says the following:
The codec information that determines the bit rate is contained in the SBC frame
header and repeatedly sent to the SNK associated with audio data stream. The SRC
is capable of changing the bit rate dynamically by changing the bitpool parameter
without suspending.
Both 44.1 kHz and 48 kHz sample rate support is mandatory for audio sink
and sample rate can be changed dynamically without pausing the stream
so I saw no other way than to include it in the data callback.
There was a problem hiding this comment.
I think "bit rate" and "sampling frequency" have different meanings in the A2DP spec. "Sampling Frequency" is the raw sampled data stream which can be configured to be 44.1KHz, 48KHz, etc. "bit rate" indicates the encoded media stream(SBC frames) format which is transmitted over the air, this as you say, can be altered by A2DP source and is related to "bitpool" value as well as "Sampling Frequency."
Sampling frequency and Max/Min SBC bitpool value are determined in the AVDTP signaling procedure, they will not be changed unless reconfigured. Sampling frequency is already provided through "ESP_A2D_AUDIO_CFG_EVT".
There was a problem hiding this comment.
Sampling frequency is already provided through "ESP_A2D_AUDIO_CFG_EVT".
It is? I did not realise that, my mistake.
| { | ||
| tAVRC_STS status = BT_STATUS_UNSUPPORTED; | ||
|
|
||
| if (tl >= 16 || attr_id > ESP_AVRC_PS_MAX_ATTR - 1) { |
There was a problem hiding this comment.
Note that the return value of this function is not used, and checking of parameters such as tl and attr_id should be moved into function esp_avrc_ct_send_set_player_value_cmd() before calling function "btc_transfer_context()", then customer can immediately get the return status.
Likewise please do the same for other functions: btc_avrc_ct_send_metadata_cmd(), btc_avrc_ct_send_register_notification_cmd(). Previously the function btc_avrc_ct_send_passthrough_cmd also has this issue and I will fix this later.
There was a problem hiding this comment.
Ok. I will fix it.
|
|
||
| attr_index += (int) vendor_msg->p_vendor_data[7 + attr_index] + 8; | ||
| } | ||
| } |
There was a problem hiding this comment.
I suggest to remove the static buffer "meta_text_buffer". Instead, in the function "handle_rc_attributes_rsp", to "malloc" and "free" a 128Byte memory is enough, as the attribute text are transfered one by one for each attribute. Or we can use the p_vendor_data[] itself and thus the 128Byte malloc-ed buffer may not be necessary.
In this way we can save the static buffer, and the application layer is responsible to copy the data in the callback function, the example code should be changed accordingly(Implement a "deep copy" function used in bt_app_rc_ct_cb when invoke "bt_app_work_dispatch", and free the buffer after it is used.)
|
Done. I made fixes you suggested @mywang-espressif. |
| *******************************************************************************/ | ||
| static tAVRC_STS avrc_bld_set_player_value_cmd(tAVRC_SET_APP_VALUE_CMD *p_cmd, BT_HDR *p_pkt) | ||
| { | ||
| int i; |
There was a problem hiding this comment.
There is a build warning <unused variable "i">, please fix this.
| //Received attribute text is not null terminated, so it's useful to know it's length | ||
| param[i].meta_rsp.attr_length = attr_length; | ||
| param[i].meta_rsp.attr_text = &vendor_msg->p_vendor_data[8 + attr_index]; | ||
|
|
There was a problem hiding this comment.
The code indentation need to be fixed. You can use the script $IDF_PATH/tools/format.sh to fix the typos.
There was a problem hiding this comment.
I ran the script on all the files I modified in this PR, should be ok now.
|
I don't have more comments for now. Thank you very much for the work, I will forward the PR to my collaborator so that it can be merged. |
…rack title, album, etc. Merges #1078
|
Cherry-picked in 86fa182 , thank you @pufstudio ! :) |
Added metadata support so A2DP sink can show track title, artist, album and various other data.