Fix Mojo IPC for HDR metadata

Fix Mojo IPC for HDR metadata

BUG=708808
Review-Url: https://codereview.chromium.org/2815953002
Cr-Commit-Position: refs/heads/master@{#464181}
Committed: https://chromium.googlesource.com/chromium/src/+/b44ee0e7a402cbc927a59c15e2ab3fe1b9ac938a

[servolk, 2017-04-12 19:02:49]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 19:04:11]

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

[servolk, 2017-04-12 19:05:10]

Description was changed from

==========
Fix Mojo IPC for HDR metadata

BUG=708808
==========

to

==========
Fix Mojo IPC for HDR metadata

BUG=708808
==========

[servolk, 2017-04-12 19:05:11]

servolk@chromium.org changed reviewers:
+ tsepez@chromium.org, xhwang@chromium.org

[servolk, 2017-04-12 19:08:39]

TBH I'm not sure why this is necessary, as the type conversion unit tests pass
with and without this change. But I've found out that this seems to be necessary
when actual inter-process transfer occurs. Xiaohan, do we need to extend our
test coverage? Something seems to be missing, but I'm not sure what. Also, do we
really need the Read method in StructTraits? The autogenerated one seems to be
correct and covers all sub-fields.

[Tom Sepez, 2017-04-12 20:33:47]

lgtm

[commit-bot: I haz the power, 2017-04-12 20:45:26]

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

[commit-bot: I haz the power, 2017-04-12 20:45:27]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-12 20:45:48]

lg

OOC, how did you find this and can we add a test case?

[servolk, 2017-04-12 20:53:04]

On 2017/04/12 20:45:48, xhwang_slow wrote:
> lg
> 
> OOC, how did you find this and can we add a test case?

I found out after I switched over Chromecast to use this new IPC path and saw
that for some reason the HDRMetadata struct that we got out of Mojo IPC had
(invalid) 0 values, even though the input struct that we gave to the Mojo type
converter was valid. I've started looking closer and noticed that other Mojo IPC
classes call Read* methods for substructs explicitly in their StructTraits::Read
implementations, so I tried that too and that seems to fix the problem. Although
TBH it's still not clear to me what's exactly causing this, since IIUR the type
converter test does exactly the same (convert a VideoDecoderConfig object
containing an HDRMetadata struct into a Mojo form and back), and it passes.

Also I've noticed that Mojo seems to generate a valid ::Read method which
automatically reads all the necessary fields
(https://cs.chromium.org/chromium/src/out/Debug/gen/media/mojo/interfaces/medi...),
so now I'm wondering why do we even need to explicitly defined the
StructTraits::Read? Can we somehow use the autogenerated one? This Mojo stuff is
confusing, I was hoping you could shed some light on this.

[xhwang, 2017-04-12 21:37:19]

lgtm

[servolk, 2017-04-12 21:37:57]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2017-04-12 21:39:37]

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-12 22:39:25]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492033077554310, "parent_rev":
"efb697309fb4c64a99e8c38b0448a4b4bb97886a", "commit_rev":
"b44ee0e7a402cbc927a59c15e2ab3fe1b9ac938a"}

[commit-bot: I haz the power, 2017-04-12 22:40:19]

Description was changed from

==========
Fix Mojo IPC for HDR metadata

BUG=708808
==========

to

==========
Fix Mojo IPC for HDR metadata

BUG=708808

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

[commit-bot: I haz the power, 2017-04-12 22:40:20]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b44ee0e7a402cbc927a59c15e2ab...