Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666333003:
Better comparison for frame analyzer (Closed)
|
Created:
3 years, 10 months ago by mandermo Modified:
3 years, 10 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, janssonWebRTC Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionBetter comparison of videos with barcode errors
The frame_analyzer which is used by compare_videos.py needs to handle
barcode errors. As before the reference and the test video can contain
repeated frames. When there are barcode decode errors in the test video,
then we should not let that contribute to the skipped frames score. When
there are barcode decode errors in the reference video, then we need to
take proper care to still calculate skipped barcodes when the
corresponding frames are not present in the test video and the test
video does not have a frame with a barcode decode error that could have
been the same frame as the one in the reference.
A new metric total number of skipped frames and for number of decode
errors is introduced. Barcodes that appears in the test video, but not
in the reference video are also listed.
BUG=webrtc:6967
Review-Url: https://codereview.webrtc.org/2666333003
Cr-Commit-Position: refs/heads/master@{#16638}
Committed: https://chromium.googlesource.com/external/webrtc/+/7cebe783326cbef0ca3350bfbe29938abaf96b54
Patch Set 1 #
Total comments: 6
Patch Set 2 : Better handling of decoding errors #
Total comments: 19
Patch Set 3 : Fixing review comments #Patch Set 4 : Fixed testcases also #Patch Set 5 : Unittesting and bug fix for CalculateFrameClusters #Patch Set 6 : Using TempFilename() instead of unique names #
Total comments: 11
Patch Set 7 : Moved things to SetUp() and close stats file #
Total comments: 3
Messages
Total messages: 34 (13 generated)
mandermo@webrtc.org changed reviewers: + kjellander@webrtc.org
Better video comparison
On 2017/02/01 17:28:05, mandermo wrote: > Better video comparison Could you provide a sample test output of this change?
On 2017/02/03 08:43:06, jansson wrote: > On 2017/02/01 17:28:05, mandermo wrote: > > Better video comparison > > Could you provide a sample test output of this change? I.e. one before and one after the change would be preferred. It will make it easier for the reviewer to see what's going on.
I'd like to see some test cases starting to get written. It shouldn't be hard to unit test the PrintMaxRepeatedAndSkippedFrames function if we start returning exit codes (introduce an enum for the ones we distinguish between). https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... File webrtc/tools/barcode_tools/barcode_decoder.py (right): https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... webrtc/tools/barcode_tools/barcode_decoder.py:135: entry_frame_number = helper_functions.zero_pad(i) So this means we start numbering frames from 1 instead of zero? Please make it very clear in the CL description that this changes. https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... webrtc/tools/frame_analyzer/video_quality_analysis.cc:347: decoded_frame_number = DECODE_ERROR; Can we increment a counter for this so we can log this as a perf metric? https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... webrtc/tools/frame_analyzer/video_quality_analysis.cc:414: return; It seems we should start returning an exit code from the application instead of just printing something and returning void. Then https://cs.chromium.org/chromium/src/third_party/webrtc/tools/frame_analyzer/... could just be updated to return the int and we would get test failures for cases like this assuming the calling script for frame analyzer is looking at the exit code. With that, it would make sense to start writing a few unit tests for PrintMaxRepeatedAndSkippedFrames that verifies the code for the different cases. With this CL there's now so much more error handling logic so I think it makes a lot of sense to have such tests now.
On 2017/02/06 12:30:38, kjellander_webrtc wrote: > I'd like to see some test cases starting to get written. It shouldn't be hard to > unit test the PrintMaxRepeatedAndSkippedFrames function if we start returning > exit codes (introduce an enum for the ones we distinguish between). > > https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... > File webrtc/tools/barcode_tools/barcode_decoder.py (right): > > https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... > webrtc/tools/barcode_tools/barcode_decoder.py:135: entry_frame_number = > helper_functions.zero_pad(i) > So this means we start numbering frames from 1 instead of zero? Please make it > very clear in the CL description that this changes. > > https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... > File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): > > https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:347: decoded_frame_number > = DECODE_ERROR; > Can we increment a counter for this so we can log this as a perf metric? > > https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:414: return; > It seems we should start returning an exit code from the application instead of > just printing something and returning void. > > Then > https://cs.chromium.org/chromium/src/third_party/webrtc/tools/frame_analyzer/... > could just be updated to return the int and we would get test failures for cases > like this assuming the calling script for frame analyzer is looking at the exit > code. > > With that, it would make sense to start writing a few unit tests for > PrintMaxRepeatedAndSkippedFrames that verifies the code for the different cases. > With this CL there's now so much more error handling logic so I think it makes a > lot of sense to have such tests now. Also: the CL description is too generic and is missing a BUG= reference. Please explain more in detail what this CL does and why.
Description was changed from ========== Better frame analyzer for competitive analysis Total skipped and other stats. Taking better into account barcode decode errors. BUG= ========== to ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, that should not contribute to the skipped frames score. When there are barcode decode errors, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and no barcode decoding errors. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ==========
Description was changed from ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, that should not contribute to the skipped frames score. When there are barcode decode errors, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and no barcode decoding errors. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ========== to ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ==========
https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... File webrtc/tools/barcode_tools/barcode_decoder.py (right): https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/barcode_tools/ba... webrtc/tools/barcode_tools/barcode_decoder.py:135: entry_frame_number = helper_functions.zero_pad(i) On 2017/02/06 12:30:38, kjellander_webrtc wrote: > So this means we start numbering frames from 1 instead of zero? Please make it > very clear in the CL description that this changes. jansson@ is working on this change in another CL, I just wanted to be a bit more compatible with that change before it is landed. I have removed it from here now, since it can wait for jansson@s CL. https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... webrtc/tools/frame_analyzer/video_quality_analysis.cc:347: decoded_frame_number = DECODE_ERROR; On 2017/02/06 12:30:38, kjellander_webrtc wrote: > Can we increment a counter for this so we can log this as a perf metric? I have now added a separate counter for reference and test and print them in the end. https://codereview.webrtc.org/2666333003/diff/1/webrtc/tools/frame_analyzer/v... webrtc/tools/frame_analyzer/video_quality_analysis.cc:414: return; On 2017/02/06 12:30:38, kjellander_webrtc wrote: > It seems we should start returning an exit code from the application instead of > just printing something and returning void. > > Then > https://cs.chromium.org/chromium/src/third_party/webrtc/tools/frame_analyzer/... > could just be updated to return the int and we would get test failures for cases > like this assuming the calling script for frame analyzer is looking at the exit > code. > > With that, it would make sense to start writing a few unit tests for > PrintMaxRepeatedAndSkippedFrames that verifies the code for the different cases. > With this CL there's now so much more error handling logic so I think it makes a > lot of sense to have such tests now. Have added test cases covering different scenarios. The cases that return early are less interesting and complicated than the different combinations of decode errors I cover.
Description was changed from ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ========== to ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. BUG=webrtc:6967 A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ==========
Description was changed from ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. BUG=webrtc:6967 A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. ========== to ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. BUG=webrtc:6967 ==========
Thanks for the tests! Just need to make them execute in parallel. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:151: std::string stats_filename_ref = OutputPath() + "stats-1.txt"; Please utilize a temporary directory or unique filenames for each test case. They must be able to execute in parallel without errors. You should be able to find code examples in other tests on how to do that.
https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:340: // to 3. Can you explain in the comment what happens if there's are 3 frames in a row that gets frame number decoded like this: [1, DECODE_ERROR, 2] How many clusters are returned? Please write a unit test for that. Also write unit tests for: [1] [1,1,2] [1, 1, DECODE_ERROR, 3] [DECODE_ERROR, DECODE_ERROR] https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:394: int max_repeated_frames = 1; Does max_repeated_frames = 1 as a starting value make sense or should it also be 0? https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:441: // The test frames that does not have corresponding barcode in the reference Do we have proof that this ever happens? If not I don't see why we would add such complex code to look for it. I don't see when it should happen if we always sample the reference screen fast enough. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); Seeing this, I'd like to have total_repeated_frames as well. Could be done in a follow-up CL though. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); Name this variable total_skipped_frames to be consistent with the max_*_frames variables. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:499: fprintf(output, "RESULT No_matched: ["); This is not suitable as a perf number. The perf dashboard is monitoring the metrics reported and plots them in a graph in order to detect regressions when the values are diverging. Since we're printing frame numbers here, there'll always be a variation on the numbers output here, making it pointless from a perf regression perspective. It could be useful to output them to the console for future debugging, but not as a RESULT line. Plotting the size of no_match_in_ref as a RESULT line would make sense though.
On 2017/02/08 12:39:34, kjellander_webrtc wrote: > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:340: // to 3. > Can you explain in the comment what happens if there's are 3 frames in a row > that gets frame number decoded like this: > [1, DECODE_ERROR, 2] > > How many clusters are returned? Please write a unit test for that. > > Also write unit tests for: > [1] > [1,1,2] > [1, 1, DECODE_ERROR, 3] > [DECODE_ERROR, DECODE_ERROR] > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:394: int > max_repeated_frames = 1; > Does max_repeated_frames = 1 as a starting value make sense or should it also be > 0? > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:441: // The test frames > that does not have corresponding barcode in the reference > Do we have proof that this ever happens? If not I don't see why we would add > such complex code to look for it. > I don't see when it should happen if we always sample the reference screen fast > enough. > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); > Seeing this, I'd like to have total_repeated_frames as well. Could be done in a > follow-up CL though. > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); > Name this variable total_skipped_frames to be consistent with the max_*_frames > variables. > > https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... > webrtc/tools/frame_analyzer/video_quality_analysis.cc:499: fprintf(output, > "RESULT No_matched: ["); > This is not suitable as a perf number. The perf dashboard is monitoring the > metrics reported and plots them in a graph in order to detect regressions when > the values are diverging. > Since we're printing frame numbers here, there'll always be a variation on the > numbers output here, making it pointless from a perf regression perspective. > It could be useful to output them to the console for future debugging, but not > as a RESULT line. > > Plotting the size of no_match_in_ref as a RESULT line would make sense though. Finally, please format the CL description to fit better for Git: http://chris.beams.io/posts/git-commit/
Description was changed from ========== The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. BUG=webrtc:6967 ========== to ========== Better comparison of videos with barcode errors The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. BUG=webrtc:6967 ==========
https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:340: // to 3. On 2017/02/08 12:39:34, kjellander_webrtc wrote: > Can you explain in the comment what happens if there's are 3 frames in a row > that gets frame number decoded like this: > [1, DECODE_ERROR, 2] > > How many clusters are returned? Please write a unit test for that. > > Also write unit tests for: > [1] > [1,1,2] > [1, 1, DECODE_ERROR, 3] > [DECODE_ERROR, DECODE_ERROR] Added comment. Should I move CalculateFrameClusters() out of the unnamed namespace and put a declaration in video_quality_analysis.cc to make a test case for it? https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:394: int max_repeated_frames = 1; On 2017/02/08 12:39:33, kjellander_webrtc wrote: > Does max_repeated_frames = 1 as a starting value make sense or should it also be > 0? You have a point. Fixing it will change the old behavior, let's change it in a follow up CL. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:441: // The test frames that does not have corresponding barcode in the reference On 2017/02/08 12:39:33, kjellander_webrtc wrote: > Do we have proof that this ever happens? If not I don't see why we would add > such complex code to look for it. > I don't see when it should happen if we always sample the reference screen fast > enough. Have removed that code and print an error instead if it happens anyway. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); On 2017/02/08 12:39:33, kjellander_webrtc wrote: > Name this variable total_skipped_frames to be consistent with the max_*_frames > variables. Done. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); On 2017/02/08 12:39:33, kjellander_webrtc wrote: > Seeing this, I'd like to have total_repeated_frames as well. Could be done in a > follow-up CL though. I don't think such a value provide any meaningful info. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:499: fprintf(output, "RESULT No_matched: ["); On 2017/02/08 12:39:34, kjellander_webrtc wrote: > This is not suitable as a perf number. The perf dashboard is monitoring the > metrics reported and plots them in a graph in order to detect regressions when > the values are diverging. > Since we're printing frame numbers here, there'll always be a variation on the > numbers output here, making it pointless from a perf regression perspective. > It could be useful to output them to the console for future debugging, but not > as a RESULT line. > > Plotting the size of no_match_in_ref as a RESULT line would make sense though. Done. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:151: std::string stats_filename_ref = OutputPath() + "stats-1.txt"; On 2017/02/08 10:00:29, kjellander_webrtc wrote: > Please utilize a temporary directory or unique filenames for each test case. > They must be able to execute in parallel without errors. > > You should be able to find code examples in other tests on how to do that. Done.
https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:340: // to 3. On 2017/02/13 17:04:11, mandermo wrote: > On 2017/02/08 12:39:34, kjellander_webrtc wrote: > > Can you explain in the comment what happens if there's are 3 frames in a row > > that gets frame number decoded like this: > > [1, DECODE_ERROR, 2] > > > > How many clusters are returned? Please write a unit test for that. > > > > Also write unit tests for: > > [1] > > [1,1,2] > > [1, 1, DECODE_ERROR, 3] > > [DECODE_ERROR, DECODE_ERROR] > > Added comment. Should I move CalculateFrameClusters() out of the unnamed > namespace and put a declaration in video_quality_analysis.cc to make a test case > for it? Please do, I think that'll be very useful in the future as this is complex stuff with many edge cases. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); On 2017/02/13 17:04:11, mandermo wrote: > On 2017/02/08 12:39:33, kjellander_webrtc wrote: > > Seeing this, I'd like to have total_repeated_frames as well. Could be done in > a > > follow-up CL though. > > I don't think such a value provide any meaningful info. Why not? If we have periodic freezes, let's say every second, we will still only see the maximum which may not be that much different from a normal run. If we would track the total as well, such a scenario would show significant increase.
https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:340: // to 3. On 2017/02/14 20:55:58, kjellander_webrtc wrote: > On 2017/02/13 17:04:11, mandermo wrote: > > On 2017/02/08 12:39:34, kjellander_webrtc wrote: > > > Can you explain in the comment what happens if there's are 3 frames in a row > > > that gets frame number decoded like this: > > > [1, DECODE_ERROR, 2] > > > > > > How many clusters are returned? Please write a unit test for that. > > > > > > Also write unit tests for: > > > [1] > > > [1,1,2] > > > [1, 1, DECODE_ERROR, 3] > > > [DECODE_ERROR, DECODE_ERROR] > > > > Added comment. Should I move CalculateFrameClusters() out of the unnamed > > namespace and put a declaration in video_quality_analysis.cc to make a test > case > > for it? > > Please do, I think that'll be very useful in the future as this is complex stuff > with many edge cases. Done. Found I missed +1 when there are decode errors between frames with identical barcodes. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis.cc:494: total_skipped); On 2017/02/14 20:55:58, kjellander_webrtc wrote: > On 2017/02/13 17:04:11, mandermo wrote: > > On 2017/02/08 12:39:33, kjellander_webrtc wrote: > > > Seeing this, I'd like to have total_repeated_frames as well. Could be done > in > > a > > > follow-up CL though. > > > > I don't think such a value provide any meaningful info. > > Why not? If we have periodic freezes, let's say every second, we will still only > see the maximum which may not be that much different from a normal run. If we > would track the total as well, such a scenario would show significant increase. Lets fix it in a follow up CL where we initialize max_repeated_frames to 0 instead of 1 and remove + 1 from the update of max_repeated_frames. https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/20001/webrtc/tools/frame_analyz... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:151: std::string stats_filename_ref = OutputPath() + "stats-1.txt"; On 2017/02/08 10:00:29, kjellander_webrtc wrote: > Please utilize a temporary directory or unique filenames for each test case. > They must be able to execute in parallel without errors. > > You should be able to find code examples in other tests on how to do that. I am using the unittest library to get unique filenames using TempFilename() now.
Nice testing! I think I found some more. Have you tested running the tests in parallel using third_party/gtest-parallel/gtest-parallel? https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:30: std::string log_filename = TempFilename(webrtc::test::OutputPath(), "Google Test automatically calls SetUpTestCase() before running the first test in the FooTest test case". https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... This means the same log file will be shared between all the test cases, which doesn't seem to be what we want. This should probably be moved into a SetUp() method instead, which executes before each individual test case. Same for TearDownTestCase(). https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:98: std::string stats_filename_ref = TempFilename(OutputPath(), "stats-1.txt"); The declaration pattern at line 98-100 is repeated a lot so I suggest you move those variables into private members of the VideoQualityAnalysisTest class (+suffix them with _) and initialize the first two in a SetUp() function. Then you can remove a lot of the boilerplate. I realize not all test cases will use stats_filename_ref_ but it won't really matter (most do). https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:241: TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneValue) { Please add a test case for an empty stats file as well. https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:315: TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneOneErrErrOne) { Following your established naming convention this should be called CalculateFrameClustersOneOneErrErrOneOne (or remove line 325 and change the size to 5).
https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:30: std::string log_filename = TempFilename(webrtc::test::OutputPath(), On 2017/02/15 12:16:58, kjellander_webrtc wrote: > "Google Test automatically calls SetUpTestCase() before running the first test > in the FooTest test case". > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > This means the same log file will be shared between all the test cases, which > doesn't seem to be what we want. This should probably be moved into a SetUp() > method instead, which executes before each individual test case. Same for > TearDownTestCase(). This code was there before I started to work on this. It should be fine, since they don't care about the output to the log. I have changed it to SetUp() and TearDown() anyway, since it also works. https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:98: std::string stats_filename_ref = TempFilename(OutputPath(), "stats-1.txt"); On 2017/02/15 12:16:58, kjellander_webrtc wrote: > The declaration pattern at line 98-100 is repeated a lot so I suggest you move > those variables into private members of the VideoQualityAnalysisTest class > (+suffix them with _) and initialize the first two in a SetUp() function. > > Then you can remove a lot of the boilerplate. I realize not all test cases will > use stats_filename_ref_ but it won't really matter (most do). Done. https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:241: TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneValue) { On 2017/02/15 12:16:58, kjellander_webrtc wrote: > Please add a test case for an empty stats file as well. Done. https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:315: TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneOneErrErrOne) { On 2017/02/15 12:16:58, kjellander_webrtc wrote: > Following your established naming convention this should be called > CalculateFrameClustersOneOneErrErrOneOne (or remove line 325 and change the size > to 5). Done. https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:328: FILE* stats_filef = fopen(stats_filename.c_str(), "r"); I forgot to close the file. Doing it now.
Almost there! https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:30: std::string log_filename = TempFilename(webrtc::test::OutputPath(), On 2017/02/15 12:52:54, mandermo wrote: > On 2017/02/15 12:16:58, kjellander_webrtc wrote: > > "Google Test automatically calls SetUpTestCase() before running the first test > > in the FooTest test case". > > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > > > This means the same log file will be shared between all the test cases, which > > doesn't seem to be what we want. This should probably be moved into a SetUp() > > method instead, which executes before each individual test case. Same for > > TearDownTestCase(). > > This code was there before I started to work on this. It should be fine, since > they don't care about the output to the log. I have changed it to SetUp() and > TearDown() anyway, since it also works. OK, it feels like things could break if we write and read to the same log file in multiple parallel tests. https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:36: stats_filename_ = TempFilename(OutputPath(), "stats-2.txt"); Nice, how about moving stats_file in here as well as open+closing in Setup() and TearDown(). It will reduce a lot of lines and for the tests that don't use it, the overhead will be insignificant I think (they're not that many).
https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/100001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:30: std::string log_filename = TempFilename(webrtc::test::OutputPath(), On 2017/02/15 14:00:37, kjellander_webrtc wrote: > On 2017/02/15 12:52:54, mandermo wrote: > > On 2017/02/15 12:16:58, kjellander_webrtc wrote: > > > "Google Test automatically calls SetUpTestCase() before running the first > test > > > in the FooTest test case". > > > > > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > > > > > This means the same log file will be shared between all the test cases, > which > > > doesn't seem to be what we want. This should probably be moved into a > SetUp() > > > method instead, which executes before each individual test case. Same for > > > TearDownTestCase(). > > > > This code was there before I started to work on this. It should be fine, since > > they don't care about the output to the log. I have changed it to SetUp() and > > TearDown() anyway, since it also works. > > OK, it feels like things could break if we write and read to the same log file > in multiple parallel tests. That log file is just for putting the data somewhere and is never read from, but I agree it is nicer to put it into SetUp() instead of SetUpTestCase(). https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:36: stats_filename_ = TempFilename(OutputPath(), "stats-2.txt"); On 2017/02/15 14:00:37, kjellander_webrtc wrote: > Nice, how about moving stats_file in here as well as open+closing in Setup() > and TearDown(). It will reduce a lot of lines and for the tests that don't use > it, the overhead will be insignificant I think (they're not that many). I just felt it was proper to close the file before the call PrintMaxRepeatedAndSkippedFrames() since it opens the same file. Should I move it into Setup() and TearDown() and put a flush before the call to PrintMaxRepeatedAndSkippedFrames()?
https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right): https://codereview.webrtc.org/2666333003/diff/120001/webrtc/tools/frame_analy... webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:36: stats_filename_ = TempFilename(OutputPath(), "stats-2.txt"); On 2017/02/15 14:50:29, mandermo wrote: > On 2017/02/15 14:00:37, kjellander_webrtc wrote: > > Nice, how about moving stats_file in here as well as open+closing in Setup() > > and TearDown(). It will reduce a lot of lines and for the tests that don't use > > it, the overhead will be insignificant I think (they're not that many). > > I just felt it was proper to close the file before the call > PrintMaxRepeatedAndSkippedFrames() since it opens the same file. Should I move > it into Setup() and TearDown() and put a flush before the call to > PrintMaxRepeatedAndSkippedFrames()? I see. Do whatever you think is cleanest.
I think it is ok to keep as is.
lgtm
The CQ bit was checked by mandermo@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 mandermo@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": 120001, "attempt_start_ts": 1487237681308500,
"parent_rev": "12b3e03bde0f1a5be1b2773d44d4eccc5fa29f28", "commit_rev":
"7cebe783326cbef0ca3350bfbe29938abaf96b54"}
Message was sent while issue was closed.
Description was changed from ========== Better comparison of videos with barcode errors The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. BUG=webrtc:6967 ========== to ========== Better comparison of videos with barcode errors The frame_analyzer which is used by compare_videos.py needs to handle barcode errors. As before the reference and the test video can contain repeated frames. When there are barcode decode errors in the test video, then we should not let that contribute to the skipped frames score. When there are barcode decode errors in the reference video, then we need to take proper care to still calculate skipped barcodes when the corresponding frames are not present in the test video and the test video does not have a frame with a barcode decode error that could have been the same frame as the one in the reference. A new metric total number of skipped frames and for number of decode errors is introduced. Barcodes that appears in the test video, but not in the reference video are also listed. BUG=webrtc:6967 Review-Url: https://codereview.webrtc.org/2666333003 Cr-Commit-Position: refs/heads/master@{#16638} Committed: https://chromium.googlesource.com/external/webrtc/+/7cebe783326cbef0ca3350bfb... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/7cebe783326cbef0ca3350bfb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
