Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2515253004:
Added tool for reference less video analysis (Closed)
DescriptionAdded tool for reference less video analysis (go/refless-video-analysis)
This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame.
BUG=webrtc:6759
Committed: https://crrev.com/0f01c7f9304e8912bf13d934dc971ecdc63db230
Cr-Commit-Position: refs/heads/master@{#15386}
Patch Set 1 #Patch Set 2 : Fixed bug in frame analyzer API and other changes. #Patch Set 3 : Fixed lint errors #Patch Set 4 : Fixed lint errors #
Total comments: 12
Patch Set 5 : Incorporated review comments and added testcases #Patch Set 6 : Added test case and refactored code #Patch Set 7 : Added header file and renamed lib file #Patch Set 8 : Latest changes #
Total comments: 17
Patch Set 9 : Remove lint errors #Patch Set 10 : Incorporated Review comments #
Total comments: 1
Patch Set 11 : Modified to read one video file at a time. #Patch Set 12 : Rebase #Patch Set 13 : Rebased #Patch Set 14 : Made test case independent #Patch Set 15 : Added define to support strtok_r in windows #Patch Set 16 : precision and size_t to int #
Total comments: 15
Patch Set 17 : review comments and used libvpx util function to get height, width #Patch Set 18 : fix indent #Patch Set 19 : Added newline character #
Total comments: 2
Patch Set 20 : Removed reference to libvpx file and added new implememtation #
Total comments: 8
Messages
Total messages: 54 (27 generated)
Description was changed from ========== Reference less video analysis BUG= ========== to ========== Draft CL : Reference less video analysis BUG= ==========
charujain@webrtc.org changed reviewers: + phoglund@webrtc.org
Created this draft CL, so that you can take a look if i am doing something wrong.
Description was changed from ========== Draft CL : Reference less video analysis BUG= ========== to ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video. Also fixed the bug in video_quality_analysis. The issue here was, we were calculating the file_header and frame_header offset only for the first frame which is not the right logic. We need to skip the file and frame header every time we extract a new frame. BUG= ==========
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Can you break out the bugfix for video_quality_analysis into a separate CL + add a unit test for that code? I think it's likely to affect some perf data and then it's important from the CL title to see what change was made.
Can you add a unit test? Add a tiny .y4m video to //resources for this use. Make sure to keep it small. https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/reference_less_video_analysis.cc (right): https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:2: * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:76: 0 0 0 0 0 I don't quite understand these zeros. Does the first 0 mean that frame 29 was identical to frame 28, but that frame 30 wasn't identical (then it would be a 1 instead) ?
On 2016/11/23 14:44:23, kjellander_webrtc wrote: > Can you add a unit test? Add a tiny .y4m video to //resources for this use. Make > sure to keep it small. > > https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... > File webrtc/tools/frame_analyzer/reference_less_video_analysis.cc (right): > > https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:2: * Copyright (c) > 2012 The WebRTC project authors. All Rights Reserved. > 2016 > > https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:76: 0 0 0 0 0 > I don't quite understand these zeros. > Does the first 0 mean that frame 29 was identical to frame 28, but that frame 30 > wasn't identical (then it would be a 1 instead) ? Please create a BUG= as well and reference it.
https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/reference_less_video_analysis.cc (right): https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:2: * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. Nit: 2016 https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:19: #define NO_OF_STATS 5 What does stats mean? I don't get it. https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:21: #define PSNR_THRESHOLD 47 PSNR_FREEZE_THRESHOLD? https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:89: if (PSNR_per_frame[i] >= PSNR_THRESHOLD || SSIM_per_frame[i] This algorithm is a bit hard to read, maybe because it both counts consecutive frames, prints them and detects if they're similar enough. What if we split up this function a bit? I can imagine something like the following: std::vector<int> identical_frame_clusters = find_frame_clusters( psnr_per_frame, ssim_per_frame); int total_identical_frames = std::accumulate( identical_frame_clusters.begin(), identical_frame_clusters.end()); int unique_frames = ... That separates out most of tricky stuff out of this function. I think you can implement the for loop simpler in the new find_frame_clusters simpler than what you have, since you know frames are always adjacent to each other. What about for (each frame) { if (similar enough to previous frame) { num_frozen++; } else if (num_frozen > 0) { // Not frozen anymore. identical_frame_clusters.push_back(num_frozen); num_frozen = 0; } } if (num_frozen > 0) { identical_frame_clusters.push_back(num_frozen); } You can now easily write unit tests for find_frame_clusters as well! In fact, try to make the print function not do any computation at all and extract all computation to functions so you can unit test them. https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:120: * This script captures the freezing metrics for reference less I think in general avoid c-style comments. And it's not a script (which implies bash or python) https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:144: const char * video_file = video_names_file.c_str(); Nit: char* https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:146: FILE* video_file_ptr = fopen(video_file, "r"); Extract 146- 215 out to a new function. Short, focused functions is good. https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:177: if (!(webrtc::test::ExtractFrameFromY4mFile (video_file_name, !webrtc::test::Extract... https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:178: width, height, Nit: indent https://codereview.webrtc.org/2515253004/diff/60001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:186: no_of_frames+1, Nit: frames + 1
Description was changed from ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video. Also fixed the bug in video_quality_analysis. The issue here was, we were calculating the file_header and frame_header offset only for the first frame which is not the right logic. We need to skip the file and frame header every time we extract a new frame. BUG= ========== to ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video. BUG=webrtc:6759 ==========
Description was changed from ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video. BUG=webrtc:6759 ========== to ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame. BUG=webrtc:6759 ==========
PTAL.
lgtm https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:29: * Parse file header to get height width and fpd. Nit: fps Actually, the first line is pretty obvious from the code. The file header layout is useful info though. Maybe make this into C++ comments and drop the first line: // File header looks like: // YUV4MPEG2 ... https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:57: std::vector<double> ssim_per_frame, int frame) { Nit: indent https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:58: if (psnr_per_frame[frame] >= PSNR_FREEZE_THRESHOLD Just do return psnr_per_frame[frame] >= PSNR_FREEZE_THRESHOLD || ssim_per_frame[frame] >= SSIM_FREEZE_THRESHOLD; https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:73: } else if (num_frozen > 0) { Nit: indent
https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn File webrtc/tools/BUILD.gn (right): https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:99: "//build/win:default_exe_manifest", This should be a dependency for the executable target. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:14: #include <vector> sort https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:20: #include "webrtc/tools/simple_command_line_parser.h" is this used? If not, also move the dependency to the executable target. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:26: void get_height_width_fps(int *height, int *width, int *fps, Can we use https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/y4minpu... instead? I think it's fine to compile that code since it doesn't have dependencies on other stuff in libvpx (except two headers). I prefer if we can avoid reinventing the wheel. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:93: * Printing metrics for file: /usr/local/google/home/charujain/restore/ Please remove paths referencing your local checkout. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:106: 1 1 1 1 1 Can you provide an example where the result is 1 1 2 1 1 instead? It will be easier to understand. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:184: printf("Error opening file\n"); print the filename here as well for easier debugging. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:185: return; Make sure the program exits with a non-zero return code when this happens (i.e. make run_analysis return an integer). https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:188: char video_file_name[200]; 200? Please use some established (platform dependent constant) https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:192: // Check for video file extension. I think the checks for the file should be done in a separate function so run_analysis can be more focused. It can be an internal function called from it though. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:198: } Can you add a check that the file is actually y4m so we can fail early and nicely on incorrect/corrupt files? You could use something like file_is_y4m in https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/vpxenc.... https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc (right): https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:23: webrtc::test::ResourcePath("reference_less_video_test_file", "y4m"); Please add tests for an invalid test file as well, to get coverage for the error handling code. https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:56: Remove blank lines.
Realized I posted comments on PS#8, but they all seem valid for PS#10 as well + a new one. https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc (right): https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:24: std::string video_file = Use a ReferenceLessVideoAnalysisTest test fixture instead to avoid the duplication between test methods.
On 2016/11/28 15:02:08, kjellander_webrtc wrote: > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn > File webrtc/tools/BUILD.gn (right): > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn#ne... > webrtc/tools/BUILD.gn:99: "//build/win:default_exe_manifest", > This should be a dependency for the executable target. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:14: #include > <vector> > sort > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:20: #include > "webrtc/tools/simple_command_line_parser.h" > is this used? If not, also move the dependency to the executable target. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:26: void > get_height_width_fps(int *height, int *width, int *fps, > Can we use > https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/y4minpu... > instead? > I think it's fine to compile that code since it doesn't have dependencies on > other stuff in libvpx (except two headers). > I prefer if we can avoid reinventing the wheel. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:93: * Printing > metrics for file: /usr/local/google/home/charujain/restore/ > Please remove paths referencing your local checkout. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:106: 1 1 1 1 1 > Can you provide an example where the result is 1 1 2 1 1 instead? It will be > easier to understand. I tweaked the example to show the numbers. > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:184: > printf("Error opening file\n"); > print the filename here as well for easier debugging. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:185: return; > Make sure the program exits with a non-zero return code when this happens (i.e. > make run_analysis return an integer). > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:188: char > video_file_name[200]; > 200? Please use some established (platform dependent constant) Could you please point me to some pre-defined constant. Or should i define one here. > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:192: // Check > for video file extension. > I think the checks for the file should be done in a separate function so > run_analysis can be more focused. It can be an internal function called from it > though. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:198: } > Can you add a check that the file is actually y4m so we can fail early and > nicely on incorrect/corrupt files? You could use something like file_is_y4m in > https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/vpxenc.... I an add a check for the file extension only after reading the input file. And this is the place where I start reading the input file and read line by line for video file names. > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc > (right): > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:23: > webrtc::test::ResourcePath("reference_less_video_test_file", "y4m"); > Please add tests for an invalid test file as well, to get coverage for the error > handling code. > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:56: > Remove blank lines.
On 2016/11/28 15:04:13, kjellander_webrtc wrote: > Realized I posted comments on PS#8, but they all seem valid for PS#10 as well + > a new one. > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc > (right): > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:24: > std::string video_file = > Use a ReferenceLessVideoAnalysisTest test fixture instead to avoid the > duplication between test methods. Hmm, I spent quite a time to do the same. I wrote something like this: class ReferenceLessVideoAnalysisTest : public ::testing::Test { public: static void SetUpTestCase() { } static std::string video_file; }; std::string ReferenceLessVideoAnalysisTest::video_file; but it throws error: use of undeclared identifier 'video_file'; Could you help me what is that I am doing wrong.
On 2016/11/29 09:33:27, charujain wrote: > On 2016/11/28 15:04:13, kjellander_webrtc wrote: > > Realized I posted comments on PS#8, but they all seem valid for PS#10 as well > + > > a new one. > > > > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > > File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:24: > > std::string video_file = > > Use a ReferenceLessVideoAnalysisTest test fixture instead to avoid the > > duplication between test methods. > > Hmm, I spent quite a time to do the same. > > I wrote something like this: > > class ReferenceLessVideoAnalysisTest : public ::testing::Test { > public: > static void SetUpTestCase() { > > } > static std::string video_file; > }; > std::string ReferenceLessVideoAnalysisTest::video_file; > > but it throws error: use of undeclared identifier 'video_file'; > > Could you help me what is that I am doing wrong. class ReferenceLessVideoAnalysisTest : public ::testing::Test { public: static void SetUpTestCase() { video_file = webrtc::test::ResourcePath("reference_less_video_test_file", "y4m"); } static std::string video_file; }; std::string ReferenceLessVideoAnalysisTest::video_file;
On 2016/11/29 09:31:50, charujain wrote: > On 2016/11/28 15:02:08, kjellander_webrtc wrote: > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn > > File webrtc/tools/BUILD.gn (right): > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/BUILD.gn#ne... > > webrtc/tools/BUILD.gn:99: "//build/win:default_exe_manifest", > > This should be a dependency for the executable target. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:14: #include > > <vector> > > sort > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:20: #include > > "webrtc/tools/simple_command_line_parser.h" > > is this used? If not, also move the dependency to the executable target. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:26: void > > get_height_width_fps(int *height, int *width, int *fps, > > Can we use > > > https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/y4minpu... > > instead? > > I think it's fine to compile that code since it doesn't have dependencies on > > other stuff in libvpx (except two headers). > > I prefer if we can avoid reinventing the wheel. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:93: * > Printing > > metrics for file: /usr/local/google/home/charujain/restore/ > > Please remove paths referencing your local checkout. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:106: 1 1 1 1 > 1 > > Can you provide an example where the result is 1 1 2 1 1 instead? It will be > > easier to understand. > > I tweaked the example to show the numbers. I don't see a new patch set yet. Did you forget to upload? > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:184: > > printf("Error opening file\n"); > > print the filename here as well for easier debugging. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:185: return; > > Make sure the program exits with a non-zero return code when this happens > (i.e. > > make run_analysis return an integer). > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:188: char > > video_file_name[200]; > > 200? Please use some established (platform dependent constant) > > Could you please point me to some pre-defined constant. Or should i define one > here. Sure, just use FILENAME_MAX in <cstdio>. > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:192: // Check > > for video file extension. > > I think the checks for the file should be done in a separate function so > > run_analysis can be more focused. It can be an internal function called from > it > > though. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:198: } > > Can you add a check that the file is actually y4m so we can fail early and > > nicely on incorrect/corrupt files? You could use something like file_is_y4m > in > > > https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/vpxenc.... > > I an add a check for the file extension only after reading the input file. And > this is the > place where I start reading the input file and read line by line for video file > names. Oh, not until now I realized the program takes a --input_file flag which is supposed to be a text file. Why is that? I expected 'video_file' being passed into reference_less_video_analysis_lib.h to be the y4m video file (not a text file). This confusing setup really need to change. I don't see why we pass a text file with multiple videos instead of just invoking the program multiple times, feeding it one y4m video file at a time. If batch processing multiple video files, won't the output be harder to parse as well? > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:23: > > webrtc::test::ResourcePath("reference_less_video_test_file", "y4m"); > > Please add tests for an invalid test file as well, to get coverage for the > error > > handling code. > > > > > https://codereview.webrtc.org/2515253004/diff/140001/webrtc/tools/frame_analy... > > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:56: > > Remove blank lines.
On 2016/11/29 09:35:03, charujain wrote: > On 2016/11/29 09:33:27, charujain wrote: > > On 2016/11/28 15:04:13, kjellander_webrtc wrote: > > > Realized I posted comments on PS#8, but they all seem valid for PS#10 as > well > > + > > > a new one. > > > > > > > > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > > > File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc > > > (right): > > > > > > > > > https://codereview.webrtc.org/2515253004/diff/180001/webrtc/tools/frame_analy... > > > webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:24: > > > std::string video_file = > > > Use a ReferenceLessVideoAnalysisTest test fixture instead to avoid the > > > duplication between test methods. > > > > Hmm, I spent quite a time to do the same. > > > > I wrote something like this: > > > > class ReferenceLessVideoAnalysisTest : public ::testing::Test { > > public: > > static void SetUpTestCase() { > > > > } > > static std::string video_file; > > }; > > std::string ReferenceLessVideoAnalysisTest::video_file; > > > > but it throws error: use of undeclared identifier 'video_file'; > > > > Could you help me what is that I am doing wrong. > > class ReferenceLessVideoAnalysisTest : public ::testing::Test { > public: > static void SetUpTestCase() { > video_file = webrtc::test::ResourcePath("reference_less_video_test_file", > "y4m"); > } > static std::string video_file; > }; > std::string ReferenceLessVideoAnalysisTest::video_file; See https://cs.chromium.org/chromium/src/third_party/webrtc/test/frame_generator_... for an example. The trick may be that you need to change from TEST to TEST_F macro to actually make it use the fixture class.
> > Oh, not until now I realized the program takes a --input_file flag which is > supposed to be a text file. Why is that? > I expected 'video_file' being passed into reference_less_video_analysis_lib.h to > be the y4m video file (not a text file). > This confusing setup really need to change. I don't see why we pass a text file > with multiple videos instead of just invoking > the program multiple times, feeding it one y4m video file at a time. > > If batch processing multiple video files, won't the output be harder to parse as > well? > +1, I missed that in my review. Yeah, I think parsing several files at once only makes sense if we need to merge their results somehow. We don't though, so this just becomes speculative generality.
Patchset #16 (id:300001) has been deleted
Patchset #17 (id:340001) has been deleted
Patchset #16 (id:320001) has been deleted
Incorporated most of the comments. PTAL. Although as I discussed offline about using https://cs.chromium.org/chromium/src/third_party/libvpx/source/libvpx/y4minpu..., we will have to ask libvpx team to expose this as a library. So for now not using that.
It was quite easy to use libvpx's y4minput.c function. I did a proof-of-concept CL here: https://codereview.webrtc.org/2540893003 Please use that instead of reinventing y4m parsing here (just check the diff from PS#1->2 to see what I did). https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis.cc:42: return 0; return run_analysis(video_file); and then delete line 41. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:25: /* We are on Windows */ This comment doesn't add any value. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:30: const char* video_file) { Please make this const std::string& video_file https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:65: std::vector<int> find_frame_clusters(std::vector<double> psnr_per_frame, Use const std::vector<double>& for the arguments https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:76: identical_frame_clusters.push_back(num_frozen); -2 spaces indent https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:83: void print_freezing_metrics(std::vector<double> psnr_per_frame, Use const std::vector<double>& for the arguments https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:135: void compute_metrics(std::string video_file_name, Please use const std::string& https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:181: bool check_file_extension(std::string video_file_name) { const std::string& https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:190: int run_analysis(std::string video_file) { const std::string& https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:25: compute_metrics(video_file, &psnr_per_frame, &ssim_per_frame); Don't have this in the test fixture SetUp. Then if this function fails all your tests will fail (even the ones that are designed to test different parts of the code and don't even wants to run compute_metrics) https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:59: EXPECT_TRUE(check_file_extension(video_file)); add a test for an incorrect file as well.
PTAL. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:30: const char* video_file) { On 2016/11/30 07:52:45, kjellander_webrtc wrote: > Please make this const std::string& video_file So y4m_input_open need FILE* as an argument. So I am passing const char* to this function. Is there any specific reason we should use const string& instead of const char*. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_unittest.cc:25: compute_metrics(video_file, &psnr_per_frame, &ssim_per_frame); On 2016/11/30 07:52:46, kjellander_webrtc wrote: > Don't have this in the test fixture SetUp. Then if this function fails all your > tests will fail (even the ones that are designed to test different parts of the > code and don't even wants to run compute_metrics) I did this because MatchIdenticalFrameClusters test needs to call first compute_metrics and then call FindIdenicalClusters. So I thought instead of calling it twice to put it in setup function.
lgtm with remaining comments addressed. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:30: const char* video_file) { On 2016/11/30 13:56:27, charujain wrote: > On 2016/11/30 07:52:45, kjellander_webrtc wrote: > > Please make this const std::string& video_file > > So y4m_input_open need FILE* as an argument. So I am passing const char* to this > function. > Is there any specific reason we should use const string& instead of const char*. I was thinking for API consistency and ease of using the strings. It's easy to go to a c-string with .c_str() for the need of creating the FILE*. https://codereview.webrtc.org/2515253004/diff/360001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:76: identical_frame_clusters.push_back(num_frozen); On 2016/11/30 07:52:45, kjellander_webrtc wrote: > -2 spaces indent Please fix
The CQ bit was checked by charujain@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12982)
The CQ bit was checked by charujain@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13058)
https://codereview.webrtc.org/2515253004/diff/420001/BUILD.gn File BUILD.gn (right): https://codereview.webrtc.org/2515253004/diff/420001/BUILD.gn#newcode25 BUILD.gn:25: rtc_source_set("libvpx_y4minput") { I'm not sure about the decision to create a rule reaching into libvpx code at the top level. I thought we tried to not do that, and blaze will certainly not allow that kind of going past package boundaries. Shouldn't that be in the libvpx gn file in that case?
https://codereview.webrtc.org/2515253004/diff/420001/BUILD.gn File BUILD.gn (right): https://codereview.webrtc.org/2515253004/diff/420001/BUILD.gn#newcode25 BUILD.gn:25: rtc_source_set("libvpx_y4minput") { On 2016/12/02 09:51:41, phoglund wrote: > I'm not sure about the decision to create a rule reaching into libvpx code at > the top level. I thought we tried to not do that, and blaze will certainly not > allow that kind of going past package boundaries. > > Shouldn't that be in the libvpx gn file in that case? Patrik you're right. My obsession with not duplicating code is going to far here :) I suggest we drop this and go back to Charu's original implementation instead. Sorry about all the extra work - we shouldn't introduce all this extra uncertainty for that tiny function.
The CQ bit was checked by charujain@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 charujain@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2515253004/#ps440001 (title: "Removed reference to libvpx file and added new implememtation")
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": 440001, "attempt_start_ts": 1480683401644690,
"parent_rev": "63cbfb1f4212b1b536d6546d0c12e52a0b3e78cf", "commit_rev":
"12011c616866eae48f531b41b4c0f026810760de"}
Message was sent while issue was closed.
Description was changed from ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame. BUG=webrtc:6759 ========== to ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame. BUG=webrtc:6759 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame. BUG=webrtc:6759 ========== to ========== Added tool for reference less video analysis (go/refless-video-analysis) This tool takes list of video file names as input and calculates freezing metrics score for the video files without having reference to original video by comparing the PSNR and SSIM values of current and previous frame. BUG=webrtc:6759 Committed: https://crrev.com/0f01c7f9304e8912bf13d934dc971ecdc63db230 Cr-Commit-Position: refs/heads/master@{#15386} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/0f01c7f9304e8912bf13d934dc971ecdc63db230 Cr-Commit-Position: refs/heads/master@{#15386}
Message was sent while issue was closed.
mandermo@webrtc.org changed reviewers: + mandermo@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:20: #define STATS_LINE_LENGTH 28 The Style Guide prefer constants over defines https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:144: uint8_t* current_frame = new uint8_t[size]; Maybe std::unique_ptr<uint8_t[]> would simplify the code. https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h (right): https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h:16: Using _ in function names is different from the other files in webrtc/tools/frame_analyzer where CamelCase is used. https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h:48: int run_analysis(const std::string& video_file); Maybe it could be worth putting the functions in this header in a namespace. Especially int run_analysis(const std::string&) is quite a general name and could clash in the future if it is in the global namespace.
Message was sent while issue was closed.
Thanks for the post-commit review Magnus! Charu: can you do a follow-up CL to address the comments? https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc (right): https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:20: #define STATS_LINE_LENGTH 28 On 2017/02/07 14:57:50, mandermo wrote: > The Style Guide prefer constants over defines > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros +1 https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.cc:144: uint8_t* current_frame = new uint8_t[size]; On 2017/02/07 14:57:50, mandermo wrote: > Maybe std::unique_ptr<uint8_t[]> would simplify the code. +1 https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h (right): https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h:16: On 2017/02/07 14:57:50, mandermo wrote: > Using _ in function names is different from the other files in > webrtc/tools/frame_analyzer where CamelCase is used. You're right, the style guide says CamelCase: https://google.github.io/styleguide/cppguide.html#Function_Names Charu, can you address in a follow-up CL? https://codereview.webrtc.org/2515253004/diff/440001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/reference_less_video_analysis_lib.h:48: int run_analysis(const std::string& video_file); On 2017/02/07 14:57:51, mandermo wrote: > Maybe it could be worth putting the functions in this header in a namespace. > Especially int run_analysis(const std::string&) is quite a general name and > could clash in the future if it is in the global namespace. Yes, please use the webrtc::test namespace. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
