fix for auto spanification for video encoder [chromium/src : main]
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andres Munoz Medina
- Danilo Françoso Tedeschi
- 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. |
Danilo Françoso Tedeschi (Gerrit)
Danilo Françoso Tedeschi added 1 comment![]( "Open in Gerrit")
auto rect_span = frame_data.subspan(rect_offset);I'd prefer to declare the type here as base::span<const uint8_t> rather than use auto, even though it is clear that we are creating a span with subspan, but it would be nice to know it is constant and this also makes it more clear that you are not making a copy.
Related details
- Andres Munoz Medina
- Daniel Angulo
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
auto rect_span = frame_data.subspan(rect_offset);I'd prefer to declare the type here as base::span<const uint8_t> rather than use auto, even though it is clear that we are creating a span with subspan, but it would be nice to know it is constant and this also makes it more clear that you are not making a copy.
- Andres Munoz Medina
- Danilo Françoso Tedeschi
- 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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
return base::as_writable_bytes(base::span(*packet->mutable_data()));nit: you don't need base::span the vector will implicitly convert to base::span.
static_cast<size_t>(frame.size().height() * frame.size().width() *height and/or width could be negative (see https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/desktop_capture/desktop_geometry.h;l=69;drc=a7cdebe0e5025250794e1407e6880141e3520bb1)
we can't therefore use a static cast for these. We need to assert they are >= zero. We can either do that or rely on the fact that is_empty() function currently does that.
So we need a
`CHECK(!frame.size().is_empty() && in_stride > 0);`
or
`CHECK(frame.size().height() > 0 && frame.size().width() > 0 && in_stride > 0);`
and then we could static cast.
Or we can use
base::checked_cast<size_t> or base::CheckedNumerics<size_t> and then compute the size.
I'd probably go with the last one.
// SAFETY: The `.data()` pointer is valid for the lifetime of `frame`, and
// the span is constructed with the correct size.
base::span<const uint8_t> frame_data = UNSAFE_BUFFERS(base::span(
frame.data(),
static_cast<size_t>(frame.size().height() * frame.size().width() *
webrtc::DesktopFrame::kBytesPerPixel)));I'm not sure this is correct and the safety comment of "it is the correct size" is not enough to convince me the reader that it is.
For example take [DesktopFrameSkia](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/media/capture/desktop_frame_skia.cc;l=11;drc=a7cdebe0e5025250794e1407e6880141e3520bb1)
```
DesktopFrameSkia::DesktopFrameSkia(const SkBitmap& bitmap)
: webrtc::DesktopFrame(webrtc::DesktopSize(bitmap.width(), bitmap.height()),
bitmap.rowBytes(),
static_cast<uint8_t*>(bitmap.getPixels()),
nullptr),
bitmap_(bitmap) {}
```
data is `bitmap.getPixels()`, and DesktopSize is `bitmap.width()` + `bitmap.height()`
However this doesn't account for extra padding space which might exist inside the bitmap.
You can see this in the [documentation for rowbytes](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/core/SkPixmap.h;l=69;drc=a7cdebe0e5025250794e1407e6880141e3520bb1)
```
@param rowBytes size of one row of addr; width times pixel size, or larger
```
The or larger is what poses the problem. We have restricted the the span to only kBytesPerPixel but that actually isn't the right computation. You need rowBytes. I believe in this code it is instride. From DesktopFrame
```
// Distance in the buffer between two neighboring rows in bytes.
int stride() const { return stride_; }
```
Really ideally we would want to update DesktopFrame to have a method to get a span, but since WebRTC is third_party so to spanify it we would need to add a new method.
So for this CL we really really need to make sure we get this right, and as it stands I'm not so sure we've computed this properly.
It should be
```
in_stride * frame.size().width() * frame.size().height()
```
I'm not sure kBytesPerPixel is required? I think it is included in `in_stride`
Also because this is quite complex and we have no API or simple thing I would live this as UNSAFE_TODO, we aren't confident enough to do UNSAFE_BUFFERS.
static_cast<size_t>(rect.top()) * in_stride +this one is "sort" of fine because if it is negative we only use it to access into our span so we'll properly bounds check so at worst we crash in the next line.
Related details
- Andres Munoz Medina
- Daniel Angulo
- Danilo Françoso Tedeschi
- 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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
const size_t rect_offset =
static_cast<size_t>(rect.top()) * in_stride +
static_cast<size_t>(rect.left()) * webrtc::DesktopFrame::kBytesPerPixel;
auto rect_span = frame_data.subspan(rect_offset);
for (int y = 0; y < rect.height(); ++y) {
auto row_span = rect_span.subspan(static_cast<size_t>(y) * in_stride,
static_cast<size_t>(row_size));
out.first(static_cast<size_t>(row_size)).copy_from(row_span);
out = out.subspan(static_cast<size_t>(row_size));Daniel AnguloJust please triple check there is a test for this function. The change from pointer arithmetic to span arithmetic is subtle. I think it is correct but we should make sure that there's a test properly exercising this function.
Daniel Angulothis file test calling the Encode remoting/protocol/video_frame_pump_unittest.cc:182 and this file tests the rects from encoding remoting/codec/codec_test.cc:236
Acknowledged
Related details
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. |
Daniel Angulo (Gerrit)
Daniel Angulo added 2 comments![]( "Open in Gerrit")
return base::as_writable_bytes(base::span(*packet->mutable_data()));nit: you don't need base::span the vector will implicitly convert to base::span.
Done
static_cast<size_t>(frame.size().height() * frame.size().width() *height and/or width could be negative (see https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/desktop_capture/desktop_geometry.h;l=69;drc=a7cdebe0e5025250794e1407e6880141e3520bb1)
we can't therefore use a static cast for these. We need to assert they are >= zero. We can either do that or rely on the fact that is_empty() function currently does that.
So we need a
`CHECK(!frame.size().is_empty() && in_stride > 0);`
or
`CHECK(frame.size().height() > 0 && frame.size().width() > 0 && in_stride > 0);`and then we could static cast.
Or we can use
base::checked_cast<size_t> or base::CheckedNumerics<size_t> and then compute the size.
I'd probably go with the last one.
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. |
Kalvin Lee (Gerrit)
Kalvin Lee added 2 comments![]( "Open in Gerrit")
return UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(data->data()), size));Unless I'm wrong about how `resize()` works, I think this can be much terser as
```
base::as_writable_bytes(base::span(*data))
```
and as a bonus it sheds `UNSAFE_BUFFERS`, too.
CHECK(frame.size().height() > 0 && frame.size().width() > 0 && in_stride > 0);IMHO this section is pretty hard to follow. I'm not comfortable being primary reviewer for this and I think you can consider directly roping in a proper owner for this.
However, looking at this clump of casts, I think I would prefer putting off this change for now, and
1. Look at implementing span accessors for `webrtc::DesktopFrame` (and inherited classes). [You may need to check with WebRTC folks if this should be `ArrayView` or `std::span`](https://issues.webrtc.org/issues/439801349).
1. (Wait for the WebRTC roll into Chromium --- refresh your tree)
1. Come back to this CL and see if it's possible to reorganize the casts. (Maybe you could even implement checked accessors for `webrtc::DesktopSize` --- I don't think there's any real reason we should _want_ to get negative values out of that class.)
If you feel strongly about what you have written so far, though, feel free to loop in a proper owner directly and I'm happy to cast a second CR+1 (as long as they're happy, too).
Related details
- Daniel Angulo
- 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. |
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
return UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(data->data()), size));Unless I'm wrong about how `resize()` works, I think this can be much terser as
```
base::as_writable_bytes(base::span(*data))
```and as a bonus it sheds `UNSAFE_BUFFERS`, too.
Huh, looking at the history of this CL, my suggestion was actually what was written from patchsets 1 through 10, and then it mysteriously changed to this `UNSAFE_BUFFERS()` clump in patchset 11. What happened here? Did somebody suggest this format for some reason?
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
return UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(data->data()), size));Kalvin LeeUnless I'm wrong about how `resize()` works, I think this can be much terser as
```
base::as_writable_bytes(base::span(*data))
```and as a bonus it sheds `UNSAFE_BUFFERS`, too.
Huh, looking at the history of this CL, my suggestion was actually what was written from patchsets 1 through 10, and then it mysteriously changed to this `UNSAFE_BUFFERS()` clump in patchset 11. What happened here? Did somebody suggest this format for some reason?
The problem is that base::as_writable_bytes no longer accepts a std::string as an argument, std::string is not intended to be used as a raw byte buffer.
Related details
- Kalvin Lee
- Stephen Nusko
- Yuwei Huang
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. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
I'm fine with this CL if the objective is just to get rid of all the `UNSAFE_TODO`s, but this CL does not seem to improve readability given the amount of introduced `UNSAFE_BUFFERS` and casts. As pointed out by kdlee@, it is more preferable to add a span/span-friendly accessor to WebRTC and update this CL accordingly.
Related details
- Daniel Angulo
- Kalvin Lee
- Stephen Nusko
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
return UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(data->data()), size));Kalvin LeeUnless I'm wrong about how `resize()` works, I think this can be much terser as
```
base::as_writable_bytes(base::span(*data))
```and as a bonus it sheds `UNSAFE_BUFFERS`, too.
Daniel AnguloHuh, looking at the history of this CL, my suggestion was actually what was written from patchsets 1 through 10, and then it mysteriously changed to this `UNSAFE_BUFFERS()` clump in patchset 11. What happened here? Did somebody suggest this format for some reason?
The problem is that base::as_writable_bytes no longer accepts a std::string as an argument, std::string is not intended to be used as a raw byte buffer.
I'm still not understanding.
I am not suggesting we feed a `std::string` as an argument to `base::as_writable_bytes()`. It's wrapped in `base::span()`.
If the concern is that `std::string` should not be used as a raw byte buffer, that is unfortunately the status quo. Your `UNSAFE_BUFFERS()` formulation does not do anything to address this, since you also force a `reinterpret_cast`.
The stated goal of this CL is to improve spatial safety. If you think there is a type issue, I suggest you tackle that in a (prequel) separate CL.
Related details
- Daniel Angulo
- Stephen Nusko
Stephen Nusko (Gerrit)
Stephen Nusko added 5 comments![]( "Open in Gerrit")
return UNSAFE_BUFFERS(
base::span(reinterpret_cast<uint8_t*>(data->data()), size));Kalvin LeeUnless I'm wrong about how `resize()` works, I think this can be much terser as
```
base::as_writable_bytes(base::span(*data))
```and as a bonus it sheds `UNSAFE_BUFFERS`, too.
Daniel AnguloHuh, looking at the history of this CL, my suggestion was actually what was written from patchsets 1 through 10, and then it mysteriously changed to this `UNSAFE_BUFFERS()` clump in patchset 11. What happened here? Did somebody suggest this format for some reason?
Kalvin LeeThe problem is that base::as_writable_bytes no longer accepts a std::string as an argument, std::string is not intended to be used as a raw byte buffer.
I'm still not understanding.
I am not suggesting we feed a `std::string` as an argument to `base::as_writable_bytes()`. It's wrapped in `base::span()`.
If the concern is that `std::string` should not be used as a raw byte buffer, that is unfortunately the status quo. Your `UNSAFE_BUFFERS()` formulation does not do anything to address this, since you also force a `reinterpret_cast`.
The stated goal of this CL is to improve spatial safety. If you think there is a type issue, I suggest you tackle that in a (prequel) separate CL.
What do you mean "no longer accepts a std::string"?
+1 we should be able to remove the UNSAFE_BUFFERS and just return a correct span.
CHECK(frame.size().height() > 0 && frame.size().width() > 0 && in_stride > 0);IMHO this section is pretty hard to follow. I'm not comfortable being primary reviewer for this and I think you can consider directly roping in a proper owner for this.
However, looking at this clump of casts, I think I would prefer putting off this change for now, and
1. Look at implementing span accessors for `webrtc::DesktopFrame` (and inherited classes). [You may need to check with WebRTC folks if this should be `ArrayView` or `std::span`](https://issues.webrtc.org/issues/439801349).
1. (Wait for the WebRTC roll into Chromium --- refresh your tree)
1. Come back to this CL and see if it's possible to reorganize the casts. (Maybe you could even implement checked accessors for `webrtc::DesktopSize` --- I don't think there's any real reason we should _want_ to get negative values out of that class.)
If you feel strongly about what you have written so far, though, feel free to loop in a proper owner directly and I'm happy to cast a second CR+1 (as long as they're happy, too).
Yeah fixing webrtc is the best "fix", however I've added a suggestion below for a UNSAFE_RTC_DESKTOP_FRAME_TO_BYTE_SPAN macro if there is push back from their side.
If they don't mind adding a span API (or ArrayView) then great! lets do that, if they do mind or there is some larger technical work on their side, adding the macro in a single location allows you to add all the checks and link to why they are correct, as well as gives us a central location to track which APIs we want to get spanified.
// SAFETY: The `.data()` pointer is valid for the lifetime of `frame`. The
// total size of the frame's buffer is `stride * height`, stride is the
// distance in bytes between two neighboring rows. See `DesktopFrame::stride`
// for more details.
base::span<const uint8_t> frame_data = UNSAFE_BUFFERS(base::span(
frame.data(),
base::checked_cast<size_t>(in_stride) * frame.size().height()));How often is a DesktopFrame converted into a span? If this occurs in other files consider adding a macro to auto_spanification_helper.h like
```
UNSAFE_RTC_DESKTOP_FRAME_TO_BYTE_SPAN()
```
Bonus you can move all those checks into the macro and verify things.
auto row_span = rect_span.subspan(static_cast<size_t>(y) * in_stride,This looks like it should be a spanReader?
out.first(static_cast<size_t>(row_size)).copy_from(row_span);This looks like it should be a spanWriter
Related details
- Daniel Angulo