Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2976373002:
Report minimum PSNR in VideoQualityTest and save corresponding frame to file (Closed)
DescriptionReport minimum PSNR in VideoQualityTest and save corresponding frame to file
BUG=none
Review-Url: https://codereview.webrtc.org/2976373002
Cr-Commit-Position: refs/heads/master@{#19130}
Committed: https://chromium.googlesource.com/external/webrtc/+/59cac99c9a80668110ad8f25f711a0d1111d71d9
Patch Set 1 #
Total comments: 8
Patch Set 2 : Implement Sprang@ comments #Patch Set 3 : Disable frame writing on android #Patch Set 4 : cleanup #
Total comments: 2
Messages
Total messages: 19 (8 generated)
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
just some nits https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:832: frame_writer.Init(); Maybe DCHECK the return value? https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:841: frame_writer.WriteFrame(extracted_buffer.data()); DCHECK return value https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:865: if (!min_psnr_ || *min_psnr_ > psnr) { Merge these nested ifs into one https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:1074: rtc::Optional<VideoFrame> bad_frame_ GUARDED_BY(comparison_lock_); Maybe you could make a struct for these two instead. Comment in that on the purpose of these two values, and have a single optional.
https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:832: frame_writer.Init(); On 2017/07/24 14:24:09, sprang_webrtc wrote: > Maybe DCHECK the return value? Done. https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:841: frame_writer.WriteFrame(extracted_buffer.data()); On 2017/07/24 14:24:09, sprang_webrtc wrote: > DCHECK return value Done. https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:865: if (!min_psnr_ || *min_psnr_ > psnr) { On 2017/07/24 14:24:09, sprang_webrtc wrote: > Merge these nested ifs into one Done. https://codereview.webrtc.org/2976373002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:1074: rtc::Optional<VideoFrame> bad_frame_ GUARDED_BY(comparison_lock_); On 2017/07/24 14:24:09, sprang_webrtc wrote: > Maybe you could make a struct for these two instead. Comment in that on the > purpose of these two values, and have a single optional. Done.
lgtm
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/26036)
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2976373002/#ps60001 (title: "cleanup")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500983802182170,
"parent_rev": "d3f3c3497bad07a5346269d4e5b2afda7c266cc2", "commit_rev":
"59cac99c9a80668110ad8f25f711a0d1111d71d9"}
Message was sent while issue was closed.
Description was changed from ========== Report minimum PSNR in VideoQualityTest and save corresponding frame to file BUG=none ========== to ========== Report minimum PSNR in VideoQualityTest and save corresponding frame to file BUG=none Review-Url: https://codereview.webrtc.org/2976373002 Cr-Commit-Position: refs/heads/master@{#19130} Committed: https://chromium.googlesource.com/external/webrtc/+/59cac99c9a80668110ad8f25f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/59cac99c9a80668110ad8f25f...
Message was sent while issue was closed.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:832: test::Y4mFrameWriterImpl frame_writer(test_label_ + ".y4m", Isn't it better to write this to some directory other than the root? I think it's a bit polluting right now. Maybe webrtc/out/?
Message was sent while issue was closed.
On 2017/08/22 11:32:18, stefan-webrtc wrote: > https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... > File webrtc/video/video_quality_test.cc (right): > > https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... > webrtc/video/video_quality_test.cc:832: test::Y4mFrameWriterImpl > frame_writer(test_label_ + ".y4m", > Isn't it better to write this to some directory other than the root? I think > it's a bit polluting right now. Maybe webrtc/out/? FYI https://codereview.webrtc.org/2998143002/ converts this to jpeg, and also requires a command line flag to enable it in the first place. Feel free to lgtm if you want it landed ;)
Message was sent while issue was closed.
https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2976373002/diff/60001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:832: test::Y4mFrameWriterImpl frame_writer(test_label_ + ".y4m", On 2017/08/22 11:32:18, stefan-webrtc wrote: > Isn't it better to write this to some directory other than the root? I think > it's a bit polluting right now. Maybe webrtc/out/? Yes, this problem is acknowledged. As Erik stated, it's being fixed in https://codereview.webrtc.org/2998143002/ : directory is now configurable from the command-line (/out by default), special flag is required to enable the saving in the first place, and files are much smaller. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
