WebVR: Add sanity checks for decoded pose index values

WebVR: Add sanity checks for decoded pose index values

We seem to be getting glitches in the display pipeline that
manifest as all-white pixels for a few frames before regular
rendering starts. These get misinterpreted as an invalid
pose index, and apparently supplying a wrong pose to GVR
permanently breaks reprojection for that instance.

Add sanity checks to help identify and avoid bad pose values
during initialization.

BUG=667327
Committed: https://crrev.com/490f4294847d427265ae138de9932926d61b885e
Cr-Commit-Position: refs/heads/master@{#435866}

[klausw, 2016-12-01 03:46:42]

klausw@chromium.org changed reviewers:
+ bajones@chromium.org

[klausw, 2016-12-01 05:20:05]

This doesn't quite seem to do it yet, I'll add an extra sanity check to
ignore the first few received poses until things settle down. I haven't
seen the black frames after extended use, it just seems to happen at the
beginning or not at all.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mthiesse, 2016-12-01 15:39:11]

mthiesse@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2016-12-01 15:39:11]

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:634: head_pose =
webvr_head_pose_[webvr_pose_frame % kPoseRingBufferSize];
You should make webvr_head_pose_ a vector of unique pointers, and reset each
pose after we use it. If the pose we're about to set is null, we've clearly
received an invalid pose frame index.

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:563: bool
VrShell::WebVrPoseIsValid(uint32_t pose_frame) {
This doesn't guarantee the pose is valid; we should fill the top 16(?) bits with
a signature of some sort to check if the frame is valid.

[klausw, 2016-12-01 16:10:18]

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:634: head_pose =
webvr_head_pose_[webvr_pose_frame % kPoseRingBufferSize];
On 2016/12/01 15:39:11, mthiesse wrote:
> You should make webvr_head_pose_ a vector of unique pointers, and reset each
> pose after we use it. If the pose we're about to set is null, we've clearly
> received an invalid pose frame index.

Poses do get reused unfortunately. The compositor resends a duplicate copy of an
old frame if the new frame hasn't finished rendering yet at vsync, usually
followed by a dropped frame.

I don't think we can safely skip these frames, we only know it's a duplicate
after binding the GVR framebuffer, rendering it, and reading the pixel, and then
we've done pretty much all the work already. Not sure if it's legal to discard
an acquired frame without submitting it.

I'll add a comment along these lines to the source. I had actually implemented a
parallel pose_valid bool vector, then removed it again when I realized it
doesn't work.

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:563: bool
VrShell::WebVrPoseIsValid(uint32_t pose_frame) {
On 2016/12/01 15:39:11, mthiesse wrote:
> This doesn't guarantee the pose is valid; we should fill the top 16(?) bits
with
> a signature of some sort to check if the frame is valid.

Will do, in earlier versions I did have a magic number but had removed that for
simplification at some point. I'll add it back. This would also fix the current
issue of missing reprojection for a few frames every 3.2 days when the current
24bit index wraps back to zero.

I think a 16-bit constant magic number should be fairly reliable, and I think
it's a bit simpler and more predictable. A checksum would need to be carefully
chosen to ensure that all-black or all-white pixels don't read as valid, and in
any case there's a 1-in-65536 chance that a completely random pixel would be
considered valid.

In practice I don't think I've ever seen wrong pixels other than pure white or
black.

[klausw, 2016-12-01 19:50:41]

PTAL. The changes so far had not fixed it, I haven't seen the "not valid pose
branch" getting hit, but got a black screen after 90 tries. (The first 50 didn't
have a VLOG for the "not valid" pose branch yet.)

I've now added the "minimum valid poses" counter as discussed, and am continuing
to test.

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:634: head_pose =
webvr_head_pose_[webvr_pose_frame % kPoseRingBufferSize];
On 2016/12/01 16:10:18, klausw wrote:
> On 2016/12/01 15:39:11, mthiesse wrote:
> > You should make webvr_head_pose_ a vector of unique pointers, and reset each
> > pose after we use it. If the pose we're about to set is null, we've clearly
> > received an invalid pose frame index.
> 
> Poses do get reused unfortunately. The compositor resends a duplicate copy of
an
> old frame if the new frame hasn't finished rendering yet at vsync, usually
> followed by a dropped frame.
> 
> I don't think we can safely skip these frames, we only know it's a duplicate
> after binding the GVR framebuffer, rendering it, and reading the pixel, and
then
> we've done pretty much all the work already. Not sure if it's legal to discard
> an acquired frame without submitting it.
> 
> I'll add a comment along these lines to the source. I had actually implemented
a
> parallel pose_valid bool vector, then removed it again when I realized it
> doesn't work.

Done, I've added a "valid" vector that's set to false on initialization (in
InitializeGl), but still allowing pose reuse. Also added a comment.

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2541023003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:563: bool
VrShell::WebVrPoseIsValid(uint32_t pose_frame) {
On 2016/12/01 16:10:18, klausw wrote:
> On 2016/12/01 15:39:11, mthiesse wrote:
> > This doesn't guarantee the pose is valid; we should fill the top 16(?) bits
> with
> > a signature of some sort to check if the frame is valid.
> 
> Will do, in earlier versions I did have a magic number but had removed that
for
> simplification at some point. I'll add it back. This would also fix the
current
> issue of missing reprojection for a few frames every 3.2 days when the current
> 24bit index wraps back to zero.
> 
> I think a 16-bit constant magic number should be fairly reliable, and I think
> it's a bit simpler and more predictable. A checksum would need to be carefully
> chosen to ensure that all-black or all-white pixels don't read as valid, and
in
> any case there's a 1-in-65536 chance that a completely random pixel would be
> considered valid.
> 
> In practice I don't think I've ever seen wrong pixels other than pure white or
> black.

Added a 16-bit magic number. Currently hardcoded since there's no shared header
file with blink's VRDisplay.cpp, but this will all go away soon anyway I hope.

[klausw, 2016-12-01 20:51:31]

I've increased the min valid pose counter, I was testing with 120 frame wait
and still saw a black screen on try #67 (after 2 seconds), so it's still not
fixed
but seems less frequent. (While testing, I waited each time for the
"enabling reprojection" message.) I've decreased the wait to 60 frames
(~1 second) in the latest patchset.

[mthiesse, 2016-12-01 21:53:16]

https://codereview.chromium.org/2541023003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2541023003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:213: static constexpr int
webvr_min_valid_poses_ = 60;
I don't think we should land this min_valid_poses_ hack, unless you're convinced
that it really helps.

https://codereview.chromium.org/2541023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2541023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:618: gl->ClearColor((idx &
255) / 255.0f, 42 / 255.0f, 142 / 255.0f, 1.0f);
Make constants for the magic numbers, here and in vr_shell

[klausw, 2016-12-01 22:36:04]

PTAL

https://codereview.chromium.org/2541023003/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2541023003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:213: static constexpr int
webvr_min_valid_poses_ = 60;
On 2016/12/01 21:53:15, mthiesse wrote:
> I don't think we should land this min_valid_poses_ hack, unless you're
convinced
> that it really helps.

I've removed it, it's unclear. Got a black screen on try 21 with it removed just
now as opposed to try 67 with a 120 frame delay earlier, but that's not enough
data for good statistics.

https://codereview.chromium.org/2541023003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2541023003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:618: gl->ClearColor((idx &
255) / 255.0f, 42 / 255.0f, 142 / 255.0f, 1.0f);
On 2016/12/01 21:53:15, mthiesse wrote:
> Make constants for the magic numbers, here and in vr_shell

Done.

[mthiesse, 2016-12-01 22:38:46]

lgtm

New compositor path *cannot come soon enough*

[bajones, 2016-12-01 23:50:03]

LGTM, where "G" == "Generally acceptable at this time for getting around a hard
problem that ultimately needs a much larger refactor"

On 2016/12/01 22:38:46, mthiesse wrote:
> New compositor path *cannot come soon enough*

+1000

[klausw, 2016-12-02 02:24:24]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2016-12-02 02:24:50]

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

[commit-bot: I haz the power, 2016-12-02 02:36:05]

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

[commit-bot: I haz the power, 2016-12-02 02:36:06]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[klausw, 2016-12-02 04:30:02]

The CQ bit was checked by klausw@chromium.org

[klausw, 2016-12-02 04:30:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2541023003/#ps140001
(title: "Work around bogus initialization warning")

[commit-bot: I haz the power, 2016-12-02 04:30:21]

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

[commit-bot: I haz the power, 2016-12-02 05:58:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480653002711890,
"parent_rev": "ced18e845de24473aebf9b4268cc8d0104661b17", "commit_rev":
"bf0f12b70db69134e8621597ea1d4ad0ccd2d186"}

[commit-bot: I haz the power, 2016-12-02 05:58:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-02 06:00:25]

Description was changed from

==========
WebVR: Add sanity checks for decoded pose index values

We seem to be getting glitches in the display pipeline that
manifest as all-white pixels for a few frames before regular
rendering starts. These get misinterpreted as an invalid
pose index, and apparently supplying a wrong pose to GVR
permanently breaks reprojection for that instance.

Add sanity checks to help identify and avoid bad pose values
during initialization.

BUG=667327
==========

to

==========
WebVR: Add sanity checks for decoded pose index values

We seem to be getting glitches in the display pipeline that
manifest as all-white pixels for a few frames before regular
rendering starts. These get misinterpreted as an invalid
pose index, and apparently supplying a wrong pose to GVR
permanently breaks reprojection for that instance.

Add sanity checks to help identify and avoid bad pose values
during initialization.

BUG=667327

Committed: https://crrev.com/490f4294847d427265ae138de9932926d61b885e
Cr-Commit-Position: refs/heads/master@{#435866}
==========

[commit-bot: I haz the power, 2016-12-02 06:00:26]

Patchset 8 (id:??) landed as
https://crrev.com/490f4294847d427265ae138de9932926d61b885e
Cr-Commit-Position: refs/heads/master@{#435866}