Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1097793006:
vea_unittest: Add --output_log option (Closed)
|
Created:
5 years, 8 months ago by Justin Chuang Modified:
5 years, 7 months ago 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionvea_unittest: Add --output_log option
This can help autotest to parse the performance data.
BUG=345181
TEST=Enable this option, see FPS info in log.
TEST=Unittest program exits if it cannot create the file.
Committed: https://crrev.com/94afe7beaa924b3133c2d7259181f39cb7084532
Cr-Commit-Position: refs/heads/master@{#327261}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Owen's comments #Patch Set 3 : #
Total comments: 4
Patch Set 4 : Address Pawel's comments #Patch Set 5 : Rebase #Messages
Total messages: 16 (4 generated)
jchuang@chromium.org changed reviewers: + owenlin@chromium.org, wuchengli@chromium.org
just some nits https://chromiumcodereview.appspot.com/1097793006/diff/1/content/common/gpu/m... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1097793006/diff/1/content/common/gpu/m... content/common/gpu/media/video_encode_accelerator_unittest.cc:361: // and end of unittest. We only need to setup once for all test cases. I only see the temporary aligned file getting deleted in TearDone. Change the comment ? https://chromiumcodereview.appspot.com/1097793006/diff/1/content/common/gpu/m... content/common/gpu/media/video_encode_accelerator_unittest.cc:369: test_stream_data_ = data.Pass(); Move these two lines into the initializer list. https://chromiumcodereview.appspot.com/1097793006/diff/1/content/common/gpu/m... content/common/gpu/media/video_encode_accelerator_unittest.cc:386: if (log_file_.get()) log_file_.reset() should works. https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&... https://chromiumcodereview.appspot.com/1097793006/diff/1/content/common/gpu/m... content/common/gpu/media/video_encode_accelerator_unittest.cc:391: if (log_file_.get()) if (log_file_)
jchuang@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/1097793006/diff/1/content/common/gpu/media/vi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1097793006/diff/1/content/common/gpu/media/vi... content/common/gpu/media/video_encode_accelerator_unittest.cc:361: // and end of unittest. We only need to setup once for all test cases. On 2015/04/24 05:10:32, Owen Lin wrote: > I only see the temporary aligned file getting deleted in TearDone. Change the > comment ? Done. PTAL again https://codereview.chromium.org/1097793006/diff/1/content/common/gpu/media/vi... content/common/gpu/media/video_encode_accelerator_unittest.cc:369: test_stream_data_ = data.Pass(); On 2015/04/24 05:10:32, Owen Lin wrote: > Move these two lines into the initializer list. Done. https://codereview.chromium.org/1097793006/diff/1/content/common/gpu/media/vi... content/common/gpu/media/video_encode_accelerator_unittest.cc:386: if (log_file_.get()) On 2015/04/24 05:10:32, Owen Lin wrote: > log_file_.reset() should works. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file.h&... Cool. Thanks https://codereview.chromium.org/1097793006/diff/1/content/common/gpu/media/vi... content/common/gpu/media/video_encode_accelerator_unittest.cc:391: if (log_file_.get()) On 2015/04/24 05:10:32, Owen Lin wrote: > if (log_file_) Done.
https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:403: scoped_ptr<base::File> log_file_; Please document the log format somewhere. https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1416: base::FilePath::StringType(it->second.begin(), it->second.end())); Would log_path = base::FilePath(it->second) work?
lgtm
PTAL again. https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:403: scoped_ptr<base::File> log_file_; On 2015/04/27 02:08:56, Pawel Osciak wrote: > Please document the log format somewhere. Done. https://chromiumcodereview.appspot.com/1097793006/diff/40001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1416: base::FilePath::StringType(it->second.begin(), it->second.end())); On 2015/04/27 02:08:56, Pawel Osciak wrote: > Would > > log_path = base::FilePath(it->second) > > work? it->second is base:string16 on OS_WIN, which is wstring on OS_WIN, and FilePath::StringType is also wstring on OS_WIN. (Note that base::string16 may be different than wstring on other platforms.) So yes, it happens to be correct to initialize with it->second. But the type conversion is not straightforward. Use iterator can convert combinations of std::string and wstring. How do you think?
Pawel. PTAL.
lgtm
The CQ bit was checked by jchuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1097793006/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097793006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/94afe7beaa924b3133c2d7259181f39cb7084532 Cr-Commit-Position: refs/heads/master@{#327261} |
