Comparison of videos with reference frame not starting from zero

Comparison of videos with reference frame not starting from zero

BUG=webrtc:6967
Review-Url: https://codereview.webrtc.org/2553693002
Cr-Commit-Position: refs/heads/master@{#16112}
Committed: https://chromium.googlesource.com/external/webrtc/+/7456817d40f15ad6e4e9a7acf204c27e12509bc3

[mandermo, 2016-12-05 16:00:12]

mandermo@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[mandermo, 2016-12-05 16:00:13]

Code for comparing videos when the reference video does not start with barcode
zero. Some barcode numbers can also be missing. Uses two stat files.

[mandermo, 2016-12-06 10:04:24]

Added missing file compare_videos_fuzzy.py

[mandermo, 2016-12-06 14:09:57]

mandermo@webrtc.org changed reviewers:
+ jansson@webrtc.org

[mandermo, 2016-12-06 14:09:57]

Updated with new code for PrintMaxRepeatedAndSkippedFrames() to handle two stat
files. The code should handle if the reference file loops (not repetition of
frames). Only basic manual testing done.

[phoglund, 2016-12-08 09:55:44]

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
The convention in these files is MakeStatFile (main is always lowercase by
convention though).

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
Sure, this function makes a stat file, but it's better to allude to what this
function does and why in the naming. What actually happens is that we take a
file in and decode the barcodes in it, so call it DecodeBarcodesInVideo or
something instead.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
Passing null_filehandle seems silly. This function doesn't care if it's the null
filehandle or something else; I'd just call it stdin_handle.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:44: // TODO(mandermo) fix comment
...yes :)

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:256: // In case two frames
have same id, then we only save the first one.
Add: "This can happen if there are freezes in the video. There's no point to
compare the same frame twice, which is why we throw away duplicates."

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:263: // Insert will only
add if it is not in map already, so we only keep first.
nit: End this sentence at the ,

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:274: // TODO(mandermo)
print
Yeah, print an error here and continue. You'll have to verify manually this
doesn't happen when you deploy the change.

We could also tend to fail the tool when this happens or we fail to open files
etc, but I think continuing isn't unreasonable. If we fail to find many frames
in the ref file we should get a big drop in unique frames, and the perf sheriff
will take action. So, just add a fprintf here and remove the TODO and we're
good.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:331:
std::vector<std::pair<int, int> > GetBarcodeNrToFrequency(FILE* file) {
Number

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:331:
std::vector<std::pair<int, int> > GetBarcodeNrToFrequency(FILE* file) {
I would love to have unit tests for these guys since they're so complex. Reading
files in the unit tests is fine, just check in some sample ref files for test
data. 

The second problem is the output of the functions is printed to stdout. 
PrintMaxRepeatedAndSkippedFrames cleverly takes a FILE* output parameter
already, so it's easy to just pass in a suitable FILE* from your test and verify
the right things were written in. I'd just mkstemp() a file in the test and then
verify the right things were printed.

You can also consider refactoring 

PrintMaxRepeatedAndSkippedFrames(stdout, label, stats_file_ref_name,
                                 stats_file_test_name);

to

int max_repeated = ComputeMaxRepeated(...);
int skipped = ComputeSkipped(...);
printf(... max_repeated, max_skipped)

and write tests for the Compute* functions.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
frame_frequency?

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
It feels a lot more natural to make this a map. Frame id mapped to frequency, I
think that makes sense. I think you can make your update logic cleaner as well.
I think this will work:

frame_frequency.insert(std::pair<int, int>(decoded_frame_number, 0);
frame_frequency[decoded_frame_number]++;

insert is a no-op if the key already exists. If that doesn't work you can look
at the retval from insert to get an iterator pointing at the element, and
increment that instead.

http://www.cplusplus.com/reference/map/map/insert/

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:367:
std::vector<std::pair<int, int> > frame_cnt_ref =
frame_frequency_ref

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:381: if (it_test !=
end_test) {
if (it_test == end_test) the test file is empty, which is pretty absurd. I think
you can make this boundary condition a guard clause:

if (it_test == end_test || it_ref == end_ref) {
  fprintf(output, "Either test or ref file is empty, nothing to print");
  return;
}

...

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:382: while (it_ref !=
end_ref && it_ref->first != it_test->first) {
It's hard to read what happens here. Maybe extract a function
FindFirstFrameInRefVideo?

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:391: max_repeated_frames =
Also not sure if this algorithm is correct. To compute repeated frames, suppose
the following sequence:

ref  111234444445678
test 111111234444445678

In that case, freq(ref, 1) = 3 and freq(test, 1) = 6, so we correctly get 6-3 =
3 repeated frames. But what if the test runs so long that the ref file wraps:

ref  111234444445678
test 111111234444445678111234444445678

In that case freq(ref, 1) = 3 but freq(test, 1) = 9, and it claims that frame 1
is repeated 6 times. The correct answer is 3. In fact, the second 111 sequence
isn't even a freeze since it didn't freeze any more frames than were already
frozen in the ref file. The frequency count approach fails if there's wrapping.
I guess we can avoid that by always having a longer ref file than the time we
run tests...

Or wait actually, I think your approach is correct after all. We can assume the
ref recording will be about as long as the test recording, and it should _also_
wrap unless the ref file is super long:

ref  111234444445678111234444445678
test 111111234444445678111234444445678

In that case your algorithm produces the right answer 3. It breaks down in some
cases though:

ref  121212121212
test 1112111211121112

Here freq(ref, 1) = 6 and freq(test, 1) = 12, so max_repeated = 6, but the
longest freeze is actually 3. I doubt this is actually a problem though since
the above freeze pattern looks pretty bizarre. It will tend to be

123444444445677778 etc, and then it will work.

Try to comment how this works, and write unit tests to illustrate the various
edge cases and assumptions here. You can also make the code more readable with
some variables:

This is also hard to read, extracting some variables might help:

int test_frame_frequency = it_test->second;
int ref_frame_frequency = it_ref->second;

// If a frame shows up more in the test video it's likely been frozen.
int suspected_freeze = test_frame_frequency - ref_frame_frequency + 1;

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.h (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.h:46: // position in the
original video. We should provide a statistics file along
Only now we're providing stats file for both files, so update the comment to
reflect that. Also note down we'll treat both files the same way.

[mandermo, 2016-12-08 18:08:31]

Updated based on review comments.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
On 2016/12/08 09:55:43, phoglund wrote:
> The convention in these files is MakeStatFile (main is always lowercase by
> convention though).

Done.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
On 2016/12/08 09:55:43, phoglund wrote:
> Sure, this function makes a stat file, but it's better to allude to what this
> function does and why in the naming. What actually happens is that we take a
> file in and decode the barcodes in it, so call it DecodeBarcodesInVideo or
> something instead.

Renamed to DecodeBarcodesInVideo

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:81: def make_stat_file(options, path_to_decoder,
null_filehandle, video, stat_file):
On 2016/12/08 09:55:43, phoglund wrote:
> Passing null_filehandle seems silly. This function doesn't care if it's the
null
> filehandle or something else; I'd just call it stdin_handle.

From the comment for null_filehandle, I understand that it very much matters
that it is a null file handle and not any handle. I have made a copy of the
declaration of the null_filehandle variable and put it to
make_stat_file/DecodeBarcodesInVideo and removed it as a parameter. That caused
some more duplication, not sure what is best.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:44: // TODO(mandermo) fix comment
On 2016/12/08 09:55:44, phoglund wrote:
> ...yes :)

Done.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:256: // In case two frames
have same id, then we only save the first one.
On 2016/12/08 09:55:44, phoglund wrote:
> Add: "This can happen if there are freezes in the video. There's no point to
> compare the same frame twice, which is why we throw away duplicates."

It can also happen that the camera captures two frames after each other and the
barcode did not change in between. In that case the two frames could be
different, but I choose not to handle that fully because it has some other
problems. What scenarios should we comment?

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:263: // Insert will only
add if it is not in map already, so we only keep first.
On 2016/12/08 09:55:44, phoglund wrote:
> nit: End this sentence at the ,

Done.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:331:
std::vector<std::pair<int, int> > GetBarcodeNrToFrequency(FILE* file) {
On 2016/12/08 09:55:44, phoglund wrote:
> I would love to have unit tests for these guys since they're so complex.
Reading
> files in the unit tests is fine, just check in some sample ref files for test
> data. 
> 
> The second problem is the output of the functions is printed to stdout. 
> PrintMaxRepeatedAndSkippedFrames cleverly takes a FILE* output parameter
> already, so it's easy to just pass in a suitable FILE* from your test and
verify
> the right things were written in. I'd just mkstemp() a file in the test and
then
> verify the right things were printed.
> 
> You can also consider refactoring 
> 
> PrintMaxRepeatedAndSkippedFrames(stdout, label, stats_file_ref_name,
>                                  stats_file_test_name);
> 
> to
> 
> int max_repeated = ComputeMaxRepeated(...);
> int skipped = ComputeSkipped(...);
> printf(... max_repeated, max_skipped)
> 
> and write tests for the Compute* functions.

I agree the code needs to be easier to test. Maybe it convenient to have the
repeated and skipped code in the same function. How should we signal errors for
the Compute function(s)? How should errors be signaled for the user? Should the
errors be printed to stderr or stdout? We can still verify the output to file by
opening the output file for reading after in the test case.

Is there a reason we use C IO and not the C++ iostreams? Is it performance
critical? We could send in std::istream&s for the stat files and an
std::ostream& for the output. The test code can use stringstreams to stub the
input and output.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
On 2016/12/08 09:55:44, phoglund wrote:
> It feels a lot more natural to make this a map. Frame id mapped to frequency,
I
> think that makes sense. I think you can make your update logic cleaner as
well.
> I think this will work:
> 
> frame_frequency.insert(std::pair<int, int>(decoded_frame_number, 0);
> frame_frequency[decoded_frame_number]++;
> 
> insert is a no-op if the key already exists. If that doesn't work you can look
> at the retval from insert to get an iterator pointing at the element, and
> increment that instead.
> 
> http://www.cplusplus.com/reference/map/map/insert/
frame_frequency.insert(std::pair<int, int>(decoded_frame_number, 0);
frame_frequency[decoded_frame_number]++;
Can be simplified to either only
frame_frequency[decoded_frame_number]++;
because if it is not there, the value is value-initialized and is initialized to
zero because it is an int.
It can also be replaced with:
frame_frequency.insert(std::make_pair(decoded_frame_number, 0)).first->second++;

The current function name for this function is probably not the best. The reason
I choose a vector is to remember the order of the frames. To handle situations
where the video wrap around like this:
ref:  4567123
test:   67 23
Then I want 1 skipped frame. With a map for the test video, then I don't know
how to arrive at 1 skipped frame. It is easy to make a mistake and think that
test video start at frame 2 and that frame 4,5 are skipped.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
On 2016/12/08 09:55:44, phoglund wrote:
> frame_frequency?

Maybe it is more of a frame count than a frequency.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:367:
std::vector<std::pair<int, int> > frame_cnt_ref =
On 2016/12/08 09:55:44, phoglund wrote:
> frame_frequency_ref

Maybe it is more of a frame count than a frequency.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:381: if (it_test !=
end_test) {
On 2016/12/08 09:55:44, phoglund wrote:
> if (it_test == end_test) the test file is empty, which is pretty absurd. I
think
> you can make this boundary condition a guard clause:
> 
> if (it_test == end_test || it_ref == end_ref) {
>   fprintf(output, "Either test or ref file is empty, nothing to print");
>   return;
> }
> 
> ...

Done. Should I print to output or to stderr like the rest of the code?

I have done some other changes that was possible after this change.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:382: while (it_ref !=
end_ref && it_ref->first != it_test->first) {
On 2016/12/08 09:55:44, phoglund wrote:
> It's hard to read what happens here. Maybe extract a function
> FindFirstFrameInRefVideo?

Done. Should I print to output or to stderr like the rest of the code?

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:382: while (it_ref !=
end_ref && it_ref->first != it_test->first) {
On 2016/12/08 09:55:44, phoglund wrote:
> It's hard to read what happens here. Maybe extract a function
> FindFirstFrameInRefVideo?

Commented the code better.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:391: max_repeated_frames =
On 2016/12/08 09:55:44, phoglund wrote:
> Also not sure if this algorithm is correct. To compute repeated frames,
suppose
> the following sequence:
> 
> ref  111234444445678
> test 111111234444445678
> 
> In that case, freq(ref, 1) = 3 and freq(test, 1) = 6, so we correctly get 6-3
=
> 3 repeated frames. But what if the test runs so long that the ref file wraps:
> 
> ref  111234444445678
> test 111111234444445678111234444445678
> 
> In that case freq(ref, 1) = 3 but freq(test, 1) = 9, and it claims that frame
1
> is repeated 6 times. The correct answer is 3. In fact, the second 111 sequence
> isn't even a freeze since it didn't freeze any more frames than were already
> frozen in the ref file. The frequency count approach fails if there's
wrapping.
> I guess we can avoid that by always having a longer ref file than the time we
> run tests...
> 
> Or wait actually, I think your approach is correct after all. We can assume
the
> ref recording will be about as long as the test recording, and it should
_also_
> wrap unless the ref file is super long:
> 
> ref  111234444445678111234444445678
> test 111111234444445678111234444445678
> 
> In that case your algorithm produces the right answer 3. It breaks down in
some
> cases though:
> 
> ref  121212121212
> test 1112111211121112
> 
> Here freq(ref, 1) = 6 and freq(test, 1) = 12, so max_repeated = 6, but the
> longest freeze is actually 3. I doubt this is actually a problem though since
> the above freeze pattern looks pretty bizarre. It will tend to be
> 
> 123444444445677778 etc, and then it will work.
> 
> Try to comment how this works, and write unit tests to illustrate the various
> edge cases and assumptions here. You can also make the code more readable with
> some variables:
> 
> This is also hard to read, extracting some variables might help:
> 
> int test_frame_frequency = it_test->second;
> int ref_frame_frequency = it_ref->second;
> 
> // If a frame shows up more in the test video it's likely been frozen.
> int suspected_freeze = test_frame_frequency - ref_frame_frequency + 1;
> 

Lets make the unittests and see what output we get.

[phoglund, 2016-12-12 12:46:20]

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:44: // TODO(mandermo) fix comment
On 2016/12/08 18:08:30, mandermo wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > ...yes :)
> 
> Done.

... Also remove the TODO

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:256: // In case two frames
have same id, then we only save the first one.
On 2016/12/08 18:08:31, mandermo wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > Add: "This can happen if there are freezes in the video. There's no point to
> > compare the same frame twice, which is why we throw away duplicates."
> 
> It can also happen that the camera captures two frames after each other and
the
> barcode did not change in between. In that case the two frames could be
> different, but I choose not to handle that fully because it has some other
> problems. What scenarios should we comment?

I don't understand when you say the frames have the same barcode but is
different. You mean that the barcode pixels are the same, but other pixels in
the image change? I think the frames should be pixel identical if we capture
faster than the refresh rate.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:274: // TODO(mandermo)
print
On 2016/12/08 09:55:44, phoglund wrote:
> Yeah, print an error here and continue. You'll have to verify manually this
> doesn't happen when you deploy the change.
> 
> We could also tend to fail the tool when this happens or we fail to open files
> etc, but I think continuing isn't unreasonable. If we fail to find many frames
> in the ref file we should get a big drop in unique frames, and the perf
sheriff
> will take action. So, just add a fprintf here and remove the TODO and we're
> good.

Please address

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:331:
std::vector<std::pair<int, int> > GetBarcodeNrToFrequency(FILE* file) {
On 2016/12/08 18:08:31, mandermo wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > I would love to have unit tests for these guys since they're so complex.
> Reading
> > files in the unit tests is fine, just check in some sample ref files for
test
> > data. 
> > 
> > The second problem is the output of the functions is printed to stdout. 
> > PrintMaxRepeatedAndSkippedFrames cleverly takes a FILE* output parameter
> > already, so it's easy to just pass in a suitable FILE* from your test and
> verify
> > the right things were written in. I'd just mkstemp() a file in the test and
> then
> > verify the right things were printed.
> > 
> > You can also consider refactoring 
> > 
> > PrintMaxRepeatedAndSkippedFrames(stdout, label, stats_file_ref_name,
> >                                  stats_file_test_name);
> > 
> > to
> > 
> > int max_repeated = ComputeMaxRepeated(...);
> > int skipped = ComputeSkipped(...);
> > printf(... max_repeated, max_skipped)
> > 
> > and write tests for the Compute* functions.
> 
> I agree the code needs to be easier to test. Maybe it convenient to have the
> repeated and skipped code in the same function. How should we signal errors
for
> the Compute function(s)? How should errors be signaled for the user? Should
the
> errors be printed to stderr or stdout? We can still verify the output to file
by
> opening the output file for reading after in the test case.
> 
> Is there a reason we use C IO and not the C++ iostreams? Is it performance
> critical? We could send in std::istream&s for the stat files and an
> std::ostream& for the output. The test code can use stringstreams to stub the
> input and output.

Errors are not returned up the chain today, and I don't see you need to do that
here either? fprintf + return is fine, unless you see any benefits of returning
errors up the chain.

<iostream> is banned for Chrome code because it bloats the binary size, and
string streams are only allowed for logging. See
https://google.github.io/styleguide/cppguide.html#Streams. I really don't see
how that would make anything better.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
On 2016/12/08 18:08:31, mandermo wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > frame_frequency?
> 
> Maybe it is more of a frame count than a frequency.

Right, it's actually a vector of frame "clusters", e.g. where you found repeated
frames. I was mislead by the bad function name. My comment below assumed you
actually had a frame:frequency mapping.

So actually this
ref  111234444445678
test 111111234444445678

produces 3-1-1-6-1-1-1-1
and      6-1-1-6-1-1-1-1

I think that does make sense. In that case the vector should be called
frame_clusters or something.

It would be nice if we could subtract 6-3 = 3 here, but you need to do some
serious thinking to ensure the videos are correctly lined up. What if the ref
video starts recording 1 second before the test video? In that case you need to
align the start of the test and ref videos.

Maybe it's better to hope there are no freezes in the ref video and just print
the freezes in the test video...

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:391: max_repeated_frames =
On 2016/12/08 18:08:30, mandermo wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > Also not sure if this algorithm is correct. To compute repeated frames,
> suppose
> > the following sequence:
> > 
> > ref  111234444445678
> > test 111111234444445678
> > 
> > In that case, freq(ref, 1) = 3 and freq(test, 1) = 6, so we correctly get
6-3
> =
> > 3 repeated frames. But what if the test runs so long that the ref file
wraps:
> > 
> > ref  111234444445678
> > test 111111234444445678111234444445678
> > 
> > In that case freq(ref, 1) = 3 but freq(test, 1) = 9, and it claims that
frame
> 1
> > is repeated 6 times. The correct answer is 3. In fact, the second 111
sequence
> > isn't even a freeze since it didn't freeze any more frames than were already
> > frozen in the ref file. The frequency count approach fails if there's
> wrapping.
> > I guess we can avoid that by always having a longer ref file than the time
we
> > run tests...
> > 
> > Or wait actually, I think your approach is correct after all. We can assume
> the
> > ref recording will be about as long as the test recording, and it should
> _also_
> > wrap unless the ref file is super long:
> > 
> > ref  111234444445678111234444445678
> > test 111111234444445678111234444445678
> > 
> > In that case your algorithm produces the right answer 3. It breaks down in
> some
> > cases though:
> > 
> > ref  121212121212
> > test 1112111211121112
> > 
> > Here freq(ref, 1) = 6 and freq(test, 1) = 12, so max_repeated = 6, but the
> > longest freeze is actually 3. I doubt this is actually a problem though
since
> > the above freeze pattern looks pretty bizarre. It will tend to be
> > 
> > 123444444445677778 etc, and then it will work.
> > 
> > Try to comment how this works, and write unit tests to illustrate the
various
> > edge cases and assumptions here. You can also make the code more readable
with
> > some variables:
> > 
> > This is also hard to read, extracting some variables might help:
> > 
> > int test_frame_frequency = it_test->second;
> > int ref_frame_frequency = it_ref->second;
> > 
> > // If a frame shows up more in the test video it's likely been frozen.
> > int suspected_freeze = test_frame_frequency - ref_frame_frequency + 1;
> > 
> 
> Lets make the unittests and see what output we get.

Ok, I think counting the frame clusters like your code is today, is a better
approach. Disregard most of my comment above, since it finds that frame
frequency counting is bad when the videos wrap and not necessarily start in the
same place.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.h (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.h:46: // position in the
original video. We should provide a statistics file along
On 2016/12/08 09:55:44, phoglund wrote:
> Only now we're providing stats file for both files, so update the comment to
> reflect that. Also note down we'll treat both files the same way.

Please address this.

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:100: null_filehandle = open(os.devnull, 'r')
Create a helper which you invoke

...Popen(cmd, stdin=_DevNull(),...

and put the comment on the helper instead.

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:57: "  - stats_file_ref(string):
The full name of the file containing the"
That's a mouthful. What about

"The stats file, as produced by the barcode_decoder script, for the reference
file."

Ditto for the test file.

[mandermo, 2016-12-21 16:42:05]

mandermo@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[mandermo, 2016-12-21 16:42:06]

+kjellander

Fixed review comments.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:44: // TODO(mandermo) fix comment
On 2016/12/12 12:46:20, phoglund_ooo_parental_leave wrote:
> On 2016/12/08 18:08:30, mandermo wrote:
> > On 2016/12/08 09:55:44, phoglund wrote:
> > > ...yes :)
> > 
> > Done.
> 
> ... Also remove the TODO

Done.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:332:
std::vector<std::pair<int, int> > frame_cnt;
On 2016/12/12 12:46:20, phoglund wrote:
> On 2016/12/08 18:08:31, mandermo wrote:
> > On 2016/12/08 09:55:44, phoglund wrote:
> > > frame_frequency?
> > 
> > Maybe it is more of a frame count than a frequency.
> 
> Right, it's actually a vector of frame "clusters", e.g. where you found
repeated
> frames. I was mislead by the bad function name. My comment below assumed you
> actually had a frame:frequency mapping.
> 
> So actually this
> ref  111234444445678
> test 111111234444445678
> 
> produces 3-1-1-6-1-1-1-1
> and      6-1-1-6-1-1-1-1
> 
> I think that does make sense. In that case the vector should be called
> frame_clusters or something.
> 
> It would be nice if we could subtract 6-3 = 3 here, but you need to do some
> serious thinking to ensure the videos are correctly lined up. What if the ref
> video starts recording 1 second before the test video? In that case you need
to
> align the start of the test and ref videos.
> 
> Maybe it's better to hope there are no freezes in the ref video and just print
> the freezes in the test video...

I have renamed the function now and added a comment. I align the videos in the
beginning

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:391: max_repeated_frames =
On 2016/12/12 12:46:20, phoglund wrote:
> On 2016/12/08 18:08:30, mandermo wrote:
> > On 2016/12/08 09:55:44, phoglund wrote:
> > > Also not sure if this algorithm is correct. To compute repeated frames,
> > suppose
> > > the following sequence:
> > > 
> > > ref  111234444445678
> > > test 111111234444445678
> > > 
> > > In that case, freq(ref, 1) = 3 and freq(test, 1) = 6, so we correctly get
> 6-3
> > =
> > > 3 repeated frames. But what if the test runs so long that the ref file
> wraps:
> > > 
> > > ref  111234444445678
> > > test 111111234444445678111234444445678
> > > 
> > > In that case freq(ref, 1) = 3 but freq(test, 1) = 9, and it claims that
> frame
> > 1
> > > is repeated 6 times. The correct answer is 3. In fact, the second 111
> sequence
> > > isn't even a freeze since it didn't freeze any more frames than were
already
> > > frozen in the ref file. The frequency count approach fails if there's
> > wrapping.
> > > I guess we can avoid that by always having a longer ref file than the time
> we
> > > run tests...
> > > 
> > > Or wait actually, I think your approach is correct after all. We can
assume
> > the
> > > ref recording will be about as long as the test recording, and it should
> > _also_
> > > wrap unless the ref file is super long:
> > > 
> > > ref  111234444445678111234444445678
> > > test 111111234444445678111234444445678
> > > 
> > > In that case your algorithm produces the right answer 3. It breaks down in
> > some
> > > cases though:
> > > 
> > > ref  121212121212
> > > test 1112111211121112
> > > 
> > > Here freq(ref, 1) = 6 and freq(test, 1) = 12, so max_repeated = 6, but the
> > > longest freeze is actually 3. I doubt this is actually a problem though
> since
> > > the above freeze pattern looks pretty bizarre. It will tend to be
> > > 
> > > 123444444445677778 etc, and then it will work.
> > > 
> > > Try to comment how this works, and write unit tests to illustrate the
> various
> > > edge cases and assumptions here. You can also make the code more readable
> with
> > > some variables:
> > > 
> > > This is also hard to read, extracting some variables might help:
> > > 
> > > int test_frame_frequency = it_test->second;
> > > int ref_frame_frequency = it_ref->second;
> > > 
> > > // If a frame shows up more in the test video it's likely been frozen.
> > > int suspected_freeze = test_frame_frequency - ref_frame_frequency + 1;
> > > 
> > 
> > Lets make the unittests and see what output we get.
> 
> Ok, I think counting the frame clusters like your code is today, is a better
> approach. Disregard most of my comment above, since it finds that frame
> frequency counting is bad when the videos wrap and not necessarily start in
the
> same place.

Done.

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/video_quality_analysis.h (right):

https://codereview.webrtc.org/2553693002/diff/60001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/video_quality_analysis.h:46: // position in the
original video. We should provide a statistics file along
On 2016/12/12 12:46:20, phoglund wrote:
> On 2016/12/08 09:55:44, phoglund wrote:
> > Only now we're providing stats file for both files, so update the comment to
> > reflect that. Also note down we'll treat both files the same way.
> 
> Please address this.

Done.

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:56: help=('Path to the temporary stats file to be
created and '
Maybe --stats_file_test should be called --stats_file like before? But
--stats_file_ref can still be called stats_file_ref.

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:100: null_filehandle = open(os.devnull, 'r')
On 2016/12/12 12:46:20, phoglund wrote:
> Create a helper which you invoke
> 
> ...Popen(cmd, stdin=_DevNull(),...
> 
> and put the comment on the helper instead.

Done.

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/frame_analyz...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/frame_analyz...
webrtc/tools/frame_analyzer/frame_analyzer.cc:57: "  - stats_file_ref(string):
The full name of the file containing the"
On 2016/12/12 12:46:20, phoglund wrote:
> That's a mouthful. What about
> 
> "The stats file, as produced by the barcode_decoder script, for the reference
> file."
> 
> Ditto for the test file.

Improved text. Is new text better?

[kjellander_webrtc, 2016-12-22 15:35:05]

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:53: 'used. Default: %default'))
Please explain briefly the difference between ref and test, even if it may seem
obvious.

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:139: options.ref_video, options.stats_file_ref)
!= 0:
indent to alighn with above (
(when using 4 spaces indent all parameters should be on the new line)

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:142: options.test_video, options.stats_file_test)
!= 0:
indent to alight with above (

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:48: " --stats_file=stats.txt "
Remove?

[mandermo, 2017-01-02 14:59:12]

Fixed review comments. Should we rename --stats_file_test to --stats_file to be
more compatible with previous compare_videos.py?

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:53: 'used. Default: %default'))
On 2016/12/22 15:35:05, kjellander_webrtc wrote:
> Please explain briefly the difference between ref and test, even if it may
seem
> obvious.

I had a slightly longer description before that covered the differences between
ref and stat and phoglund@ thought it was too much (see bottom of message #8),
so I struggle to find the right level of detail on this one. You can give me a
concrete suggestion.

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:139: options.ref_video, options.stats_file_ref)
!= 0:
On 2016/12/22 15:35:05, kjellander_webrtc wrote:
> indent to alighn with above (
> (when using 4 spaces indent all parameters should be on the new line)

Done.

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:142: options.test_video, options.stats_file_test)
!= 0:
On 2016/12/22 15:35:04, kjellander_webrtc wrote:
> indent to alight with above (

Done.

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:48: " --stats_file=stats.txt "
On 2016/12/22 15:35:05, kjellander_webrtc wrote:
> Remove?

Done.

[kjellander_webrtc, 2017-01-02 20:01:49]

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/80001/webrtc/tools/compare_vide...
webrtc/tools/compare_videos.py:56: help=('Path to the temporary stats file to be
created and '
On 2016/12/21 16:42:06, mandermo wrote:
> Maybe --stats_file_test should be called --stats_file like before? But
> --stats_file_ref can still be called stats_file_ref.

Good point, although it's not as clean.
I suggest keeping the old one but say that it's DEPRECATED in the help string.
Then make it behave exactly the same as --stats_file_test. That way the test in
Chrome won't break when this is submitted, and a CL in Chrome can easily be
submitted to use the non-deprecated one, after which we can remove it.

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:53: 'used. Default: %default'))
On 2017/01/02 14:59:12, mandermo wrote:
> On 2016/12/22 15:35:05, kjellander_webrtc wrote:
> > Please explain briefly the difference between ref and test, even if it may
> seem
> > obvious.
> 
> I had a slightly longer description before that covered the differences
between
> ref and stat and phoglund@ thought it was too much (see bottom of message #8),
> so I struggle to find the right level of detail on this one. You can give me a
> concrete suggestion.

I'm sorry but I can't find Patrik's comment. Either way, just make it so that
they're not identical at least. No need to be too verbose.

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:26: * frames from the two videos,
a stats file should be provided. The stats file
This text needs a little adjustment to mention the now two stats files.

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:39: * --test_file=<name_of_file>
--stats_file=<name_of_file> --width=<frame_width>
I realized this needs to be updated as well.

[mandermo, 2017-01-03 17:36:46]

I added --stats_file as an argument, but ignore the value, since it should
probably work anyway. What BUG does this CL belong to?

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/100001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:53: 'used. Default: %default'))
On 2017/01/02 20:01:48, kjellander_webrtc wrote:
> On 2017/01/02 14:59:12, mandermo wrote:
> > On 2016/12/22 15:35:05, kjellander_webrtc wrote:
> > > Please explain briefly the difference between ref and test, even if it may
> > seem
> > > obvious.
> > 
> > I had a slightly longer description before that covered the differences
> between
> > ref and stat and phoglund@ thought it was too much (see bottom of message
#8),
> > so I struggle to find the right level of detail on this one. You can give me
a
> > concrete suggestion.
> 
> I'm sorry but I can't find Patrik's comment. Either way, just make it so that
> they're not identical at least. No need to be too verbose.

Sorry, mixed it up with comment on frame_analyzer. Have fixed it now.

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/frame_analyzer.cc (right):

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:26: * frames from the two videos,
a stats file should be provided. The stats file
On 2017/01/02 20:01:48, kjellander_webrtc wrote:
> This text needs a little adjustment to mention the now two stats files.

Done.

https://codereview.webrtc.org/2553693002/diff/120001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/frame_analyzer.cc:39: * --test_file=<name_of_file>
--stats_file=<name_of_file> --width=<frame_width>
On 2017/01/02 20:01:48, kjellander_webrtc wrote:
> I realized this needs to be updated as well.

Done.

[kjellander_webrtc, 2017-01-04 14:47:07]

Just one thing left, to avoid breaking the existing test when rolling this
change in.

https://codereview.webrtc.org/2553693002/diff/140001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/140001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:61: help=('DEPRECATED and ignored'))
I suggest having --stats_file_test read from this value if it's specified +
print a warning during execution that --stats_file is DEPRECATED but that its
value is being used.

If not specified, --stats_file_test should just use its own value.

If we just ignore its value, the test in Chrome will break when we land this CL
and it gets rolled in.

[mandermo, 2017-01-05 16:49:17]

Fixed review comment so that if the switch stats_file is set then it overrides
the stats_file_test switch. I am not sure the Chromium code would break without
this patch.

[mandermo, 2017-01-05 16:49:18]

Fixed review comment so that if the switch stats_file is set then it overrides
the stats_file_test switch. I am not sure the Chromium code would break without
this patch.

[kjellander_webrtc, 2017-01-09 12:12:19]

lgtm

https://codereview.webrtc.org/2553693002/diff/140001/webrtc/tools/compare_vid...
File webrtc/tools/compare_videos.py (right):

https://codereview.webrtc.org/2553693002/diff/140001/webrtc/tools/compare_vid...
webrtc/tools/compare_videos.py:61: help=('DEPRECATED and ignored'))
On 2017/01/04 14:47:07, kjellander_webrtc wrote:
> I suggest having --stats_file_test read from this value if it's specified +
> print a warning during execution that --stats_file is DEPRECATED but that its
> value is being used.
> 
> If not specified, --stats_file_test should just use its own value.
> 
> If we just ignore its value, the test in Chrome will break when we land this
CL
> and it gets rolled in. 

You raised the point off-review that the test won't fail if the value of the
flag is ignored. You're absolutely right about that, the test don't use the
stats file for anything (but it's contents might be interesting for debugging a
failed test etc, so it's still interesting). As long as the flag exists until
the call to compare_video.py in
https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_video...
is updated, this is fine.

I think we should just remove the passing of the flag entirely in that test,
since it will then just use the default value.

[mandermo, 2017-01-09 16:55:18]

Description was changed from

==========
Comparison of videos with reference frame not starting from zero

BUG=
==========

to

==========
Comparison of videos with reference frame not starting from zero

BUG=webrtc:6964
==========

[mandermo, 2017-01-09 16:55:35]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 16:55:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-09 17:17:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 17:17:24]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20050)

[mandermo, 2017-01-10 14:33:25]

Description was changed from

==========
Comparison of videos with reference frame not starting from zero

BUG=webrtc:6964
==========

to

==========
Comparison of videos with reference frame not starting from zero

BUG=webrtc:6967
==========

[mandermo, 2017-01-11 12:07:17]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 12:07:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-11 12:44:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 12:44:10]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20118)

[mandermo, 2017-01-11 13:48:34]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 13:48:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-11 14:01:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 14:01:33]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20124)

[mandermo, 2017-01-11 16:41:12]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:41:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-11 16:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 16:53:51]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20134)

[mandermo, 2017-01-13 09:35:43]

The CQ bit was checked by mandermo@webrtc.org

[mandermo, 2017-01-13 09:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2553693002/#ps200001 (title:
"Merge branch 'master' into better_compare")

[commit-bot: I haz the power, 2017-01-13 09:35:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-13 10:14:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 10:14:38]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20211)

[mandermo, 2017-01-16 10:21:36]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 10:21:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-16 10:35:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 10:35:28]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[mandermo, 2017-01-16 15:44:44]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 15:44:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-16 17:51:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:51:15]

Dry run: Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16740)

[mandermo, 2017-01-16 18:19:13]

The CQ bit was checked by mandermo@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 18:19:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-16 20:33:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 20:33:22]

Dry run: Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16751)

[janssonWebRTC, 2017-01-17 09:55:56]

The CQ bit was checked by jansson@webrtc.org

[janssonWebRTC, 2017-01-17 09:55:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2553693002/#ps220001 (title:
"Added newline in debug prints for PrintMaxRepeatedAndSkippedFrames")

[commit-bot: I haz the power, 2017-01-17 09:56:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-17 11:24:58]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484646956626730,
"parent_rev": "160e4a78e30041258a86e115bddaf5074f36d2b6", "commit_rev":
"7456817d40f15ad6e4e9a7acf204c27e12509bc3"}

[commit-bot: I haz the power, 2017-01-17 11:25:02]

Description was changed from

==========
Comparison of videos with reference frame not starting from zero

BUG=webrtc:6967
==========

to

==========
Comparison of videos with reference frame not starting from zero

BUG=webrtc:6967

Review-Url: https://codereview.webrtc.org/2553693002
Cr-Commit-Position: refs/heads/master@{#16112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7456817d40f15ad6e4e9a7acf...
==========

[commit-bot: I haz the power, 2017-01-17 11:25:03]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/7456817d40f15ad6e4e9a7acf...