VRShell: Composing Daydream controller textures from patches

VRShell: Composing Daydream controller textures from patches

This change implements the creation of textures from a base texture and a patch, in order to reduce the size of assets necessary for rendering the controller in VRShell.

BUG=644562
Review-Url: https://codereview.chromium.org/2837973002
Cr-Commit-Position: refs/heads/master@{#468121}
Committed: https://chromium.googlesource.com/chromium/src/+/c76599f4da775f3f5a992083a841ea2207831470

[acondor_, 2017-04-24 21:28:18]

acondor@google.com changed reviewers:
+ cjgrant@chromium.org, mthiesse@chromium.org

[acondor_, 2017-04-24 21:28:19]

PTAL

[acondor_, 2017-04-25 14:34:29]

On 2017/04/24 21:28:19, acondor wrote:
> PTAL

Hold on this, as we are considering changing the patches format and sizes.

[acondor_, 2017-04-26 18:54:17]

Ready to be reviewed now.

[cjgrant, 2017-04-27 18:47:30]

A couple of nits and questions; mostly deferring to Michael here.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.cc (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:43: const gfx::Point
kPatchesLocations[] = {{}, {5, 5}, {47, 165}, {47, 234}};
How will you deliver these coordinates through component updater?  Is it worth
putting the coordinates in a file now as well, rather than having texture
specifics coded into here?

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:45: sk_sp<SkImage>
LoadPNG(const base::FilePath& path) {
nit: You're probably following the gfx::PNGCodec style, but FYI:

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:202: continue;
If this fails, do we want to feed back the failure to not draw the controller or
something?  Or just go with a partial result?

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.h (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.h:33: ~Patch();
What happens if you just exclude the destructor here?

[acondor_, 2017-04-27 19:01:19]

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.cc (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:43: const gfx::Point
kPatchesLocations[] = {{}, {5, 5}, {47, 165}, {47, 234}};
On 2017/04/27 18:47:29, cjgrant wrote:
> How will you deliver these coordinates through component updater?  Is it worth
> putting the coordinates in a file now as well, rather than having texture
> specifics coded into here?

I'm in favor of delivering them in the component as text files to make it easier
to change textures if we need to. But it's ok for now as we will be delivering
as assets.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:45: sk_sp<SkImage>
LoadPNG(const base::FilePath& path) {
On 2017/04/27 18:47:29, cjgrant wrote:
> nit: You're probably following the gfx::PNGCodec style, but FYI:
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Acknowledged.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:202: continue;
On 2017/04/27 18:47:29, cjgrant wrote:
> If this fails, do we want to feed back the failure to not draw the controller
or
> something?  Or just go with a partial result?

There are only patches for clicked buttons, so it should be ok to render the
base texture. Even though I don't see this happening.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.h (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.h:33: ~Patch();
On 2017/04/27 18:47:29, cjgrant wrote:
> What happens if you just exclude the destructor here?

Chromium codestyle check asked me to provide constructor/destructor for this
struct. I will recheck just in case.

[acondor_, 2017-04-28 14:22:39]

Nits addressed.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.cc (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:45: sk_sp<SkImage>
LoadPNG(const base::FilePath& path) {
On 2017/04/27 19:01:19, acondor wrote:
> On 2017/04/27 18:47:29, cjgrant wrote:
> > nit: You're probably following the gfx::PNGCodec style, but FYI:
> > 
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.h (right):

https://codereview.chromium.org/2837973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.h:33: ~Patch();
On 2017/04/27 19:01:19, acondor wrote:
> On 2017/04/27 18:47:29, cjgrant wrote:
> > What happens if you just exclude the destructor here?
> 
> Chromium codestyle check asked me to provide constructor/destructor for this
> struct. I will recheck just in case.

Actually, I don't need this class anymore.

[mthiesse, 2017-04-28 18:26:39]

lgtm

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.cc (right):

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:133:
canvas->drawImage(base_texture_, 0, 0);
Add a TODO to avoid drawing the pixels that will be patched over anyways, since
this is CPU and we can save that work.

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:166: LOG(WARNING) <<
"Controller model files not found";
DLOG

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:174: LOG(ERROR) <<
"Failed to read controller model";
DLOG

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:614: LOG(ERROR) << "Failed
to read controller texture pixels";
DLOG

[acondor_, 2017-04-28 18:57:44]

Answers as discussed offline.

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.cc (right):

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:133:
canvas->drawImage(base_texture_, 0, 0);
On 2017/04/28 18:26:39, mthiesse wrote:
> Add a TODO to avoid drawing the pixels that will be patched over anyways,
since
> this is CPU and we can save that work.

Not worth it due to the patches' small sizes.

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:166: LOG(WARNING) <<
"Controller model files not found";
On 2017/04/28 18:26:39, mthiesse wrote:
> DLOG

This will change with the resources CL.

https://codereview.chromium.org/2837973002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:174: LOG(ERROR) <<
"Failed to read controller model";
On 2017/04/28 18:26:39, mthiesse wrote:
> DLOG

Acknowledged.

[acondor_, 2017-04-28 19:27:49]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-04-28 19:28:49]

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, 2017-04-28 20:50:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493407669606120,
"parent_rev": "caaa5c77ce09501d9dd409bde61ae226e1768b7f", "commit_rev":
"c76599f4da775f3f5a992083a841ea2207831470"}

[commit-bot: I haz the power, 2017-04-28 20:51:07]

Description was changed from

==========
VRShell: Composing Daydream controller textures from patches

This change implements the creation of textures from a base texture and a patch,
in order to reduce the size of assets necessary for rendering the controller in
VRShell.

BUG=644562
==========

to

==========
VRShell: Composing Daydream controller textures from patches

This change implements the creation of textures from a base texture and a patch,
in order to reduce the size of assets necessary for rendering the controller in
VRShell.

BUG=644562

Review-Url: https://codereview.chromium.org/2837973002
Cr-Commit-Position: refs/heads/master@{#468121}
Committed:
https://chromium.googlesource.com/chromium/src/+/c76599f4da775f3f5a992083a841...
==========

[commit-bot: I haz the power, 2017-04-28 20:51:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c76599f4da775f3f5a992083a841...