Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
430583005:
Make VEA test support videos with different coded size and visible size (Closed)
|
Created:
6 years, 4 months ago by henryhsu Modified:
6 years, 3 months ago Reviewers:
Andrew Hayden (chromium.org), groby-ooo-7-16, rpetterson, rvargas (doing something else), tony, Tom Sepez, wuchengli, Pawel Osciak, scherkus (not reviewing) CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMake VEA test support videos with different coded size and visible size
Test input streams that do not meet encoder size requirements exactly.
Because ARM platform need 64 bytes alignment for starting address of YUV
planes of frames, use MemoryMappedFile to create input buffer and copy
data line by line from visible size to coded size. So VEA test can
test more Hangout resolutions now.
Original flow: prepare input memory -> initialize VEAClient -> initialize Encoder
New flow: initialize VEAClient -> get coded size -> prepare input memory
BUG=378166
TEST=Manually run auto test with h264 and vp8 on peach pit.
Pass ./base_unittests --gtest_filter=MemoryMappedFileTest.*
Committed: https://crrev.com/fb04d233f44676c70cec847d0322c26ac32a256e
Cr-Commit-Position: refs/heads/master@{#295913}
Patch Set 1 #
Total comments: 20
Patch Set 2 : YUV data alignment #Patch Set 3 : use AppendToFile #Patch Set 4 : Coded size alignment #Patch Set 5 : Copy data line by line #
Total comments: 24
Patch Set 6 : Use MemoryMappedFile to copy data #Patch Set 7 : address wucheng's review comments #
Total comments: 4
Patch Set 8 : address rvargas review comments #
Total comments: 40
Patch Set 9 : address review comments of patch set 7 and 8 #
Total comments: 25
Patch Set 10 : address review comments of patch set 9 #
Total comments: 19
Patch Set 11 : address review comments of patch set 10 #
Total comments: 19
Patch Set 12 : address review comments of patch set 11 #
Total comments: 10
Patch Set 13 : address review comments of patch set 12 #
Total comments: 2
Patch Set 14 : use scope_ptr for MemoryMappedFile object #
Total comments: 6
Patch Set 15 : rollback to file write #
Total comments: 19
Patch Set 16 : address review comments of patch set 15 #
Total comments: 6
Patch Set 17 : address review comments of patch set 16 #Patch Set 18 : MemoryMappedFile starting address checking #
Total comments: 71
Patch Set 19 : address Pawel's review comments #
Total comments: 35
Patch Set 20 : address review comments of patch set 19 #
Total comments: 6
Patch Set 21 : address review comments of patch set 20 #
Total comments: 4
Patch Set 22 : fix all nits #
Total comments: 1
Patch Set 23 : fix comments and tested on intel platform #Patch Set 24 : fix padding bug #
Messages
Total messages: 85 (14 generated)
https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:791: input_coded_size_.width(), All strides should be test_stream_.size. Right? https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:795: frame_data + input_coded_size_.GetArea(), The offsets of u and v planes should also be calculated by test_stream_.size. Right?
On 2014/07/30 09:32:51, wuchengli wrote: > https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... > File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... > content/common/gpu/media/video_encode_accelerator_unittest.cc:791: > input_coded_size_.width(), > All strides should be test_stream_.size. Right? > > https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... > content/common/gpu/media/video_encode_accelerator_unittest.cc:795: frame_data + > input_coded_size_.GetArea(), > The offsets of u and v planes should also be calculated by test_stream_.size. > Right? Yes I tried to use test_stream_.size to calculate u v offset and there are many ioctl warning messages. So I think u v offset maybe also need 64 bytes alignment.
https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:117: static void PrepareAlignedTempFile( Plenty of things may fail here, need to return value and check it. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:121: base::File *file = new base::File(base::FilePath("temp" + filename), base::CreateAndOpenTemporaryFile() https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:123: input_file.Initialize(base::FilePath(filename)); Please check the return value. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: size_t position = 0, frame_offset = 0; off_t https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:131: file->Write(frame_offset, buffer, input_buffer_size); This function may not write all data. It returns the number of bytes written, you should check it. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:132: file->Write(frame_offset + input_buffer_size, dummy_buffer, Why not use AppendToFile? You don't need to manage the offset then and smaller chance of error? https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:137: file->Close(); Leaking file? https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: } And leaking dummy_buffer. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:192: if (input_buffer_size & 63) { Given the fact that I420 translates into V4L2_PIX_FMT_YUV420M, which is a 3-plane discontiguous/3-buffer format, you should position each plane separately aligned. I would just get rid of the branch and always do this. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:195: base::FilePath("temp" + filename))); The name should be generated by CreateTemporaryFile, but especially don't hardcode it in more than one place please. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:591: input_buffer_size_ = ALIGN_64_BYTES(input_buffer_size_); It would be good if this was calculated in one place, less chance of error.
https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:121: base::File *file = new base::File(base::FilePath("temp" + filename), On 2014/08/01 00:48:32, Pawel Osciak wrote: > base::CreateAndOpenTemporaryFile() Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:123: input_file.Initialize(base::FilePath(filename)); On 2014/08/01 00:48:32, Pawel Osciak wrote: > Please check the return value. Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: size_t position = 0, frame_offset = 0; On 2014/08/01 00:48:32, Pawel Osciak wrote: > off_t Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:137: file->Close(); On 2014/08/01 00:48:32, Pawel Osciak wrote: > Leaking file? Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: } On 2014/08/01 00:48:32, Pawel Osciak wrote: > And leaking dummy_buffer. Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:192: if (input_buffer_size & 63) { On 2014/08/01 00:48:32, Pawel Osciak wrote: > Given the fact that I420 translates into V4L2_PIX_FMT_YUV420M, which is a > 3-plane discontiguous/3-buffer format, you should position each plane separately > aligned. > > I would just get rid of the branch and always do this. Done. https://codereview.chromium.org/430583005/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:195: base::FilePath("temp" + filename))); On 2014/08/01 00:48:32, Pawel Osciak wrote: > The name should be generated by CreateTemporaryFile, but especially don't > hardcode it in more than one place please. Done.
Why not use base::AppendToFile()?
On 2014/08/01 06:27:07, Pawel Osciak wrote: > Why not use base::AppendToFile()? AppendToFile use FilePath and to open file again. Since I already have opened File object, I prefer to use the same file handler to write buffer at once.
Please add more detailed change description. The title also sounds change.
PTAL
On 2014/08/20 09:53:37, henryhsu wrote: > PTAL Please do not rebase on top of other CLs, especially partially, it makes it hard to review and will give us problems when committing anyway. Please delete the last patchset and reupload without Wu-Cheng's CL. Thanks.
On 2014/08/20 10:51:31, Pawel Osciak wrote: > On 2014/08/20 09:53:37, henryhsu wrote: > > PTAL > > Please do not rebase on top of other CLs, especially partially, it makes it hard > to review and will give us problems when committing anyway. Please delete the > last patchset and reupload without Wu-Cheng's CL. Thanks. Wu-Cheng's CL only modified RowBytes function. But I need Rows function also. I can wait We-Cheng's CL landed into ToT and submit patch again base on his changes.
PTAL
On 2014/08/26 02:48:02, henryhsu wrote: > PTAL Please try to use MemoryMappedFile for the temporary file as well. This should simplify things and let you use memcpy directly into the new file.
- Use more descriptive subject. Like "Make VEA test support videos with different coded size and visible size" - Add more detail and TEST=xxx in the change description. - Add owner review for video_frame.cc/h. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:134: DCHECK_LE(format.fmt.pix_mp.plane_fmt[i].bytesperline, Remove. This is a kernel bug.
I'll finish the review tomorrow. Here are the comments so far. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:132: Add comment to explain what this function does. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:133: static bool PrepareAlignedTempFile(TestStream* test_stream, Output parameter should be after input. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:136: off_t input_planes_count = media::VideoFrame::NumPlanes(kInputFormat); s/input_planes_count/input_num_planes/ to be consistent with NumPlanes. Should we use size_t for this? https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:137: size_t *padding_size = new size_t[input_planes_count]; s/size_t */size_t* / https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: Add comments for important parts of this function. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:178: } Remove parenthesis to be consistent with the next if clause. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:191: static void RemoveTempFile(ScopedVector<TestStream>* test_streams) { s/RemoveTempFile/RemoveTempFiles/. If a function takes a scoped_ptr, that means it takes ownership of the argument. Also, we use * to mean output and const & for input. Use const vector<TestStream>& test_streams. Actually, it's just two lines. I wouldn't use a function for this. for (size_t i = 0; i < test_streams->size(); i++) base::DeleteFile((*test_streams)[i]->temp_file, false); https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:714: // TODO(posciak): For now we only support input streams that meet encoder remove the comments https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:719: // If/when this is changed, the ARM-specific alignment check below should be Do we need to redo the below check? https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:728: // alignment, if the starting virtual addresses of the frames in it were not Explain all three planes have to be 64 byte-aligned. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:732: // that result in proper alignment, and it would have also interfered with Update the comments.
henryhsu@chromium.org changed reviewers: + rvargas@chromium.org, scherkus@chromium.org
scherkus@chromium.org: Please review changes in rvargas@chromium.org: Please review changes in
https://codereview.chromium.org/430583005/diff/140001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/430583005/diff/140001/base/files/memory_mappe... base/files/memory_mapped_file.cc:34: bool MemoryMappedFile::Initialize(const FilePath& file_name) { This method documentation states that "access is restricted to read-only". If we want to add write access, that should be explicit through the API so that callers decide what to do. Also, the protections of the mapped section should match the requested file access (unless you are not trying to write to the section, in which case base doesn't have to change). https://codereview.chromium.org/430583005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/140001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:182: file.Close(); no need to explicitly close the file
PTAL
PTAL
https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:134: DCHECK_LE(format.fmt.pix_mp.plane_fmt[i].bytesperline, On 2014/08/26 09:20:53, wuchengli wrote: > Remove. This is a kernel bug. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:132: On 2014/08/26 09:53:24, wuchengli wrote: > Add comment to explain what this function does. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:133: static bool PrepareAlignedTempFile(TestStream* test_stream, On 2014/08/26 09:53:24, wuchengli wrote: > Output parameter should be after input. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:136: off_t input_planes_count = media::VideoFrame::NumPlanes(kInputFormat); On 2014/08/26 09:53:24, wuchengli wrote: > s/input_planes_count/input_num_planes/ to be consistent with NumPlanes. Should > we use size_t for this? Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:137: size_t *padding_size = new size_t[input_planes_count]; On 2014/08/26 09:53:24, wuchengli wrote: > s/size_t */size_t* / Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: On 2014/08/26 09:53:24, wuchengli wrote: > Add comments for important parts of this function. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:178: } On 2014/08/26 09:53:24, wuchengli wrote: > Remove parenthesis to be consistent with the next if clause. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:191: static void RemoveTempFile(ScopedVector<TestStream>* test_streams) { On 2014/08/26 09:53:25, wuchengli wrote: > s/RemoveTempFile/RemoveTempFiles/. > > If a function takes a scoped_ptr, that means it takes ownership of the argument. > Also, we use * to mean output and const & for input. Use const > vector<TestStream>& test_streams. > > Actually, it's just two lines. I wouldn't use a function for this. > for (size_t i = 0; i < test_streams->size(); i++) > base::DeleteFile((*test_streams)[i]->temp_file, false); Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:714: // TODO(posciak): For now we only support input streams that meet encoder On 2014/08/26 09:53:24, wuchengli wrote: > remove the comments Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:719: // If/when this is changed, the ARM-specific alignment check below should be On 2014/08/26 09:53:25, wuchengli wrote: > Do we need to redo the below check? I think the below check is enabled when platform is ARM. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:728: // alignment, if the starting virtual addresses of the frames in it were not On 2014/08/26 09:53:25, wuchengli wrote: > Explain all three planes have to be 64 byte-aligned. Done. https://codereview.chromium.org/430583005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:732: // that result in proper alignment, and it would have also interfered with On 2014/08/26 09:53:24, wuchengli wrote: > Update the comments. Done. https://codereview.chromium.org/430583005/diff/140001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/430583005/diff/140001/base/files/memory_mappe... base/files/memory_mapped_file.cc:34: bool MemoryMappedFile::Initialize(const FilePath& file_name) { On 2014/08/26 21:25:21, rvargas wrote: > This method documentation states that "access is restricted to read-only". > > If we want to add write access, that should be explicit through the API so that > callers decide what to do. Also, the protections of the mapped section should > match the requested file access (unless you are not trying to write to the > section, in which case base doesn't have to change). > Done. https://codereview.chromium.org/430583005/diff/140001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/140001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:182: file.Close(); On 2014/08/26 21:25:21, rvargas wrote: > no need to explicitly close the file Done.
- Add TEST= in the change description. - In the change description, explain we add this support to test more Hangout resolutions. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:111: base::FilePath temp_file; Add comments for this variable. It's not obvious what's this used for. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:119: // Use |coded_size| to copy YUV data into memory from |test_stream| input file. Explain the result is saved back to test_stream->input_file. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:120: // Also calculate |input_buffer_size| frame size value. Too many sizes here. It's confusing to see "frame size" here. I thought you were talking about width,height. The following may be better. Also calculate the byte size of an input frame and set it to |input_buffer_size|. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:121: static bool PrepareAlignedTempFile(const gfx::Size& coded_size, Return value not used. Remove. Change the function name because this doesn't prepare a temp file anymore. This saves the memory back to the test_stream->input_file. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: size_t* padding_size = new size_t[input_num_planes]; nit: I'd use a vector. Up to you. s/padding_size/padding_sizes/ https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:126: size_t visible_frame_size = 0; Just use VideoFrame::AllocationSize. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:142: if (!test_stream->input_file.IsValid()) { nit: I'd return here to avoid a huge if clause. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:156: // Create a temporary file with coded_size length add period for consistency https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:158: file.Write(*input_buffer_size * num_frames - 1, ".", 1); Why do we need to minus one here? Just curious. What's the reason to choose "."? https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:169: off_t rows = media::VideoFrame::Rows(i, kInputFormat, coded_bpl, visible_bpl, and rows are similar to padding_size. They can be all pre-calculated outside while loop. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:170: test_stream->size.height()); run git cl format https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:172: char *src = reinterpret_cast<char*>( Use uint8* to avoid both castings? s/char */char* / for the entire CL. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:175: const_cast<uint8*>(test_stream->input_file.data() + dest_offset)); Why we do need to cast twice? Can we just use reinterpret_cast? https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:183: dest_offset += padding_rows * coded_bpl + padding_size[i]; Can we precalculate padding_rows * coded_bpl + padding_size[i]? padding_size can be padding_rows * coded_bpl + original padding_size. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: delete[] padding_size; Are we done with the temp_file here? If yes, delete it. Why adding temp_file for each stream if it is only used in this function? https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:709: #if defined(ARCH_CPU_ARMEL) I think this paragraph should be moved to the function comments of PrepareAlignedTempFile to explain. Also, asserting input_buffer_size_ is not enough because we require every plane to 64-byte aligned. Maybe just remove the assertions. Or you should assert every plane. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:718: // Now we test resolutions differ from coded size and prepare chunks before s/differ/different/. Differ is a verb. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:720: // So just assert that the frame size is a multiple of 64 bytes. Do you intend to make this a paragraph? If not, this should go after the previous line? https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:721: // This ensures all frames start at 64-byte boundary, because 80 chars alignment. This should be in the previous line. https://codereview.chromium.org/430583005/diff/160001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/430583005/diff/160001/media/base/video_frame.... media/base/video_frame.h:226: // Returns the number of rows for the given plane, format, and height. Add "The height may be aligned to format requirements." See Pawel's explanation in https://chromiumcodereview.appspot.com/406893002/diff/100001/media/base/video...
https://codereview.chromium.org/430583005/diff/160001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/430583005/diff/160001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:65: PROT_READ | PROT_WRITE, The read/write permission setting should be according to file permission. Otherwise, a read-only file may have permission denied error when calling mmap.
all done. PTAL https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:111: base::FilePath temp_file; On 2014/08/27 05:46:24, wuchengli wrote: > Add comments for this variable. It's not obvious what's this used for. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:119: // Use |coded_size| to copy YUV data into memory from |test_stream| input file. On 2014/08/27 05:46:24, wuchengli wrote: > Explain the result is saved back to test_stream->input_file. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:120: // Also calculate |input_buffer_size| frame size value. On 2014/08/27 05:46:24, wuchengli wrote: > Too many sizes here. It's confusing to see "frame size" here. I thought you were > talking about width,height. The following may be better. > > Also calculate the byte size of an input frame and set it to > |input_buffer_size|. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:121: static bool PrepareAlignedTempFile(const gfx::Size& coded_size, On 2014/08/27 05:46:24, wuchengli wrote: > Return value not used. Remove. > > Change the function name because this doesn't prepare a temp file anymore. This > saves the memory back to the test_stream->input_file. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: size_t* padding_size = new size_t[input_num_planes]; On 2014/08/27 05:46:23, wuchengli wrote: > nit: I'd use a vector. Up to you. > > s/padding_size/padding_sizes/ Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:126: size_t visible_frame_size = 0; On 2014/08/27 05:46:24, wuchengli wrote: > Just use VideoFrame::AllocationSize. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:142: if (!test_stream->input_file.IsValid()) { On 2014/08/27 05:46:24, wuchengli wrote: > nit: I'd return here to avoid a huge if clause. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:158: file.Write(*input_buffer_size * num_frames - 1, ".", 1); On 2014/08/27 05:46:24, wuchengli wrote: > Why do we need to minus one here? > > Just curious. What's the reason to choose "."? Because first parameter is offset. We write one byte with minus one to obtain input_buffer_size file length. No special for ".". Just want to write anything at the end of file. Do you have a suggestion for this one byte? https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:169: off_t rows = media::VideoFrame::Rows(i, kInputFormat, On 2014/08/27 05:46:25, wuchengli wrote: > coded_bpl, visible_bpl, and rows are similar to padding_size. They can be all > pre-calculated outside while loop. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:170: test_stream->size.height()); On 2014/08/27 05:46:24, wuchengli wrote: > run git cl format Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:172: char *src = reinterpret_cast<char*>( On 2014/08/27 05:46:24, wuchengli wrote: > Use uint8* to avoid both castings? > > s/char */char* / for the entire CL. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:175: const_cast<uint8*>(test_stream->input_file.data() + dest_offset)); On 2014/08/27 05:46:24, wuchengli wrote: > Why we do need to cast twice? Can we just use reinterpret_cast? input_file.data() will return const variable. Use const_cast to remove const attribute. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:183: dest_offset += padding_rows * coded_bpl + padding_size[i]; On 2014/08/27 05:46:24, wuchengli wrote: > Can we precalculate padding_rows * coded_bpl + padding_size[i]? padding_size can > be padding_rows * coded_bpl + original padding_size. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: delete[] padding_size; On 2014/08/27 05:46:24, wuchengli wrote: > Are we done with the temp_file here? If yes, delete it. Why adding temp_file for > each stream if it is only used in this function? Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:709: #if defined(ARCH_CPU_ARMEL) On 2014/08/27 05:46:24, wuchengli wrote: > I think this paragraph should be moved to the function comments of > PrepareAlignedTempFile to explain. Also, asserting input_buffer_size_ is not > enough because we require every plane to 64-byte aligned. Maybe just remove the > assertions. Or you should assert every plane. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:718: // Now we test resolutions differ from coded size and prepare chunks before On 2014/08/27 05:46:24, wuchengli wrote: > s/differ/different/. Differ is a verb. Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:720: // So just assert that the frame size is a multiple of 64 bytes. On 2014/08/27 05:46:24, wuchengli wrote: > Do you intend to make this a paragraph? If not, this should go after the > previous line? Done. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:721: // This ensures all frames start at 64-byte boundary, because On 2014/08/27 05:46:24, wuchengli wrote: > 80 chars alignment. This should be in the previous line. Done.
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/430583005/diff/200001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/430583005/diff/200001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:63: int flags = PROT_READ; I'm sorry, but you are still trying to make this automagically without callers being fully aware of what they are doing. If a caller wants to create a mapped section with write access, MemoryMappedFile should have a clear API to explicitly allow that behavior. Either an argument somewhere should specify the desired access, or some new method should declare that intent. Note that passing a file with read/write access is not an indication that the mapped memory should also have read/write access. If a caller passes a file with read only access and asks MemoryMappedFile for write access, we should let the OS reject the operation (there is no reliable way to go back to File::Flags from a File)
Remember to test on Windows since you modified memory_mapped_file_win.cc. https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/160001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:158: file.Write(*input_buffer_size * num_frames - 1, ".", 1); On 2014/08/27 10:07:27, henryhsu wrote: > On 2014/08/27 05:46:24, wuchengli wrote: > > Why do we need to minus one here? > > > > Just curious. What's the reason to choose "."? > > Because first parameter is offset. We write one byte with minus one to obtain > input_buffer_size file length. > No special for ".". Just want to write anything at the end of file. > Do you have a suggestion for this one byte? Can we use SetLength for this? Is SetLength fast? If SetLength cannot be used, add some comments for this line. It's not obvious writing one byte can create a huge file. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:124: // in it were not 64 byte-aligned, we'd have to use a separate set of input Please remove "use a separate set of input buffers and copy the frames". Just explain how you achieve the alignment now. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:127: // testing to avoid performance impact. s/testing/encoding/ https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:129: // The copyied result will be saved in test_stream->input_file. s/copyied/copied/ https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:133: TestStream* test_stream, It is clearer to pass test_stream->size, test_stream->input_file, and test_stream->in_filename to the function parameters. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:141: size_t visible_frame_size = I think it's easier to understand to s/visible_frame_size/visible_buffer_size/ and s/input_buffer_size/coded_buffer_size/. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:145: // Calculate padding size for each plane, and frame size for visible size s/frame size/frame allocation size/. Remove visible size because it's not calculated here. Update the comment. This loop calculates other stuff, too. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: ASSERT_EQ(*input_buffer_size & 63, 0u) This can be removed. The above for loop ensured it. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:189: ASSERT_EQ(reinterpret_cast<off_t>(test_stream->input_file.data()) & 63, 0) This can be removed. The first time of ASSERT_EQ below ensures it. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:204: uint8* src = const_cast<uint8*>(input_file.data() + src_offset); nit: |src| can be const and the casting can be removed. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:214: base::DeleteFile(temp_file, false); Can we delete the file and still access it from test_stream->input_file? https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:710: PrepareInputMemory(input_coded_size, s/PrepareInputMemory/PrepareInputBuffers/ to be consistent with |input_buffer_size_|.
all done. PTAL https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:124: // in it were not 64 byte-aligned, we'd have to use a separate set of input On 2014/08/28 08:33:00, wuchengli wrote: > Please remove "use a separate set of input buffers and copy the frames". Just > explain how you achieve the alignment now. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:127: // testing to avoid performance impact. On 2014/08/28 08:33:01, wuchengli wrote: > s/testing/encoding/ Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:129: // The copyied result will be saved in test_stream->input_file. On 2014/08/28 08:33:01, wuchengli wrote: > s/copyied/copied/ Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:133: TestStream* test_stream, On 2014/08/28 08:33:01, wuchengli wrote: > It is clearer to pass test_stream->size, test_stream->input_file, and > test_stream->in_filename to the function parameters. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:141: size_t visible_frame_size = On 2014/08/28 08:33:01, wuchengli wrote: > I think it's easier to understand to s/visible_frame_size/visible_buffer_size/ > and s/input_buffer_size/coded_buffer_size/. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:145: // Calculate padding size for each plane, and frame size for visible size On 2014/08/28 08:33:00, wuchengli wrote: > s/frame size/frame allocation size/. Remove visible size because it's not > calculated here. Update the comment. This loop calculates other stuff, too. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: ASSERT_EQ(*input_buffer_size & 63, 0u) On 2014/08/28 08:33:01, wuchengli wrote: > This can be removed. The above for loop ensured it. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:189: ASSERT_EQ(reinterpret_cast<off_t>(test_stream->input_file.data()) & 63, 0) On 2014/08/28 08:33:01, wuchengli wrote: > This can be removed. The first time of ASSERT_EQ below ensures it. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:204: uint8* src = const_cast<uint8*>(input_file.data() + src_offset); On 2014/08/28 08:33:01, wuchengli wrote: > nit: |src| can be const and the casting can be removed. Done. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:214: base::DeleteFile(temp_file, false); On 2014/08/28 08:33:00, wuchengli wrote: > Can we delete the file and still access it from test_stream->input_file? Yes, I tried this and encoded results are correct. https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:710: PrepareInputMemory(input_coded_size, On 2014/08/28 08:33:01, wuchengli wrote: > s/PrepareInputMemory/PrepareInputBuffers/ to be consistent with > |input_buffer_size_|. Done.
https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:214: base::DeleteFile(temp_file, false); On 2014/08/28 09:36:44, henryhsu wrote: > On 2014/08/28 08:33:00, wuchengli wrote: > > Can we delete the file and still access it from test_stream->input_file? > > Yes, I tried this and encoded results are correct. This doesn't make sense. Why we can access the mapped file after it is removed? https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file.h:67: bool Initialize(File file, const Region& region, bool write); I think this can be added in the future when someone actually uses it. https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file.h:96: // - |write| is used to enable write permission of mmap or not. s/or not// https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:30: flags = SEC_IMAGE | PAGE_READONLY; Why we don't need PAGE_READWRITE for SEC_IMAGE? https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:73: int map_flags = (flags & PAGE_READONLY) ? FILE_MAP_READ : FILE_MAP_WRITE; s/flags & PAGE_READONLY/write/ map_access is better than map_flags? The parameter name of the prototype is dwDesiredAccess. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:145: // Calculate padding size for each plane, and frame allocation size for 80 char aligned. Move this with the previous line. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:146: // coded size. And also store bytes per line information of coded size and s/And also/Also/ https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: *coded_buffer_size += ALIGN_64_BYTES(size); Move this after padding_bytes so the use of |size| are all together. Too many sizes here. :) https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:183: file.Write(*coded_buffer_size * num_frames - 1, ".", 1); Can we use SetLength for this? Is SetLength fast? If SetLength cannot be used, add some comments for this line. It's not obvious writing one byte can create a huge file. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:197: uint8* dest = const_cast<uint8*>(input_file->data() + dest_offset); We should not return a const and access it. Maybe remove the const of MemoryMappedFile::data() or add a non-const version? https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:705: const_cast<base::MemoryMappedFile*>(&test_stream_.input_file), We should remove the const of test_stream_ because we modify it in VEAClient.
all done https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/200001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:214: base::DeleteFile(temp_file, false); On 2014/08/28 10:17:48, wuchengli wrote: > On 2014/08/28 09:36:44, henryhsu wrote: > > On 2014/08/28 08:33:00, wuchengli wrote: > > > Can we delete the file and still access it from test_stream->input_file? > > > > Yes, I tried this and encoded results are correct. > This doesn't make sense. Why we can access the mapped file after it is removed? Not sure. But for safety, we can rollback to original way to delete file after test finished. https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:30: flags = SEC_IMAGE | PAGE_READONLY; On 2014/08/28 10:17:48, wuchengli wrote: > Why we don't need PAGE_READWRITE for SEC_IMAGE? image_ is true when InitializeAsImageSection function is called. And this function will initialize the file as read-only. https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:73: int map_flags = (flags & PAGE_READONLY) ? FILE_MAP_READ : FILE_MAP_WRITE; On 2014/08/28 10:17:48, wuchengli wrote: > s/flags & PAGE_READONLY/write/ > > map_access is better than map_flags? The parameter name of the prototype is > dwDesiredAccess. Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:145: // Calculate padding size for each plane, and frame allocation size for On 2014/08/28 10:17:48, wuchengli wrote: > 80 char aligned. Move this with the previous line. Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:146: // coded size. And also store bytes per line information of coded size and On 2014/08/28 10:17:48, wuchengli wrote: > s/And also/Also/ Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: *coded_buffer_size += ALIGN_64_BYTES(size); On 2014/08/28 10:17:48, wuchengli wrote: > Move this after padding_bytes so the use of |size| are all together. Too many > sizes here. :) Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:183: file.Write(*coded_buffer_size * num_frames - 1, ".", 1); On 2014/08/28 10:17:48, wuchengli wrote: > Can we use SetLength for this? Is SetLength fast? If SetLength cannot be used, > add some comments for this line. It's not obvious writing one byte can create a > huge file. Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:197: uint8* dest = const_cast<uint8*>(input_file->data() + dest_offset); On 2014/08/28 10:17:48, wuchengli wrote: > We should not return a const and access it. Maybe remove the const of > MemoryMappedFile::data() or add a non-const version? Done. https://codereview.chromium.org/430583005/diff/220001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:705: const_cast<base::MemoryMappedFile*>(&test_stream_.input_file), On 2014/08/28 10:17:48, wuchengli wrote: > We should remove the const of test_stream_ because we modify it in VEAClient. Done.
Looking good! https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/430583005/diff/220001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:30: flags = SEC_IMAGE | PAGE_READONLY; On 2014/08/29 06:36:40, henryhsu wrote: > On 2014/08/28 10:17:48, wuchengli wrote: > > Why we don't need PAGE_READWRITE for SEC_IMAGE? > > image_ is true when InitializeAsImageSection function is called. > And this function will initialize the file as read-only. From MSDN, it doesn't say SEC_IMAGE is not allowed with PAGE_READWRITE. I think the code should be like this. int flags = write ? PAGE_READWRITE : PAGE_READONLY; if (image_) flags |= SEC_IMAGE; http://msdn.microsoft.com/en-us/library/windows/desktop/aa366537(v=vs.85).aspx https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:51: // Later we may want to allow the user to specify access. Remove this line. The new function allows the user to specify access. https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:67: bool Initialize(File file, const Region& region, bool write); I think this can be removed. This can be added in the future when someone actually uses it. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:84: // - |temp_file| is used to prepare input buffers. This should go to struct TestStream. Right? https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:110: base::MemoryMappedFile input_file; in_filename, input_file, and temp_file are similar. Add comments to each variable. Explain |input_file| is the memory mapped of |temp_file|. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:112: base::FilePath temp_file; Move this after |input_file|. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:113: std::string out_filename; Move this before requested_profile. Better to keep all file-related variables together. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:185: base::File file(base::FilePath(*temp_file), flags); |temp_file| is a FilePath. Do we need base::FilePath constructor here? https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1053: for (size_t i = 0; i < test_streams.size(); i++) We should close the memory mapped files before deleting them. MemoryMappedFile::CloseHandles.
https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:63: bool Initialize(File file, bool write); These two methods look like simple supersets of the two previous methods. It seems better to remove the two previous methods (this is in effect an extra argument with a default value, which is strongly discouraged). A second alternative would be to send the write intent to the name of the method and remove the argument, but still, the first alternative seems better. This class is becoming a collection of one-off initializers... we are adding support for writable mappings and yet I can't do that from a FilePath :( ... and we need unit tests.
PTAL https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:51: // Later we may want to allow the user to specify access. On 2014/08/29 10:03:11, wuchengli wrote: > Remove this line. The new function allows the user to specify access. Done. https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:63: bool Initialize(File file, bool write); On 2014/08/29 20:08:58, rvargas wrote: > These two methods look like simple supersets of the two previous methods. It > seems better to remove the two previous methods (this is in effect an extra > argument with a default value, which is strongly discouraged). > > A second alternative would be to send the write intent to the name of the method > and remove the argument, but still, the first alternative seems better. This > class is becoming a collection of one-off initializers... we are adding support > for writable mappings and yet I can't do that from a FilePath :( > > ... and we need unit tests. If using default value for extra argument is strongly discouraged, why not keep all Initialize functions? If we have writable mapping from a FilePath, we should consider the file access is FILE_OPEN, FILE_CREATE, FILE_CREATE_ALWAYS, or FILE_OPEN_ALWAYS. So I prefer that let caller decide this. BTW, Could you tell me how to test windows part? I added two test cases and passed in posix environment. Do you have instructions to use virtual machine to run unittest in Windows? https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:67: bool Initialize(File file, const Region& region, bool write); On 2014/08/29 10:03:11, wuchengli wrote: > I think this can be removed. This can be added in the future when someone > actually uses it. I prefer to keep this because every Initialize function can use this. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:84: // - |temp_file| is used to prepare input buffers. On 2014/08/29 10:03:11, wuchengli wrote: > This should go to struct TestStream. Right? Done. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:110: base::MemoryMappedFile input_file; On 2014/08/29 10:03:11, wuchengli wrote: > in_filename, input_file, and temp_file are similar. Add comments to each > variable. Explain |input_file| is the memory mapped of |temp_file|. Done. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:112: base::FilePath temp_file; On 2014/08/29 10:03:11, wuchengli wrote: > Move this after |input_file|. Done. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:113: std::string out_filename; On 2014/08/29 10:03:11, wuchengli wrote: > Move this before requested_profile. Better to keep all file-related variables > together. Done. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:185: base::File file(base::FilePath(*temp_file), flags); On 2014/08/29 10:03:11, wuchengli wrote: > |temp_file| is a FilePath. Do we need base::FilePath constructor here? Done. https://codereview.chromium.org/430583005/diff/240001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1053: for (size_t i = 0; i < test_streams.size(); i++) On 2014/08/29 10:03:11, wuchengli wrote: > We should close the memory mapped files before deleting them. > MemoryMappedFile::CloseHandles. Done.
https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/240001/base/files/memory_mappe... base/files/memory_mapped_file.h:63: bool Initialize(File file, bool write); On 2014/09/01 05:53:44, henryhsu wrote: > On 2014/08/29 20:08:58, rvargas wrote: > > These two methods look like simple supersets of the two previous methods. It > > seems better to remove the two previous methods (this is in effect an extra > > argument with a default value, which is strongly discouraged). > > > > A second alternative would be to send the write intent to the name of the > method > > and remove the argument, but still, the first alternative seems better. This > > class is becoming a collection of one-off initializers... we are adding > support > > for writable mappings and yet I can't do that from a FilePath :( > > > > ... and we need unit tests. > > If using default value for extra argument is strongly discouraged, why not keep > all Initialize functions? rvargas means removing Initialize(File) and Initialize(File, const Region&) and changing the callers to use your functions. > > If we have writable mapping from a FilePath, we should consider the file access > is FILE_OPEN, FILE_CREATE, FILE_CREATE_ALWAYS, or FILE_OPEN_ALWAYS. > So I prefer that let caller decide this. > > BTW, Could you tell me how to test windows part? > I added two test cases and passed in posix environment. > Do you have instructions to use virtual machine to run unittest in Windows? > https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.h:81: void CloseHandles(); We shouldn't expose an API for the testing purpose. Please rearrange the test code to avoid calling this. https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:176: const char* test_string = "TEST_STRING"; Can we use CreateTestBuffer with a non-0 offset? https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:186: ASSERT_TRUE(memcmp(read_map.data(), test_string, strlen(test_string)) == 0); Use CheckBufferContents with a non-0 offset? https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:218: memcmp(read_map.data() + kOffset, test_string, strlen(test_string)) == 0); Use CheckBufferContents with a non-0 offset? https://codereview.chromium.org/430583005/diff/260001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/260001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:115: // A temporary file is used to prepare input buffers. s/is//
henryhsu@chromium.org changed reviewers: + groby@chromium.org, tony@chromium.org, toyoshim@chromium.org
groby@chromium.org: Please review changes in chrome/renderer/spellchecker/hunspell_engine.cc toyoshim@chromium.org: Please review changes in components/translate/content/renderer/data_file_renderer_cld_data_provider.cc tony@chromium.org: Please review changes in ui/base/resource/data_pack.cc https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.h:81: void CloseHandles(); On 2014/09/01 06:57:15, wuchengli wrote: > We shouldn't expose an API for the testing purpose. Please rearrange the test > code to avoid calling this. Done. https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:176: const char* test_string = "TEST_STRING"; On 2014/09/01 06:57:15, wuchengli wrote: > Can we use CreateTestBuffer with a non-0 offset? Done. https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:186: ASSERT_TRUE(memcmp(read_map.data(), test_string, strlen(test_string)) == 0); On 2014/09/01 06:57:15, wuchengli wrote: > Use CheckBufferContents with a non-0 offset? Done. https://codereview.chromium.org/430583005/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:218: memcmp(read_map.data() + kOffset, test_string, strlen(test_string)) == 0); On 2014/09/01 06:57:15, wuchengli wrote: > Use CheckBufferContents with a non-0 offset? Done. https://codereview.chromium.org/430583005/diff/260001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/260001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:115: // A temporary file is used to prepare input buffers. On 2014/09/01 06:57:15, wuchengli wrote: > s/is// Done.
https://codereview.chromium.org/430583005/diff/280001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/280001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:114: base::MemoryMappedFile* input_file; Use scoped_ptr to indicate ownership.
henryhsu@chromium.org changed reviewers: + rlp@chromium.org - groby@chromium.org
rlp@chromium.org: Please review changes in chrome/renderer/spellchecker/hunspell_engine.cc https://codereview.chromium.org/430583005/diff/280001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/280001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:114: base::MemoryMappedFile* input_file; On 2014/09/01 08:20:42, wuchengli wrote: > Use scoped_ptr to indicate ownership. Done.
https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... base/files/memory_mapped_file.h:53: // As above, works with an already-opened file, closes it when done, and s/works/but works/? https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... base/files/memory_mapped_file.h:58: // write permission when |write| is true. Remove "and enables write permission when |write| is true." That's included by "As above" https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:32: flags = write ? PAGE_READWRITE : PAGE_READONLY; You forgot to address this comment? From MSDN, it doesn't say SEC_IMAGE is not allowed with PAGE_READWRITE. I think the code should be like this. int flags = write ? PAGE_READWRITE : PAGE_READONLY; if (image_) flags |= SEC_IMAGE;
toyoshim@chromium.org changed reviewers: + andrewhayden@chromium.org - toyoshim@chromium.org
For Translate, Andrew is the best reviewer on the modified file.
On 2014/09/01 13:55:50, Takashi Toyoshima (chromium) wrote: > For Translate, Andrew is the best reviewer on the modified file. components/translate/content/renderer/data_file_renderer_cld_data_provider.cc: LGTM
groby@chromium.org changed reviewers: + groby@chromium.org, tsepez@chromium.org
I'm extremely uncomfortable having the API for memory mapped files changed to be writable via a simple unnamed boolean param at Init time - there are _huge_ implications if that ever gets set accidentally. NOT LGTM until somebody from chrome-security explicitly approves that. (Added tsepez)
Well, I'm definitely no expert, but I would assume this wouldn't be the only API in Chrome whose misuse could have important consequences :) Also, I might be missing something, but I'm wondering if disallowing writing via mmaping helps in any way, if we can just write to the file using WriteFile and friends anyway? But this is just a unittest, so we might be reaching the point of diminishing returns here. Perhaps going back to the previous version, before the write permission, would be fast enough.
Henry. You can use test fixture and per-testcase SetUp to construct the test files only once. That should be fast and writable MemoryMappedFile is not absolutely required. https://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Bet...
I agree that we don't need to change the public interface for MemoryMappedFile just for a test. Another option would be to subclass MemoryMappedFile in the test.cc file and make a version that allows writing to the file (you may need to make some member variables protected instead).
https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... base/files/memory_mapped_file.h:53: // As above, works with an already-opened file, closes it when done, and I'd rather have a InitializeForTesting() method than a |write| parameter; its too easy down the road for someone to think that it's OK to call this with write=true in other parts of chrome. https://codereview.chromium.org/430583005/diff/300001/base/files/memory_mappe... base/files/memory_mapped_file.h:67: uint8* data() const { return data_; } You might want, say, mutable_data_for_testing() to return the non-const version. https://codereview.chromium.org/430583005/diff/300001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/430583005/diff/300001/media/base/video_frame.... media/base/video_frame.h:228: static int Rows(size_t plane, Format format, int height); nit: could the result be negative? If not, why are we using signed types here?
rollback to file write approach. PTAL.
https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:120: unsigned int default_requested_bitrate; We really should not mix global variables and testcase variables in the same object. The four non-default variables should be moved out of this struct. Modify UpdateTestStreamData so it returns bitrate and framerate in other ways. Change VEAClient constructor so you can pass these four variables. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:129: ScopedVector<TestStream> g_test_streams; This has indeterminate order of destruction. Use an array of plain pointer and destruct them in VideoEncodeAcceleratorEnvironment. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:135: size_t write_bytes = 0; s/write_bytes/written_bytes/ https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:139: if (!bytes) return false; Should this be if (bytes == -1)? The function comments says "Returns the number of bytes written, or -1 on error." https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&... https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:156: // Use |visible_size| and |coded_size| to copy YUV data into memory from YUV data are copied to file directly. Use |visible_size| and |coded_size| to write YUV data from |in_filename| to |input_file|. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:210: base::File file(*temp_file, flags); s/file/dest_file/ is more readable. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:218: const uint8* ptr = input_file->data() + dest_offset; Is it valid to access data() before Initialize? https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1014: class VideoEncodeAcceleratorEnvironment : public ::testing::Environment { s/VideoEncodeAcceleratorEnvironment/VideoEncodeAcceleratorTestEnvironment/ https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1098: remove blank line
all done. PTAL https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:120: unsigned int default_requested_bitrate; On 2014/09/04 03:34:29, wuchengli wrote: > We really should not mix global variables and testcase variables in the same > object. The four non-default variables should be moved out of this struct. > Modify UpdateTestStreamData so it returns bitrate and framerate in other ways. > Change VEAClient constructor so you can pass these four variables. Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:129: ScopedVector<TestStream> g_test_streams; On 2014/09/04 03:34:29, wuchengli wrote: > This has indeterminate order of destruction. Use an array of plain pointer and > destruct them in VideoEncodeAcceleratorEnvironment. > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:135: size_t write_bytes = 0; On 2014/09/04 03:34:29, wuchengli wrote: > s/write_bytes/written_bytes/ Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:139: if (!bytes) return false; On 2014/09/04 03:34:30, wuchengli wrote: > Should this be if (bytes == -1)? > > The function comments says "Returns the number of bytes written, or -1 on > error." > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&... Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:156: // Use |visible_size| and |coded_size| to copy YUV data into memory from On 2014/09/04 03:34:29, wuchengli wrote: > YUV data are copied to file directly. Use |visible_size| and |coded_size| to > write YUV data from |in_filename| to |input_file|. Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:210: base::File file(*temp_file, flags); On 2014/09/04 03:34:30, wuchengli wrote: > s/file/dest_file/ is more readable. Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:218: const uint8* ptr = input_file->data() + dest_offset; On 2014/09/04 03:34:30, wuchengli wrote: > Is it valid to access data() before Initialize? IsValid function will check data_ != NULL. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1014: class VideoEncodeAcceleratorEnvironment : public ::testing::Environment { On 2014/09/04 03:34:29, wuchengli wrote: > s/VideoEncodeAcceleratorEnvironment/VideoEncodeAcceleratorTestEnvironment/ Done. https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1098: On 2014/09/04 03:34:29, wuchengli wrote: > remove blank line Done.
https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/320001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:218: const uint8* ptr = input_file->data() + dest_offset; On 2014/09/04 08:06:19, henryhsu wrote: > On 2014/09/04 03:34:30, wuchengli wrote: > > Is it valid to access data() before Initialize? > > IsValid function will check data_ != NULL. So if the code reaches here, data_ is NULL and you are checking the wrong address. Right? https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:122: unsigned int default_requested_subsequent_bitrate; I don't think the name needs to be changed. The name is too long. https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:655: // Initialize the test streams. Initialize the parameters of the test streams. https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1032: delete[] g_test_streams; nit: set g_test_streams to NULL after deleting.
all done https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:122: unsigned int default_requested_subsequent_bitrate; On 2014/09/04 08:24:40, wuchengli wrote: > I don't think the name needs to be changed. The name is too long. Done. https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:655: // Initialize the test streams. On 2014/09/04 08:24:40, wuchengli wrote: > Initialize the parameters of the test streams. Done. https://codereview.chromium.org/430583005/diff/340001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1032: delete[] g_test_streams; On 2014/09/04 08:24:40, wuchengli wrote: > nit: set g_test_streams to NULL after deleting. Done.
lgtm
On 2014/09/01 21:41:20, groby (OOO until Sep. 8) wrote: > I'm extremely uncomfortable having the API for memory mapped files changed to be > writable via a simple unnamed boolean param at Init time - there are _huge_ > implications if that ever gets set accidentally. > > NOT LGTM until somebody from chrome-security explicitly approves that. (Added > tsepez) groby@ MemoryMappedFile changes are reverted. I don't know if Rietveld will prevent the CL from submitting with NOT LGTM. If yes, please LGTM the CL. Thanks.
On 2014/09/05 02:23:31, wuchengli wrote: > On 2014/09/01 21:41:20, groby (OOO until Sep. 8) wrote: > > I'm extremely uncomfortable having the API for memory mapped files changed to > be > > writable via a simple unnamed boolean param at Init time - there are _huge_ > > implications if that ever gets set accidentally. > > > > NOT LGTM until somebody from chrome-security explicitly approves that. (Added > > tsepez) > groby@ MemoryMappedFile changes are reverted. I don't know if Rietveld will > prevent the CL from submitting with NOT LGTM. If yes, please LGTM the CL. > Thanks. lgtm...
https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:36: #define ALIGN_64_BYTES(x) (((x) + 63) & ~63) Inline static function please. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:110: std::string in_filename; Please say that his is the original unaligned file provided as an argument to the test. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:113: base::MemoryMappedFile input_file; Please rename this and temp_file to (mapped_)aligned_input_file, or something similar to make code easier understandable. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:115: // A temporary file used to prepare input buffers. Please say what it will contain and why. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: TestStream *g_test_streams; Could this be a property of the Environment impl? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:128: static bool WriteFile(base::File *file, Documentation please. Also star next to type name. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:130: const char* data, Perhaps take uint8* and cast here in the method, not in the caller. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:151: // Now we test resolutions different from coded size and prepare chunks before "Now we test..." is not needed, we don't need to document that we changed a previous behavior, just say what's happening now please. Also, you are using a conditional "we'd have to prepare a memory", but we are doing this already. You should say instead: "Since we are just mmapping and passing chunks of the input file directly to the VEA as input frames to avoid copying large chunks of raw data on each frame and thus affecting performance measurements, we have to prepare a temporary file with all planes aligned to 64-byte boundaries beforehand." https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:157: static void PrepareInputBuffers(const gfx::Size& visible_size, The name is misleading. Please call it CreateAlignedInputStreamFile. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:162: size_t* coded_buffer_size) { Can we make coded_buffer_size a property of TestStream perhaps? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:163: size_t input_num_planes = media::VideoFrame::NumPlanes(kInputFormat); s/input_num_planes/num_planes/ https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: std::vector<size_t> padding_sizes(input_num_planes); These require explanation. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:169: // YUV plane starting address should be 64 bytes alignment. Calculate padding s/be 64 bytes alignment/aligned to 64 byte boundary/ https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:171: // bytes per line information of coded size and visible size. Please in general don't comment describing what code does, but rather give a high-level picture and explain why. Example: - less useful: "Calculate coded size, frame allocation size and store bytes per line". - more useful is to explain what those things are for and what we are doing: "Calculate padding in bytes to be added after each plane required to keep starting addresses of all planes at a 64 byte boundary. This padding will be added after each plane when copying to the temporary file. At the same time we also need to take into account coded_size requested by the VEA; each row of visible_bpl bytes in the original file needs to be copied into a row of coded_bpl bytes in the aligned file." https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:172: *coded_buffer_size = 0; aligned_buffer_size https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:188: padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes; Your destination buffer is coded_bpl[i] * coded_size.height() bytes. So I think you should instead calculate coded_buffer_size += ALIGN(coded_bpl[i] * coded_size.height()) size and padding_bytes should not be necessary I think? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:192: if (input_file->IsValid()) This should go at the beginning of the method. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:197: CHECK(src_file.Initialize(base::FilePath(in_filename))); This should go one line up, or l195 should go one line down. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:201: size_t num_frames = src_file.length() / visible_buffer_size; The check that was here before should still be here, i.e. CHECK_EQ(test_stream_.input_file.length() % input_buffer_size_, 0U) << "Stream byte size is not a product of calculated frame byte size"; https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:210: while (src_offset < static_cast<off_t>(src_file.length())) { Iterate over num_frames? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:212: #if defined(ARCH_CPU_ARMEL) We are now doing alignment for all platforms, so no need for ifdef. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:218: const char* src = Perhaps keep a rolling pointer to the data, instead of calculating data + offset every time? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:228: #if defined(ARCH_CPU_ARMEL) Please remove ifdef. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:248: *test_streams = new TestStream[*test_streams_size]; std::vector of TestStream instead? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:518: TestStream& test_stream_; I think you can keep const here if you don't UpdateTestStreamData from the client, as suggested below. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:757: PrepareInputBuffers(test_stream_.size, Please just pass the test stream, instead of its members. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:769: num_frames_in_stream_ = test_stream_.input_file.length() / input_buffer_size_; This is already calculated in PrepareInputBuffers. Let's make it a property of the TestStream instead and don't calculate it again here. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:872: uint8* frame_data_u = It would be better to have these as the property of TestStream, instead of recalculating it here every frame, and also risking that the algorithm changes in PrepareInputBuffers changes, but not here. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:987: if (requested_subsequent_bitrate_ != current_requested_bitrate_ || Please keep those as a property of TestStream. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1029: class VideoEncodeAcceleratorTestEnvironment : public ::testing::Environment { Documentation please. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1081: mid_stream_bitrate_switch, Please update the streams from here like before, not through VEAClient. And I think you should be able to pull g_test_streams from Environment class if you move it into it? https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1198: content::ParseAndReadTestStreamData(*content::g_test_stream_data, There is a Environment::SetUp() call, a counterpart to TearDown I believe. Perhaps use that? Also, perhaps we could store the globals in VideoEncodeAcceleratorTestEnvironment instead?
LGTM
On 2014/09/09 18:07:32, groby (OOO until Sep. 8) wrote: > LGTM To clarify: RSLGTM in terms of removing my previous hold. Sorry for the late reply, I was OOO.
Patchset #19 (id:400001) has been deleted
Hi Pawel, PTAL. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:36: #define ALIGN_64_BYTES(x) (((x) + 63) & ~63) On 2014/09/05 07:21:32, Pawel Osciak wrote: > Inline static function please. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:110: std::string in_filename; On 2014/09/05 07:21:32, Pawel Osciak wrote: > Please say that his is the original unaligned file provided as an argument to > the test. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:113: base::MemoryMappedFile input_file; On 2014/09/05 07:21:34, Pawel Osciak wrote: > Please rename this and temp_file to (mapped_)aligned_input_file, or something > similar to make code easier understandable. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:115: // A temporary file used to prepare input buffers. On 2014/09/05 07:21:32, Pawel Osciak wrote: > Please say what it will contain and why. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:125: TestStream *g_test_streams; On 2014/09/05 07:21:32, Pawel Osciak wrote: > Could this be a property of the Environment impl? Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:128: static bool WriteFile(base::File *file, On 2014/09/05 07:21:33, Pawel Osciak wrote: > Documentation please. Also star next to type name. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:130: const char* data, On 2014/09/05 07:21:34, Pawel Osciak wrote: > Perhaps take uint8* and cast here in the method, not in the caller. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:151: // Now we test resolutions different from coded size and prepare chunks before On 2014/09/05 07:21:33, Pawel Osciak wrote: > "Now we test..." is not needed, we don't need to document that we changed a > previous behavior, just say what's happening now please. > Also, you are using a conditional "we'd have to prepare a memory", but we are > doing this already. You should say instead: > > "Since we are just mmapping and passing chunks of the input file directly to the > VEA as input frames to avoid copying large chunks of raw data on each frame and > thus affecting performance measurements, we have to prepare a temporary file > with all planes aligned to 64-byte boundaries beforehand." Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:157: static void PrepareInputBuffers(const gfx::Size& visible_size, On 2014/09/05 07:21:33, Pawel Osciak wrote: > The name is misleading. > Please call it CreateAlignedInputStreamFile. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:162: size_t* coded_buffer_size) { On 2014/09/05 07:21:32, Pawel Osciak wrote: > Can we make coded_buffer_size a property of TestStream perhaps? Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:163: size_t input_num_planes = media::VideoFrame::NumPlanes(kInputFormat); On 2014/09/05 07:21:32, Pawel Osciak wrote: > s/input_num_planes/num_planes/ Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: std::vector<size_t> padding_sizes(input_num_planes); On 2014/09/05 07:21:34, Pawel Osciak wrote: > These require explanation. I think the below comments are enough to explain these variables. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:169: // YUV plane starting address should be 64 bytes alignment. Calculate padding On 2014/09/05 07:21:33, Pawel Osciak wrote: > s/be 64 bytes alignment/aligned to 64 byte boundary/ Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:171: // bytes per line information of coded size and visible size. On 2014/09/05 07:21:33, Pawel Osciak wrote: > Please in general don't comment describing what code does, but rather give a > high-level picture and explain why. > Example: > > - less useful: "Calculate coded size, frame allocation size and store bytes per > line". > > - more useful is to explain what those things are for and what we are doing: > "Calculate padding in bytes to be added after each plane required to keep > starting addresses of all planes at a 64 byte boundary. This padding will be > added after each plane when copying to the temporary file. > > At the same time we also need to take into account coded_size requested by the > VEA; each row of visible_bpl bytes in the original file needs to be copied into > a row of coded_bpl bytes in the aligned file." Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:172: *coded_buffer_size = 0; On 2014/09/05 07:21:33, Pawel Osciak wrote: > aligned_buffer_size Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:188: padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes; On 2014/09/05 07:21:33, Pawel Osciak wrote: > Your destination buffer is coded_bpl[i] * coded_size.height() bytes. > > So I think you should instead calculate > > coded_buffer_size += ALIGN(coded_bpl[i] * coded_size.height()) > > size and padding_bytes should not be necessary I think? coded_bpl[i] * coded_size.height() is exactly PlaneAllocationSize. But it doesn't guarantee the size is 64 byte alignment. We still need padding_bytes to set offset to correct value after copied data of rows of visible plane. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:192: if (input_file->IsValid()) On 2014/09/05 07:21:33, Pawel Osciak wrote: > This should go at the beginning of the method. Because we should set coded_buffer_size for each encoder. If we put coded_buffer_size into TestStream class, we can move this line to beginning of the method. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:197: CHECK(src_file.Initialize(base::FilePath(in_filename))); On 2014/09/05 07:21:33, Pawel Osciak wrote: > This should go one line up, or l195 should go one line down. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:201: size_t num_frames = src_file.length() / visible_buffer_size; On 2014/09/05 07:21:34, Pawel Osciak wrote: > The check that was here before should still be here, i.e. > > CHECK_EQ(test_stream_.input_file.length() % input_buffer_size_, 0U) > << "Stream byte size is not a product of calculated frame byte size"; Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:210: while (src_offset < static_cast<off_t>(src_file.length())) { On 2014/09/05 07:21:32, Pawel Osciak wrote: > Iterate over num_frames? Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:212: #if defined(ARCH_CPU_ARMEL) On 2014/09/05 07:21:32, Pawel Osciak wrote: > We are now doing alignment for all platforms, so no need for ifdef. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:218: const char* src = On 2014/09/05 07:21:33, Pawel Osciak wrote: > Perhaps keep a rolling pointer to the data, instead of calculating data + offset > every time? Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:228: #if defined(ARCH_CPU_ARMEL) On 2014/09/05 07:21:33, Pawel Osciak wrote: > Please remove ifdef. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:248: *test_streams = new TestStream[*test_streams_size]; On 2014/09/05 07:21:33, Pawel Osciak wrote: > std::vector of TestStream instead? Static or global variables of class type are forbidden. Maybe let test_stream be a property of the Environment impl. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:518: TestStream& test_stream_; On 2014/09/05 07:21:33, Pawel Osciak wrote: > I think you can keep const here if you don't UpdateTestStreamData from the > client, as suggested below. We put input_buffer_size_ as a property of TestStream and we have to update it. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:757: PrepareInputBuffers(test_stream_.size, On 2014/09/05 07:21:32, Pawel Osciak wrote: > Please just pass the test stream, instead of its members. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:769: num_frames_in_stream_ = test_stream_.input_file.length() / input_buffer_size_; On 2014/09/05 07:21:32, Pawel Osciak wrote: > This is already calculated in PrepareInputBuffers. Let's make it a property of > the TestStream instead and don't calculate it again here. We put input_buffer_size and num_frames into TestStream. So we don't need num_frames_in_stream_ and input_buffer_size_ in VEAClient any more. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:872: uint8* frame_data_u = On 2014/09/05 07:21:32, Pawel Osciak wrote: > It would be better to have these as the property of TestStream, instead of > recalculating it here every frame, and also risking that the algorithm changes > in PrepareInputBuffers changes, but not here. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:987: if (requested_subsequent_bitrate_ != current_requested_bitrate_ || On 2014/09/05 07:21:32, Pawel Osciak wrote: > Please keep those as a property of TestStream. Since TestStream is a property of environment object, I prefer TestStream is used to store values which can be used for all test cases and VEAClient has updated test parameters for each test case. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1029: class VideoEncodeAcceleratorTestEnvironment : public ::testing::Environment { On 2014/09/05 07:21:33, Pawel Osciak wrote: > Documentation please. Done. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1081: mid_stream_bitrate_switch, On 2014/09/05 07:21:34, Pawel Osciak wrote: > Please update the streams from here like before, not through VEAClient. > > And I think you should be able to pull g_test_streams from Environment class if > you move it into it? requested_subsequent_bitrate and requested_subsequent_framerate are set by test case parameters. I think putting these in VEAClient is better than TestStream because TestStream will be used for all test cases. https://codereview.chromium.org/430583005/diff/380001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1198: content::ParseAndReadTestStreamData(*content::g_test_stream_data, On 2014/09/05 07:21:33, Pawel Osciak wrote: > There is a Environment::SetUp() call, a counterpart to TearDown I believe. > Perhaps use that? > > Also, perhaps we could store the globals in > VideoEncodeAcceleratorTestEnvironment instead? Done.
lgtm
https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:188: padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes; On 2014/09/10 11:01:49, henryhsu wrote: > On 2014/09/05 07:21:33, Pawel Osciak wrote: > > Your destination buffer is coded_bpl[i] * coded_size.height() bytes. > > > > So I think you should instead calculate > > > > coded_buffer_size += ALIGN(coded_bpl[i] * coded_size.height()) > > > > size and padding_bytes should not be necessary I think? > > coded_bpl[i] * coded_size.height() is exactly PlaneAllocationSize. But it > doesn't guarantee the size is 64 byte alignment. We still need padding_bytes to > set offset to correct value after copied data of rows of visible plane. That's why I suggested ALIGN(coded_bpl[i] * coded_size.height()) above, not just coded_bpl[i] * coded_size.height()... https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:192: if (input_file->IsValid()) On 2014/09/10 11:01:49, henryhsu wrote: > On 2014/09/05 07:21:33, Pawel Osciak wrote: > > This should go at the beginning of the method. > > Because we should set coded_buffer_size for each encoder. > If we put coded_buffer_size into TestStream class, we can move this line to > beginning of the method. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:248: *test_streams = new TestStream[*test_streams_size]; On 2014/09/10 11:01:50, henryhsu wrote: > On 2014/09/05 07:21:33, Pawel Osciak wrote: > > std::vector of TestStream instead? > Static or global variables of class type are forbidden. > Maybe let test_stream be a property of the Environment impl. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:518: TestStream& test_stream_; On 2014/09/10 11:01:49, henryhsu wrote: > On 2014/09/05 07:21:33, Pawel Osciak wrote: > > I think you can keep const here if you don't UpdateTestStreamData from the > > client, as suggested below. > > We put input_buffer_size_ as a property of TestStream and we have to update it. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:769: num_frames_in_stream_ = test_stream_.input_file.length() / input_buffer_size_; On 2014/09/10 11:01:49, henryhsu wrote: > On 2014/09/05 07:21:32, Pawel Osciak wrote: > > This is already calculated in PrepareInputBuffers. Let's make it a property of > > the TestStream instead and don't calculate it again here. > > We put input_buffer_size and num_frames into TestStream. So we don't need > num_frames_in_stream_ and input_buffer_size_ in VEAClient any more. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:987: if (requested_subsequent_bitrate_ != current_requested_bitrate_ || On 2014/09/10 11:01:50, henryhsu wrote: > On 2014/09/05 07:21:32, Pawel Osciak wrote: > > Please keep those as a property of TestStream. > > Since TestStream is a property of environment object, I prefer TestStream is > used to store values which can be used for all test cases and VEAClient has > updated test parameters for each test case. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/380001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1081: mid_stream_bitrate_switch, On 2014/09/10 11:01:50, henryhsu wrote: > On 2014/09/05 07:21:34, Pawel Osciak wrote: > > Please update the streams from here like before, not through VEAClient. > > > > And I think you should be able to pull g_test_streams from Environment class > if > > you move it into it? > requested_subsequent_bitrate and requested_subsequent_framerate are set by test > case parameters. I think putting these in VEAClient is better than TestStream > because TestStream will be used for all test cases. Acknowledged. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (left): https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1061: Please don't remove this line. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:117: // The memory mapped of |aligned_in_file| s/mapped/mapping/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:124: std::vector<size_t>* aligned_plane_size; Just std::vector<> ? https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:134: inline static uint32 Align64Bytes(uint32 value) { s/uint32/size_t/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: // Write |data| with |size| bytes into |offset| bytes of |file|. s/with/of/ s/into/at/ s/of/into/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:166: if (test_stream->mapped_aligned_in_file.IsValid()) If coded_size is different from the one you got passed before, this will not work. I know that this may not happen, but this assumption violates the encapsulation, so we have to guard against this. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:182: for (off_t i = 0; i < num_planes; i++) { s/off_t/size_t/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:185: size_t padding_bytes = Align64Bytes(size) - size; This is not needed. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: test_stream->aligned_buffer_size += Align64Bytes(size); += aligned_plane_size https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:195: size_t padding_rows = This should just be coded_size.height() - size.height(). We don't want to modify coded_size.height() required by VEA. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:198: padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes; This can be Align64Bytes(padding_rows * coded_bpl[i]) https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:222: for (off_t frame = 0; frame < test_stream->num_frames; frame++) { s/off_t/size_t/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:223: for (off_t i = 0; i < num_planes; i++) { s/off_t/size_t/ https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:227: for (off_t j = 0; j < visible_plane_rows[i]; j++) { size_t https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:530: TestStream& test_stream_; I'm not sure this is allowed. Pointer? https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1036: VideoEncodeAcceleratorTestEnvironment* g_env; Please define globals together at the top of the file. https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1177: content::g_env->test_stream_data = test_stream_data.get(); This should be an argument to the constructor of environment. Or at least please have a setter. Also, you could just make g_env own test_stream_data, creating env after parsing arguments.
all done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (left): https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1061: On 2014/09/11 09:51:47, Pawel Osciak wrote: > Please don't remove this line. Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:117: // The memory mapped of |aligned_in_file| On 2014/09/11 09:51:47, Pawel Osciak wrote: > s/mapped/mapping/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:124: std::vector<size_t>* aligned_plane_size; On 2014/09/11 09:51:46, Pawel Osciak wrote: > Just std::vector<> ? Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:134: inline static uint32 Align64Bytes(uint32 value) { On 2014/09/11 09:51:47, Pawel Osciak wrote: > s/uint32/size_t/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:138: // Write |data| with |size| bytes into |offset| bytes of |file|. On 2014/09/11 09:51:47, Pawel Osciak wrote: > s/with/of/ > s/into/at/ > s/of/into/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:166: if (test_stream->mapped_aligned_in_file.IsValid()) On 2014/09/11 09:51:46, Pawel Osciak wrote: > If coded_size is different from the one you got passed before, this will not > work. > I know that this may not happen, but this assumption violates the encapsulation, > so we have to guard against this. Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:182: for (off_t i = 0; i < num_planes; i++) { On 2014/09/11 09:51:46, Pawel Osciak wrote: > s/off_t/size_t/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:185: size_t padding_bytes = Align64Bytes(size) - size; On 2014/09/11 09:51:46, Pawel Osciak wrote: > This is not needed. Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:187: test_stream->aligned_buffer_size += Align64Bytes(size); On 2014/09/11 09:51:46, Pawel Osciak wrote: > += aligned_plane_size Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:195: size_t padding_rows = On 2014/09/11 09:51:47, Pawel Osciak wrote: > This should just be coded_size.height() - size.height(). We don't want to modify > coded_size.height() required by VEA. For UV planes, the value should be (coded_size.height() - size.height()) / 2. So I use Rows function to get correct value for each plane. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:198: padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes; On 2014/09/11 09:51:47, Pawel Osciak wrote: > This can be > Align64Bytes(padding_rows * coded_bpl[i]) Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:222: for (off_t frame = 0; frame < test_stream->num_frames; frame++) { On 2014/09/11 09:51:47, Pawel Osciak wrote: > s/off_t/size_t/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:223: for (off_t i = 0; i < num_planes; i++) { On 2014/09/11 09:51:47, Pawel Osciak wrote: > s/off_t/size_t/ Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:227: for (off_t j = 0; j < visible_plane_rows[i]; j++) { On 2014/09/11 09:51:46, Pawel Osciak wrote: > size_t Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:530: TestStream& test_stream_; On 2014/09/11 09:51:47, Pawel Osciak wrote: > I'm not sure this is allowed. Pointer? Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1036: VideoEncodeAcceleratorTestEnvironment* g_env; On 2014/09/11 09:51:47, Pawel Osciak wrote: > Please define globals together at the top of the file. Done. https://codereview.chromium.org/430583005/diff/420001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1177: content::g_env->test_stream_data = test_stream_data.get(); On 2014/09/11 09:51:46, Pawel Osciak wrote: > This should be an argument to the constructor of environment. Or at least please > have a setter. > > Also, you could just make g_env own test_stream_data, creating env after parsing > arguments. Done.
Just tiny nits :) https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/420001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:195: size_t padding_rows = On 2014/09/11 11:39:10, henryhsu wrote: > On 2014/09/11 09:51:47, Pawel Osciak wrote: > > This should just be coded_size.height() - size.height(). We don't want to > modify > > coded_size.height() required by VEA. > > For UV planes, the value should be (coded_size.height() - size.height()) / 2. > So I use Rows function to get correct value for each plane. Ah sorry, you are right. My bad. https://chromiumcodereview.appspot.com/430583005/diff/440001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/440001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:171: if (test_stream->mapped_aligned_in_file.IsValid()) instead of: if (a) if (b) please: if (a && b) https://chromiumcodereview.appspot.com/430583005/diff/440001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:174: // Delete previous temporary aligned file if coded_size changed. What if we have a multi-instance test? We may delete data prepared for other instances. I think we should just bail out, just ASSERT(!valid || size == size) above. Just make sure this doesn't happen, but it's not worth handling this. https://chromiumcodereview.appspot.com/430583005/diff/440001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1188: content::g_env = Could you move this to l.1214 and test_stream_data.Pass() to g_env and have g_env own the scoper?
PTAL https://codereview.chromium.org/430583005/diff/440001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/440001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:171: if (test_stream->mapped_aligned_in_file.IsValid()) On 2014/09/17 09:58:28, Pawel Osciak wrote: > instead of: > > if (a) > if (b) > > please: > if (a && b) Done. https://codereview.chromium.org/430583005/diff/440001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:174: // Delete previous temporary aligned file if coded_size changed. On 2014/09/17 09:58:28, Pawel Osciak wrote: > What if we have a multi-instance test? We may delete data prepared for other > instances. I think we should just bail out, just ASSERT(!valid || size == size) > above. Just make sure this doesn't happen, but it's not worth handling this. Good idea. We just make sure multi-instance use the same coded size. https://codereview.chromium.org/430583005/diff/440001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1188: content::g_env = On 2014/09/17 09:58:28, Pawel Osciak wrote: > Could you move this to l.1214 and test_stream_data.Pass() to g_env and have > g_env own the scoper? Done.
lgtm % nits https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:174: ASSERT_TRUE(!test_stream->mapped_aligned_in_file.IsValid() || Please add a comment why. https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1048: scoped_ptr<base::FilePath::StringType> test_stream_data; Class members should be suffixed with a "_". Sorry I didn't notice this before.
all done https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:174: ASSERT_TRUE(!test_stream->mapped_aligned_in_file.IsValid() || On 2014/09/17 13:42:02, Pawel Osciak wrote: > Please add a comment why. Done. https://codereview.chromium.org/430583005/diff/460001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1048: scoped_ptr<base::FilePath::StringType> test_stream_data; On 2014/09/17 13:42:02, Pawel Osciak wrote: > Class members should be suffixed with a "_". Sorry I didn't notice this before. Done.
lgtm, please also test on Intel before submitting https://chromiumcodereview.appspot.com/430583005/diff/480001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/430583005/diff/480001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:174: // Multiple encoders should have the same coded size "All encoders in multiple encoder test reuse the same test_stream, make sure they requested the same coded_size"
The CQ bit was checked by henryhsu@chromium.org
The CQ bit was unchecked by henryhsu@chromium.org
The CQ bit was checked by henryhsu@chromium.org
The CQ bit was unchecked by henryhsu@chromium.org
The CQ bit was checked by henryhsu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/430583005/500001
The CQ bit was unchecked by henryhsu@chromium.org
The CQ bit was checked by henryhsu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/430583005/520001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by henryhsu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/430583005/520001
Message was sent while issue was closed.
Committed patchset #24 (id:520001) as 454546abe018d0b1a1241720e4777cecb2459311
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/fb04d233f44676c70cec847d0322c26ac32a256e Cr-Commit-Position: refs/heads/master@{#295913} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
