Implementing Binary glTF reader for the VR controller model

Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with minimal size.

Binary glTF Specification in: https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binary_glTF

BUG=644562
Review-Url: https://codereview.chromium.org/2852533004
Cr-Original-Commit-Position: refs/heads/master@{#468198}
Committed: https://chromium.googlesource.com/chromium/src/+/b416391e0ae62217afb7ea5ffeb04ab2640a96ed
Review-Url: https://codereview.chromium.org/2852533004
Cr-Commit-Position: refs/heads/master@{#468739}
Committed: https://chromium.googlesource.com/chromium/src/+/244b557adb6887dd26e5ff1876004de8acbcfdea

[acondor_, 2017-04-28 14:40:33]

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

[acondor_, 2017-04-28 14:40:34]

PTAL

[mthiesse, 2017-04-28 15:51:34]

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

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:24: constexpr const char
kFailedtoReadBinaryGltfMsg[] =
Wrap this in #if defined(DCHECK_IS_ON)

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:30: enum {
Why is this an enum? Looks like just making constants would be a better fit?

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:119: size_t buffer_cnt = 0;
nit: avoid using abbreviations in variable names like 'cnt'

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:377: LOG(ERROR) <<
kFailedtoReadBinaryGltfMsg << "Incomplete data";
Make these DLOGs? Best to avoid adding strings to release builds.

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:385: LOG(ERROR) <<
kFailedtoReadBinaryGltfMsg << "Unknown version";
nit: s/Unknown/Unsupported

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:398: LOG(ERROR) <<
kFailedtoReadBinaryGltfMsg << "Unknown glTF format";
nit: s/Unknown/Unsupported

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:81: kVersionStart = 4,
Remove this

[acondor_, 2017-04-28 16:07:01]

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

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:30: enum {
On 2017/04/28 15:51:34, mthiesse wrote:
> Why is this an enum? Looks like just making constants would be a better fit?

Done.

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:119: size_t buffer_cnt = 0;
On 2017/04/28 15:51:34, mthiesse wrote:
> nit: avoid using abbreviations in variable names like 'cnt'

Done.

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:81: kVersionStart = 4,
On 2017/04/28 15:51:34, mthiesse wrote:
> Remove this

Done.

[mthiesse, 2017-04-28 16:11:46]

lgtm

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

https://codereview.chromium.org/2852533004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:24: constexpr const char
kFailedtoReadBinaryGltfMsg[] =
On 2017/04/28 15:51:34, mthiesse wrote:
> Wrap this in #if defined(DCHECK_IS_ON)

I know this makes the code cleaner, but to avoid extra strings, I think you
should just hard-code it where it's used in the debug logs and delete the
constexpr here.

[bajones, 2017-04-28 17:02:37]

LGTM

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

https://codereview.chromium.org/2852533004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:421: DLOG(ERROR) <<
kFailedtoReadBinaryGltfMsg << "Content is not a valid glTF";
Nit: For the sake of future debugging it would be nice if this error string were
at least slightly different than the previous one.

[acondor_, 2017-04-28 22:11:47]

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

https://codereview.chromium.org/2852533004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:421: DLOG(ERROR) <<
kFailedtoReadBinaryGltfMsg << "Content is not a valid glTF";
On 2017/04/28 17:02:37, bajones wrote:
> Nit: For the sake of future debugging it would be nice if this error string
were
> at least slightly different than the previous one.

Done.

[acondor_, 2017-04-28 22:12:06]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-04-28 22:12:07]

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/2852533004/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2017-04-28 22:12:54]

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-29 00:49:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493417526871710,
"parent_rev": "6309d69c659e25087f0fc2d94e01c09b4c628458", "commit_rev":
"b416391e0ae62217afb7ea5ffeb04ab2640a96ed"}

[commit-bot: I haz the power, 2017-04-29 00:49:30]

Description was changed from

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

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

to

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

BUG=644562

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

[commit-bot: I haz the power, 2017-04-29 00:49:31]

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

[aelias_OOO_until_Jul13, 2017-05-01 17:13:21]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2852103002/ by aelias@chromium.org.

The reason for reverting is: Test added in this patch fails on downstream bot
(see http://crbug.com/717088)

BUG=717088.

[mthiesse, 2017-05-01 18:36:06]

Description was changed from

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

BUG=644562

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

to

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

BUG=644562

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

[acondor_, 2017-05-02 17:50:07]

acondor@google.com changed reviewers:
- bajones@chromium.org

[acondor_, 2017-05-02 17:50:08]

PTAL

[mthiesse, 2017-05-02 17:53:57]

lgtm

https://codereview.chromium.org/2852533004/diff/50001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2852533004/diff/50001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:398: auto gltf_content =
glb_content.substr(kContentStart, content_length);
nit: Prefer not to use auto here because the type isn't immediately obvious

[acondor_, 2017-05-02 18:03:47]

https://codereview.chromium.org/2852533004/diff/50001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2852533004/diff/50001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:398: auto gltf_content =
glb_content.substr(kContentStart, content_length);
On 2017/05/02 17:53:57, mthiesse wrote:
> nit: Prefer not to use auto here because the type isn't immediately obvious

Done.

[acondor_, 2017-05-02 18:03:52]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-05-02 18:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2852533004/#ps70001
(title: "Avoiding auto")

[commit-bot: I haz the power, 2017-05-02 18:04:13]

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-05-02 19:28:11]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493748232353970,
"parent_rev": "6616a885b4f18edd4f6aa178146c14538ca1d429", "commit_rev":
"244b557adb6887dd26e5ff1876004de8acbcfdea"}

[commit-bot: I haz the power, 2017-05-02 19:28:23]

Description was changed from

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

BUG=644562

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

to

==========
Implementing Binary glTF reader for the VR controller model

The binary glTF format enables us to deliver 3D models on a single file with
minimal size.

Binary glTF Specification in:
https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_binar...

BUG=644562

Review-Url: https://codereview.chromium.org/2852533004
Cr-Original-Commit-Position: refs/heads/master@{#468198}
Committed:
https://chromium.googlesource.com/chromium/src/+/b416391e0ae62217afb7ea5ffeb0...
Review-Url: https://codereview.chromium.org/2852533004
Cr-Commit-Position: refs/heads/master@{#468739}
Committed:
https://chromium.googlesource.com/chromium/src/+/244b557adb6887dd26e5ff187600...
==========

[commit-bot: I haz the power, 2017-05-02 19:28:26]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/244b557adb6887dd26e5ff187600...