spanification: spanify PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels. [chromium/src : main]
Weidong Liu (Gerrit)
Weidong Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
1 comment
Weidong LiuThanks! Is it possible to split up this CL into multiple CLs attacking different chunks, i.e., are there independent spanifications happening in the one file in this CL?
Colin BlundellAll of these files are required by spanify `paint_canvas_video_renderer.cc`. Modifications to other files are primarily due to API changes in `PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels`.
Weidong LiuThanks - What I meant is: Can we spanify `paint_canvas_video_renderer.cc` incrementally, e.g. API entrypoint by API entrypoint starting at leaf entrypoints? These types of CLs are easier to review the smaller they are.
Thank you for your reply. I removed the spanning of PlaneMetaData. Currently, only the modifications related to `PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels` remain.
Related details
- Colin Blundell
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. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks, Weidong! +Stephen, could you do the technical review given your expertise in this domain? Then I can stamp for //media OWNERS.
Related details
- Stephen Nusko
- 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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 6 comments![]( "Open in Gerrit")
I'll take another review round after changes.
base::span<uint8_t> pixmap_span = UNSAFE_BUFFERS(base::span(
static_cast<uint8_t*>(bitmap.getPixels()), bitmap.computeByteSize()));Please add a macro in https://source.chromium.org/chromium/chromium/src/+/main:base/containers/auto_spanification_helper.h similar to `UNSAFE_SKBITMAP_GETADDR32` (probably something like `UNSAFE_SKBITMAP_TO_BYTES_SPAN`)/
// SAFETY: Skia's API states that it will request memory of size
// `bitmap.computeByteSize()`.nit: can you link to the API docs where it states this?
// SAFETY: Skia's API states that it will request memory of size
// `info.computeByteSize(row_bytes)`.
base::span<uint8_t> pixmap_span = UNSAFE_BUFFERS(base::span(
static_cast<uint8_t*>(pixels), info.computeByteSize(row_bytes)));Same here can you a macro as well. Other places in Chrome will want to similarly wrap this.
base::span<const uint8_t> row = row_head.subspan(This has changed type? Why?
This introduces the tricky logic of endian, whereas if you left this as uint16_t you wouldn't need to add that logic?
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
Related details
- Weidong Liu
Weidong Liu (Gerrit)
Weidong Liu added 5 comments![]( "Open in Gerrit")
base::span<uint8_t> pixmap_span = UNSAFE_BUFFERS(base::span(
static_cast<uint8_t*>(bitmap.getPixels()), bitmap.computeByteSize()));Please add a macro in https://source.chromium.org/chromium/chromium/src/+/main:base/containers/auto_spanification_helper.h similar to `UNSAFE_SKBITMAP_GETADDR32` (probably something like `UNSAFE_SKBITMAP_TO_BYTES_SPAN`)/
Done
// SAFETY: Skia's API states that it will request memory of size
// `bitmap.computeByteSize()`.nit: can you link to the API docs where it states this?
Done
// SAFETY: Skia's API states that it will request memory of size
// `info.computeByteSize(row_bytes)`.
base::span<uint8_t> pixmap_span = UNSAFE_BUFFERS(base::span(
static_cast<uint8_t*>(pixels), info.computeByteSize(row_bytes)));Same here can you a macro as well. Other places in Chrome will want to similarly wrap this.
Done
base::span<const uint8_t> row = row_head.subspan(This has changed type? Why?
This introduces the tricky logic of endian, whereas if you left this as uint16_t you wouldn't need to add that logic?
If we're using `uint16_t` (including the following within this function), then we need to introduce a new `UNSAFE_BUFFERS` to convert the `span` to other types.
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
I've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
Related details
- Stephen Nusko
- 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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 10 comments![]( "Open in Gerrit")
LGTM after comments + owner's approval of Endian changes.
#define UNSAFE_SKPIXMAP_TO_BYTES_SPAN(arg_self) \
([](auto&& self) { \
uint8_t* row = static_cast<uint8_t*>(self->writable_addr()); \
size_t size = self->computeByteSize(); \
return UNSAFE_TODO(base::span<uint8_t>(row, size)); \
}(::base::spanification_internal::ToPointer(arg_self)))
// https://source.chromium.org/chromium/chromium/src/+/main:cc/paint/paint_canvas.h;l=66;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkCanvas.cpp;l=1264;drc=6c24069ae3996c883ea5d5886d0c013cb78f8394;bpv=1;bpt=1
#define UNSAFE_PAINTCANVAS_TOP_LAYER_TO_BYTES_SPAN(arg_self, arg_info, \
arg_row_bytes, arg_origin) \
([](auto&& self, SkImageInfo* info, size_t* rowBytes, SkIPoint* origin) { \
uint8_t* row = static_cast<uint8_t*>( \
self->accessTopLayerPixels(info, rowBytes, origin)); \
size_t size = info->computeByteSize(*rowBytes); \
return UNSAFE_TODO(base::span<uint8_t>(row, size)); \
}(::base::spanification_internal::ToPointer(arg_self), arg_info, \
arg_row_bytes, arg_origin))Similarly for these we should attempt to verify the assumptions to ensure the state is all correct, so that the span we return should always be safe to index in bounds.
size_t size = self->computeByteSize(); \The link says this returns SIZE_MAX if the result doesn't fit inside, should we check for this?
```
CHECK_NE(size, SIZE_MAX);
```
?
uint8_t* row = static_cast<uint8_t*>(self->getPixels()); \getPixels appears like it returns a member variable which can sometimes be a nullptr. We should ensure that if it is a nullptr that computeByteSize is properly zero.
So could you add a
```
CHECK(row || size == 0);
```
I.E. if row is not a valid pointer, size should be zero.
explicit VideoImageGenerator(scoped_refptr<VideoFrame> frame)This seems to move this into style guide compliance (a good thing), but do you know why it wasn't explicit before? And why this was done in this CL?
base::span<const uint8_t> row = row_head.subspan(Weidong LiuThis has changed type? Why?
This introduces the tricky logic of endian, whereas if you left this as uint16_t you wouldn't need to add that logic?
If we're using `uint16_t` (including the following within this function), then we need to introduce a new `UNSAFE_BUFFERS` to convert the `span` to other types.
As long as we do a
```
CHECK_EQ(row_head.size() % sizeof(uint16_t), 0u);
```
I think I'm completely okay with adding a
```
// SAFETY: We've verified that this fits inside of a uint16_t and access it as such.
row = UNSAFE_BUFFERS(base::span<uint16_t>(row_head.data(), row_head.size() / sizeof(uint16_t));
```
As needed, but I'll resolve this comment change and let @tgui...@chromium.org as a media owner decide.
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
Weidong Liu+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
I've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
I'm going to leave this final decision up to @tgui...@chromium.org, I'm a spanification expert not a media expert.
With U16 methods on SpanWriter/SpanReader this seems "okay" to me, but the other option is to not introduce this endian at all and just reinterpret cast the spans to uint16_t (or whatever) as the current code was doing (with a couple UNSAFE_BUFFERS and checks to ensure it is all in bounds).
If no one relies then this change to use Write/ReadNativeEndian reads pretty reasonable to me.
case PIXEL_FORMAT_Y16: {nit: I don't think you need the bracket anymore since you are introducing a new variable here.
writer.Skip(offset_y * stride * 2);this `*2` confused me until I realized that the old code was in uint16_t.
Perhaps we do
```
constexpr size_t kBytesPerPixel = 2;
```
(name to be decided).
`kU16Size` or `kBytesPerPixel` both seem fine.
Or as described we could just convert the span to uin16_t and keep the logic.
[](base::raw_span<uint8_t> pixels, const gfx::Size& size, int x,generally function parameters shouldn't be raw_span, is there a reason this is a raw_span and not just a regular span?
Related details
- Thomas Guilbert
- 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 voted and added 8 comments![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
8 comments
#define UNSAFE_SKPIXMAP_TO_BYTES_SPAN(arg_self) \
([](auto&& self) { \
uint8_t* row = static_cast<uint8_t*>(self->writable_addr()); \
size_t size = self->computeByteSize(); \
return UNSAFE_TODO(base::span<uint8_t>(row, size)); \
}(::base::spanification_internal::ToPointer(arg_self)))
// https://source.chromium.org/chromium/chromium/src/+/main:cc/paint/paint_canvas.h;l=66;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkCanvas.cpp;l=1264;drc=6c24069ae3996c883ea5d5886d0c013cb78f8394;bpv=1;bpt=1
#define UNSAFE_PAINTCANVAS_TOP_LAYER_TO_BYTES_SPAN(arg_self, arg_info, \
arg_row_bytes, arg_origin) \
([](auto&& self, SkImageInfo* info, size_t* rowBytes, SkIPoint* origin) { \
uint8_t* row = static_cast<uint8_t*>( \
self->accessTopLayerPixels(info, rowBytes, origin)); \
size_t size = info->computeByteSize(*rowBytes); \
return UNSAFE_TODO(base::span<uint8_t>(row, size)); \
}(::base::spanification_internal::ToPointer(arg_self), arg_info, \
arg_row_bytes, arg_origin))Similarly for these we should attempt to verify the assumptions to ensure the state is all correct, so that the span we return should always be safe to index in bounds.
Done
The link says this returns SIZE_MAX if the result doesn't fit inside, should we check for this?
```
CHECK_NE(size, SIZE_MAX);
```?
Done
uint8_t* row = static_cast<uint8_t*>(self->getPixels()); \getPixels appears like it returns a member variable which can sometimes be a nullptr. We should ensure that if it is a nullptr that computeByteSize is properly zero.
So could you add a
```
CHECK(row || size == 0);
```I.E. if row is not a valid pointer, size should be zero.
Done
explicit VideoImageGenerator(scoped_refptr<VideoFrame> frame)This seems to move this into style guide compliance (a good thing), but do you know why it wasn't explicit before? And why this was done in this CL?
I don't know the reason. I checked the blame, and the code here is too old (10 years ago). Currently, it's only used within the current file, so it shouldn't introduce any new problems.
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
Weidong Liu+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
Stephen NuskoI've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
I'm going to leave this final decision up to @tgui...@chromium.org, I'm a spanification expert not a media expert.
With U16 methods on SpanWriter/SpanReader this seems "okay" to me, but the other option is to not introduce this endian at all and just reinterpret cast the spans to uint16_t (or whatever) as the current code was doing (with a couple UNSAFE_BUFFERS and checks to ensure it is all in bounds).
If no one relies then this change to use Write/ReadNativeEndian reads pretty reasonable to me.
Thank you for reviewing. I will await comments from @tgui...@chromium.org.
nit: I don't think you need the bracket anymore since you are introducing a new variable here.
Done
this `*2` confused me until I realized that the old code was in uint16_t.
Perhaps we do
```
constexpr size_t kBytesPerPixel = 2;
```(name to be decided).
`kU16Size` or `kBytesPerPixel` both seem fine.
Or as described we could just convert the span to uin16_t and keep the logic.
Done
[](base::raw_span<uint8_t> pixels, const gfx::Size& size, int x,generally function parameters shouldn't be raw_span, is there a reason this is a raw_span and not just a regular span?
- Stephen Nusko
- 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. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 3 comments![]( "Open in Gerrit")
const float result[] = {gray_value, gray_value, gray_value, 1.0f};The AI review helper outlined this as a potential performance regression. This code is executed per-pixel, so this might be called millions of times per frame.
It might be worth using a micro-benchmark to measure the impact of this change.
An alternative could be to reuse an `std::array<float, 4>` defined outside of the loop. Using `out_row.take_first<4*sizeof(float)>()` instead of the `SpanReader` also seems to be ok, from the examples [here](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/unsafe_buffers.md#avoid-reinterpret_cast).
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
Weidong Liu+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
Stephen NuskoI've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
Weidong LiuI'm going to leave this final decision up to @tgui...@chromium.org, I'm a spanification expert not a media expert.
With U16 methods on SpanWriter/SpanReader this seems "okay" to me, but the other option is to not introduce this endian at all and just reinterpret cast the spans to uint16_t (or whatever) as the current code was doing (with a couple UNSAFE_BUFFERS and checks to ensure it is all in bounds).
If no one relies then this change to use Write/ReadNativeEndian reads pretty reasonable to me.
Thank you for reviewing. I will await comments from @tgui...@chromium.org.
The preference for explicitly using LittleEndianess is interesting... I learnt something new, thanks! Some of my concern was whether the video format itself had a specific endianess (e.g. like when we parse an Opus header), but this might not be relevant here.
[...] reinterpret cast the spans to uint16t_t...
I thought this was generally discouraged, following [this guidance](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/unsafe_buffers.md#avoid-reinterpret_cast). If this is be acceptable here, I think it is preferable, as we could use the ranged-based for-loops, which don't have a performance impact versus pointer arithmetic.
I would argue that given that this code is a "hot path", it might be worth it in this case.
base::CheckMul<size_t>(width * 4, height, 4u).ValueOrDie()));Is this multiplying by 4 twice? Unintended?
Related details
- Stephen Nusko
- Weidong Liu
Weidong Liu (Gerrit)
Weidong Liu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
2 comments
const float result[] = {gray_value, gray_value, gray_value, 1.0f};The AI review helper outlined this as a potential performance regression. This code is executed per-pixel, so this might be called millions of times per frame.
It might be worth using a micro-benchmark to measure the impact of this change.
An alternative could be to reuse an `std::array<float, 4>` defined outside of the loop. Using `out_row.take_first<4*sizeof(float)>()` instead of the `SpanReader` also seems to be ok, from the examples [here](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/unsafe_buffers.md#avoid-reinterpret_cast).
Done
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
Weidong Liu+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
Stephen NuskoI've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
Weidong LiuI'm going to leave this final decision up to @tgui...@chromium.org, I'm a spanification expert not a media expert.
With U16 methods on SpanWriter/SpanReader this seems "okay" to me, but the other option is to not introduce this endian at all and just reinterpret cast the spans to uint16_t (or whatever) as the current code was doing (with a couple UNSAFE_BUFFERS and checks to ensure it is all in bounds).
If no one relies then this change to use Write/ReadNativeEndian reads pretty reasonable to me.
Thomas GuilbertThank you for reviewing. I will await comments from @tgui...@chromium.org.
The preference for explicitly using LittleEndianess is interesting... I learnt something new, thanks! Some of my concern was whether the video format itself had a specific endianess (e.g. like when we parse an Opus header), but this might not be relevant here.
[...] reinterpret cast the spans to uint16t_t...
I thought this was generally discouraged, following [this guidance](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/unsafe_buffers.md#avoid-reinterpret_cast). If this is be acceptable here, I think it is preferable, as we could use the ranged-based for-loops, which don't have a performance impact versus pointer arithmetic.
I would argue that given that this code is a "hot path", it might be worth it in this case.
- Stephen Nusko
- Thomas Guilbert
Weidong Liu (Gerrit)
Weidong Liu added 1 comment![]( "Open in Gerrit")
base::CheckMul<size_t>(width * 4, height, 4u).ValueOrDie()));Is this multiplying by 4 twice? Unintended?
Done
Related details
- Stephen Nusko
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
CHECK_EQ(span.size() % sizeof(T), 0u);If we go this route can you also add
CHECK(base::IsAligned(span.data(), alignof(T)));`
reader.ReadU16LittleEndian(value);Stephen NuskoI would double check that these are all little endian.
Weidong Liu+1 I think we shouldn't move to uint8_t here and instead should remain as uint16_t and let the system track endianness.
Stephen NuskoI've consulted some resources. The base library states that Chromium only supports small segment orders: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=18?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
The comments for `U16FromNativeEndian` also indicate that we prefer explicit little endian: https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/byte_conversions.h;l=44?q=byte_conversions.h&ss=chromium%2Fchromium%2Fsrc
Due to your concerns, I changed it to use the `NativeEndian` version. However, the implementation still uses little endian.
Weidong LiuI'm going to leave this final decision up to @tgui...@chromium.org, I'm a spanification expert not a media expert.
With U16 methods on SpanWriter/SpanReader this seems "okay" to me, but the other option is to not introduce this endian at all and just reinterpret cast the spans to uint16_t (or whatever) as the current code was doing (with a couple UNSAFE_BUFFERS and checks to ensure it is all in bounds).
If no one relies then this change to use Write/ReadNativeEndian reads pretty reasonable to me.
Thomas GuilbertThank you for reviewing. I will await comments from @tgui...@chromium.org.
Weidong LiuThe preference for explicitly using LittleEndianess is interesting... I learnt something new, thanks! Some of my concern was whether the video format itself had a specific endianess (e.g. like when we parse an Opus header), but this might not be relevant here.
[...] reinterpret cast the spans to uint16t_t...
I thought this was generally discouraged, following [this guidance](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/unsafe_buffers.md#avoid-reinterpret_cast). If this is be acceptable here, I think it is preferable, as we could use the ranged-based for-loops, which don't have a performance impact versus pointer arithmetic.
I would argue that given that this code is a "hot path", it might be worth it in this case.
Done
Thanks for pointing that out. It is indeed discouraged.
So talking to the base::span owners they much prefer folks use these Endian (either Native or Little/Big) or the suggested take_first<2> to then pull into the value of your type.
Apparently if you reinterpret_cast and the object of that type has not previously existed at that location it is technically undefined behaviour (until c++23 and lifetime annotations).
However we do have occurrences of it in some areas already and pdfium uses it as well. However they also usually include an alignment check for extra safety. So I suggest we at least add that, and if we don't mind the endian we can use the readers or the slightly more verbose take_first<2>.copy_from(byte_span_from_ref()),
```
CHECK(base::IsAligned(span.data(), alignof(uint16_t)));
```
So the take_first<2> method would look like:
```
row.take_first<2>.copy_from(base::byte_span_from_ref(value));
```
If we had a benchmark on this code it might be worth trying both approaches to see if one is better than the other.
Related details
- Thomas Guilbert
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
2 comments
If we go this route can you also add
CHECK(base::IsAligned(span.data(), alignof(T)));`
Done
Considering this is a popular path, I will follow Thomas's suggestion and maintain the status quo.
Related details
- Stephen Nusko
- 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")
temp_buffer[3] = 1.0f;NIT: Move `temp_buffer[3] = 1.0f;` outside the loop
const size_t byte_size = stride * coded_size.height() * 2;NIT: Use `kBytesPerPixel` here too.
const size_t extra_width = frame->stride(0) - kWidth;NIT: `extra_width` might be easier to understand as `padding`.
uint16_t result[] = {
fp16_ieee_from_fp32_value(1), fp16_ieee_from_fp32_value(0),
fp16_ieee_from_fp32_value(0), fp16_ieee_from_fp32_value(1)};TINY NIT: Not performance critical because these are tests... But move out of both loops
base::span<const uint8_t> src = cropped_frame()->data_span(plane);
base::SpanWriter writer(frame->writable_span(plane));
for (int row = 0; row < cropped_frame()->rows(plane); row++) {
for (int col = 0; col < width; col++) {
writer.WriteU16NativeEndian(src[col] << (param.bit_depth - 8));
}
src = src.subspan(frame->stride(plane));
writer.Skip(frame->stride(plane) - width * 2);
}NIT: Using
```
base::span<const uint8_t> src = cropped_frame->data_span(plane);
base::span<uint8_t> dest = frame->writable_span(plane);
[...]
auto src_row = src.take_first(cropped_frame->stride(plane));
auto dest_row = dest.take_first(frame->stride(plane));
for (...) {
const uint16_t value = src_row[col] << (param.bit_depth - 8);
dest_row.take_first<sizeof(uint16_t)>()
.copy_from(base::byte_span_from_ref(value));
}
```avoids skipping bytes on the write, and is easier to follow IMO.
Also, we should double check the frame vs cropped frame logic holds.
writer_u.WriteU16NativeEndian(2048);Same as:
```
const uint16_t value = 4095
std::fill(y_plane, base::byte_span_from_ref(value));
```
and
```
const uint16_t value = 2048
std::fill(u_plane, base::byte_span_from_ref(value));
std::fill(v_plane, base::byte_span_from_ref(value));
```
base::CheckMul<size_t>(width, height, sizeof(float)).ValueOrDie()));Use 4 rather than `sizeof(float)`, since this is an element count, not a byte size.
Related details
- Stephen Nusko
- 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 7 comments![]( "Open in Gerrit")
NIT: Move `temp_buffer[3] = 1.0f;` outside the loop
Done
const size_t byte_size = stride * coded_size.height() * 2;NIT: Use `kBytesPerPixel` here too.
Done
const size_t extra_width = frame->stride(0) - kWidth;NIT: `extra_width` might be easier to understand as `padding`.
Done
uint16_t result[] = {
fp16_ieee_from_fp32_value(1), fp16_ieee_from_fp32_value(0),
fp16_ieee_from_fp32_value(0), fp16_ieee_from_fp32_value(1)};TINY NIT: Not performance critical because these are tests... But move out of both loops
Done
base::span<const uint8_t> src = cropped_frame()->data_span(plane);
base::SpanWriter writer(frame->writable_span(plane));
for (int row = 0; row < cropped_frame()->rows(plane); row++) {
for (int col = 0; col < width; col++) {
writer.WriteU16NativeEndian(src[col] << (param.bit_depth - 8));
}
src = src.subspan(frame->stride(plane));
writer.Skip(frame->stride(plane) - width * 2);
}NIT: Using
```
base::span<const uint8_t> src = cropped_frame->data_span(plane);
base::span<uint8_t> dest = frame->writable_span(plane);[...]
auto src_row = src.take_first(cropped_frame->stride(plane));
auto dest_row = dest.take_first(frame->stride(plane));for (...) {
const uint16_t value = src_row[col] << (param.bit_depth - 8);
dest_row.take_first<sizeof(uint16_t)>()
.copy_from(base::byte_span_from_ref(value));
}
```avoids skipping bytes on the write, and is easier to follow IMO.
Also, we should double check the frame vs cropped frame logic holds.
Done
Same as:
```
const uint16_t value = 4095
std::fill(y_plane, base::byte_span_from_ref(value));
```and
```
const uint16_t value = 2048
std::fill(u_plane, base::byte_span_from_ref(value));
std::fill(v_plane, base::byte_span_from_ref(value));
```
Done
base::CheckMul<size_t>(width, height, sizeof(float)).ValueOrDie()));Use 4 rather than `sizeof(float)`, since this is an element count, not a byte size.
- Stephen Nusko
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Stephen Nusko
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |