spanification: automatically spanify .../speech/endpointer/endpointer_unittest.cc etc. [chromium/src : main]
Evan Liu (Gerrit)
Evan Liu added 4 comments![]( "Open in Gerrit")
CHECK_EQ(event_args.audio_data->data.size(),
static_cast<size_t>(event_args.audio_data->frame_count));Is this always guaranteed to be true? What about for multi-channel audio?
const int16_t* AudioChunk::SamplesData16() const {Is this still used?
// The only concern would be if the length is not multiple of sizeof(int16_t)Is this a concern?
CHECK_GE(frame_size_, 0);If sample_rate is low or kFrameRate is high, frame_size_ could truncate to 0
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Liu (Gerrit)
Evan Liu added 1 comment![]( "Open in Gerrit")
FYI, you technically only need to add one OWNER as a reviewer for each of the files that you change in your CL. If you're collaborating with someone and want them to be aware of your change but you don't necessarily need them to review it, you can add them to the CC line.
https://chromium.googlesource.com/chromium/src/+/main/docs/code_reviews.md
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
CHECK_EQ(event_args.audio_data->data.size(),
static_cast<size_t>(event_args.audio_data->frame_count));Is this always guaranteed to be true? What about for multi-channel audio?
Just chiming in here Bryan,
if you don't know you have three routes available.
1) Keep the behaviour using `event_args.audio_data->data.first(frame_count)` and don't add the CHECK
2) Research and determine if all cases are indeed equal in which case add this CHECK
3) If you aren't sure you can keep the behaviour by adding `first()` and then adding a CHECK that also passes NotFatal for a couple milestones to let it roll out and verify if you get any crash reports. Then after a couple milestones you can upgrade it to a full CHECK and remove the `first()` for readability.
// The only concern would be if the length is not multiple of sizeof(int16_t)Is this a concern?
I think perhaps it should say
```
// The only concern would be if the length is not multiple of sizeof(int16_t), which we CHECK below.
```
? because yeah this isn't a concern at all given we've verified that this is the case.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |