Create FP16 LUTs when needed

Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2648043002
Cr-Commit-Position: refs/heads/master@{#446766}
Committed: https://chromium.googlesource.com/chromium/src/+/b97f42dae25f0481f687e442b79cb3c774391b5d

[hubbe, 2017-01-20 23:32:46]

Description was changed from

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
==========

to

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[hubbe, 2017-01-20 23:32:50]

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

[commit-bot: I haz the power, 2017-01-20 23:33:23]

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

[hubbe, 2017-01-20 23:33:30]

hubbe@chromium.org changed reviewers:
+ ccameron@chromium.org

[commit-bot: I haz the power, 2017-01-21 00:35:11]

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

[commit-bot: I haz the power, 2017-01-21 00:35:12]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-01-21 01:10:49]

Creating 16-bit floating point textures doesn't require conversion CPU-side.
PTAL at the suggested change. With that, we also don't need to templatize (we
just specify GL_RGBA versus GL_RGBA16F to switch between the modes).

I should reiterate, especially for HDR, I believe that analytical color
conversion is a more appropriate solution. In the analysis that I did in [1], I
was using an 8-bit LUT -- the cost of a 37-tap 16-bit LUT is higher than
anything tested.

[1]
https://docs.google.com/document/d/1lTyFs7_BtBAdL0ChoUtq1RVmP_1boGcnpqiNuNY5U...

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.cc
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.c...
cc/output/color_lut_cache.cc:173: gl_->TexParameteri(GL_TEXTURE_2D,
GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
To use 16-bit floating point, the upload data doesn't need to be 16-bit floating
point (in fact, I'm a bit surprised that this worked).

The easier way to do this is to do:

  // Create storage in the requested format. Note that the last 3
  // args are irrelevant, because the "data" ptr is nullptr
  TexImage2D(
      GL_TEXTURE_2D, 0, GL_RGBA16F, width, height, 0,
      GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
  // Populate the storage with 32-bit floating point data. This will
  // convert from 32-bit floating point to 16-bit floating point in
  // the DMA engine.
  TexSubImage2D(
      GL_TEXTURE_2D, 0, 0, 0, width, height,
      GL_RGBA, GL_FLOAT, data)

I think you can also use TexStorage2DEXT instead of TexImage2D to create the
texture storage.

[chromium-reviews, 2017-01-21 08:44:30]

On Fri, Jan 20, 2017 at 5:10 PM, <ccameron@chromium.org> wrote:

> Creating 16-bit floating point textures doesn't require conversion
> CPU-side.
> PTAL at the suggested change. With that, we also don't need to templatize
> (we
> just specify GL_RGBA versus GL_RGBA16F to switch between the modes).
>
>
Some of drivers cannot do conversion in dma, and switch to REALLY
slow uploads when they have to do conversion. (Because the conversion
engine is
generic and can convert from anything to anything.)  It might not matter
much here,
but I know that when you upload/download videoframes, you absolutely have
to match
the format.  (On older intel drivers, using GL_RGBA instead of GL_BGRA_EXT
results
in a 10x slowdown when doing glReadPixels if memory serves...)


> I should reiterate, especially for HDR, I believe that analytical color
> conversion is a more appropriate solution. In the analysis that I did in
> [1], I
> was using an 8-bit LUT -- the cost of a 37-tap 16-bit LUT is higher than
> anything tested.
>
> [1]
> https://docs.google.com/document/d/1lTyFs7_BtBAdL0ChoUtq1RVmP_
> 1boGcnpqiNuNY5Ugk/edit
>
>
>
I can implement an analytical solution I suppose.
I tend to view it as an optimization though, and the LUT is the fallback,
so I go for that first.
It would be different if we could do everything analytically and just drop
the LUT completely.
This would be easy if it wasn't for ICC profiles....

Anyways, HDR is currently only relevant on platforms where performance is
not much of
an issue. (Win10 machines with recent graphics cards.)

So, if you don't mind I'll implement the analytical version in a separate
CL?


>
> https://codereview.chromium.org/2648043002/diff/1/cc/
> output/color_lut_cache.cc
> File cc/output/color_lut_cache.cc (right):
>
> https://codereview.chromium.org/2648043002/diff/1/cc/
> output/color_lut_cache.cc#newcode173
> cc/output/color_lut_cache.cc:173: gl_->TexParameteri(GL_TEXTURE_2D,
> GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> To use 16-bit floating point, the upload data doesn't need to be 16-bit
> floating point (in fact, I'm a bit surprised that this worked).
>
>
Last I checked, this was the only way to assure that the upload would be
fast.


> The easier way to do this is to do:
>
> // Create storage in the requested format. Note that the last 3
> // args are irrelevant, because the "data" ptr is nullptr
> TexImage2D(
> GL_TEXTURE_2D, 0, GL_RGBA16F, width, height, 0,
> GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
> // Populate the storage with 32-bit floating point data. This will
> // convert from 32-bit floating point to 16-bit floating point in
> // the DMA engine.
> TexSubImage2D(
> GL_TEXTURE_2D, 0, 0, 0, width, height,
> GL_RGBA, GL_FLOAT, data)
>
> I think you can also use TexStorage2DEXT instead of TexImage2D to create
> the texture storage.
>
>
What's the advantage?


> https://codereview.chromium.org/2648043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ccameron, 2017-01-21 19:49:20]

WRT analytic implementation...

No problem. Actually I'm working to add analytic representations to the shaders
(work-in-progress at https://codereview.chromium.org/2645953007/), so it's
better we don't step on each others' toes with getting that initially checked
in.

WRT performance b/c of glReadPixels performance... 

I wouldn't generalize performance observations from glReadPixels to
glTexSubImage. glTexSubImage is generally a very fast path in drivers (every
application uses it), while glReadPixels is a slow-and-rarely-used path. E.g,
glReadPixels has an implicit glFinish, which makes using it not-an-option for
any performant applications. E.g, we've hit lots of glReadPixels bugs (basically
anything but 8-bit RGBA is iffy).

WRT performance benefits of uploading half-float instead of GL_FLOAT...

What we're looking at here is a one-time cost (over the lifetime of playing a
video), so even if there exists a difference, it would have to be substantial to
matter more than readability and maintainability.

The argument in favor (a generalization from past-observed behavior of
glReadPixels) isn't compelling enough for me.

WRT the drawbacks of the code complexity...

In my weighing of code complexity versus optimization, it is not okay to
copy-paste a hundred-line-long block of code from third_party/qcms/ into cc/ for
a performance benefit that has not been measured.

Please consider an implementation that uploads using GL_FLOAT.

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.cc
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.c...
cc/output/color_lut_cache.cc:40: unsigned short baseTable[512] = {
See the source of this block,
https://cs.chromium.org/chromium/src/third_party/qcms/src/halffloat.h?rcl=0&l=27

[hubbe, 2017-01-22 05:10:35]

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

[commit-bot: I haz the power, 2017-01-22 05:10:46]

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

[hubbe, 2017-01-22 05:12:39]

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.cc
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.c...
cc/output/color_lut_cache.cc:40: unsigned short baseTable[512] = {
On 2017/01/21 19:49:20, ccameron wrote:
> See the source of this block,
>
https://cs.chromium.org/chromium/src/third_party/qcms/src/halffloat.h?rcl=0&l=27

That's not actually where I copied it from.
Anywho, gone now.

https://codereview.chromium.org/2648043002/diff/1/cc/output/color_lut_cache.c...
cc/output/color_lut_cache.cc:173: gl_->TexParameteri(GL_TEXTURE_2D,
GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
On 2017/01/21 01:10:49, ccameron wrote:
> To use 16-bit floating point, the upload data doesn't need to be 16-bit
floating
> point (in fact, I'm a bit surprised that this worked).
> 
> The easier way to do this is to do:
> 
>   // Create storage in the requested format. Note that the last 3
>   // args are irrelevant, because the "data" ptr is nullptr
>   TexImage2D(
>       GL_TEXTURE_2D, 0, GL_RGBA16F, width, height, 0,
>       GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
>   // Populate the storage with 32-bit floating point data. This will
>   // convert from 32-bit floating point to 16-bit floating point in
>   // the DMA engine.
>   TexSubImage2D(
>       GL_TEXTURE_2D, 0, 0, 0, width, height,
>       GL_RGBA, GL_FLOAT, data)
> 
> I think you can also use TexStorage2DEXT instead of TexImage2D to create the
> texture storage.

Done.

[commit-bot: I haz the power, 2017-01-22 06:09:17]

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

[commit-bot: I haz the power, 2017-01-22 06:09:18]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-01-23 18:30:26]

lgtm % nit + suggestion

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:41: int data_type) {
Small nit: should be "GLenum type" instead of "int data_type"

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0, GL_RGBA,
data_type, nullptr);
So I'm not 100% sure what exactly triggers use of 16-bit floating point in
glTexImage2D (it might be the data type, or it might be internalformat) -- I
think the spec is intentionally vague.

The extension which isn't vague is
https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt

which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width, height).

[sorry, should have researched the precise call on the last patch]

[hubbe, 2017-01-23 18:44:57]

On 2017/01/23 18:30:26, ccameron wrote:
> lgtm % nit + suggestion
> 
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> File cc/output/color_lut_cache.cc (right):
> 
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> cc/output/color_lut_cache.cc:41: int data_type) {
> Small nit: should be "GLenum type" instead of "int data_type"
> 
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0, GL_RGBA,
> data_type, nullptr);
> So I'm not 100% sure what exactly triggers use of 16-bit floating point in
> glTexImage2D (it might be the data type, or it might be internalformat) -- I
> think the spec is intentionally vague.
> 
> The extension which isn't vague is
> https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt
> 
> which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width, height).
> 
> [sorry, should have researched the precise call on the last patch]

Hmm, I don't think the whole "dma engine does the conversion" thing is going to
pan out:

[20933:20933:0123/104355.536939:ERROR:texture_manager.cc(2738)]
[.DisplayCompositor-0x2b0d1b4ed920]GL ERROR :GL_INVALID_OPERATION :
glTexSubImage2D: type does not match type of texture.

[ccameron, 2017-01-23 20:33:07]

Description was changed from

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-01-23 20:45:00]

On 2017/01/23 18:44:57, hubbe wrote:
> On 2017/01/23 18:30:26, ccameron wrote:
> > lgtm % nit + suggestion
> > 
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > File cc/output/color_lut_cache.cc (right):
> > 
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > cc/output/color_lut_cache.cc:41: int data_type) {
> > Small nit: should be "GLenum type" instead of "int data_type"
> > 
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0, GL_RGBA,
> > data_type, nullptr);
> > So I'm not 100% sure what exactly triggers use of 16-bit floating point in
> > glTexImage2D (it might be the data type, or it might be internalformat) -- I
> > think the spec is intentionally vague.
> > 
> > The extension which isn't vague is
> > https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt
> > 
> > which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width,
height).
> > 
> > [sorry, should have researched the precise call on the last patch]
> 
> Hmm, I don't think the whole "dma engine does the conversion" thing is going
to
> pan out:
> 
> [20933:20933:0123/104355.536939:ERROR:texture_manager.cc(2738)]
> [.DisplayCompositor-0x2b0d1b4ed920]GL ERROR :GL_INVALID_OPERATION :
> glTexSubImage2D: type does not match type of texture.

Huh -- it does look as though GLES removed that functionality. Adding zmo to be
certain.
zmo: is it possible to upload GL_FLOAT data and have it be converted by GLES2 to
HALF_FLOAT? Or did they get rid of that?
If they did (which it seems), then, grumble, we'll have to add that conversion.

The objectionable part of the conversion isn't so much doing the conversion as
the the big-block-o-float-to-half-float code and template-ization.
So, if we move the code block in [1] to base/ or ui/gfx/, then I'm fine with
doing the conversion.
[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[ccameron, 2017-01-23 20:49:48]

On 2017/01/23 20:45:00, ccameron wrote:
> On 2017/01/23 18:44:57, hubbe wrote:
> > On 2017/01/23 18:30:26, ccameron wrote:
> > > lgtm % nit + suggestion
> > > 
> > >
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > > File cc/output/color_lut_cache.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > > cc/output/color_lut_cache.cc:41: int data_type) {
> > > Small nit: should be "GLenum type" instead of "int data_type"
> > > 
> > >
> >
>
https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
> > > cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0, GL_RGBA,
> > > data_type, nullptr);
> > > So I'm not 100% sure what exactly triggers use of 16-bit floating point in
> > > glTexImage2D (it might be the data type, or it might be internalformat) --
I
> > > think the spec is intentionally vague.
> > > 
> > > The extension which isn't vague is
> > >
https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt
> > > 
> > > which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width,
> height).
> > > 
> > > [sorry, should have researched the precise call on the last patch]
> > 
> > Hmm, I don't think the whole "dma engine does the conversion" thing is going
> to
> > pan out:
> > 
> > [20933:20933:0123/104355.536939:ERROR:texture_manager.cc(2738)]
> > [.DisplayCompositor-0x2b0d1b4ed920]GL ERROR :GL_INVALID_OPERATION :
> > glTexSubImage2D: type does not match type of texture.
> 
> Huh -- it does look as though GLES removed that functionality. Adding zmo to
be
> certain.
> zmo: is it possible to upload GL_FLOAT data and have it be converted by GLES2
to
> HALF_FLOAT? Or did they get rid of that?
> If they did (which it seems), then, grumble, we'll have to add that
conversion.
> 
> The objectionable part of the conversion isn't so much doing the conversion as
> the the big-block-o-float-to-half-float code and template-ization.
> So, if we move the code block in [1] to base/ or ui/gfx/, then I'm fine with
> doing the conversion.
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

Also, in moving floatToHalfFloat to elsewhere, it'll probably be necessary to
write a
  void FloatToHalfFloat(const float* src, void* dst, size_t n)
to do a bunch of conversions at one -- places like [2] will need it to not slow
down when going over the library boundary

[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[chromium-reviews, 2017-01-23 20:56:40]

The "elsewhere" might be libyuv.
We already have a function which converts int16s to half-floats in there,
and float-to-half float could probably be added too.

    /Hubbe

On Mon, Jan 23, 2017 at 12:49 PM, <ccameron@chromium.org> wrote:

> On 2017/01/23 20:45:00, ccameron wrote:
> > On 2017/01/23 18:44:57, hubbe wrote:
> > > On 2017/01/23 18:30:26, ccameron wrote:
> > > > lgtm % nit + suggestion
> > > >
> > > >
> > >
> >
> https://codereview.chromium.org/2648043002/diff/20001/cc/
> output/color_lut_cache.cc
> > > > File cc/output/color_lut_cache.cc (right):
> > > >
> > > >
> > >
> >
> https://codereview.chromium.org/2648043002/diff/20001/cc/
> output/color_lut_cache.cc#newcode41
> > > > cc/output/color_lut_cache.cc:41: int data_type) {
> > > > Small nit: should be "GLenum type" instead of "int data_type"
> > > >
> > > >
> > >
> >
> https://codereview.chromium.org/2648043002/diff/20001/cc/
> output/color_lut_cache.cc#newcode79
> > > > cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0,
> GL_RGBA,
> > > > data_type, nullptr);
> > > > So I'm not 100% sure what exactly triggers use of 16-bit floating
> point in
> > > > glTexImage2D (it might be the data type, or it might be
> internalformat) --
> I
> > > > think the spec is intentionally vague.
> > > >
> > > > The extension which isn't vague is
> > > >
> https://www.khronos.org/registry/gles/extensions/EXT/
> EXT_texture_storage.txt
> > > >
> > > > which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width,
> > height).
> > > >
> > > > [sorry, should have researched the precise call on the last patch]
> > >
> > > Hmm, I don't think the whole "dma engine does the conversion" thing is
> going
> > to
> > > pan out:
> > >
> > > [20933:20933:0123/104355.536939:ERROR:texture_manager.cc(2738)]
> > > [.DisplayCompositor-0x2b0d1b4ed920]GL ERROR :GL_INVALID_OPERATION :
> > > glTexSubImage2D: type does not match type of texture.
> >
> > Huh -- it does look as though GLES removed that functionality. Adding
> zmo to
> be
> > certain.
> > zmo: is it possible to upload GL_FLOAT data and have it be converted by
> GLES2
> to
> > HALF_FLOAT? Or did they get rid of that?
> > If they did (which it seems), then, grumble, we'll have to add that
> conversion.
> >
> > The objectionable part of the conversion isn't so much doing the
> conversion as
> > the the big-block-o-float-to-half-float code and template-ization.
> > So, if we move the code block in [1] to base/ or ui/gfx/, then I'm fine
> with
> > doing the conversion.
> > [1]
> >
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> graphics/gpu/WebGLImageConversion.cpp?rcl=0&l=259
>
> Also, in moving floatToHalfFloat to elsewhere, it'll probably be necessary
> to
> write a
> void FloatToHalfFloat(const float* src, void* dst, size_t n)
> to do a bunch of conversions at one -- places like [2] will need it to not
> slow
> down when going over the library boundary
>
> [2]
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> graphics/gpu/WebGLImageConversion.cpp?rcl=1485173813&l=1299
>
>
>
> https://codereview.chromium.org/2648043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ccameron, 2017-01-23 21:02:31]

On 2017/01/23 20:56:40, chromium-reviews wrote:
> The "elsewhere" might be libyuv.
> We already have a function which converts int16s to half-floats in there,
> and float-to-half float could probably be added too.

Yeah, we also have it in QCMS and Skia (though Skia's algorithms are different
AFAICT), plus the WebGL link above.

In terms of dependency messes, I think that putting it in ui/gfx is probably the
path of least resistance (the source_set "color_space" in ui/gfx may be a good
example to copy -- should be even easier cause it doesn't involve including
Skia).

I know it's a bit of a pain (sorry!), but these things can't proliferate.

[chromium-reviews, 2017-01-23 21:29:32]

On Mon, Jan 23, 2017 at 1:02 PM, <ccameron@chromium.org> wrote:

> On 2017/01/23 20:56:40, chromium-reviews wrote:
> > The "elsewhere" might be libyuv.
> > We already have a function which converts int16s to half-floats in there,
> > and float-to-half float could probably be added too.
>
> Yeah, we also have it in QCMS and Skia (though Skia's algorithms are
> different
> AFAICT), plus the WebGL link above.
>
> In terms of dependency messes, I think that putting it in ui/gfx is
> probably the
> path of least resistance (the source_set "color_space" in ui/gfx may be a
> good
> example to copy -- should be even easier cause it doesn't involve including
> Skia).
>
> I know it's a bit of a pain (sorry!), but these things can't proliferate.
>
>
No need to apologize. I am all for doing things the right way, so when I'm
being lazy, PLEASE tell me to do it better!


> https://codereview.chromium.org/2648043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-01-23 21:30:30]

Oh, and I forgot:

What's wrong with using a template?

    /Hubbe

On Mon, Jan 23, 2017 at 1:29 PM, Fredrik Hubinette <hubbe@google.com> wrote:

>
>
> On Mon, Jan 23, 2017 at 1:02 PM, <ccameron@chromium.org> wrote:
>
>> On 2017/01/23 20:56:40, chromium-reviews wrote:
>> > The "elsewhere" might be libyuv.
>> > We already have a function which converts int16s to half-floats in
>> there,
>> > and float-to-half float could probably be added too.
>>
>> Yeah, we also have it in QCMS and Skia (though Skia's algorithms are
>> different
>> AFAICT), plus the WebGL link above.
>>
>> In terms of dependency messes, I think that putting it in ui/gfx is
>> probably the
>> path of least resistance (the source_set "color_space" in ui/gfx may be a
>> good
>> example to copy -- should be even easier cause it doesn't involve
>> including
>> Skia).
>>
>> I know it's a bit of a pain (sorry!), but these things can't proliferate.
>>
>>
> No need to apologize. I am all for doing things the right way, so when I'm
> being lazy, PLEASE tell me to do it better!
>
>
>> https://codereview.chromium.org/2648043002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-01-23 21:37:56]

It's too bad skia doesn't expose it's half-float functions, they look nice
:(

    /Hubbe

On Mon, Jan 23, 2017 at 1:30 PM, Fredrik Hubinette <hubbe@google.com> wrote:

> Oh, and I forgot:
>
> What's wrong with using a template?
>
>     /Hubbe
>
> On Mon, Jan 23, 2017 at 1:29 PM, Fredrik Hubinette <hubbe@google.com>
> wrote:
>
>>
>>
>> On Mon, Jan 23, 2017 at 1:02 PM, <ccameron@chromium.org> wrote:
>>
>>> On 2017/01/23 20:56:40, chromium-reviews wrote:
>>> > The "elsewhere" might be libyuv.
>>> > We already have a function which converts int16s to half-floats in
>>> there,
>>> > and float-to-half float could probably be added too.
>>>
>>> Yeah, we also have it in QCMS and Skia (though Skia's algorithms are
>>> different
>>> AFAICT), plus the WebGL link above.
>>>
>>> In terms of dependency messes, I think that putting it in ui/gfx is
>>> probably the
>>> path of least resistance (the source_set "color_space" in ui/gfx may be
>>> a good
>>> example to copy -- should be even easier cause it doesn't involve
>>> including
>>> Skia).
>>>
>>> I know it's a bit of a pain (sorry!), but these things can't proliferate.
>>>
>>>
>> No need to apologize. I am all for doing things the right way, so when
>> I'm being lazy, PLEASE tell me to do it better!
>>
>>
>>> https://codereview.chromium.org/2648043002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2017-01-24 18:32:17]

Broke out half-float conversion into it's own file.
I know you didn't like the templetizing before, but I'm not clear on why, so I
didn't change it.

PTAL.

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:41: int data_type) {
On 2017/01/23 18:30:25, ccameron wrote:
> Small nit: should be "GLenum type" instead of "int data_type"

Acknowledged.

https://codereview.chromium.org/2648043002/diff/20001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:79: lut_samples * lut_samples, 0, GL_RGBA,
data_type, nullptr);
On 2017/01/23 18:30:25, ccameron wrote:
> So I'm not 100% sure what exactly triggers use of 16-bit floating point in
> glTexImage2D (it might be the data type, or it might be internalformat) -- I
> think the spec is intentionally vague.
> 
> The extension which isn't vague is
> https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt
> 
> which would be TexStorage2D(GL_TEXTURE_2D, 0, GL_RGBA16F_EXT, width, height).
> 
> [sorry, should have researched the precise call on the last patch]

Seems clear enough to me that it's the data type that triggers it.
Since the extension isn't guaranteed to be available, I don't really see any
advantage to using it.

[ccameron, 2017-01-24 18:38:49]

lgtm, thanks

It would be great to merge
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
to use ui/gfx as well, but I can grab that in a second pass.

https://codereview.chromium.org/2648043002/diff/60001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/60001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:71: FloatToLUT(&one, &alpha, 1);
The preference against template-ization is that in principle uint16_t could be
16-bit fixed-point. But ... that's just a matter of personal preference.

https://codereview.chromium.org/2648043002/diff/60001/ui/gfx/half_float.h
File ui/gfx/half_float.h (right):

https://codereview.chromium.org/2648043002/diff/60001/ui/gfx/half_float.h#new...
ui/gfx/half_float.h:19: HalfFloat* output,
I'd vaguely prefer output to be a void*, but let me see how that goes when we
merge in the stuff at

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

that's the easy direction to change.

[hubbe, 2017-01-24 18:47:46]

https://codereview.chromium.org/2648043002/diff/60001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2648043002/diff/60001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:71: FloatToLUT(&one, &alpha, 1);
On 2017/01/24 18:38:49, ccameron wrote:
> The preference against template-ization is that in principle uint16_t could be
> 16-bit fixed-point. But ... that's just a matter of personal preference.

I thought about passing GL_UNSIGNED_BYTE / GL_HALF_FLOAT_OES as a second
argument to the template, decided that it was overkill.

https://codereview.chromium.org/2648043002/diff/60001/ui/gfx/half_float.h
File ui/gfx/half_float.h (right):

https://codereview.chromium.org/2648043002/diff/60001/ui/gfx/half_float.h#new...
ui/gfx/half_float.h:19: HalfFloat* output,
On 2017/01/24 18:38:49, ccameron wrote:
> I'd vaguely prefer output to be a void*, but let me see how that goes when we
> merge in the stuff at
> 

That doesn't sound like a good idea to me.
Any 2-byte data type would be better than void*.
(Like maybe struct HalfFloat { unsigned char data[2]; } )

>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> 
> that's the easy direction to change.

Yeah, I figured it would be better to fix that in a separate CL.

[hubbe, 2017-01-24 18:47:57]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2017-01-24 18:48:47]

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-01-24 19:44:07]

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

[commit-bot: I haz the power, 2017-01-24 19:44:08]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hubbe, 2017-01-27 18:50:55]

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

[commit-bot: I haz the power, 2017-01-27 18:51:34]

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, 2017-01-27 20:51:03]

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

[commit-bot: I haz the power, 2017-01-27 20:51:04]

Dry run: This issue passed the CQ dry run.

[hubbe, 2017-01-27 20:51:45]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2017-01-27 20:51:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2648043002/#ps80001
(title: "mark all floats as floats (not doubles)")

[commit-bot: I haz the power, 2017-01-27 20:52:36]

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-01-27 20:56:15]

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

[commit-bot: I haz the power, 2017-01-27 20:56:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2017-01-27 20:57:24]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2017-01-27 20:58:02]

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-01-27 21:04:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485550644583940,
"parent_rev": "e358e50b02e5dca9633b9a96291c659871b56fe5", "commit_rev":
"b97f42dae25f0481f687e442b79cb3c774391b5d"}

[commit-bot: I haz the power, 2017-01-27 21:05:22]

Description was changed from

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Create FP16 LUTs when needed

When maping HDR data to scRGB, the output can be outside of 0-1.
Using half-floats neatly solves this problem.
Also, we need the LUTs to be a bit larger in this case.

BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-01-27 21:05:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b97f42dae25f0481f687e442b79c...