Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2990563002:
Add Jpeg frame writer for test support. (Closed)
DescriptionAdd Jpeg frame writer for test support.
Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view.
BUG=webrtc:8030
Review-Url: https://codereview.webrtc.org/2990563002
Cr-Commit-Position: refs/heads/master@{#19414}
Committed: https://chromium.googlesource.com/external/webrtc/+/26e5cbd6bb38ed734559ab70cb7362870d76c923
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : cleanup #Patch Set 4 : Dont use libjpeg on ios. Disable frame writer #Patch Set 5 : Disable Jpeg frame writer on iOS correct way. #Patch Set 6 : Rebase #Patch Set 7 : Wire up jpeg frame writer to video quality tests #Patch Set 8 : Fix memory issues #
Total comments: 4
Patch Set 9 : Improved jpeg quality #
Total comments: 18
Patch Set 10 : Rebase #Patch Set 11 : Implement Pbos@ comments #
Total comments: 17
Patch Set 12 : Implemented Sprang@ comments. #
Messages
Total messages: 52 (36 generated)
ilnik@webrtc.org changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22256) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27577)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/22040)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
+sprang for video/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/26627)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (left): https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:827: // TODO(ilnik): enable frame writing for android, once jpeg frame writer Shouldn't this be lossless to actually show what it looks like? E.g. would you want to write a png here? https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:842: res = frame_writer.WriteFrame(worst_frame_->frame, 90 /*quality*/); 90 is definitely nowhere near lossless? Are sizes that big of a concern if you're only saving one? Why wasn't saving a y4m frame OK?
Description was changed from ========== Add Jpeg frame writer for test support. BUG=none ========== to ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. BUG=none ==========
https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (left): https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:827: // TODO(ilnik): enable frame writing for android, once jpeg frame writer On 2017/07/25 18:10:28, pbos-webrtc wrote: > Shouldn't this be lossless to actually show what it looks like? E.g. would you > want to write a png here? Space is probably the most concern here, as these frames will be saved for all ~30 testcases several times a day. Also, main point here is to manually check for corruptions in the video which will be noticable even with minor jpeg artifacts. https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:842: res = frame_writer.WriteFrame(worst_frame_->frame, 90 /*quality*/); On 2017/07/25 18:10:28, pbos-webrtc wrote: > 90 is definitely nowhere near lossless? Are sizes that big of a concern if > you're only saving one? Why wasn't saving a y4m frame OK? We are saving one for each of ~30 testcases each run on perfbots. There will be more testcases in the future. Also, jpeg is much nicer to view than y4m. I am changing that quality setting to 100. This is the best possible. Still not lossless, but with very minor artifacts.
Description was changed from ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. BUG=none ========== to ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view. BUG=none ==========
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view. BUG=none ========== to ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view. BUG=webrtc:8030 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Erik, if there are no comments from your side and if Peter approves this CL, please land it.
https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/jpeg_frame_writer.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:38: RTC_CHECK(!frame_written_) << "Only single frame can be saved to Jpeg."; a single https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:43: fprintf(stderr, "Could not allocate memory for RGB image.\n"); This can't happen, per C++ new never fails (it throws), so you can ignore the rgb_buf == nullptr check. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:77: row_stride = input_frame.width() * kColorPlanes; int row_stride = https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/test_output.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:23: strdup(webrtc::test::OutputPath().c_str()), I think this should be a const std::string kDefaultOutputPath = webrtc::test::OutputPath(); then, this strdup leaks. Use kDefaultOutputPath.c_str() as a default value then. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:30: if (FLAG_test_output_dir == nullptr || strlen(FLAG_test_output_dir) == 0) { Can this be if (std::string(FLAG_test_output_dir) != "", or strcmp(FLAG_test_output_dir, "") == 0)? This should never be nullptr, right? https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:41: if (FLAG_test_output_dir == nullptr || strlen(FLAG_test_output_dir) == 0) { ditto https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:832: if (worst_frame_) { Put a comment here saying why we try to save the worst frame. Since this intends to track corruptions and not frame quality, jpeg is acceptable. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:836: output_dir = ""; if (!test::GetTestOutputDir(&output_dir)) { } No need to store an intermediate res. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:843: RTC_DCHECK(res); RTC_CHECK(frame_writer.WriteFrame(...)); And remove res.
https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/jpeg_frame_writer.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:38: RTC_CHECK(!frame_written_) << "Only single frame can be saved to Jpeg."; On 2017/07/26 18:48:03, pbos-webrtc wrote: > a single Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:43: fprintf(stderr, "Could not allocate memory for RGB image.\n"); On 2017/07/26 18:48:03, pbos-webrtc wrote: > This can't happen, per C++ new never fails (it throws), so you can ignore the > rgb_buf == nullptr check. Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:77: row_stride = input_frame.width() * kColorPlanes; On 2017/07/26 18:48:03, pbos-webrtc wrote: > int row_stride = Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/test_output.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:23: strdup(webrtc::test::OutputPath().c_str()), On 2017/07/26 18:48:03, pbos-webrtc wrote: > I think this should be a const std::string kDefaultOutputPath = > webrtc::test::OutputPath(); then, this strdup leaks. > > Use kDefaultOutputPath.c_str() as a default value then. Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:30: if (FLAG_test_output_dir == nullptr || strlen(FLAG_test_output_dir) == 0) { On 2017/07/26 18:48:03, pbos-webrtc wrote: > Can this be if (std::string(FLAG_test_output_dir) != "", or > strcmp(FLAG_test_output_dir, "") == 0)? > > This should never be nullptr, right? Yes, it can't be nullptr ever. Still, I think strlen fits here better. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/test_output.cc:41: if (FLAG_test_output_dir == nullptr || strlen(FLAG_test_output_dir) == 0) { On 2017/07/26 18:48:03, pbos-webrtc wrote: > ditto Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:832: if (worst_frame_) { On 2017/07/26 18:48:03, pbos-webrtc wrote: > Put a comment here saying why we try to save the worst frame. Since this intends > to track corruptions and not frame quality, jpeg is acceptable. Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:836: output_dir = ""; On 2017/07/26 18:48:03, pbos-webrtc wrote: > if (!test::GetTestOutputDir(&output_dir)) { > } > > No need to store an intermediate res. Done. https://codereview.webrtc.org/2990563002/diff/160001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:843: RTC_DCHECK(res); On 2017/07/26 18:48:03, pbos-webrtc wrote: > RTC_CHECK(frame_writer.WriteFrame(...)); > > And remove res. Done.
https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... File webrtc/test/testsupport/frame_writer.h (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/frame_writer.h:90: JpegFrameWriter(std::string output_filename); nit: make parameter a const ref https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... File webrtc/test/testsupport/jpeg_frame_writer.cc (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:38: RTC_CHECK(!frame_written_) << "Only s single frame can be saved to Jpeg."; s? You mean "a"? https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:41: std::unique_ptr<uint8_t> rgb_buf(new uint8_t[rgb_len]); Hm, shouldn't this be a std::unique_ptr<uint8_t[]> ? https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:45: fprintf(stderr, "Could not convert input frame to RGB.\n"); Use LOG macros https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:62: jpeg_stdio_dest(&cinfo, output_file_); Would it be possible to write to a buffer instead, and write to file using FileWrapper? Would like avoid direct access to os primitives. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:75: (void)jpeg_write_scanlines(&cinfo, row_pointer, 1); why the (void) ? https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:838: output_dir = ""; Should not write to file instead, if flag is not set? https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:842: fprintf(stderr, "Saving worst frame to %s\n", output_path.c_str()); use LOG macro instead
https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... File webrtc/test/testsupport/frame_writer.h (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/frame_writer.h:90: JpegFrameWriter(std::string output_filename); On 2017/08/17 15:27:50, sprang_webrtc wrote: > nit: make parameter a const ref Done. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... File webrtc/test/testsupport/jpeg_frame_writer.cc (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:38: RTC_CHECK(!frame_written_) << "Only s single frame can be saved to Jpeg."; On 2017/08/17 15:27:50, sprang_webrtc wrote: > s? You mean "a"? Yep, a typo. Thanks. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:41: std::unique_ptr<uint8_t> rgb_buf(new uint8_t[rgb_len]); On 2017/08/17 15:27:50, sprang_webrtc wrote: > Hm, shouldn't this be a std::unique_ptr<uint8_t[]> ? Done. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:45: fprintf(stderr, "Could not convert input frame to RGB.\n"); On 2017/08/17 15:27:50, sprang_webrtc wrote: > Use LOG macros Done. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:62: jpeg_stdio_dest(&cinfo, output_file_); On 2017/08/17 15:27:50, sprang_webrtc wrote: > Would it be possible to write to a buffer instead, and write to file using > FileWrapper? > Would like avoid direct access to os primitives. It's problematic. Outputting to memory is available in the latest versions of libjpeg, but not in the one in third_party. There's a workaround, there we write our own output wrapper with special libjpeg interface, but I think it's an overkill here. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:75: (void)jpeg_write_scanlines(&cinfo, row_pointer, 1); On 2017/08/17 15:27:50, sprang_webrtc wrote: > why the (void) ? Came from the example code. Probably has to do something with warnings. But it's not needed here. Removed. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:838: output_dir = ""; On 2017/08/17 15:27:50, sprang_webrtc wrote: > Should not write to file instead, if flag is not set? Currently it always outputs the worst frame. This is here just to not ignore return values. I have added a separate flag for that. Also, the check here isn't needed because if there is no output dir defined, it will be left as "" anyway. Removed. https://codereview.webrtc.org/2990563002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:842: fprintf(stderr, "Saving worst frame to %s\n", output_path.c_str()); On 2017/08/17 15:27:50, sprang_webrtc wrote: > use LOG macro instead Done.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... File webrtc/test/testsupport/jpeg_frame_writer.cc (right): https://codereview.webrtc.org/2990563002/diff/200001/webrtc/test/testsupport/... webrtc/test/testsupport/jpeg_frame_writer.cc:62: jpeg_stdio_dest(&cinfo, output_file_); On 2017/08/18 08:42:05, ilnik wrote: > On 2017/08/17 15:27:50, sprang_webrtc wrote: > > Would it be possible to write to a buffer instead, and write to file using > > FileWrapper? > > Would like avoid direct access to os primitives. > > It's problematic. Outputting to memory is available in the latest versions of > libjpeg, but not in the one in third_party. There's a workaround, there we write > our own output wrapper with special libjpeg interface, but I think it's an > overkill here. Fair enough, let's leave it as is.
Pbos, could you please check that all your comments are addressed and give the approval?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
webrtc/test 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/...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1503071816964140,
"parent_rev": "d64072c598875cdf3a8ce5806d3410125363596d", "commit_rev":
"26e5cbd6bb38ed734559ab70cb7362870d76c923"}
Message was sent while issue was closed.
Description was changed from ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view. BUG=webrtc:8030 ========== to ========== Add Jpeg frame writer for test support. Also, use it to save worst psnr frame in video quality tests. It is indented that these saved frames from perfbots will be uploaded to the cloud and will be available in chrome perf dashboard. Because of that size of the saved frame is somewhat an issue. Also, y4m is not convenient to view. BUG=webrtc:8030 Review-Url: https://codereview.webrtc.org/2990563002 Cr-Commit-Position: refs/heads/master@{#19414} Committed: https://chromium.googlesource.com/external/webrtc/+/26e5cbd6bb38ed734559ab70c... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/26e5cbd6bb38ed734559ab70c...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.webrtc.org/2998123002/ by charujain@google.com. The reason for reverting is: breaks webrtc.linux.
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.webrtc.org/2998133002/ by charujain@webrtc.org. The reason for reverting is: Breaks webrtc.linux. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
