Try switching black frame algorithm to a luma-based algorithm.

Try switching black frame algorithm to a luma-based algorithm.

Sheriffs: if WebRtcBrowserTest.CanSetupVideoCallAndDisableLocalVideo
flakes, revert this patch. It shouldn't flake after this fix.

Instead of looking at color values of individual pixels, see if we can
get more reliable results by looking at luma values instead. This way
we can detect black frames more reliably in the tests.

This is a partial re-land of codereview.chromium.org/1108803002.

This is essentially the black frame algorithm used in testrtc.

BUG=477498
Committed: https://crrev.com/859d9a26e396f802f6aafa13c45a1ce056fe6154
Cr-Commit-Position: refs/heads/master@{#330539}

[phoglund_chromium, 2015-05-18 14:29:30]

phoglund@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2015-05-19 08:08:54]

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:192: for (var i = 4; i <
pixels.length; i += 4) {
Please keep the comment about that |pixels| is in RGBA and why we ignore the
first pixel.

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:193: // Use Luma as in Rec.
709: Y′709 = 0.21R + 0.72G + 0.07B;
I'm new to color space standards, but according to
http://en.wikipedia.org/wiki/Rec._709 "HDTV according to Rec. 709 forms luma
(Y’) using R’G’B’ coefficients 0.2126, 0.7152, and 0.0722".
I guess the decimals are not a big deal, but we might as well use the full
constants.

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:195: 0.21 * pixels[i] + 0.72 *
pixels[i + 1] + 0.07 * pixels[i + 2];
Can you extract what's after += to a function?

[phoglund_chromium, 2015-05-19 09:01:20]

PTAL: note I re-enabled the test and added back a better reporting mechanism I
had in a previous patch. I forgot to add those earlier.

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:192: for (var i = 4; i <
pixels.length; i += 4) {
On 2015/05/19 08:08:54, kjellander wrote:
> Please keep the comment about that |pixels| is in RGBA and why we ignore the
> first pixel.

Done.

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:193: // Use Luma as in Rec.
709: Y′709 = 0.21R + 0.72G + 0.07B;
On 2015/05/19 08:08:54, kjellander wrote:
> I'm new to color space standards, but according to
> http://en.wikipedia.org/wiki/Rec._709 "HDTV according to Rec. 709 forms luma
> (Y’) using R’G’B’ coefficients 0.2126, 0.7152, and 0.0722".
> I guess the decimals are not a big deal, but we might as well use the full
> constants.

Done.

https://codereview.chromium.org/1132333006/diff/1/content/test/data/media/web...
content/test/data/media/webrtc_test_utilities.js:195: 0.21 * pixels[i] + 0.72 *
pixels[i + 1] + 0.07 * pixels[i + 2];
On 2015/05/19 08:08:54, kjellander wrote:
> Can you extract what's after += to a function?

Done.

[kjellander_chromium, 2015-05-19 09:10:01]

Just some minor changes left.

Can you put something in the CL description to make it clear that this is a
re-land of https://codereview.chromium.org/1108803002?

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
content/test/data/media/webrtc_test_utilities.js:203: // |pixels| is in RGBA.
Ignore the alpha channel.
I'm still missing the explanation on why we ignore the first pixel. I would find
it confusing to read the for loop starting at element 4 in the array if I didn't
know the reason behind it.
I guess the comment could go before the for loop also.

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
content/test/data/media/webrtc_test_utilities.js:212: // Use Luma as in Rec.
709: Y′709 = 0.2126R + 0.7152G + 0.0722B;
Can you move this up to be function documentation instead?
Maybe also link to http://en.wikipedia.org/wiki/Rec._709.

[phoglund_chromium, 2015-05-19 09:37:08]

PTAL

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
content/test/data/media/webrtc_test_utilities.js:203: // |pixels| is in RGBA.
Ignore the alpha channel.
On 2015/05/19 09:10:01, kjellander wrote:
> I'm still missing the explanation on why we ignore the first pixel. I would
find
> it confusing to read the for loop starting at element 4 in the array if I
didn't
> know the reason behind it.
> I guess the comment could go before the for loop also.

Oh sorry, I misread your comment and thought you were talking about why we were
ignoring the alpha channel. I have no idea why we would ignore the first pixel,
so I'll just change that to 0 (I assume that must be a bug).

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
content/test/data/media/webrtc_test_utilities.js:212: // Use Luma as in Rec.
709: Y′709 = 0.2126R + 0.7152G + 0.0722B;
On 2015/05/19 09:10:01, kjellander wrote:
> Can you move this up to be function documentation instead?
> Maybe also link to http://en.wikipedia.org/wiki/Rec._709.

Done.

[kjellander_chromium, 2015-05-19 10:06:22]

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/1132333006/diff/40001/content/test/data/media...
content/test/data/media/webrtc_test_utilities.js:203: // |pixels| is in RGBA.
Ignore the alpha channel.
On 2015/05/19 09:37:08, phoglund wrote:
> On 2015/05/19 09:10:01, kjellander wrote:
> > I'm still missing the explanation on why we ignore the first pixel. I would
> find
> > it confusing to read the for loop starting at element 4 in the array if I
> didn't
> > know the reason behind it.
> > I guess the comment could go before the for loop also.
> 
> Oh sorry, I misread your comment and thought you were talking about why we
were
> ignoring the alpha channel. I have no idea why we would ignore the first
pixel,
> so I'll just change that to 0 (I assume that must be a bug).

Hmm, it sounds a bit risky to just assume that's a bug in YUV conversion that
has been fixed. 
I suggest ignoring the first pixel here and follow up with rileya@ who wrote
https://codereview.chromium.org/601083002 to see if it can be removed (or just
do that in a separate CL after committing this).

It's more important to get this back running than fixing two things in the same
CL.

[phoglund_chromium, 2015-05-19 11:25:21]

> Hmm, it sounds a bit risky to just assume that's a bug in YUV conversion that
> has been fixed. 
> I suggest ignoring the first pixel here and follow up with rileya@ who wrote
> https://codereview.chromium.org/601083002 to see if it can be removed (or just
> do that in a separate CL after committing this).
> 
> It's more important to get this back running than fixing two things in the
same
> CL.

That is a different thing. Starting at i=4 means we skip ARGB for the first
pixel, whereas the change you point to allows the _value_ of each RGB value to
be 0 or 1. When it comes to the latter, this patch is even more generous than
allowing 0 or 1. For the former, I don't see why the first pixel should be
different than the other 640x480 pixels.

[kjellander_chromium, 2015-05-19 12:09:11]

On 2015/05/19 11:25:21, phoglund wrote:
> > Hmm, it sounds a bit risky to just assume that's a bug in YUV conversion
that
> > has been fixed. 
> > I suggest ignoring the first pixel here and follow up with rileya@ who wrote
> > https://codereview.chromium.org/601083002 to see if it can be removed (or
just
> > do that in a separate CL after committing this).
> > 
> > It's more important to get this back running than fixing two things in the
> same
> > CL.
> 
> That is a different thing. Starting at i=4 means we skip ARGB for the first
> pixel, whereas the change you point to allows the _value_ of each RGB value to
> be 0 or 1. 

I guess the order is RGBA and not ARGB? Otherwise we're calculating the luma for
the wrong pixels in this CL.

> When it comes to the latter, this patch is even more generous than
> allowing 0 or 1. For the former, I don't see why the first pixel should be
> different than the other 640x480 pixels.

Ah, you're right. I misinterpreted the comment in PS#1.

lgtm. Bonus point if you add a link to the TestRTC code.

[phoglund_chromium, 2015-05-19 13:26:19]

The CQ bit was checked by phoglund@chromium.org

[phoglund_chromium, 2015-05-19 13:26:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@chromium.org
Link to the patchset: https://codereview.chromium.org/1132333006/#ps80001
(title: "Fixing threshold for first pixel after offline discussion.")

[commit-bot: I haz the power, 2015-05-19 13:26:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132333006/80001

[commit-bot: I haz the power, 2015-05-19 17:15:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-19 17:17:15]

Patchset 5 (id:??) landed as
https://crrev.com/859d9a26e396f802f6aafa13c45a1ce056fe6154
Cr-Commit-Position: refs/heads/master@{#330539}