Bug 83392 - [chromium] Texture copies should happen after incremental updates to preserve commit atomicity
Summary: [chromium] Texture copies should happen after incremental updates to preserve...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-06 14:35 PDT by James Robinson
Modified: 2012-04-09 14:25 PDT (History)
9 users (show)

See Also:


Attachments
work in progress (13.44 KB, patch)
2012-04-06 14:39 PDT, James Robinson
no flags Details | Formatted Diff | Diff
with proper flushing (13.31 KB, patch)
2012-04-09 11:18 PDT, James Robinson
enne: review+
jamesr: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-06 14:35:46 PDT
[chromium] Texture copies should happen after incremental updates to preserve commit atomicity
Comment 1 James Robinson 2012-04-06 14:39:41 PDT
Created attachment 136067 [details]
work in progress
Comment 2 James Robinson 2012-04-06 14:41:23 PDT
This is a first cut at fixing the webgl/canvas swap timing in their respective texture copy paths (currently webgl with preserveDrawingBuffer=true and canvas 2d in the threaded compositing case).  In offline discussion with David Reveman we thought that really the scheduler should be more aware of the # of pending copies and be driving this operation explicitly rather than "hiding" it with the partial updates, but I think this is a decent first cut.
Comment 3 Stephen White 2012-04-06 14:56:57 PDT
Comment on attachment 136067 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=136067&action=review

> Source/WebCore/ChangeLog:11
> +        This enqueues texture copy operations in the CCTextureUpdater's list instead of evaluating them immediately so
> +        if the update is spread over multiple frames we can properly defer the texture copy until the end. Otherwise,
> +        the texture copy for WebGL / Canvas 2D content happens before the commit completes and the compositor might pick
> +        up a frame for the canvas that's "ahead" of the content around it.

Yikes, nasty.  Does this affect Chrome on Android, currently?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:-139
> -    GLC(context, context->flush());

What happened to this flush?  Doesn't this still need to happen before the draw, or is it subsumed by a flush somewhere else?
Comment 4 James Robinson 2012-04-06 14:58:40 PDT
(In reply to comment #3)
> (From update of attachment 136067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136067&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        This enqueues texture copy operations in the CCTextureUpdater's list instead of evaluating them immediately so
> > +        if the update is spread over multiple frames we can properly defer the texture copy until the end. Otherwise,
> > +        the texture copy for WebGL / Canvas 2D content happens before the commit completes and the compositor might pick
> > +        up a frame for the canvas that's "ahead" of the content around it.
> 
> Yikes, nasty.  Does this affect Chrome on Android, currently?

It would if they had incremental texture uploads enabled (I think they have it off right now).  It's not the worst possible bug if it's happening (you see valid stuff most of the time, but some pages will look out of sync).

> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:-139
> > -    GLC(context, context->flush());
> 
> What happened to this flush?  Doesn't this still need to happen before the draw, or is it subsumed by a flush somewhere else?

This is the compositor's context so I don't think we need a flush since we're going to issue the draw sourcing this texture from the same context.
Comment 5 Stephen White 2012-04-06 15:04:44 PDT
Comment on attachment 136067 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=136067&action=review

>>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:-139
>>> -    GLC(context, context->flush());
>> 
>> What happened to this flush?  Doesn't this still need to happen before the draw, or is it subsumed by a flush somewhere else?
> 
> This is the compositor's context so I don't think we need a flush since we're going to issue the draw sourcing this texture from the same context.

I think it's more about making sure the copy is finished before canvas tries to write to the source texture again, but I could be wrong.  At any rate, it was added recently to fix flashing issues, in  http://trac.webkit.org/changeset/113033, so you might want to check with skyostil@.
Comment 6 James Robinson 2012-04-06 19:07:01 PDT
(In reply to comment #5)
> (From update of attachment 136067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136067&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:-139
> >>> -    GLC(context, context->flush());
> >> 
> >> What happened to this flush?  Doesn't this still need to happen before the draw, or is it subsumed by a flush somewhere else?
> > 
> > This is the compositor's context so I don't think we need a flush since we're going to issue the draw sourcing this texture from the same context.
> 
> I think it's more about making sure the copy is finished before canvas tries to write to the source texture again, but I could be wrong.  At any rate, it was added recently to fix flashing issues, in  http://trac.webkit.org/changeset/113033, so you might want to check with skyostil@.

Gotcha, we have to issue a flush on the compositor context before we let the main thread start issuing draw calls that might touch the source texture.  Makes perfect sense - I think that doing one flush after all copies are done (if we did copies) would work and be more efficient than flushing on each copy.
Comment 7 David Reveman 2012-04-09 10:18:58 PDT
This looks great! Do we still need to pass a GC3D pointer to updateCompositorResources after making this change? Would be great if we could remove that.

A nice follow up to this patch would be to move the texture upload part of CCTextureUpdater into a TextureUploader class just like we currently have a TextureCopier class. That would be a nice solution to: https://bugs.webkit.org/show_bug.cgi?id=83382 which is currently blocking us from landing throttled texture updates.
Comment 8 James Robinson 2012-04-09 10:43:38 PDT
(In reply to comment #7)
> This looks great! Do we still need to pass a GC3D pointer to updateCompositorResources after making this change? Would be great if we could remove that.
> 

I'd like to remove that next.  Another change I'd like to make is move this stuff into paintContents...() and get rid of the updateCompositorResources() completely.  That means that paintContents...() would need access to an interface by which it could enqueue texture uploads / copies (i.e. not directly access CCTextureUpdater).  Then we can map the queued texture operations to the appropriate updater/copier classes on the impl thread later on in the commit.
Comment 9 James Robinson 2012-04-09 11:18:12 PDT
Created attachment 136274 [details]
with proper flushing
Comment 10 Adrienne Walker 2012-04-09 11:34:41 PDT
Comment on attachment 136274 [details]
with proper flushing

Looks great.  R=me.
Comment 11 James Robinson 2012-04-09 14:25:19 PDT
Committed r113618: <http://trac.webkit.org/changeset/113618>