cc: Hide Gpu Rasterization details in Resource Provider.

cc: Hide Gpu Rasterization details in Resource Provider.

This moves ganesh and gl context code out to resource
provider from gpurasterworkerpool to avoid making any
assumptions how the gpu raster buffers are implemented.

BUG=390399
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281225

[sohanjg, 2014-07-01 12:04:00]

I have tried to hide implementation details from GpuRasterWorkerPool, can you
please take a look.
Thanks.

[reveman, 2014-07-01 14:49:23]

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:377: // Hide Gpu Raster implementation details.
Please explain what these functions are used for instead of what you're trying
to do with this patch.

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:382: void FlushGpuContext();
I think we can come up with a better API than this. Please combine these two
pair of functions into one pair. And I think it'd be nice to expose it using a
"Scoped" class. ie. private API Begin/EndGpuRaster(), public API "class
ScopedGpuRaster".

[sohanjg, 2014-07-01 16:25:59]

Can you please take a look.
Thanks.

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:377: // Hide Gpu Raster implementation details.
On 2014/07/01 14:49:23, reveman wrote:
> Please explain what these functions are used for instead of what you're trying
> to do with this patch.

Done.

https://codereview.chromium.org/363563006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:382: void FlushGpuContext();
On 2014/07/01 14:49:23, reveman wrote:
> I think we can come up with a better API than this. Please combine these two
> pair of functions into one pair. And I think it'd be nice to expose it using a
> "Scoped" class. ie. private API Begin/EndGpuRaster(), public API "class
> ScopedGpuRaster".

Done.

[reveman, 2014-07-01 17:19:02]

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:190: // arguments even when tracing is
disabled.
this comment should be moved to the resource provider and where the code it
refers to is.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:192: resource_provider_,
"GpuRasterWorkerPool::RunTasksOnOriginThread");
I'm not sure how useful the "GpuRasterWorkerPool::RunTasksOnOriginThread" name
is here. Maybe we can remove the name parameter from ScopedGpuRaster ctor and
just use "GpuRasterization" from within the ResourceProvider implementation.
That would also provide a simple hint to look for in traces to determine if GPU
raster is used.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1240: return;
looks wrong. you still want to call PushGroupMarkerEXT.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1255: gr_context_->flush();
what if gr_context_ is NULL?

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:322: // This class is used to begin and end gpu
rasterization.
How about something like this instead:

// The following class is needed to modify GL resources using GPU
// raster. The user must ensure that they only use GPU raster on
// GL resources while an instance of this class is alive.

provides some more details for how this is supposed to be used. It would also be
nice to enforce this using assertions but that might be outside the scope of
this patch.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:330: void EndGpuRaster();
How about keeping these as private ResourceProvider methods instead? I think
that would be a bit more consistent with ScopedSamplerGL and a bit more flexible
in case we need to use this differently in the future.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:335: };
Please move this class above the Fence class so all Scoped* classes are grouped
together.

[sohanjg, 2014-07-02 07:40:37]

Can you please take a look.
Thanks.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:190: // arguments even when tracing is
disabled.
On 2014/07/01 17:19:01, reveman wrote:
> this comment should be moved to the resource provider and where the code it
> refers to is.

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:192: resource_provider_,
"GpuRasterWorkerPool::RunTasksOnOriginThread");
On 2014/07/01 17:19:01, reveman wrote:
> I'm not sure how useful the "GpuRasterWorkerPool::RunTasksOnOriginThread" name
> is here. Maybe we can remove the name parameter from ScopedGpuRaster ctor and
> just use "GpuRasterization" from within the ResourceProvider implementation.
> That would also provide a simple hint to look for in traces to determine if
GPU
> raster is used.

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1240: return;
On 2014/07/01 17:19:01, reveman wrote:
> looks wrong. you still want to call PushGroupMarkerEXT.

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1255: gr_context_->flush();
On 2014/07/01 17:19:01, reveman wrote:
> what if gr_context_ is NULL?

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:322: // This class is used to begin and end gpu
rasterization.
On 2014/07/01 17:19:01, reveman wrote:
> How about something like this instead:
> 
> // The following class is needed to modify GL resources using GPU
> // raster. The user must ensure that they only use GPU raster on
> // GL resources while an instance of this class is alive.
> 
> provides some more details for how this is supposed to be used. It would also
be
> nice to enforce this using assertions but that might be outside the scope of
> this patch.

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:330: void EndGpuRaster();
On 2014/07/01 17:19:01, reveman wrote:
> How about keeping these as private ResourceProvider methods instead? I think
> that would be a bit more consistent with ScopedSamplerGL and a bit more
flexible
> in case we need to use this differently in the future.

Done.

https://codereview.chromium.org/363563006/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:335: };
On 2014/07/01 17:19:01, reveman wrote:
> Please move this class above the Fence class so all Scoped* classes are
grouped
> together.

Done.

[reveman, 2014-07-02 14:06:18]

lgtm, thanks!

[sohanjg, 2014-07-02 17:07:11]

On 2014/07/02 14:06:18, reveman wrote:
> lgtm, thanks!

alokp@, are you ok with this change ?

[alokp, 2014-07-02 17:18:06]

On 2014/07/02 17:07:11, sohanjg wrote:
> On 2014/07/02 14:06:18, reveman wrote:
> > lgtm, thanks!
> 
> alokp@, are you ok with this change ?

looking...

[alokp, 2014-07-02 17:25:23]

On 2014/07/02 17:18:06, Alok Priyadarshi wrote:
> On 2014/07/02 17:07:11, sohanjg wrote:
> > On 2014/07/02 14:06:18, reveman wrote:
> > > lgtm, thanks!
> > 
> > alokp@, are you ok with this change ?
> 
> looking...

I personally do not like random things going into ResourceProvider. It should
just provide resources, not manage buffers for rasterization. That should be the
job of raster worker pools. I know this is not the case at present, but we
should be moving in the direction of simplifying it rather than putting more
stuff into it.

That said, I do not want to block this patch. It does not make anything worse. I
just do not like the trend towards making ResourceProvider do everything!

[sohanjg, 2014-07-02 17:46:21]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-07-02 17:47:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/363563006/40001

[danakj, 2014-07-02 18:37:45]

On Wed, Jul 2, 2014 at 1:25 PM, <alokp@chromium.org> wrote:

> On 2014/07/02 17:18:06, Alok Priyadarshi wrote:
>
>> On 2014/07/02 17:07:11, sohanjg wrote:
>> > On 2014/07/02 14:06:18, reveman wrote:
>> > > lgtm, thanks!
>> >
>> > alokp@, are you ok with this change ?
>>
>
>  looking...
>>
>
> I personally do not like random things going into ResourceProvider. It
> should
> just provide resources, not manage buffers for rasterization. That should
> be the
> job of raster worker pools. I know this is not the case at present, but we
> should be moving in the direction of simplifying it rather than putting
> more
> stuff into it.
>
> That said, I do not want to block this patch. It does not make anything
> worse. I
> just do not like the trend towards making ResourceProvider do everything!
>

Thanks, that's my thought too. ResourceProvider shouldn't be a dumping
ground for all things that use a context. We should pull these things into
another class if they don't belong anywhere else.


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-07-02 18:59:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-02 19:05:58]

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

[commit-bot: I haz the power, 2014-07-02 19:05:59]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[reveman, 2014-07-02 19:31:33]

On 2014/07/02 18:37:45, danakj wrote:
> On Wed, Jul 2, 2014 at 1:25 PM, <mailto:alokp@chromium.org> wrote:
> 
> > On 2014/07/02 17:18:06, Alok Priyadarshi wrote:
> >
> >> On 2014/07/02 17:07:11, sohanjg wrote:
> >> > On 2014/07/02 14:06:18, reveman wrote:
> >> > > lgtm, thanks!
> >> >
> >> > alokp@, are you ok with this change ?
> >>
> >
> >  looking...
> >>
> >
> > I personally do not like random things going into ResourceProvider. It
> > should
> > just provide resources, not manage buffers for rasterization. That should
> > be the
> > job of raster worker pools. I know this is not the case at present, but we
> > should be moving in the direction of simplifying it rather than putting
> > more
> > stuff into it.
> >
> > That said, I do not want to block this patch. It does not make anything
> > worse. I
> > just do not like the trend towards making ResourceProvider do everything!
> >
> 
> Thanks, that's my thought too. ResourceProvider shouldn't be a dumping
> ground for all things that use a context. We should pull these things into
> another class if they don't belong anywhere else.

That's fine and relatively easy to refactor after this lands. This change
removes inappropriate assumptions made by the GpuRasterWorkerPool about how GPU
raster is implemented. That's more important to fix than ResourceProvider class
doing too much IMO. We should still fix the later though..

> 
> 
> >
> > https://codereview.chromium.org/363563006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sohanjg, 2014-07-03 04:59:18]

On 2014/07/02 19:31:33, reveman wrote:
> On 2014/07/02 18:37:45, danakj wrote:
> > On Wed, Jul 2, 2014 at 1:25 PM, <mailto:alokp@chromium.org> wrote:
> > 
> > > On 2014/07/02 17:18:06, Alok Priyadarshi wrote:
> > >
> > >> On 2014/07/02 17:07:11, sohanjg wrote:
> > >> > On 2014/07/02 14:06:18, reveman wrote:
> > >> > > lgtm, thanks!
> > >> >
> > >> > alokp@, are you ok with this change ?
> > >>
> > >
> > >  looking...
> > >>
> > >
> > > I personally do not like random things going into ResourceProvider. It
> > > should
> > > just provide resources, not manage buffers for rasterization. That should
> > > be the
> > > job of raster worker pools. I know this is not the case at present, but we
> > > should be moving in the direction of simplifying it rather than putting
> > > more
> > > stuff into it.
> > >
> > > That said, I do not want to block this patch. It does not make anything
> > > worse. I
> > > just do not like the trend towards making ResourceProvider do everything!
> > >
> > 
> > Thanks, that's my thought too. ResourceProvider shouldn't be a dumping
> > ground for all things that use a context. We should pull these things into
> > another class if they don't belong anywhere else.
> 
> That's fine and relatively easy to refactor after this lands. This change
> removes inappropriate assumptions made by the GpuRasterWorkerPool about how
GPU
> raster is implemented. That's more important to fix than ResourceProvider
class
> doing too much IMO. We should still fix the later though..
> 
> > 
> > 
> > >
> > > https://codereview.chromium.org/363563006/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

OK, i will raise a bug as to how we want to refactor ResoureProvider.

[sohanjg, 2014-07-03 04:59:30]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-07-03 05:00:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/363563006/60001

[commit-bot: I haz the power, 2014-07-03 08:46:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[commit-bot: I haz the power, 2014-07-03 08:50:17]

Change committed as 281225

[sohanjg, 2015-11-18 09:42:38]

Description was changed from

==========
cc: Hide Gpu Rasterization details in Resource Provider.

This moves ganesh and gl context code out to resource
provider from gpurasterworkerpool to avoid making any
assumptions how the gpu raster buffers are implemented.

BUG=390399

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281225
==========

to

==========
cc: Hide Gpu Rasterization details in Resource Provider.

This moves ganesh and gl context code out to resource
provider from gpurasterworkerpool to avoid making any
assumptions how the gpu raster buffers are implemented.

BUG=390399

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281225
==========