Call libyuv for short->half-float conversion

Call libyuv for short->half-float conversion

Libyuv has a more optimized version of the half-float conversion code now, and it
will probably become more optimized in the future.

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

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

[hubbe, 2016-10-03 21:45:39]

Description was changed from

==========
Call libyuv for short->half-float conversion

Libyuv has a more optimized version of the half-float conversion code now, and
it
will probably become more optimized in the future.

BUG=445071
==========

to

==========
Call libyuv for short->half-float conversion

Libyuv has a more optimized version of the half-float conversion code now, and
it
will probably become more optimized in the future.

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

[hubbe, 2016-10-03 21:45:44]

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

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

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-10-03 22:04:57]

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

[commit-bot: I haz the power, 2016-10-03 22:04:58]

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

[hubbe, 2016-10-03 22:22:58]

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

[commit-bot: I haz the power, 2016-10-03 22:23:47]

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

[hubbe, 2016-10-03 22:28:03]

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

[danakj, 2016-10-03 22:48:15]

LGTM

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:302: libyuv::HalfFloatPlane(src, 0, dst,
0, 1.0f / ((1 << bits_per_channel) - 1),
Can you use a temp var to give "1.0f / ((1 << bits_per_channel) - 1)" a name?
Same for the "1" at the end.. maybe for all the literals here.

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:487: if
(!plane_resource.Matches(video_frame->unique_id(), i)) {
I'm starting to wonder if everything inside this if statement belongs in media/
code, dep'ing libraries for this really makes me think.

[hubbe, 2016-10-03 22:58:15]

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

[commit-bot: I haz the power, 2016-10-03 22:59:01]

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

[hubbe, 2016-10-03 23:30:49]

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:302: libyuv::HalfFloatPlane(src, 0, dst,
0, 1.0f / ((1 << bits_per_channel) - 1),
On 2016/10/03 22:48:15, danakj wrote:
> Can you use a temp var to give "1.0f / ((1 << bits_per_channel) - 1)" a name?
> Same for the "1" at the end.. maybe for all the literals here.

Better?

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:487: if
(!plane_resource.Matches(video_frame->unique_id(), i)) {
On 2016/10/03 22:48:15, danakj wrote:
> I'm starting to wonder if everything inside this if statement belongs in
media/
> code, dep'ing libraries for this really makes me think.

Agreed, although I'm not sure exactly what that would look like.
It might be best if we could just arrange it so that all texture
uploads happens before we get to this point. That is what happens
in most cases anyways. (Most video texture uploads is done in
src/media/video/gpu_memory_buffer_video_frame_pool.cc)  Unfortunately
gpu memory buffers can't easily support half-float textures, however, there is a
separate CL that uses RG_88 textures instead of half-floats, and it that
happens, we should be able to move high-bit uploads to
gpu_memory_buffer_video_frame_pool.cc as well, and then maybe we can just scrap
most of this code....

[danakj, 2016-10-03 23:47:44]

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:302: libyuv::HalfFloatPlane(src, 0, dst,
0, 1.0f / ((1 << bits_per_channel) - 1),
On 2016/10/03 23:30:49, hubbe wrote:
> On 2016/10/03 22:48:15, danakj wrote:
> > Can you use a temp var to give "1.0f / ((1 << bits_per_channel) - 1)" a
name?
> > Same for the "1" at the end.. maybe for all the literals here.
> 
> Better?

Much, thanks.

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:487: if
(!plane_resource.Matches(video_frame->unique_id(), i)) {
On 2016/10/03 23:30:49, hubbe wrote:
> On 2016/10/03 22:48:15, danakj wrote:
> > I'm starting to wonder if everything inside this if statement belongs in
> media/
> > code, dep'ing libraries for this really makes me think.
> 
> Agreed, although I'm not sure exactly what that would look like.
> It might be best if we could just arrange it so that all texture
> uploads happens before we get to this point. That is what happens
> in most cases anyways. (Most video texture uploads is done in
> src/media/video/gpu_memory_buffer_video_frame_pool.cc)  Unfortunately
> gpu memory buffers can't easily support half-float textures, however, there is
a
> separate CL that uses RG_88 textures instead of half-floats, and it that
> happens, we should be able to move high-bit uploads to
> gpu_memory_buffer_video_frame_pool.cc as well, and then maybe we can just
scrap
> most of this code....

Yeh, I don't know that it belongs on VideoFrame. Doing it before making a
VideoFrame could work but this code depends on using the compositor context atm.

I was thinking just the code that makes a bitmap in the correct format for
upload, ie the code producing a pixels pointer, but not the upload call at the
end, would be a somewhat easy-to-do thing.

https://codereview.chromium.org/2389003002/diff/40001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/40001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:307: // Copy one row.
this comment's redundant i think, the var says the same. the others are nice as
they explain why the value is what it is.

[hubbe, 2016-10-03 23:56:35]

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:302: libyuv::HalfFloatPlane(src, 0, dst,
0, 1.0f / ((1 << bits_per_channel) - 1),
On 2016/10/03 23:47:43, danakj wrote:
> On 2016/10/03 23:30:49, hubbe wrote:
> > On 2016/10/03 22:48:15, danakj wrote:
> > > Can you use a temp var to give "1.0f / ((1 << bits_per_channel) - 1)" a
> name?
> > > Same for the "1" at the end.. maybe for all the literals here.
> > 
> > Better?
> 
> Much, thanks.

Acknowledged.

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:487: if
(!plane_resource.Matches(video_frame->unique_id(), i)) {
On 2016/10/03 23:47:44, danakj wrote:
> On 2016/10/03 23:30:49, hubbe wrote:
> > On 2016/10/03 22:48:15, danakj wrote:
> > > I'm starting to wonder if everything inside this if statement belongs in
> > media/
> > > code, dep'ing libraries for this really makes me think.
> > 
> > Agreed, although I'm not sure exactly what that would look like.
> > It might be best if we could just arrange it so that all texture
> > uploads happens before we get to this point. That is what happens
> > in most cases anyways. (Most video texture uploads is done in
> > src/media/video/gpu_memory_buffer_video_frame_pool.cc)  Unfortunately
> > gpu memory buffers can't easily support half-float textures, however, there
is
> a
> > separate CL that uses RG_88 textures instead of half-floats, and it that
> > happens, we should be able to move high-bit uploads to
> > gpu_memory_buffer_video_frame_pool.cc as well, and then maybe we can just
> scrap
> > most of this code....
> 
> Yeh, I don't know that it belongs on VideoFrame. Doing it before making a
> VideoFrame could work but this code depends on using the compositor context
atm.
> 
> I was thinking just the code that makes a bitmap in the correct format for
> upload, ie the code producing a pixels pointer, but not the upload call at the
> end, would be a somewhat easy-to-do thing.

All true, but probably better resolved in a separate CL.
Also, I would prefer to do a little wait-and-see on the RG88 stuff
before I start cleaning this stuff up, as that sort of determines
the right way to clean this up.

Is it ok if I just add a TODO for now?

https://codereview.chromium.org/2389003002/diff/40001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/40001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:307: // Copy one row.
On 2016/10/03 23:47:44, danakj wrote:
> this comment's redundant i think, the var says the same. the others are nice
as
> they explain why the value is what it is.

Done.

[danakj, 2016-10-03 23:58:56]

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2389003002/diff/20001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:487: if
(!plane_resource.Matches(video_frame->unique_id(), i)) {
On 2016/10/03 23:56:35, hubbe wrote:
> On 2016/10/03 23:47:44, danakj wrote:
> > On 2016/10/03 23:30:49, hubbe wrote:
> > > On 2016/10/03 22:48:15, danakj wrote:
> > > > I'm starting to wonder if everything inside this if statement belongs in
> > > media/
> > > > code, dep'ing libraries for this really makes me think.
> > > 
> > > Agreed, although I'm not sure exactly what that would look like.
> > > It might be best if we could just arrange it so that all texture
> > > uploads happens before we get to this point. That is what happens
> > > in most cases anyways. (Most video texture uploads is done in
> > > src/media/video/gpu_memory_buffer_video_frame_pool.cc)  Unfortunately
> > > gpu memory buffers can't easily support half-float textures, however,
there
> is
> > a
> > > separate CL that uses RG_88 textures instead of half-floats, and it that
> > > happens, we should be able to move high-bit uploads to
> > > gpu_memory_buffer_video_frame_pool.cc as well, and then maybe we can just
> > scrap
> > > most of this code....
> > 
> > Yeh, I don't know that it belongs on VideoFrame. Doing it before making a
> > VideoFrame could work but this code depends on using the compositor context
> atm.
> > 
> > I was thinking just the code that makes a bitmap in the correct format for
> > upload, ie the code producing a pixels pointer, but not the upload call at
the
> > end, would be a somewhat easy-to-do thing.
> 
> All true, but probably better resolved in a separate CL.
> Also, I would prefer to do a little wait-and-see on the RG88 stuff
> before I start cleaning this stuff up, as that sort of determines
> the right way to clean this up.
> 
> Is it ok if I just add a TODO for now?

A TODO is more than I was expecting for this CL, that'd be wonderful. Thanks!

[hubbe, 2016-10-04 00:00:14]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-10-04 00:00:14]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2389003002/#ps80001
(title: "todo added")

[commit-bot: I haz the power, 2016-10-04 00:01:30]

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-04 01:35:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-04 01:37:43]

Description was changed from

==========
Call libyuv for short->half-float conversion

Libyuv has a more optimized version of the half-float conversion code now, and
it
will probably become more optimized in the future.

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

to

==========
Call libyuv for short->half-float conversion

Libyuv has a more optimized version of the half-float conversion code now, and
it
will probably become more optimized in the future.

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

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

[commit-bot: I haz the power, 2016-10-04 01:37:44]

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