media: Spanify IvfParser::ParseNextFrame [chromium/src : main]
Stephen Nusko (Gerrit)
Stephen Nusko added 7 comments![]( "Open in Gerrit")
base::raw_span<const uint8_t> ivf_frame_data_;Is this file only used in tests? (I.E. no performance concerns about this raw_span?
base::span<const uint8_t> temp_ivf_frame_data;
if (!ivf_parser_->ParseNextFrame(&ivf_frame_header_,
&temp_ivf_frame_data)) {
return ParsingResult::kEOStream;
}
ivf_frame_data_ = temp_ivf_frame_data;What do you think about restricting the scope of this temporary (along with swiching to returning a span?
```
if (auto frame = ivf_parser_->ParseNextFrame(&ivf_frame_header_); frame.empty()) {
return ParsingResult::kEOStream;
} else {
ivf_frame_data_ = frame;
}
```
buffer->mmapped_planes()[0].CopyIn(ivf_frame_data_.data(),
ivf_frame_data_.size());
}This looks like test code and also it seems like it is very common to break apart the span and pass it through as a pointer + size and they store it already in a raw_ptr + length.
How do you think about switching MappedPlane's raw_ptr + length to a base::raw_span and adding a span overload for this case (then a follow up could remove usage of the non-span version), but here we could start by using it.
media::Vp8FrameHeader vp8_frame_header;Strangely this type also holds a raw_ptr + length so should really just hold a raw_span, but definitely out of scope here, but there are only 6 callers of this `vp8_parser.ParseFrame` should we just add a span version and update it as well?
vp9_parser_->SetStream(ivf_frame_data.data(), ivf_frame_data.size(),Same here it holds a raw_ptr already but at least we should add a span version of this function for now and migrate users over.
base::raw_span<const uint8_t> ivf_frame_data_;Is this file only used in tests? I.E. no performance concerns about this raw_span?
bool ParseNextFrame(IvfFrameHeader* frame_header,I think I agree with the TODO `// TODO(crbug.com/40284755): 'ParseNextFrame' should return a span.` shouldn't ParseNextFrame return the span not update a span out param?
I.E this should be
```
for (auto bytes = ivf_parser.ParseNextFrame(...);
!bytes.empty(); bytes = ivf_parser.ParseNextFrame()) {
```
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 7 comments![]( "Open in Gerrit")
Is this file only used in tests? (I.E. no performance concerns about this raw_span?
Yes, it is only used in `//media/gpu/v4l2:v4l2_stateless_decoder`
base::span<const uint8_t> temp_ivf_frame_data;
if (!ivf_parser_->ParseNextFrame(&ivf_frame_header_,
&temp_ivf_frame_data)) {
return ParsingResult::kEOStream;
}
ivf_frame_data_ = temp_ivf_frame_data;What do you think about restricting the scope of this temporary (along with swiching to returning a span?
```
if (auto frame = ivf_parser_->ParseNextFrame(&ivf_frame_header_); frame.empty()) {
return ParsingResult::kEOStream;
} else {
ivf_frame_data_ = frame;
}
```
Done
buffer->mmapped_planes()[0].CopyIn(ivf_frame_data_.data(),
ivf_frame_data_.size());
}This looks like test code and also it seems like it is very common to break apart the span and pass it through as a pointer + size and they store it already in a raw_ptr + length.
How do you think about switching MappedPlane's raw_ptr + length to a base::raw_span and adding a span overload for this case (then a follow up could remove usage of the non-span version), but here we could start by using it.
Done
Strangely this type also holds a raw_ptr + length so should really just hold a raw_span, but definitely out of scope here, but there are only 6 callers of this `vp8_parser.ParseFrame` should we just add a span version and update it as well?
Done
vp9_parser_->SetStream(ivf_frame_data.data(), ivf_frame_data.size(),Same here it holds a raw_ptr already but at least we should add a span version of this function for now and migrate users over.
Done
Is this file only used in tests? I.E. no performance concerns about this raw_span?
Yes, it is only used in `//media/gpu/vaapi:decode_test`
I think I agree with the TODO `// TODO(crbug.com/40284755): 'ParseNextFrame' should return a span.` shouldn't ParseNextFrame return the span not update a span out param?
I.E this should be
```
for (auto bytes = ivf_parser.ParseNextFrame(...);
!bytes.empty(); bytes = ivf_parser.ParseNextFrame()) {
```
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
4 comments
EXPECT_FALSE(payload.empty());Isn't this impossible? we have a !payload.empty() inside the for loop condition.
raw_ptr<const uint8_t, AllowPtrArithmetic | DanglingUntriaged> stream_;Add a TODO that this should be a raw_span? I think I remember there being somewhere else as well in one of the other headers could you double check?
void SetStream(const uint8_t* stream,Should we add UNSAFE_BUFFER_USAGE? or to many callsites?
auto buffer_span = base::span(buffer);We don't really need `buffer_span` anymore just pass `base::span(buffer)` into the function directly?
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 4 comments![]( "Open in Gerrit")
Isn't this impossible? we have a !payload.empty() inside the for loop condition.
You're right! Thanks!
raw_ptr<const uint8_t, AllowPtrArithmetic | DanglingUntriaged> stream_;Add a TODO that this should be a raw_span? I think I remember there being somewhere else as well in one of the other headers could you double check?
Done
Should we add UNSAFE_BUFFER_USAGE? or to many callsites?
There were like 2 extra files, so I did update those too.
We don't really need `buffer_span` anymore just pass `base::span(buffer)` into the function directly?
- Stephen Nusko
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")
UNSAFE_TODO(
std::memcpy(vp9_data + vp9_data_size, packet->data, packet->size));It might be nice to make this a span earlier to cover this one as well?
CreateMemoryMappedFile above has methods to get access to a bytes span, if we wrap the packet into a span at the top we can do it once and do the rest of the work using span checked methods.
void Vp9ParserTest::CheckSubsampleValues(There isn't a lot of callsites, just update `superframe` to be span?
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 2 comments![]( "Open in Gerrit")
UNSAFE_TODO(
std::memcpy(vp9_data + vp9_data_size, packet->data, packet->size));It might be nice to make this a span earlier to cover this one as well?
CreateMemoryMappedFile above has methods to get access to a bytes span, if we wrap the packet into a span at the top we can do it once and do the rest of the work using span checked methods.
Done
There isn't a lot of callsites, just update `superframe` to be span?
- Stephen Nusko
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 11 comments![]( "Open in Gerrit")
I've retriggered the gpu-fyi to see if the test failure reproduces in which case we'll need to investigate it.
for (base::span<const uint8_t> data =optional nit: `auto` might be reasonable here if you change the name from `data` to `bytes`?
I.E.
```
for (auto bytes = ...
```
If that reads okay then update here and others.
base::span(packet->data, base::checked_cast<size_t>(packet->size)));I think we can upgrade this to a UNSAFE_BUFFERS with a safety comment.
```
// SAFETY: `av_read_frame` returns 0 if okay, and < 0 on error/end of file. On error the packet will be blank, however we checked that `av_read_frame` is >= 0 in the while loop above. On Success which is our case here `packet->buf` will be initialized, this has the side effect of also initializing the `packet->data` and `packet->size` fields that we create this span from.
// See:
// * `av_read_frame`: https://ffmpeg.org/doxygen/6.1/group__lavf__decoding.html#ga4fdb3084415a82e3810de6ee60e46a61
// * `AVPacket`: https://ffmpeg.org/doxygen/6.1/structAVPacket.html
base::span<const uint8_t> packet_span = UNSAFE_BUFFERS(
base::span(packet->data, base::checked_cast<size_t>(packet->size)));
```
const DecoderBuffer& decoder_buffer = *buffer;Do we need this variable anymore? I think we can remove it and just go `*buffer` when calling `ParseFrame`?
Here and below
MmappedPlane(void* start, size_t len)
: buffer(base::span(static_cast<uint8_t*>(start), len)) {}How many times is this called? hard to tell from code search since we don't have cross referencing...
Can you either
1) Remove this constructor replacing it with a base::span<uint8_t> one?
2) Add a new constructor that takes a span? and then just delegate this one to it? and add a comment deprecating this constructor.
Thanks!
base::raw_span<uint8_t> buffer;Can you make this const?
I.E.
`const base::raw_span<uint8_t> buffer`
The reason I want to is because we eventually call munmap using this buffer, and we don't want to accidentally cause this buffer to ever get changed in position and size (even if we CopyIn data inside (the data that is pointed to is mutable the buffer itself isn't).
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40285824): Remove this and spanify to fix the errors.
#pragma allow_unsafe_buffers
#endifI think you can remove this, you'll just need to I htink add a single UNSAFE_TODO in the MmappedPlane constructor?
const uint8_t* src = planes[0].buffer.data();
const uint8_t* src_uv = src + src_size.width() * src_size.height();Let's change this a bit so that we avoid the UNSAFE_TODO pointer + size logic (`src + ...`).
```
const size_t kFullSize = src_size.width() * src_size.height();
base::span<const uint8_t> src = planes[0].buffer;
base::span<const uint8_t> src_uv = src.subspan(kFullSize);
libyuv::NV12ToI420(src.data(), src_size.width(), src_uv.data(), src_size.width(), ... );
```
And I THINK my understanding is that each of these should be kFullSize, so if you want you could also try
```
const size_t kFullSize = src_size.width() * src_size.height();
base::span<const uint8_t> src = planes[0].buffer.subspan(0, kFullSize);
base::span<const uint8_t> src_uv = planes[0].buffer.subspan(kFullSize, kFullSize);
libyuv::NV12ToI420(src.data(), src_size.width(), src_uv.data(), src_size.width(), ... );
```
But only if it passes tests and doesn't crash 😄 because I'm not sure if 2 is the "real" semantics intended, I think it is because `src_size.width()` is passed as the stride which then is multipled by the number of rows (height), so option 1) is completely safe (it just does what the current code does a bit safer (because the index to `src_uv` is now bounds checked), but 2 is even safer because it ensures that assuming no bug in libyuv we will not write out of bounds of the `planes[0].buffer`.
raw_ptr<const uint8_t, AllowPtrArithmetic> ptr_;Is it unreasonable to make this private member variable a raw_span directly?
Feels like that shouldn't be to hard, since nothing outside can call it.
I see that Initialize might take a bit more work (although there isn't to many), but this switch of `ptr_` and `end_` to be a single raw_span should be straight forward and easy, and actually a performance improvement.
bool Initialize(const uint8_t* stream,
size_t size,Add a TODO to switch this to a span.
return {};It says in the comment you added in the header:
```
// It is also guaranteed the span will be of size |frame_header->frame_size|
```
But in these error conditions we don't ensure that `frame_header->frame_size` is zero.
Can you either
1) Update the header to make clear that only on success do we promise this
2) Actually set it to zero here and below.
But it appears that 1) is easier to do since the second if condition implies that sometimes there isn't enough data for frame_size, but frame_size still has a valid value.
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 10 comments![]( "Open in Gerrit")
optional nit: `auto` might be reasonable here if you change the name from `data` to `bytes`?
I.E.
```
for (auto bytes = ...
```If that reads okay then update here and others.
Done
base::span(packet->data, base::checked_cast<size_t>(packet->size)));I think we can upgrade this to a UNSAFE_BUFFERS with a safety comment.
```
// SAFETY: `av_read_frame` returns 0 if okay, and < 0 on error/end of file. On error the packet will be blank, however we checked that `av_read_frame` is >= 0 in the while loop above. On Success which is our case here `packet->buf` will be initialized, this has the side effect of also initializing the `packet->data` and `packet->size` fields that we create this span from.
// See:
// * `av_read_frame`: https://ffmpeg.org/doxygen/6.1/group__lavf__decoding.html#ga4fdb3084415a82e3810de6ee60e46a61
// * `AVPacket`: https://ffmpeg.org/doxygen/6.1/structAVPacket.html
base::span<const uint8_t> packet_span = UNSAFE_BUFFERS(
base::span(packet->data, base::checked_cast<size_t>(packet->size)));
```
Done
Do we need this variable anymore? I think we can remove it and just go `*buffer` when calling `ParseFrame`?
Here and below
Done
MmappedPlane(void* start, size_t len)
: buffer(base::span(static_cast<uint8_t*>(start), len)) {}How many times is this called? hard to tell from code search since we don't have cross referencing...
Can you either
1) Remove this constructor replacing it with a base::span<uint8_t> one?
2) Add a new constructor that takes a span? and then just delegate this one to it? and add a comment deprecating this constructor.Thanks!
Done
Can you make this const?
I.E.
`const base::raw_span<uint8_t> buffer`
The reason I want to is because we eventually call munmap using this buffer, and we don't want to accidentally cause this buffer to ever get changed in position and size (even if we CopyIn data inside (the data that is pointed to is mutable the buffer itself isn't).
Done
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40285824): Remove this and spanify to fix the errors.
#pragma allow_unsafe_buffers
#endifI think you can remove this, you'll just need to I htink add a single UNSAFE_TODO in the MmappedPlane constructor?
Done
const uint8_t* src = planes[0].buffer.data();
const uint8_t* src_uv = src + src_size.width() * src_size.height();Let's change this a bit so that we avoid the UNSAFE_TODO pointer + size logic (`src + ...`).
```
const size_t kFullSize = src_size.width() * src_size.height();
base::span<const uint8_t> src = planes[0].buffer;
base::span<const uint8_t> src_uv = src.subspan(kFullSize);
libyuv::NV12ToI420(src.data(), src_size.width(), src_uv.data(), src_size.width(), ... );
```And I THINK my understanding is that each of these should be kFullSize, so if you want you could also try
```
const size_t kFullSize = src_size.width() * src_size.height();
base::span<const uint8_t> src = planes[0].buffer.subspan(0, kFullSize);
base::span<const uint8_t> src_uv = planes[0].buffer.subspan(kFullSize, kFullSize);
libyuv::NV12ToI420(src.data(), src_size.width(), src_uv.data(), src_size.width(), ... );
```But only if it passes tests and doesn't crash 😄 because I'm not sure if 2 is the "real" semantics intended, I think it is because `src_size.width()` is passed as the stride which then is multipled by the number of rows (height), so option 1) is completely safe (it just does what the current code does a bit safer (because the index to `src_uv` is now bounds checked), but 2 is even safer because it ensures that assuming no bug in libyuv we will not write out of bounds of the `planes[0].buffer`.
Done
Is it unreasonable to make this private member variable a raw_span directly?
Feels like that shouldn't be to hard, since nothing outside can call it.
I see that Initialize might take a bit more work (although there isn't to many), but this switch of `ptr_` and `end_` to be a single raw_span should be straight forward and easy, and actually a performance improvement.
Done
Add a TODO to switch this to a span.
Done
It says in the comment you added in the header:
```
// It is also guaranteed the span will be of size |frame_header->frame_size|
```But in these error conditions we don't ensure that `frame_header->frame_size` is zero.
Can you either
1) Update the header to make clear that only on success do we promise this
2) Actually set it to zero here and below.But it appears that 1) is easier to do since the second if condition implies that sometimes there isn't enough data for frame_size, but frame_size still has a valid value.
- Stephen Nusko
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 3 comments![]( "Open in Gerrit")
for (const auto& [buffer, bytes_used] : mmapped_planes_) {nit: '_' is the normal name for an unused variable.
```suggestion
for (const auto& [buffer, _] : mapped_planes_) {
```
base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);
base::span<const uint8_t> src_uv = src.subspan(kFullSize, kFullSize);Please add a comment about what each represents and check that it matches, this code as written makes src_uv an empty span.
Why?
Because
`base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);`
makes `src` be a span of size kFullSpan
`base::span<const uint8_t> src_uv = src.subspan(kFullSize, kFullSize);`
Makes srv_uv a span that starts kFullSpan after the start of `src` (which remember is size kFullSpan, and should have kFullSpan elements (this should crash when tested.
You need something like
```
base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);
base::span<const uint8_t> src_uv = planes[0].buffer.subspan(kFullSize, kFullSize);
```
So each is independently sized, this is a little tricky and as such you should add a comment describing that state.
```
// planes[0].buffer represents a buffer split into a single plan where the main pixels are found in the kFullSize bytes and the srv_uv is encoded in the next kFullSize bytes.
// planes[0].buffer == | kFullSize src bytes | kFullSize src_uv bytes|.
...
```
This also likely means that these aren't tested on the CQ, could you see if you can track down how they work and verify them locally?
return {};Bryan Enrique GonzalezIt says in the comment you added in the header:
```
// It is also guaranteed the span will be of size |frame_header->frame_size|
```But in these error conditions we don't ensure that `frame_header->frame_size` is zero.
Can you either
1) Update the header to make clear that only on success do we promise this
2) Actually set it to zero here and below.But it appears that 1) is easier to do since the second if condition implies that sometimes there isn't enough data for frame_size, but frame_size still has a valid value.
Done
In this scenario it helps the reviewer if you said which you did (you did 1.)
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 2 comments![]( "Open in Gerrit")
for (const auto& [buffer, bytes_used] : mmapped_planes_) {nit: '_' is the normal name for an unused variable.
```suggestion
for (const auto& [buffer, _] : mapped_planes_) {
```
Done
return {};Bryan Enrique GonzalezIt says in the comment you added in the header:
```
// It is also guaranteed the span will be of size |frame_header->frame_size|
```But in these error conditions we don't ensure that `frame_header->frame_size` is zero.
Can you either
1) Update the header to make clear that only on success do we promise this
2) Actually set it to zero here and below.But it appears that 1) is easier to do since the second if condition implies that sometimes there isn't enough data for frame_size, but frame_size still has a valid value.
Stephen NuskoDone
In this scenario it helps the reviewer if you said which you did (you did 1.)
Oops, sorry, forgot to mention. Thanks!
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. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
LGTM, lets do the local chromeOS test to ensure it actually works, and then do follow up CL to switch some of these raw_ptr + size into raw_spans.
Related details
- Bryan Enrique Gonzalez
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
LGTM, lets do the local chromeOS test to ensure it actually works, and then do follow up CL to switch some of these raw_ptr + size into raw_spans.
CL passed local tests :)
Thanks @hi...@chromium.org
Related details
- Dale Curtis
- Hirokazu Honda
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
Hi! Could you please help me with a review? I was hoping you could help me with following areas:
- @gui...@chromium.org: `//third_party`.
- @hi...@chromium.org: `v4l2` and `vaapi` related code.
- @dalec...@chromium.org: Remaining `//media` code.
Thank you everyone
Related details
- Dale Curtis
- Guido Urdaneta
- Hirokazu Honda
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);
base::span<const uint8_t> src_uv = src.subspan(kFullSize, kFullSize);Please add a comment about what each represents and check that it matches, this code as written makes src_uv an empty span.
Why?
Because
`base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);`
makes `src` be a span of size kFullSpan
`base::span<const uint8_t> src_uv = src.subspan(kFullSize, kFullSize);`Makes srv_uv a span that starts kFullSpan after the start of `src` (which remember is size kFullSpan, and should have kFullSpan elements (this should crash when tested.
You need something like
```
base::span<const uint8_t> src = planes[0].buffer.first(kFullSize);
base::span<const uint8_t> src_uv = planes[0].buffer.subspan(kFullSize, kFullSize);
```So each is independently sized, this is a little tricky and as such you should add a comment describing that state.
```
// planes[0].buffer represents a buffer split into a single plan where the main pixels are found in the kFullSize bytes and the srv_uv is encoded in the next kFullSize bytes.
// planes[0].buffer == | kFullSize src bytes | kFullSize src_uv bytes|.
...
```This also likely means that these aren't tested on the CQ, could you see if you can track down how they work and verify them locally?
Done
Related details
- Dale Curtis
- Guido Urdaneta
- Hirokazu Honda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hirokazu Honda (Gerrit)
Hirokazu Honda voted Code-Review+1![]( "Open in Gerrit")
- Bryan Enrique Gonzalez
- Dale Curtis
- Guido Urdaneta
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |