Bug 83392 – [chromium] Texture copies should happen after incremental updates to preserve commit atomicity

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Texture copies should happen after incremental updates to preserve commit atomicity

[James Robinson]

Created attachment 136067 [details]
work in progress

[James Robinson]

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.

[Stephen White]

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?

[James Robinson]

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

[Stephen White]

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

[James Robinson]

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

[David Reveman]

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.

[James Robinson]

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

[James Robinson]

Created attachment 136274 [details]
with proper flushing

[Adrienne Walker]

Comment on attachment 136274 [details]
with proper flushing

Looks great.  R=me.

[James Robinson]

Committed r113618: <http://trac.webkit.org/changeset/113618>