gpu: Implement idle async pixel transfers.

gpu: Implement idle async pixel transfers.

This makes the fallback pixel transfer implementation perform
texture uploads when command buffer is idle.

BUG=161337
R=epenner@chromium.org


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

[reveman, 2013-03-07 01:13:21]

This patch implements "idle" async uploads for use with impl-side painting on
non-android platforms.

"idle" is currently in respect to the command buffer that the async uploads are
issued on so webgl contents for example shouldn't be able to starve the uploads.
There's still the problem of compositor drawing itself starving uploads,
kMaxTimeSinceIdleMicroseconds exists for that reason.

Sami, this also takes advantage of your WaitForTransfer change and performs
those uploads immediately.

Please take a look.

[Sami, 2013-03-07 16:57:19]

https://codereview.chromium.org/12040049/diff/5001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/5001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:94: // Use a shorter delay when
there'ss idle work to be done.
Typo: there'ss

https://codereview.chromium.org/12040049/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9106:
RestoreCurrentTexture2DBindings();
Maybe only restore the bindings only if we actually did something -- that is if
we can get here without any pending transfers.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate.h (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate.h:116: virtual void
ProcessPendingTransfers() = 0;
Can we name these somehow so that the distinction between off- and on-thread
transfers is clear? I think having the Android delegate always return false from
a function called HasPendingTransfers seems weird.

ProcessPendingIdleTransfers maybe? At least let's put a comment here.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate_stub.cc (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:91: // Indicates there is a new
EGLImage and the 'real'
No EGLImages here :)

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:110: return
internal_->TransferIsInProgress();
Nit: indent by 2.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:113:
internal_->BindTransfer(bound_params);
Ditto.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:232: iter->task.Run();
Neat!

Is it worth wrapping this bit into PerformTransfer or something to avoid
duplicating the code in ProcessPendingTransfers?

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate_stub.h (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.h:20: // This class just does the
uploads synchronously.
Please update this comment. Maybe the class could be renamed too since it's not
a simple stub any longer -- up to you.

[reveman, 2013-03-07 20:21:03]

Addressed review feedback and fixed
TextureManager::BindFinishedAsyncPixelTransfers as I realized it didn't handle
out of order completions properly. PTAL.

https://codereview.chromium.org/12040049/diff/5001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/5001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:94: // Use a shorter delay when
there'ss idle work to be done.
On 2013/03/07 16:57:19, Sami wrote:
> Typo: there'ss

Done.

https://codereview.chromium.org/12040049/diff/5001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/5001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9106:
RestoreCurrentTexture2DBindings();
On 2013/03/07 16:57:19, Sami wrote:
> Maybe only restore the bindings only if we actually did something -- that is
if
> we can get here without any pending transfers.

Done.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate.h (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate.h:116: virtual void
ProcessPendingTransfers() = 0;
On 2013/03/07 16:57:19, Sami wrote:
> Can we name these somehow so that the distinction between off- and on-thread
> transfers is clear? I think having the Android delegate always return false
from
> a function called HasPendingTransfers seems weird.

I agree.

> 
> ProcessPendingIdleTransfers maybe? At least let's put a comment here.

I renamed HasPendingTransfers() to CanProcessPendingTransfers() and added a
comment that explains what these functions are for.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate_stub.cc (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:91: // Indicates there is a new
EGLImage and the 'real'
On 2013/03/07 16:57:19, Sami wrote:
> No EGLImages here :)

Done.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:110: return
internal_->TransferIsInProgress();
On 2013/03/07 16:57:19, Sami wrote:
> Nit: indent by 2.

Done.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:113:
internal_->BindTransfer(bound_params);
On 2013/03/07 16:57:19, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.cc:232: iter->task.Run();
On 2013/03/07 16:57:19, Sami wrote:
> Neat!
> 
> Is it worth wrapping this bit into PerformTransfer or something to avoid
> duplicating the code in ProcessPendingTransfers?

Done. Added ProcessTransfer.

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
File ui/gl/async_pixel_transfer_delegate_stub.h (right):

https://codereview.chromium.org/12040049/diff/5001/ui/gl/async_pixel_transfer...
ui/gl/async_pixel_transfer_delegate_stub.h:20: // This class just does the
uploads synchronously.
On 2013/03/07 16:57:19, Sami wrote:
> Please update this comment. Maybe the class could be renamed too since it's
not
> a simple stub any longer -- up to you.

Updated the comment.

I think there's some cleanup to be done in terms of naming and structure of the
AsyncPixelTransferDelegate in general. This was never really a stub class. I
think we might want to structure this more like GLSurface but would prefer to do
that as a follow up with Eric's input.

[Sami, 2013-03-08 12:01:23]

Thanks, lgtm. Let's get Eric's opinion too.

[greggman, 2013-03-08 21:06:21]

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9129:
RestoreCurrentTexture2DBindings();
What do think about moving RestoreCurrentTexture2DBindings inside
ProcessPendingTransfers?

I've made all the state restore functions public from GLES2Decoder so you can
pass 'this' into ProcessPendingTransfers.

In this particular case you really want to call

decoder->RestoreTextureUnitBindings(0);
decoder->RestoreActiveTexture();

The advantage is ProcessPendingTransfers can clean up after itself. The caller
doesn't need to know what happens inside like it does here.

I did this for DoCopyTextureCHROMIUM recently.
see http://goo.gl/XfXlE

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1225:
pending_async_transfers_.erase(iter++);
this is not the correct way to erase in the stl is it?
it should be

  iter = pending_async_tranfers_.erase(iter);

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1229: iter++;
this should be ++iter no?

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1251:
pending_async_transfers_.erase(iter++);
same as above

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_stub.cc (right):

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_stub.cc:67: static const
AsyncTexImage2DParams zero_params = {0, 0, 0, 0, 0, 0, 0, 0};
just fyi, you only need one 0, C++ will zero the rest. That means if you add or
remove fields this won't have to change.

In other words

    ... zero_params = { 0, }

Is enough to init the entire struct to zero.

[epenner, 2013-03-08 21:44:57]

Sorry for the delay, and looking good!

Let me know how you would like to order landing this with the refactor I did to
the delegate interface. I can probably finish that and have it up for review
today. Bit if you want to land this first that's fine too.

https://codereview.chromium.org/12040049/diff/13001/content/common/gpu/gpu_co...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/13001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:289: // Consider the command
buffer idle if no messages are received between
It seems like there might be a way this could be done more accurately with less
poll tasks.

- Do we even need a timer delay at all? (If there is future work shouldn't it
already be posted to the message loop)?

- Similarly, if there is no idle work, do we need to poll at all?  Doesn't the
idle work arrive via GPU commands, at which point we could do one idle poll
followed by one more poll after each idle job?

It also seems like we could do this globally for all contexts somehow (maybe not
important right now). Perhaps by querying the message loop directly somehow to
know it's truly idle? Wdyt?

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1220: while (iter !=
pending_async_transfers_.end()) {
I think this makes this more expensive right? Is it due to uploads finishing in
an arbitrary order now?

I think this will benefit from my patch to move this logic into the delegate,
since the single-thread delegate can do the 'late bind' callback on the same
thread when the upload completes, and avoid this loop all together.

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:376: virtual bool
CanProcessPendingTransfers() OVERRIDE;
Nit: I'd prefer NeedsProcessPendingTransfers.

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_stub.cc (left):

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_stub.cc:4: 
I've found it useful to have this 'no-op' stub around for testing purposes
(mainly isolating whether bugs are upload related). Do you think we could
copy/rename this class and keep the stub class around?

[reveman, 2013-03-12 00:31:41]

Addressed feedback and improved performance significantly. PTAL.

https://codereview.chromium.org/12040049/diff/13001/content/common/gpu/gpu_co...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/13001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:289: // Consider the command
buffer idle if no messages are received between
On 2013/03/08 21:44:57, epenner wrote:
> It seems like there might be a way this could be done more accurately with
less
> poll tasks.

Latest patch has a much more efficient way of detecting if we're in an idle
state. It doesn't do less polling but uses 0 delay when possible.

> 
> - Do we even need a timer delay at all? (If there is future work shouldn't it
> already be posted to the message loop)?

The message deferring system for each gpu channel made this a bit tricky but
what's in the latest patch seem to be working well.

> 
> - Similarly, if there is no idle work, do we need to poll at all?  Doesn't the
> idle work arrive via GPU commands, at which point we could do one idle poll
> followed by one more poll after each idle job?

That's basically how this works. ScheduleDelayedWork() is called after some
commands are processed. If no messages are processed or scheduled to be
processed between ScheduleDelayedWork() and PollWork(), we're idle.

> 
> It also seems like we could do this globally for all contexts somehow (maybe
not
> important right now). Perhaps by querying the message loop directly somehow to
> know it's truly idle? Wdyt?

Latest patch also does this globally instead of per command buffer.

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9129:
RestoreCurrentTexture2DBindings();
On 2013/03/08 21:06:21, greggman wrote:
> What do think about moving RestoreCurrentTexture2DBindings inside
> ProcessPendingTransfers?
> 
> I've made all the state restore functions public from GLES2Decoder so you can
> pass 'this' into ProcessPendingTransfers.
> 
> In this particular case you really want to call
> 
> decoder->RestoreTextureUnitBindings(0);
> decoder->RestoreActiveTexture();
> 
> The advantage is ProcessPendingTransfers can clean up after itself. The caller
> doesn't need to know what happens inside like it does here.
> 
> I did this for DoCopyTextureCHROMIUM recently.
> see http://goo.gl/XfXlE

Makes sense. The latest patch calls ProcessFinishedAsyncTransfers() here instead
as that's more appropriate. ProcessFinishedAsyncTransfers() calls
RestoreCurrentTexture2DBindings() and we probably want to clean that up as you
described but I would prefer to do that in a follow up patch. Latest patch at
least doesn't make things worse.

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1220: while (iter !=
pending_async_transfers_.end()) {
On 2013/03/08 21:44:57, epenner wrote:
> I think this makes this more expensive right? Is it due to uploads finishing
in
> an arbitrary order now?

yes, but I'm pretty sure the number of pending uploads are never enough to have
a significant impact on perf.

> 
> I think this will benefit from my patch to move this logic into the delegate,
> since the single-thread delegate can do the 'late bind' callback on the same
> thread when the upload completes, and avoid this loop all together.

Sounds like a good cleanup. How soon do you think we can land that change of
yours? I'd like to get this idle upload stuff landed asap so it's possible to
turn on impl-side painting on cros.

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1225:
pending_async_transfers_.erase(iter++);
On 2013/03/08 21:06:21, greggman wrote:
> this is not the correct way to erase in the stl is it?
> it should be
> 
>   iter = pending_async_tranfers_.erase(iter);

changed to that. seem to be a mix in chromium but I like this better.

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1229: iter++;
On 2013/03/08 21:06:21, greggman wrote:
> this should be ++iter no?

Done. I guess that's a bit more efficient.

https://codereview.chromium.org/12040049/diff/13001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1251:
pending_async_transfers_.erase(iter++);
On 2013/03/08 21:06:21, greggman wrote:
> same as above

Done.

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_android.cc:376: virtual bool
CanProcessPendingTransfers() OVERRIDE;
On 2013/03/08 21:44:57, epenner wrote:
> Nit: I'd prefer NeedsProcessPendingTransfers.

Done.

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_stub.cc (left):

https://codereview.chromium.org/12040049/diff/13001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_stub.cc:4: 
On 2013/03/08 21:44:57, epenner wrote:
> I've found it useful to have this 'no-op' stub around for testing purposes
> (mainly isolating whether bugs are upload related). Do you think we could
> copy/rename this class and keep the stub class around?

How about _delegate_sync.cc and _delegate_idle.cc? Current impl is not really a
no-op.

Long term I think we'd also want to instantiate these in a similar way to
GLSurface. _delegate_android.cc becomes _delegate_eglimage.cc and we have
_android.cc/_linux.cc that create an instance based on config and best chose for
platform.

[epenner, 2013-03-12 20:00:47]

On 2013/03/12 00:31:41, David Reveman wrote:
> How about _delegate_sync.cc and _delegate_idle.cc? Current impl is not really
a no-op. Long term I think we'd also want to instantiate these in a similar way
to GLSurface. _delegate_android.cc becomes _delegate_eglimage.cc and we have
_android.cc/_linux.cc that create an instance based on config and best chose for
platform.

This all sounds really good. Good call on it being _eglimage rather than
_android.

[reveman, 2013-03-12 21:14:16]

Idle upload impl was moved to _idle.cc in latest patch. I'll do the
_android.cc/_linux.cc cleanup in a follow up patch.

gman, what do you think about my latest changes to "content/common/gpu"?

[reveman, 2013-03-13 17:47:25]

Latest patch has been rebased on Eric's delegate cleanup. PTAL.

[epenner, 2013-03-13 18:58:01]

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:86: class AsyncTransferStateIdle :
public AsyncPixelTransferState {
See my comment on the transfers_ list in the header.

Hmm, on that note, possibly you don't need the 'internal' pattern for this
implementation?  Could you use weak-pointers to the original
AsyncPixelTransferState anywhere that accesses the state? If you do need some
state to persist after the AsyncPixelTransferState is deleted, then the
'internal' pattern is good, so long as you respect the deletion of the
AsyncPixelTransferState where it matters (accessing the texture id, and the
bind_callback).

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:124: bool texture_dirty =
texture_dirty_;
This feels brittle to me and not implied from the function name etc.

Would it make sense to restore when the actual state is changed? If we _really_
want, we could use a 'restore state' closure to do this anywhere, but if it's
just one more function returning a boolean I still prefer that myself.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:295: bind_callback.Run();
Same comment on possible use-after-free when invoking this callback. Let me know
if you have a better design on how we could protect or better indicate this
restriction. Although it's not completely clear, I do think it makes sense not
to call it after AsyncPixelTrasferState is deleted.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.h (right):

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.h:48: virtual void
ProcessPendingTransfers() OVERRIDE;
Nit: This name sounds like it does all the transfers. Very mildly prefer
'ProcessSomePendingTransfers' or something like that.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.h:56:
scoped_refptr<TransferStateInternalIdle> state;
Can you confirm what happens if an AsyncPixelTransferState is deleted while an
upload is pending? 

I was thinking that this would be a weak pointer to an AsyncPixelTransferState,
which indicates the texture's lifetime, and would allow pending uploads to be
cancelled etc. Similarly, the bind_callback passed into AsyncTexImage2D is
unsafe to call if the AsyncPixelTransferState is deleted.

[reveman, 2013-03-14 01:12:45]

PTAL

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:86: class AsyncTransferStateIdle :
public AsyncPixelTransferState {
Internal class gone in latest patch.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:124: bool texture_dirty =
texture_dirty_;
I made ProcessMorePendingTransfers() have a return value instead. We can do
better and not restore after each idle upload but I think this is good enough
for this patch.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:295: bind_callback.Run();
On 2013/03/13 18:58:01, epenner wrote:
> Same comment on possible use-after-free when invoking this callback. Let me
know
> if you have a better design on how we could protect or better indicate this
> restriction. Although it's not completely clear, I do think it makes sense not
> to call it after AsyncPixelTrasferState is deleted.

Done.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.h (right):

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.h:48: virtual void
ProcessPendingTransfers() OVERRIDE;
On 2013/03/13 18:58:01, epenner wrote:
> Nit: This name sounds like it does all the transfers. Very mildly prefer
> 'ProcessSomePendingTransfers' or something like that.

Now ProcessMorePendingTransfers.

https://codereview.chromium.org/12040049/diff/33001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.h:56:
scoped_refptr<TransferStateInternalIdle> state;
On 2013/03/13 18:58:01, epenner wrote:
> Can you confirm what happens if an AsyncPixelTransferState is deleted while an
> upload is pending? 
> 
> I was thinking that this would be a weak pointer to an
AsyncPixelTransferState,
> which indicates the texture's lifetime, and would allow pending uploads to be
> cancelled etc. Similarly, the bind_callback passed into AsyncTexImage2D is
> unsafe to call if the AsyncPixelTransferState is deleted.

oops, messed that up in my previous patch. I think the latest patch does this
properly.

[Sami, 2013-03-14 15:18:07]

Drive-by nits.

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:140: bool handle_messages_scheduled() { return
handle_messages_scheduled_; }
Nit: these could be const.

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:92: const int64
kHandleMoreWorkPeriodBusyMs = 1;
This seems unused now?

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:301: // not waiting on unschedule
fences.
Typo: unschedule

[epenner, 2013-03-14 18:03:56]

LGTM. I haven't looked extensively at the GPU side changes but the Async changes
were looking good!

[greggman, 2013-03-14 18:15:45]

Hey Al, can you take a look at the gpu_command_buffer_stub.cc changes? You know
what's expected there better than me.

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
(async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
I think I asked this in another CL. Would it be better to pass 'this' to
ProcessModePendingTransfers so that ProcessMorePendingTransfers can restore its
own state? It seems strange that the code out here has to know what state to
restore.

Also, you either need to remove the calls to glActiveTexture in
async_pixel_transfer_delegate_idle.cc or else you need to call
RestoreTextureUnitBindings(0)

[greggman, 2013-03-14 18:15:59]

Hey Al, can you take a look at the gpu_command_buffer_stub.cc changes? You know
what's expected there better than me.

[epenner, 2013-03-14 18:23:24]

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:300: // No need for a delay when
there's idle work to be done and we're
I think I get this, but would help to have "because..." at the end of this
comment.

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
(async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
On 2013/03/14 18:15:46, greggman wrote:
> I think I asked this in another CL. Would it be better to pass 'this' to
> ProcessModePendingTransfers so that ProcessMorePendingTransfers can restore
its
> own state? It seems strange that the code out here has to know what state to
> restore.
> 
> Also, you either need to remove the calls to glActiveTexture in
> async_pixel_transfer_delegate_idle.cc or else you need to call
> RestoreTextureUnitBindings(0)

It's currently a layering violation to do that, but we could make a
"RestoreCurrentTexture2DBindings" closure or interface that could be called. I
suggested we keep the booleans before since it's just a few functions, but it's
your call.

If the code moved the layering violation would go away, but I kind of like how
it currently has no dependency on the decoder.

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:99: task(task) {
id=id? Would we call it 'id_' by making it a class with public members?

[greggman, 2013-03-14 18:33:58]

On 2013/03/14 18:23:24, epenner wrote:
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> content/common/gpu/gpu_command_buffer_stub.cc:300: // No need for a delay when
> there's idle work to be done and we're
> I think I get this, but would help to have "because..." at the end of this
> comment.
> 
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
> (async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
> On 2013/03/14 18:15:46, greggman wrote:
> > I think I asked this in another CL. Would it be better to pass 'this' to
> > ProcessModePendingTransfers so that ProcessMorePendingTransfers can restore
> its
> > own state? It seems strange that the code out here has to know what state to
> > restore.
> > 
> > Also, you either need to remove the calls to glActiveTexture in
> > async_pixel_transfer_delegate_idle.cc or else you need to call
> > RestoreTextureUnitBindings(0)
> 
> It's currently a layering violation to do that, but we could make a
> "RestoreCurrentTexture2DBindings" closure or interface that could be called. I
> suggested we keep the booleans before since it's just a few functions, but
it's
> your call.
> 
> If the code moved the layering violation would go away, but I kind of like how
> it currently has no dependency on the decoder.


We need to move async_pixel transfer out of ui where it doesn't belong and into
gpu where it does.  I know you had problems before but we need to fix those
problems.  It doesn't belong in ui.



> 
>
https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
> File ui/gl/async_pixel_transfer_delegate_idle.cc (right):
> 
>
https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:99: task(task) {
> id=id? Would we call it 'id_' by making it a class with public members?

[reveman, 2013-03-14 19:21:47]

PTAL

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:140: bool handle_messages_scheduled() { return
handle_messages_scheduled_; }
On 2013/03/14 15:18:07, Sami wrote:
> Nit: these could be const.

Done.

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:92: const int64
kHandleMoreWorkPeriodBusyMs = 1;
On 2013/03/14 15:18:07, Sami wrote:
> This seems unused now?

Oh, thanks for noticing this. Latest patch makes sure we don't change the usage
of kHandleMoreWorkPeriodBusyMs.

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:300: // No need for a delay when
there's idle work to be done and we're
On 2013/03/14 18:23:24, epenner wrote:
> I think I get this, but would help to have "because..." at the end of this
> comment.

I've updated this comment to make it more clear what we're doing here.

https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
content/common/gpu/gpu_command_buffer_stub.cc:301: // not waiting on unschedule
fences.
On 2013/03/14 15:18:07, Sami wrote:
> Typo: unschedule

It sounds awkward but this is what we currently call these fences. See:
scheduler_->PollUnscheduleFences()

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
(async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
On 2013/03/14 18:15:46, greggman wrote:
> I think I asked this in another CL. Would it be better to pass 'this' to
> ProcessModePendingTransfers so that ProcessMorePendingTransfers can restore
its
> own state? It seems strange that the code out here has to know what state to
> restore.

Oh sorry, so we've gone back and forth here without me noticing. I switch to
just using the existing ProcessFinishedAsyncTransfers() call here based on your
feedback but epenner pointed out that this was a bit fragile so now we're back
to using a bool return value again.

I'd prefer to keep the current bool return value for now
(BindCompletedAsyncTransfers already does this) and clean it up be removing this
behavior from the AsyncPixelTransferDelegate completely in a follow up patch.
I've added TODO comments to the code and filed crbug.com/196303. gman, you OK
with that?

> 
> Also, you either need to remove the calls to glActiveTexture in
> async_pixel_transfer_delegate_idle.cc or else you need to call
> RestoreTextureUnitBindings(0)

Done. Thanks for noticing this.

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:99: task(task) {
On 2013/03/14 18:23:24, epenner wrote:
> id=id? Would we call it 'id_' by making it a class with public members?

Yes, but I prefer using a struct for this simple container.

[reveman, 2013-03-14 19:27:51]

On 2013/03/14 18:33:58, greggman wrote:
> On 2013/03/14 18:23:24, epenner wrote:
> >
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> > File content/common/gpu/gpu_command_buffer_stub.cc (right):
> > 
> >
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> > content/common/gpu/gpu_command_buffer_stub.cc:300: // No need for a delay
when
> > there's idle work to be done and we're
> > I think I get this, but would help to have "because..." at the end of this
> > comment.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
> > (async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
> > On 2013/03/14 18:15:46, greggman wrote:
> > > I think I asked this in another CL. Would it be better to pass 'this' to
> > > ProcessModePendingTransfers so that ProcessMorePendingTransfers can
restore
> > its
> > > own state? It seems strange that the code out here has to know what state
to
> > > restore.
> > > 
> > > Also, you either need to remove the calls to glActiveTexture in
> > > async_pixel_transfer_delegate_idle.cc or else you need to call
> > > RestoreTextureUnitBindings(0)
> > 
> > It's currently a layering violation to do that, but we could make a
> > "RestoreCurrentTexture2DBindings" closure or interface that could be called.
I
> > suggested we keep the booleans before since it's just a few functions, but
> it's
> > your call.
> > 
> > If the code moved the layering violation would go away, but I kind of like
how
> > it currently has no dependency on the decoder.
> 
> 
> We need to move async_pixel transfer out of ui where it doesn't belong and
into
> gpu where it does.  I know you had problems before but we need to fix those
> problems.  It doesn't belong in ui.

Can we figure out how to do that as part of solving crbug.com/187886?

[greggman, 2013-03-14 20:09:07]

On 2013/03/14 19:27:51, David Reveman wrote:
> On 2013/03/14 18:33:58, greggman wrote:
> > On 2013/03/14 18:23:24, epenner wrote:
> > >
> >
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> > > File content/common/gpu/gpu_command_buffer_stub.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12040049/diff/43001/content/common/gpu/gpu_co...
> > > content/common/gpu/gpu_command_buffer_stub.cc:300: // No need for a delay
> when
> > > there's idle work to be done and we're
> > > I think I get this, but would help to have "because..." at the end of this
> > > comment.
> > > 
> > >
> >
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12040049/diff/43001/gpu/command_buffer/servic...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:9257: if
> > > (async_pixel_transfer_delegate_->ProcessMorePendingTransfers())
> > > On 2013/03/14 18:15:46, greggman wrote:
> > > > I think I asked this in another CL. Would it be better to pass 'this' to
> > > > ProcessModePendingTransfers so that ProcessMorePendingTransfers can
> restore
> > > its
> > > > own state? It seems strange that the code out here has to know what
state
> to
> > > > restore.
> > > > 
> > > > Also, you either need to remove the calls to glActiveTexture in
> > > > async_pixel_transfer_delegate_idle.cc or else you need to call
> > > > RestoreTextureUnitBindings(0)
> > > 
> > > It's currently a layering violation to do that, but we could make a
> > > "RestoreCurrentTexture2DBindings" closure or interface that could be
called.
> I
> > > suggested we keep the booleans before since it's just a few functions, but
> > it's
> > > your call.
> > > 
> > > If the code moved the layering violation would go away, but I kind of like
> how
> > > it currently has no dependency on the decoder.
> > 
> > 
> > We need to move async_pixel transfer out of ui where it doesn't belong and
> into
> > gpu where it does.  I know you had problems before but we need to fix those
> > problems.  It doesn't belong in ui.
> 
> Can we figure out how to do that as part of solving crbug.com/187886?

sounds good. lgtm

[epenner, 2013-03-14 20:31:00]

> We need to move async_pixel transfer out of ui where it
> doesn't belong and into gpu where it does.  I know you had
> problems before but we need to fix those problems.  It 
> doesn't belong in ui.

I thought it was actually okay in the end, but sure we can move it. The problems
with putting it in GPU were mainly to do with the include paths I thinks. DEPS
wise I like it's current location, but that can be enforced through review after
it moves.

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/43001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:99: task(task) {
On 2013/03/14 19:21:47, David Reveman wrote:
> On 2013/03/14 18:23:24, epenner wrote:
> > id=id? Would we call it 'id_' by making it a class with public members?
> 
> Yes, but I prefer using a struct for this simple container.

Touche! I thought it was a bug, but it's totally fine. Sorry.

[apatrick_chromium, 2013-03-14 23:30:39]

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:30: // is just DCHECKS here.
is -> are

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:115: return
static_cast<AsyncPixelTransferState*>(
static_cast is redundant?

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:275: glBindTexture(GL_TEXTURE_2D,
texture_id);
GL_TEXTURE_2D shouldn't be tex_params.target? Or is only GL_TEXTURE_2D
supported...

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:280: tex_params.target,
... in which case this should be GL_TEXTURE_2D and add a
DCHECK_EQ(GL_TEXTURE_2D, tex_params.target) above?

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:303: DCHECK_EQ(0, tex_params.level);
Same here.

[reveman, 2013-03-15 01:34:03]

Fixed issues. PTAL.

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
File ui/gl/async_pixel_transfer_delegate_idle.cc (right):

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:30: // is just DCHECKS here.
On 2013/03/14 23:30:39, apatrick_chromium wrote:
> is -> are

Done. Also fixed _android.cc and _stub.cc where this was taken from.

Btw, we should move this function into AsyncPixelTransferDelegate instead of
duplication it in every file as part of the planned re-factor.

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:115: return
static_cast<AsyncPixelTransferState*>(
On 2013/03/14 23:30:39, apatrick_chromium wrote:
> static_cast is redundant?

Yes. Fixed _android.cc/_stub.cc too.

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:275: glBindTexture(GL_TEXTURE_2D,
texture_id);
On 2013/03/14 23:30:39, apatrick_chromium wrote:
> GL_TEXTURE_2D shouldn't be tex_params.target? Or is only GL_TEXTURE_2D
> supported...

Good call. Fixed.

_android.cc is limited to TEXTURE_2D and has proper DCHECKs but this code is
not.

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:280: tex_params.target,
On 2013/03/14 23:30:39, apatrick_chromium wrote:
> ... in which case this should be GL_TEXTURE_2D and add a
> DCHECK_EQ(GL_TEXTURE_2D, tex_params.target) above?

Done.

https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
ui/gl/async_pixel_transfer_delegate_idle.cc:303: DCHECK_EQ(0, tex_params.level);
On 2013/03/14 23:30:39, apatrick_chromium wrote:
> Same here.

Done. No reason this should be limited to level=0.

[apatrick_chromium, 2013-03-15 18:12:54]

On 2013/03/15 01:34:03, David Reveman wrote:
> Fixed issues. PTAL.
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> File ui/gl/async_pixel_transfer_delegate_idle.cc (right):
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:30: // is just DCHECKS here.
> On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > is -> are
> 
> Done. Also fixed _android.cc and _stub.cc where this was taken from.
> 
> Btw, we should move this function into AsyncPixelTransferDelegate instead of
> duplication it in every file as part of the planned re-factor.
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:115: return
> static_cast<AsyncPixelTransferState*>(
> On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > static_cast is redundant?
> 
> Yes. Fixed _android.cc/_stub.cc too.
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:275: glBindTexture(GL_TEXTURE_2D,
> texture_id);
> On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > GL_TEXTURE_2D shouldn't be tex_params.target? Or is only GL_TEXTURE_2D
> > supported...
> 
> Good call. Fixed.
> 
> _android.cc is limited to TEXTURE_2D and has proper DCHECKs but this code is
> not.
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:280: tex_params.target,
> On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > ... in which case this should be GL_TEXTURE_2D and add a
> > DCHECK_EQ(GL_TEXTURE_2D, tex_params.target) above?
> 
> Done.
> 
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> ui/gl/async_pixel_transfer_delegate_idle.cc:303: DCHECK_EQ(0,
tex_params.level);
> On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > Same here.
> 
> Done. No reason this should be limited to level=0.

RestoreCurrentTexture2DBindings only restores the GL_TEXTURE_2D bindings. If you
are going to support more than just GL_TEXTURE_2D, would you not need to restore
bindings for other texture targets as well?

[reveman, 2013-03-15 18:28:54]

On 2013/03/15 18:12:54, apatrick_chromium wrote:
> On 2013/03/15 01:34:03, David Reveman wrote:
> > Fixed issues. PTAL.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > File ui/gl/async_pixel_transfer_delegate_idle.cc (right):
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > ui/gl/async_pixel_transfer_delegate_idle.cc:30: // is just DCHECKS here.
> > On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > > is -> are
> > 
> > Done. Also fixed _android.cc and _stub.cc where this was taken from.
> > 
> > Btw, we should move this function into AsyncPixelTransferDelegate instead of
> > duplication it in every file as part of the planned re-factor.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > ui/gl/async_pixel_transfer_delegate_idle.cc:115: return
> > static_cast<AsyncPixelTransferState*>(
> > On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > > static_cast is redundant?
> > 
> > Yes. Fixed _android.cc/_stub.cc too.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > ui/gl/async_pixel_transfer_delegate_idle.cc:275:
glBindTexture(GL_TEXTURE_2D,
> > texture_id);
> > On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > > GL_TEXTURE_2D shouldn't be tex_params.target? Or is only GL_TEXTURE_2D
> > > supported...
> > 
> > Good call. Fixed.
> > 
> > _android.cc is limited to TEXTURE_2D and has proper DCHECKs but this code is
> > not.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > ui/gl/async_pixel_transfer_delegate_idle.cc:280: tex_params.target,
> > On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > > ... in which case this should be GL_TEXTURE_2D and add a
> > > DCHECK_EQ(GL_TEXTURE_2D, tex_params.target) above?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/12040049/diff/55001/ui/gl/async_pixel_transfe...
> > ui/gl/async_pixel_transfer_delegate_idle.cc:303: DCHECK_EQ(0,
> tex_params.level);
> > On 2013/03/14 23:30:39, apatrick_chromium wrote:
> > > Same here.
> > 
> > Done. No reason this should be limited to level=0.
> 
> RestoreCurrentTexture2DBindings only restores the GL_TEXTURE_2D bindings. If
you
> are going to support more than just GL_TEXTURE_2D, would you not need to
restore
> bindings for other texture targets as well?

Correct. I've added DCHECKs to latest patch that limits this to GL_TEXTURE_2D
just like the android implementation.

[apatrick_chromium, 2013-03-15 18:49:16]

LGTM.

Just a note. Not for this patch.

If other texture targets were to be supported in the future, the current
AsyncTexImage2DParams is not sufficient. Consider an async upload to a cube map:

glBindTexture(GL_TEXTURE_CUBE_MAP, ...);
glTexImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X, ...);

It would need two distinct texture targets.

[commit-bot: I haz the power, 2013-03-15 19:19:45]

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

[commit-bot: I haz the power, 2013-03-16 09:25:09]

Change committed as 188558