Move gl_helper to content/browser/compositor

Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384
Committed: https://crrev.com/919cc1e06ee6e0a4ad1a217e03d5f7b912383831
Cr-Commit-Position: refs/heads/master@{#381651}

[Fady Samuel, 2016-03-15 15:15:36]

Description was changed from

==========
[WIP] Move gl_helper to content/browser/compositor

BUG=
==========

to

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper{.(h|cc)} depends on media.

It looks like it's only used by content/browser/compositor anyway, so I moved it
there.

BUG=586384
==========

[Fady Samuel, 2016-03-15 15:16:08]

Description was changed from

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper{.(h|cc)} depends on media.

It looks like it's only used by content/browser/compositor anyway, so I moved it
there.

BUG=586384
==========

to

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384
==========

[Fady Samuel, 2016-03-15 15:16:18]

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

[commit-bot: I haz the power, 2016-03-15 15:16:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/1

[Fady Samuel, 2016-03-15 15:17:51]

fsamuel@chromium.org changed reviewers:
+ piman@chromium.org

[Fady Samuel, 2016-03-15 15:17:52]

How do you feel about this refactor Antoine? gl_helper lives in
content/common/gpu/client but depends on media and is only used by
content/browser/compositor so I moved it content/browser/compositor.

WDYT? Does this move make sense or shall I place this elsewhere. I'd like for
content/common/gpu/client code to not depend on media (other than media specific
files which I'll move to a subdirectory in the interim).

[commit-bot: I haz the power, 2016-03-15 15:25:00]

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

[commit-bot: I haz the power, 2016-03-15 15:25:01]

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

[Fady Samuel, 2016-03-15 15:30:22]

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

[commit-bot: I haz the power, 2016-03-15 15:30:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/20001

[commit-bot: I haz the power, 2016-03-15 15:39:19]

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

[commit-bot: I haz the power, 2016-03-15 15:39:20]

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

[Fady Samuel, 2016-03-15 16:13:56]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/40001

[commit-bot: I haz the power, 2016-03-15 16:23:29]

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

[commit-bot: I haz the power, 2016-03-15 16:23:30]

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

[Fady Samuel, 2016-03-15 16:58:13]

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

[commit-bot: I haz the power, 2016-03-15 16:58:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/60001

[commit-bot: I haz the power, 2016-03-15 17:07:52]

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

[commit-bot: I haz the power, 2016-03-15 17:07:53]

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

[Fady Samuel, 2016-03-15 17:19:17]

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

[commit-bot: I haz the power, 2016-03-15 17:19:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/80001

[commit-bot: I haz the power, 2016-03-15 17:30:43]

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

[commit-bot: I haz the power, 2016-03-15 17:30:44]

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

[danakj, 2016-03-15 18:39:20]

On Tue, Mar 15, 2016 at 8:17 AM, <fsamuel@chromium.org> wrote:

> Reviewers: piman
> CL: https://codereview.chromium.org/1802383002/
>
> Message:
> How do you feel about this refactor Antoine? gl_helper lives in
> content/common/gpu/client but depends on media and is only used by
> content/browser/compositor so I moved it content/browser/compositor.
>
> WDYT? Does this move make sense or shall I place this elsewhere. I'd like
> for
> content/common/gpu/client code to not depend on media (other than media
> specific
> files which I'll move to a subdirectory in the interim).
>
>
> Description:
> Move gl_helper to content/browser/compositor
>
> content/common/gpu/client/gl_helper.{h|cc} depends on media.
>
> It looks like it's only used by content/browser/compositor
> anyway, so I moved it there.
>

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

Also, much of content/browser/compositor is going to move to a components/
what then? I thought there was a plan to move this class under GPU
somewhere, or no?


>
> BUG=586384
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+448, -6384 lines):
> M content/browser/compositor/buffer_queue.cc
> M content/browser/compositor/buffer_queue_unittest.cc
> M content/browser/compositor/delegated_frame_host.cc
> A + content/browser/compositor/gl_helper.h
> A + content/browser/compositor/gl_helper.cc
> A + content/browser/compositor/gl_helper_benchmark.cc
> A + content/browser/compositor/gl_helper_readback_support.h
> A + content/browser/compositor/gl_helper_readback_support.cc
> A + content/browser/compositor/gl_helper_scaling.h
> A + content/browser/compositor/gl_helper_scaling.cc
> A + content/browser/compositor/gl_helper_unittest.cc
> M content/browser/compositor/gpu_process_transport_factory.cc
> M
>
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
> M content/browser/compositor/owned_mailbox.cc
> M content/browser/compositor/reflector_texture.cc
> M content/browser/compositor/surface_utils.cc
> M content/browser/compositor/test/no_transport_image_transport_factory.cc
> M content/browser/media/capture/aura_window_capture_machine.cc
> M content/browser/renderer_host/media/video_capture_controller.cc
> M content/browser/renderer_host/render_widget_host_view_aura.cc
> M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
> D content/common/gpu/client/gl_helper.h
> D content/common/gpu/client/gl_helper.cc
> D content/common/gpu/client/gl_helper_benchmark.cc
> D content/common/gpu/client/gl_helper_readback_support.h
> D content/common/gpu/client/gl_helper_readback_support.cc
> D content/common/gpu/client/gl_helper_scaling.h
> D content/common/gpu/client/gl_helper_scaling.cc
> D content/common/gpu/client/gl_helper_unittest.cc
> M content/content_browser.gypi
> M content/content_common.gypi
> M content/renderer/media/renderer_gpu_video_accelerator_factories.cc
>
>
>

-- 
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.

[Fady Samuel, 2016-03-15 18:55:56]

fsamuel@chromium.org changed reviewers:
+ sievers@chromium.org

[Fady Samuel, 2016-03-15 18:55:57]

+sievers@ since Antoine is OOO I think..

How do you feel about this move of gl_helper? Is there a better home for this
file? This doesn't seem like it should be in content/common/gpu given its only
used by browser.

Certainly, this file should not be in gpu/ipc. Thoughts?

[no sievers, 2016-03-15 20:43:40]

On 2016/03/15 18:55:57, Fady Samuel wrote:
> +sievers@ since Antoine is OOO I think..
> 
> How do you feel about this move of gl_helper? Is there a better home for this
> file? This doesn't seem like it should be in content/common/gpu given its only
> used by browser.
> 
> Certainly, this file should not be in gpu/ipc. Thoughts?

We could also make GLHelper a component on its own (and not allow it to depend
on content since it doesn't need to).
If you move it to gpu/ (let's say gpu/gl_helper or something) we'd want to at
least do something about the media/ dependency, and maybe skia (I think both
would be somewhat straightforward).

The one thing that's a bit odd with moving it to content/browser/compositor is
that the code there is more specific to Aura and not used on Android (and only
partially on Mac I believe).

[piman, 2016-03-16 23:23:25]

On Tue, Mar 15, 2016 at 1:43 PM, <sievers@chromium.org> wrote:

> On 2016/03/15 18:55:57, Fady Samuel wrote:
> > +sievers@ since Antoine is OOO I think..
> >
> > How do you feel about this move of gl_helper? Is there a better home for
> this
> > file? This doesn't seem like it should be in content/common/gpu given
> its only
> > used by browser.
> >
> > Certainly, this file should not be in gpu/ipc. Thoughts?
>
> We could also make GLHelper a component on its own (and not allow it to
> depend
> on content since it doesn't need to).
> If you move it to gpu/ (let's say gpu/gl_helper or something) we'd want to
> at
> least do something about the media/ dependency, and maybe skia (I think
> both
> would be somewhat straightforward).
>
> The one thing that's a bit odd with moving it to
> content/browser/compositor is
> that the code there is more specific to Aura and not used on Android (and
> only
> partially on Mac I believe).
>
> https://codereview.chromium.org/1802383002/
>

There's no great answer.
I think at some point in the past I would have suggested to move it to
gpu/, but the bulk of the code now is fairly specific to our use in
content/. Teasing out the media dependency should be fairly easy if that's
the main problem (we can keep a wrapper in content/ that unpacks the
VideoFrame's planes). The skia dependency would be harder - we don't have a
replacement for SkRegion operations. We can always slice-and-dice, GLHelper
is a grab bag of useful functionality, but they don't all have to be in the
same place. There's never a great place for these things.

This functionality will eventually be necessary for mus to reach feature
parity, so componentization sounds reasonable. But it could go
along content/browser/compositor when we do that - it'll need it anyway, I
think?

Agreed that Android doesn't really use content/browser/compositor, though
there's a few bits and pieces (e.g. surface_utils).

IOW, Daniel, do you feel strongly?

Antoine

-- 
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.

[danakj, 2016-03-16 23:27:47]

On Wed, Mar 16, 2016 at 4:23 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Mar 15, 2016 at 1:43 PM, <sievers@chromium.org> wrote:
>
>> On 2016/03/15 18:55:57, Fady Samuel wrote:
>> > +sievers@ since Antoine is OOO I think..
>> >
>> > How do you feel about this move of gl_helper? Is there a better home
>> for this
>> > file? This doesn't seem like it should be in content/common/gpu given
>> its only
>> > used by browser.
>> >
>> > Certainly, this file should not be in gpu/ipc. Thoughts?
>>
>> We could also make GLHelper a component on its own (and not allow it to
>> depend
>> on content since it doesn't need to).
>> If you move it to gpu/ (let's say gpu/gl_helper or something) we'd want
>> to at
>> least do something about the media/ dependency, and maybe skia (I think
>> both
>> would be somewhat straightforward).
>>
>> The one thing that's a bit odd with moving it to
>> content/browser/compositor is
>> that the code there is more specific to Aura and not used on Android (and
>> only
>> partially on Mac I believe).
>>
>> https://codereview.chromium.org/1802383002/
>>
>
> There's no great answer.
> I think at some point in the past I would have suggested to move it to
> gpu/, but the bulk of the code now is fairly specific to our use in
> content/. Teasing out the media dependency should be fairly easy if that's
> the main problem (we can keep a wrapper in content/ that unpacks the
> VideoFrame's planes). The skia dependency would be harder - we don't have a
> replacement for SkRegion operations. We can
>

We should move cc::Region to gfx::Region (I want to use that in Blink too
similar to other gfx types).


> always slice-and-dice, GLHelper is a grab bag of useful functionality, but
> they don't all have to be in the same place. There's never a great place
> for these things.
>
> This functionality will eventually be necessary for mus to reach feature
> parity, so componentization sounds reasonable. But it could go
> along content/browser/compositor when we do that - it'll need it anyway, I
> think?
>
> Agreed that Android doesn't really use content/browser/compositor, though
> there's a few bits and pieces (e.g. surface_utils).
>
> IOW, Daniel, do you feel strongly?
>
> Antoine
>

-- 
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.

[no sievers, 2016-03-16 23:30:30]

> > IOW, Daniel, do you feel strongly?
> >


nope, agreed, let's do content/browser/compositor then where gl helper probably
fits best for now. (iirc there's some other overlay thingie that gets
selectively included for android from that folder too.)

[Fady Samuel, 2016-03-16 23:39:30]

On 2016/03/16 23:30:30, sievers (slow) wrote:
> > > IOW, Daniel, do you feel strongly?
> > >
> 
> 
> nope, agreed, let's do content/browser/compositor then where gl helper
probably
> fits best for now. (iirc there's some other overlay thingie that gets
> selectively included for android from that folder too.)

Does that mean I can get an L-G-T-M from someone? :-)

[piman, 2016-03-16 23:47:22]

lgtm

[Fady Samuel, 2016-03-16 23:51:29]

On 2016/03/16 23:47:22, piman wrote:
> lgtm

Thanks!

[Fady Samuel, 2016-03-16 23:51:40]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-03-16 23:52:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/80001

[commit-bot: I haz the power, 2016-03-17 00:03:22]

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

[commit-bot: I haz the power, 2016-03-17 00:03:24]

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

[Fady Samuel, 2016-03-17 01:30:13]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-03-17 01:30:14]

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/1802383002/#ps100001
(title: "Replace printf with VLOG")

[commit-bot: I haz the power, 2016-03-17 01:30:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/100001

[commit-bot: I haz the power, 2016-03-17 01:33:34]

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

[commit-bot: I haz the power, 2016-03-17 01:33:35]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2016-03-17 01:53:24]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-03-17 01:53:25]

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/1802383002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-17 01:53:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802383002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802383002/120001

[commit-bot: I haz the power, 2016-03-17 03:00:26]

Description was changed from

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384
==========

to

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384
==========

[commit-bot: I haz the power, 2016-03-17 03:00:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-17 03:01:25]

Description was changed from

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384
==========

to

==========
Move gl_helper to content/browser/compositor

content/common/gpu/client/gl_helper.{h|cc} depends on media.

It looks like it's only used by content/browser/compositor
anyway, so I moved it there.

BUG=586384

Committed: https://crrev.com/919cc1e06ee6e0a4ad1a217e03d5f7b912383831
Cr-Commit-Position: refs/heads/master@{#381651}
==========

[commit-bot: I haz the power, 2016-03-17 03:01:27]

Patchset 7 (id:??) landed as
https://crrev.com/919cc1e06ee6e0a4ad1a217e03d5f7b912383831
Cr-Commit-Position: refs/heads/master@{#381651}