cc: Remove TextureUploaderOption.

cc: Remove TextureUploaderOption.

After stopping to throttle partial uploads this option is no longer needed.

BUG=
TEST=cc_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158192

[reveman, 2012-09-20 01:27:32]

Hi,

here's some more cleanup. I will follow up with another CL that makes
CCTextureUpdateController just use the uploader provided by the resource
provider. I avoided doing that in this CL as it requires some more serious unit
test refactoring.

[piman, 2012-09-20 01:37:01]

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (left):

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
cc/CCResourceProvider.cpp:398: m_textureUploader =
UnthrottledTextureUploader::create();
drive-by: what about the software case (no context3d)?

[nduca, 2012-09-20 03:49:29]

LGTM

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (left):

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
cc/CCResourceProvider.cpp:398: m_textureUploader =
UnthrottledTextureUploader::create();
It'd be really nice if the uploader talked only to the resource provider, and
the query operations we need were just implemented by the software version of
the resource provider. David, can you do a patch after this that moves the
throttled uploader do that?

[piman, 2012-09-20 04:30:29]

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (left):

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
cc/CCResourceProvider.cpp:398: m_textureUploader =
UnthrottledTextureUploader::create();
On 2012/09/20 03:49:29, nduca wrote:
> It'd be really nice if the uploader talked only to the resource provider, and
> the query operations we need were just implemented by the software version of
> the resource provider. David, can you do a patch after this that moves the
> throttled uploader do that?

My main issue though is that in software mode, we won't have a
m_textureUploader. Won't we crash when trying to do a commit? What will cause
the paints to happen and put the data into resources? What am I missing?

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
cc/CCResourceProvider.cpp:431: GLC(context3d,
context3d->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &m_maxTextureSize));
another thing, IIRC in debug, command buffers like variables for the get output
to be initialized to 0 or 1. I think this will assert unless we reset
m_maxTextureSize to 0

[nduca, 2012-09-20 04:31:05]

Why dont you have a texture uploader? YOu're going to upload tile data... you
definitely want a system to control those uploads.

[piman, 2012-09-20 04:31:49]

On 2012/09/20 04:31:05, nduca wrote:
> Why dont you have a texture uploader? YOu're going to upload tile data... you
> definitely want a system to control those uploads.

I don't know, this CL removes it. I don't know why.

[nduca, 2012-09-20 04:32:11]

Oh, I seee, you're concerned about the very-near term, because we dont
initialize it to anything! My bad!

[nduca, 2012-09-20 04:33:45]

Derp derp, ~stupid me~.

David, seems like the step before this patch is to make the uploader/controller
work using resource provider instead of gc3d.

[reveman, 2012-09-20 04:52:18]

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (left):

https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
cc/CCResourceProvider.cpp:398: m_textureUploader =
UnthrottledTextureUploader::create();
On 2012/09/20 04:30:29, piman wrote:
> On 2012/09/20 03:49:29, nduca wrote:
> > It'd be really nice if the uploader talked only to the resource provider,
and
> > the query operations we need were just implemented by the software version
of
> > the resource provider. David, can you do a patch after this that moves the
> > throttled uploader do that?
> 
> My main issue though is that in software mode, we won't have a
> m_textureUploader. Won't we crash when trying to do a commit? What will cause
> the paints to happen and put the data into resources? What am I missing?

I think you're right. We might still need a texture uploader here but that's
fine. The main goal of this change is to get rid of TextureUploaderOption. 

UnthrottledTextureUploader can be moved to CCResourceProvider.cpp and renamed to
SoftwareTextureUploader. And I think we should do the same for TextureCopier
even though we might not use the copier in software mode at the moment.

[reveman, 2012-09-20 04:55:26]

On 2012/09/20 04:33:45, nduca wrote:
> Derp derp, ~stupid me~.
> 
> David, seems like the step before this patch is to make the
uploader/controller
> work using resource provider instead of gc3d.

I think it still makes sense to have separate TextureUploader/TextureCopier
classes for software and GLES mode. The big cleanup in this CL is getting rid of
the uploader option that was used by the proxy.

[reveman, 2012-09-20 04:58:28]

On 2012/09/20 04:30:29, piman wrote:
> https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp
> File cc/CCResourceProvider.cpp (left):
> 
>
https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
> cc/CCResourceProvider.cpp:398: m_textureUploader =
> UnthrottledTextureUploader::create();
> On 2012/09/20 03:49:29, nduca wrote:
> > It'd be really nice if the uploader talked only to the resource provider,
and
> > the query operations we need were just implemented by the software version
of
> > the resource provider. David, can you do a patch after this that moves the
> > throttled uploader do that?
> 
> My main issue though is that in software mode, we won't have a
> m_textureUploader. Won't we crash when trying to do a commit? What will cause
> the paints to happen and put the data into resources? What am I missing?
> 
>
https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#old...
> cc/CCResourceProvider.cpp:431: GLC(context3d,
> context3d->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE,
&m_maxTextureSize));
> another thing, IIRC in debug, command buffers like variables for the get
output
> to be initialized to 0 or 1. I think this will assert unless we reset
> m_maxTextureSize to 0

Oh, that's pretty weird behavior but I'll make sure this gets reset to 0.

[piman, 2012-09-20 06:25:12]

On Wed, Sep 19, 2012 at 9:58 PM, <reveman@chromium.org> wrote:

> On 2012/09/20 04:30:29, piman wrote:
>
>> https://codereview.chromium.**org/10961008/diff/1/cc/**
>>
CCResourceProvider.cpp<https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp>
>> File cc/CCResourceProvider.cpp (left):
>>
>
>
> https://codereview.chromium.**org/10961008/diff/1/cc/**
>
CCResourceProvider.cpp#**oldcode398<https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#oldcode398>
>
>> cc/CCResourceProvider.cpp:398: m_textureUploader =
>> UnthrottledTextureUploader::**create();
>> On 2012/09/20 03:49:29, nduca wrote:
>> > It'd be really nice if the uploader talked only to the resource
>> provider,
>>
> and
>
>> > the query operations we need were just implemented by the software
>> version
>>
> of
>
>> > the resource provider. David, can you do a patch after this that moves
>> the
>> > throttled uploader do that?
>>
>
>  My main issue though is that in software mode, we won't have a
>> m_textureUploader. Won't we crash when trying to do a commit? What will
>> cause
>> the paints to happen and put the data into resources? What am I missing?
>>
>
>
> https://codereview.chromium.**org/10961008/diff/1/cc/**
>
CCResourceProvider.cpp#**oldcode431<https://codereview.chromium.org/10961008/diff/1/cc/CCResourceProvider.cpp#oldcode431>
>
>> cc/CCResourceProvider.cpp:431: GLC(context3d,
>> context3d->getIntegerv(**GraphicsContext3D::MAX_**TEXTURE_SIZE,
>>
> &m_maxTextureSize));
>
>> another thing, IIRC in debug, command buffers like variables for the get
>>
> output
>
>> to be initialized to 0 or 1. I think this will assert unless we reset
>> m_maxTextureSize to 0
>>
>
> Oh, that's pretty weird behavior but I'll make sure this gets reset to 0.
>

It is very surprising, I know... it's there to protect against the case
where the context may get lost in which case you'd end up with an
uninitialized value.


>
>
https://codereview.chromium.**org/10961008/<https://codereview.chromium.org/1...
>

[reveman, 2012-09-20 18:24:31]

How do you feel about the latest patch. Ready to land?

[nduca, 2012-09-20 18:34:17]

Fix below and LGTM

https://chromiumcodereview.appspot.com/10961008/diff/10001/cc/CCResourceProvi...
File cc/CCResourceProvider.cpp (right):

https://chromiumcodereview.appspot.com/10961008/diff/10001/cc/CCResourceProvi...
cc/CCResourceProvider.cpp:29: class SoftwareTextureUploader : public
TextureUploader {
One class per file, please.

[piman, 2012-09-20 18:48:42]

https://chromiumcodereview.appspot.com/10961008/diff/10001/cc/CCResourceProvi...
File cc/CCResourceProvider.cpp (right):

https://chromiumcodereview.appspot.com/10961008/diff/10001/cc/CCResourceProvi...
cc/CCResourceProvider.cpp:29: class SoftwareTextureUploader : public
TextureUploader {
On 2012/09/20 18:34:17, nduca wrote:
> One class per file, please.

FYI, in chrome code having multiple private classes in .cc files is considered
ok - though it's better to put it in an anonymous namespace to make sure to
avoid collisions.

If you guys want a stricter set of local rules, that's fine, but I suggest then
to document that in a STYLE or README file at the cc/ top level.

[nduca, 2012-09-20 19:00:11]

My feedback is not a style comment per se. In this case, its an obvious
standalone class that is not a stub or a temporary thing, but rather a that we
will doubtless continue to improve upon and refine. So, I think that putting it
in a file since it will grow in the future makes sense.

[reveman, 2012-09-20 19:22:58]

I don't mind putting that class in a separate file but taking a step back it
seems wrong that we should have a texture uploader or a texture copier in
software mode. We're not using GL textures in this mode so why should we have
support texture uploading or texture copying? Am I missing something or is the
correct solution really to fix so we don't need a texture uploader in software
mode?

[piman, 2012-09-20 19:29:03]

On Thu, Sep 20, 2012 at 12:22 PM, <reveman@chromium.org> wrote:

> I don't mind putting that class in a separate file but taking a step back
> it
> seems wrong that we should have a texture uploader or a texture copier in
> software mode. We're not using GL textures in this mode so why should we
> have
> support texture uploading or texture copying? Am I missing something or is
> the
> correct solution really to fix so we don't need a texture uploader in
> software
> mode?
>

TextureUploader is wrongly named, that's what triggers the painting. So you
need it in software mode too.
TextureCopier, however, you don't need it in software mode. We don't
actually create one in that case.


>
>
https://chromiumcodereview.**appspot.com/10961008/<https://chromiumcodereview...
>

[nduca, 2012-09-20 19:42:01]

I think you're confused. We have both TextureUpdaters and TextureUploaders. One
paints, one uploads.

[piman, 2012-09-20 20:48:24]

On Thu, Sep 20, 2012 at 12:42 PM, <nduca@chromium.org> wrote:

> I think you're confused. We have both TextureUpdaters and
> TextureUploaders. One
> paints, one uploads.
>

Ok maybe I am confused, I thought that's what was doing the skpicture
rasterizing so that we can put it in textures in the zero-copy future.
In any case, TextureUpdater is what gets the data from the main thread side
(in whatever form) and puts it into the resources. You need it in software
mode too.


>
>
>
https://chromiumcodereview.**appspot.com/10961008/<https://chromiumcodereview...
>

[reveman, 2012-09-22 18:57:15]

I'm going ahead and landing this now that I've removed the CCResourceProvider
change that I think need some more thought.

[commit-bot: I haz the power, 2012-09-22 18:57:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10961008/6004

[commit-bot: I haz the power, 2012-09-22 22:22:15]

Change committed as 158192