spanification: spanify Vp9Parser. [chromium/src : main]
Weidong Liu (Gerrit)
Weidong Liu added 1 comment![]( "Open in Gerrit")
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 7 comments![]( "Open in Gerrit")
Thanks for the cleanup! FYI, I will have very limited code review bandwidth until January, due to the holidays.
Thanks
Vp9SegmentationParams();
Vp9SegmentationParams(const Vp9SegmentationParams&);
Vp9SegmentationParams(Vp9SegmentationParams&&);
Vp9SegmentationParams& operator=(const Vp9SegmentationParams&);
Vp9SegmentationParams& operator=(Vp9SegmentationParams&&);
~Vp9SegmentationParams();Since these are all default, are these needed? It's ok if this is just to silence a compiler error.
const int kVp9MaxProfile = 4;
const int kVp9NumRefFramesLog2 = 3;
const size_t kVp9NumRefFrames = 1 << kVp9NumRefFramesLog2;
const uint8_t kVp9MaxProb = 255;
const size_t kVp9NumRefsPerFrame = 3;
const size_t kVp9NumFrameContextsLog2 = 2;
const size_t kVp9NumFrameContexts = 1 << kVp9NumFrameContextsLog2;While you're here, could you make all of these constants `constexpr`? TY!
bool ContainsZero(const void* __s, size_t __n) {
return UNSAFE_TODO(memchr(__s, 0, __n)) != nullptr;
}From intuition and then confirming the syntax with Gemini, this could work:
```
#include "base/containers/contains.h"
#include "base/containers/span.h"
// Generic helper that works for 1D, 2D, ... 6D arrays
template <typename T>
bool ContainsZero(const T& data) {
return base::Contains(base::as_byte_span(data), 0);
}
```
for (auto& a : coef_probs) {
for (auto& ai : a) {
for (auto& aj : ai) {
for (auto& ak : aj) {
size_t max_l = (+ak == +aj[0]) ? 3 : 6;
const bool has_zero = std::ranges::any_of(
base::span(ak).first(max_l),
[](const auto& v) { return ContainsZero(v, sizeof(v)); });
if (has_zero) {
return false;
}
}
}
}
}(Suggested by Gemini) Assuming the `ContainsZero` helper is updated above, this could be replaced with
```
// Iterate down to the [6][6][3] array
for (const auto& a : coef_probs) {
for (const auto& ai : a) {
for (const auto& aj : ai) {
// 1. Check first 3 rows of the first block (aj[0])
if (ContainsZero(base::span(aj[0]).first(3))) {
return false;
}
// 2. Check the remaining 5 blocks (aj[1]..aj[5]) completely
if (ContainsZero(base::span(aj).subspan<1u>())) {
return false;
}
}
}
}
```
if (!loop_filter.delta_enabled) {NIT: Flip the conditional and if/else blocks.
for (size_t j = 0; j < VP9_FRAME_MAX; ++j) {
std::ranges::fill(loop_filter.lvl[i][j], levels[i]);
}You might be able to use `std::ranges::fill(base::as_writable_byte_span(loop_filter.lvl[i]), levels[i]);` and avoid the for loop.
Related details
- Weidong Liu
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. |
Weidong Liu (Gerrit)
Weidong Liu added 6 comments![]( "Open in Gerrit")
Vp9SegmentationParams();
Vp9SegmentationParams(const Vp9SegmentationParams&);
Vp9SegmentationParams(Vp9SegmentationParams&&);
Vp9SegmentationParams& operator=(const Vp9SegmentationParams&);
Vp9SegmentationParams& operator=(Vp9SegmentationParams&&);
~Vp9SegmentationParams();Since these are all default, are these needed? It's ok if this is just to silence a compiler error.
Yes. Because the modified version does not meet the Chromium-style check.
const int kVp9MaxProfile = 4;
const int kVp9NumRefFramesLog2 = 3;
const size_t kVp9NumRefFrames = 1 << kVp9NumRefFramesLog2;
const uint8_t kVp9MaxProb = 255;
const size_t kVp9NumRefsPerFrame = 3;
const size_t kVp9NumFrameContextsLog2 = 2;
const size_t kVp9NumFrameContexts = 1 << kVp9NumFrameContextsLog2;While you're here, could you make all of these constants `constexpr`? TY!
Done
bool ContainsZero(const void* __s, size_t __n) {
return UNSAFE_TODO(memchr(__s, 0, __n)) != nullptr;
}From intuition and then confirming the syntax with Gemini, this could work:
```
#include "base/containers/contains.h"
#include "base/containers/span.h"// Generic helper that works for 1D, 2D, ... 6D arrays
template <typename T>
bool ContainsZero(const T& data) {
return base::Contains(base::as_byte_span(data), 0);
}
```
In [another CL](https://chromium-review.googlesource.com/c/chromium/src/+/6779046/comment/ce42a213_4abf12cf) last time, dalecurtis@ mentioned that `memchr` performs better than `std::rangee::find`. Therefore, I didn't modify this.
Do you think performance is a concern here?
for (auto& a : coef_probs) {
for (auto& ai : a) {
for (auto& aj : ai) {
for (auto& ak : aj) {
size_t max_l = (+ak == +aj[0]) ? 3 : 6;
const bool has_zero = std::ranges::any_of(
base::span(ak).first(max_l),
[](const auto& v) { return ContainsZero(v, sizeof(v)); });
if (has_zero) {
return false;
}
}
}
}
}(Suggested by Gemini) Assuming the `ContainsZero` helper is updated above, this could be replaced with
```
// Iterate down to the [6][6][3] array
for (const auto& a : coef_probs) {
for (const auto& ai : a) {
for (const auto& aj : ai) {
// 1. Check first 3 rows of the first block (aj[0])
if (ContainsZero(base::span(aj[0]).first(3))) {
return false;
}// 2. Check the remaining 5 blocks (aj[1]..aj[5]) completely
if (ContainsZero(base::span(aj).subspan<1u>())) {
return false;
}
}
}
}
```
Thanks, this has been very enlightening. It turns out that APIs like `base::as_byte_span` can be used to compress the dimensions of `uint8_t` arrays.
NIT: Flip the conditional and if/else blocks.
Done
for (size_t j = 0; j < VP9_FRAME_MAX; ++j) {
std::ranges::fill(loop_filter.lvl[i][j], levels[i]);
}You might be able to use `std::ranges::fill(base::as_writable_byte_span(loop_filter.lvl[i]), levels[i]);` and avoid the for loop.
- Thomas Guilbert
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. |
Weidong Liu (Gerrit)
Weidong Liu added 1 comment![]( "Open in Gerrit")
Thanks for the cleanup! FYI, I will have very limited code review bandwidth until January, due to the holidays.
Thanks
Please enjoy your holiday. You may review the application whenever you wish.