Issue/104 bcg729 api#139
Conversation
(The master build is currently commented out due to LubosD#104; it can be enabled after that issue is fixed.)
** This commit only provides dummy values for all new arguments. **
|
Thank you for your contribution! Although I eventually ended up going in a different direction with my own pull requests (#152 and #153), where I attempted to keep the range of each Now, one part of your PR puzzled me: why do you pass Not that it matters much, since Thanks! |
| int frame_size; | ||
| uint8 *encoded_data_ptr = payload; | ||
| int16 *decoded_data_ptr = pcm_buf; | ||
| uint32 new_len = 0; |
There was a problem hiding this comment.
The type of new_len (uint32) does not match the return type of this function (uint16).
| uint8 *encoded_data_ptr = payload; | ||
| int16 *decoded_data_ptr = pcm_buf; | ||
| uint32 new_len = 0; | ||
| for (uint16 done = 0; done < payload_size && new_len < pcm_buf_size; done += frame_size) |
There was a problem hiding this comment.
The second half of this condition will interrupt the loop if we're about to write past the end of payload (as is its goal). However, these functions are intended to encode/decode their entire input, and have no way to tell the caller they only did part of the job.
The previous version avoided this problem with the use of assertions, to ensure that payload was big enough; I think you would have to do the same. (The asserted expressions will end up a bit more complicated, though.)
| uint32 new_len = 0; | ||
| for (uint16 done = 0; done < payload_size && new_len < pcm_buf_size; done += frame_size) | ||
| { | ||
| uint8 is_sid = (payload_size - done < 8) ? 1 : 0; |
There was a problem hiding this comment.
Out of curiosity, why 8? Not that it won't work (any expression returning true on 2 and false on 10+ will do the job), but I was wondering if it had a special significance that I was unaware of.
| { | ||
| bcg729Encoder(_context, decoded_data_ptr, encoded_data_ptr, &frame_size); | ||
| encoded_data_ptr += frame_size; | ||
| decoded_data_ptr += frame_size * 8; |
There was a problem hiding this comment.
This is a mistake, as decoded_data_ptr should always be incremented by 80, regardless of frame_size. (Of course, as long as enableVAD has been unset, frame_size will be 10 and everything will work fine. But if Annex B features are ever enabled, this will no longer be the case.)
| { | ||
| uint8 is_sid = (payload_size - done < 8) ? 1 : 0; | ||
| frame_size = (is_sid == 1) ? 2 : 10; | ||
| bcg729Decoder(_context, encoded_data_ptr, payload_size, false, is_sid, false, decoded_data_ptr); |
There was a problem hiding this comment.
I already made mention of the use of payload_size as bitStreamLength. On second thought, I guess it would make sense if the entire payload was only a single SID frame sent per RFC 3389. Which is not the case here, of course. (Personally. I'd much rather recommend sticking to a simple 0.)
fbriere
left a comment
fbriere
left a comment
There was a problem hiding this comment.
I had a few comments and issue while reading and parsing your code, which I thought I'd share with you, if you are interested. Feel free to read them or ignore them at your leisure.
confirming compiles fine with bcg729-1.0.2 and works for me ! |
|
Closing, as obsoleted by #152. Nevertheless, many thanks for taking the time to contribute to Twinkle. |
3 participants
No description provided.