Improve GL shader YUV adjustment.

Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/aa35e3ec5b767a8cbf18740e96969b72cb7a9561
Cr-Commit-Position: refs/heads/master@{#409647}

[jbauman, 2016-07-21 23:02:23]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256.

BUG=621325
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jbauman, 2016-07-21 23:02:28]

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

[commit-bot: I haz the power, 2016-07-21 23:03:12]

Dry run: 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-07-21 23:34:11]

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

[commit-bot: I haz the power, 2016-07-21 23:34:12]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[jbauman, 2016-07-22 00:21:28]

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

[commit-bot: I haz the power, 2016-07-22 00:22:13]

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

[jbauman, 2016-07-22 00:32:43]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. Per BT.601-7
section 2.5.3 these constants should be correct for 10-bit video as well.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jbauman, 2016-07-22 01:06:24]

jbauman@chromium.org changed reviewers:
+ hubbe@chromium.org, piman@chromium.org

[commit-bot: I haz the power, 2016-07-22 01:49:26]

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

[commit-bot: I haz the power, 2016-07-22 01:49:27]

Dry run: This issue passed the CQ dry run.

[piman, 2016-07-22 02:03:39]

lgtm

[jbauman, 2016-07-23 01:22:49]

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

[commit-bot: I haz the power, 2016-07-23 01:23:13]

Dry run: 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-07-23 02:29:34]

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

[commit-bot: I haz the power, 2016-07-23 02:29:35]

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

[jbauman, 2016-07-25 23:25:36]

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

[commit-bot: I haz the power, 2016-07-25 23:26:08]

Dry run: 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-07-26 00:37:35]

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

[commit-bot: I haz the power, 2016-07-26 00:37:36]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-07-26 00:40:41]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. Per BT.601-7
section 2.5.3 these constants should be correct for 10-bit video as well.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jbauman, 2016-07-26 00:41:56]

PTAL. Actually I looked at the spec some more and it seems that the 10-bit
component case needs to be treated differently, so I've added a parameter to the
Quad to specify the source format of the video.

[piman, 2016-07-26 06:46:48]

https://codereview.chromium.org/2173563002/diff/60001/cc/ipc/quads.mojom
File cc/ipc/quads.mojom (right):

https://codereview.chromium.org/2173563002/diff/60001/cc/ipc/quads.mojom#newc...
cc/ipc/quads.mojom:96: uint32 bits_per_channel;
You also need to modify the ParamTraits for YUVVideoDrawQuad in
src/cc/ipc/cc_param_traits_macros.h

https://codereview.chromium.org/2173563002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2173563002/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2269: for (int i = 0; i < 3; ++i)
nit: can you add {} per style?

[jbauman, 2016-07-26 21:16:03]

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

[commit-bot: I haz the power, 2016-07-26 21:16:38]

Dry run: 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-07-26 21:27:27]

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

[commit-bot: I haz the power, 2016-07-26 21:27:28]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jbauman, 2016-07-26 21:39:29]

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

[commit-bot: I haz the power, 2016-07-26 21:39:58]

Dry run: 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-07-26 23:18:47]

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

[commit-bot: I haz the power, 2016-07-26 23:18:48]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-07-26 23:32:25]

On 2016/07/26 06:46:48, piman - slow reviews until 8-8 wrote:
> https://codereview.chromium.org/2173563002/diff/60001/cc/ipc/quads.mojom
> File cc/ipc/quads.mojom (right):
> 
>
https://codereview.chromium.org/2173563002/diff/60001/cc/ipc/quads.mojom#newc...
> cc/ipc/quads.mojom:96: uint32 bits_per_channel;
> You also need to modify the ParamTraits for YUVVideoDrawQuad in
> src/cc/ipc/cc_param_traits_macros.h

Oops, too many separate places to add this. Done.
> 
> https://codereview.chromium.org/2173563002/diff/60001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/2173563002/diff/60001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:2269: for (int i = 0; i < 3; ++i)
> nit: can you add {} per style?

Done.

[piman, 2016-07-27 04:47:52]

lgtm

[jbauman, 2016-07-27 21:04:43]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2016-07-27 21:05:00]

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-07-27 21:05:01]

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

[commit-bot: I haz the power, 2016-07-27 21:05:03]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[jbauman, 2016-07-27 21:06:22]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
==========

[jbauman, 2016-07-27 21:09:02]

jbauman@chromium.org changed reviewers:
+ dcheng@chromium.org

[jbauman, 2016-07-27 21:09:03]

dcheng: cc/ipc IPC review.

[dcheng, 2016-07-28 01:54:26]

https://codereview.chromium.org/2173563002/diff/100001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.h (right):

https://codereview.chromium.org/2173563002/diff/100001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.h:389: static float bits_per_channel(const
cc::DrawQuad& input) {
Isn't this a uint32 / int?

https://codereview.chromium.org/2173563002/diff/100001/cc/quads/yuv_video_dra...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/2173563002/diff/100001/cc/quads/yuv_video_dra...
cc/quads/yuv_video_draw_quad.h:86: int bits_per_channel = 8;
Let's use uint32 or int uniformly throughout: the mojom is currently uint32 but
this is an int.

[jbauman, 2016-07-28 02:11:25]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jbauman, 2016-07-28 02:11:31]

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

[commit-bot: I haz the power, 2016-07-28 02:11:52]

Dry run: 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-07-28 02:11:54]

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

[commit-bot: I haz the power, 2016-07-28 02:11:55]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[jbauman, 2016-07-28 02:17:38]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jbauman, 2016-07-28 02:17:46]

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

[commit-bot: I haz the power, 2016-07-28 02:18:03]

Dry run: 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-07-28 03:28:16]

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

[commit-bot: I haz the power, 2016-07-28 03:28:18]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-07-29 00:31:45]

PTAL.

On 2016/07/28 01:54:26, dcheng (OOO Aug 1 - Aug 11) wrote:
>
https://codereview.chromium.org/2173563002/diff/100001/cc/ipc/quads_struct_tr...
> File cc/ipc/quads_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2173563002/diff/100001/cc/ipc/quads_struct_tr...
> cc/ipc/quads_struct_traits.h:389: static float bits_per_channel(const
> cc::DrawQuad& input) {
> Isn't this a uint32 / int?
> 
Oops, done.

>
https://codereview.chromium.org/2173563002/diff/100001/cc/quads/yuv_video_dra...
> File cc/quads/yuv_video_draw_quad.h (right):
> 
>
https://codereview.chromium.org/2173563002/diff/100001/cc/quads/yuv_video_dra...
> cc/quads/yuv_video_draw_quad.h:86: int bits_per_channel = 8;
> Let's use uint32 or int uniformly throughout: the mojom is currently uint32
but
> this is an int.

Done.

[dcheng, 2016-07-29 02:37:07]

https://codereview.chromium.org/2173563002/diff/120001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2173563002/diff/120001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2266: float adjustment_multiplier = (1 <<
(quad->bits_per_channel - 8)) * 1.0f /
What happens if bits_per_channel is < 8 or > 39? Are there some sanity checks we
should add here?

[jbauman, 2016-07-30 00:09:05]

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

[commit-bot: I haz the power, 2016-07-30 00:09:32]

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

[jbauman, 2016-07-30 00:28:59]

On 2016/07/29 02:37:07, dcheng (OOO Aug 1 - Aug 11) wrote:
>
https://codereview.chromium.org/2173563002/diff/120001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/2173563002/diff/120001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:2266: float adjustment_multiplier = (1 <<
> (quad->bits_per_channel - 8)) * 1.0f /
> What happens if bits_per_channel is < 8 or > 39? Are there some sanity checks
we
> should add here?

Security-wise it should be fine (it'll just give very weird results). I've added
DCHECKs, though.

[dcheng, 2016-07-30 01:26:37]

https://codereview.chromium.org/2173563002/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2173563002/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2267: DCHECK_LE(quad->bits_per_channel, 24u);
Would it be possible to do this check in ParamTraits / StructTraits.
Technically, I think that shifting by more than the width of the type on the LHS
is undefined behavior.

[commit-bot: I haz the power, 2016-07-30 01:46:17]

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

[commit-bot: I haz the power, 2016-07-30 01:46:17]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-08-01 23:03:59]

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

[commit-bot: I haz the power, 2016-08-01 23:04:13]

Dry run: 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-08-02 00:50:28]

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

[commit-bot: I haz the power, 2016-08-02 00:50:29]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-08-02 01:12:00]

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

[commit-bot: I haz the power, 2016-08-02 01:12:16]

Dry run: 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-08-02 01:23:07]

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

[commit-bot: I haz the power, 2016-08-02 01:23:08]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbauman, 2016-08-02 01:51:07]

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

[commit-bot: I haz the power, 2016-08-02 01:51:19]

Dry run: 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-08-02 03:08:13]

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

[commit-bot: I haz the power, 2016-08-02 03:08:14]

Dry run: This issue passed the CQ dry run.

[jbauman, 2016-08-02 22:20:41]

On 2016/07/30 01:26:37, dcheng (OOO Aug 1 - Aug 11) wrote:
>
https://codereview.chromium.org/2173563002/diff/140001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/2173563002/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:2267: DCHECK_LE(quad->bits_per_channel, 24u);
> Would it be possible to do this check in ParamTraits / StructTraits.
> Technically, I think that shifting by more than the width of the type on the
LHS
> is undefined behavior.

Oh, I had thought this was implementation-defined. Added checks to ParamTraits
and StructTraits.

[dcheng, 2016-08-03 15:03:31]

ipc/mojom lgtm

i'm actually not 100% sure about the undefined behavior bit (normally I'd load
up the C++11 spec but I'm a metered connection), and the best reference I could
find is https://msdn.microsoft.com/en-us/library/f96c63ed.aspx

https://codereview.chromium.org/2173563002/diff/200001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2173563002/diff/200001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:882: ParamTraits<cc::DrawQuad>::GetSize(s, p);
Interesting. I don't have any good suggestions here though, so I guess this is
fine.

[jbauman, 2016-08-03 21:13:37]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-08-03 21:13:37]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2173563002/#ps200001
(title: "use enum hack")

[commit-bot: I haz the power, 2016-08-03 21:14:26]

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-08-03 22:46:49]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-03 22:46:52]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-08-03 22:48:05]

Description was changed from

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Improve GL shader YUV adjustment.

The raw texture data is from 0-255, so the constants used to adjust the
normalized data must be divided by 255 instead of 256. For
10-bit-per-component video, the constants must be multiplied by 4 and
divided by 1023.

BUG=621325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/aa35e3ec5b767a8cbf18740e96969b72cb7a9561
Cr-Commit-Position: refs/heads/master@{#409647}
==========

[commit-bot: I haz the power, 2016-08-03 22:48:07]

Patchset 11 (id:??) landed as
https://crrev.com/aa35e3ec5b767a8cbf18740e96969b72cb7a9561
Cr-Commit-Position: refs/heads/master@{#409647}