Change half-float conversion to use 1.0 multiplier

Change half-float conversion to use 1.0 multiplier

Half-float conversion is more efficient on some platforms if the
multiplier used is 1.0. Also clean up the half-float conversion
code and extend testing up to 16 bits.

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

Committed: https://crrev.com/98626da3db8d1dec43d259d93f54b5adc9c500c2
Cr-Commit-Position: refs/heads/master@{#426885}

[hubbe, 2016-10-21 19:22:34]

Description was changed from

==========
Change half-float conversion to use 1.0 multiplier

Half-float conversion is more efficient on some platforms if the
multiplier used is 1.0. Also clean up the half-float conversion
code and extend testing up to 16 bits.

BUG=657627
==========

to

==========
Change half-float conversion to use 1.0 multiplier

Half-float conversion is more efficient on some platforms if the
multiplier used is 1.0. Also clean up the half-float conversion
code and extend testing up to 16 bits.

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

[hubbe, 2016-10-21 19:22:54]

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

[hubbe, 2016-10-21 19:23:25]

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

[commit-bot: I haz the power, 2016-10-21 19:23:31]

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

[piman, 2016-10-21 20:28:14]

lgtm

[hubbe, 2016-10-21 20:34:35]

The CQ bit was unchecked by hubbe@chromium.org

[hubbe, 2016-10-21 20:34:39]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:34:59]

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-10-21 20:59:11]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-21 21:03:20]

Description was changed from

==========
Change half-float conversion to use 1.0 multiplier

Half-float conversion is more efficient on some platforms if the
multiplier used is 1.0. Also clean up the half-float conversion
code and extend testing up to 16 bits.

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

to

==========
Change half-float conversion to use 1.0 multiplier

Half-float conversion is more efficient on some platforms if the
multiplier used is 1.0. Also clean up the half-float conversion
code and extend testing up to 16 bits.

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

Committed: https://crrev.com/98626da3db8d1dec43d259d93f54b5adc9c500c2
Cr-Commit-Position: refs/heads/master@{#426885}
==========

[commit-bot: I haz the power, 2016-10-21 21:03:21]

Patchset 1 (id:??) landed as
https://crrev.com/98626da3db8d1dec43d259d93f54b5adc9c500c2
Cr-Commit-Position: refs/heads/master@{#426885}

[fbarchard1, 2016-10-21 22:46:05]

fbarchard@google.com changed reviewers:
+ fbarchard@google.com

[fbarchard1, 2016-10-21 22:46:06]

need to pass the multiplier to shader?

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:329: dst[i] = src[i] | 0x3800;
this will be slow for compilers that dont vectorize, or if you allow 'official'
build to use -Os.

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:349: libyuv_multiplier_ = 1.0f / 4096.0f;
have you tested using 1.0?
values near 65535 will potentially become inf as a halffloat?  but we should
test this to see what really happens?
even with /4096 you can get denormals, which are value but hurt performance
dramatically on intel.

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:619:
half_float_maker->MakeHalfFloats(src, bytes_per_row / 2, dst);
libyuv:HalfFloatPlane has some overhead to detect cpu/alignment.  Suggest
calling it once per plane with width, height and stride.
If the stride == width * 2 it will row coalesce into a single call.

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.h (right):

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.h:90: // The numbers stored in |dst| will be
half floats in range 0.0..1.0
This comment needs update?

[hubbe, 2016-10-21 22:55:04]

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:329: dst[i] = src[i] | 0x3800;
On 2016/10/21 22:46:06, fbarchard1 wrote:
> this will be slow for compilers that dont vectorize, or if you allow
'official'
> build to use -Os.

Can we get rid of this code and move the decision of how to convert the floats
into half-floats into libyuv? Libyuv could just return the resource multiplier
and offset.

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:349: libyuv_multiplier_ = 1.0f / 4096.0f;
On 2016/10/21 22:46:06, fbarchard1 wrote:
> have you tested using 1.0?
> values near 65535 will potentially become inf as a halffloat?  but we should
> test this to see what really happens?
> even with /4096 you can get denormals, which are value but hurt performance
> dramatically on intel.
> 
> 


I have not tested the inf case.

The smallest possible normal value is 2^-14, 1/4096 is 4 times larger, so how
can you get subnormal values?

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:619:
half_float_maker->MakeHalfFloats(src, bytes_per_row / 2, dst);
On 2016/10/21 22:46:06, fbarchard1 wrote:
> libyuv:HalfFloatPlane has some overhead to detect cpu/alignment.  Suggest
> calling it once per plane with width, height and stride.
> If the stride == width * 2 it will row coalesce into a single call.

Seems reasonable, but I would prefer to fix that after we move the
offset/multiplier decisions into libyuv.

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.h (right):

https://codereview.chromium.org/2444463002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.h:90: // The numbers stored in |dst| will be
half floats in range 0.0..1.0
On 2016/10/21 22:46:06, fbarchard1 wrote:
> This comment needs update?

Will fix in separate cl (Since this one is checked in now)