Bug 66506 – [chromium] Root layer is not updated when only a portion of tile is updated

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

This only happens in accelerated drawing path.

[James Robinson]

http://trac.webkit.org/changeset/93360 might fix this?  It make the root and non-root layer paths much more similar.

[Alok Priyadarshi]

Created attachment 104417 [details]
proposed patch

[James Robinson]

Comment on attachment 104417 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:664
> +    GLC(context, context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0));

I'm not sure I understand why this is here - is the problem here that skia is assuming a certain texture binding, but that isn't true here?

[Alok Priyadarshi]

Comment on attachment 104417 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:664
>> +    GLC(context, context->bindTexture(GraphicsContext3D::TEXTURE_2D, 0));
> 
> I'm not sure I understand why this is here - is the problem here that skia is assuming a certain texture binding, but that isn't true here?

It is not needed to fix this particular bug. This bug was yet another case of state management issue between skia and compositor, so I took a pass through the accelerated code path and fixed the functions that left GL state in a non-default state. I think in general it is a good practice to restore the GL state if it is not too expensive.

[Alok Priyadarshi]

ping!

[James Robinson]

Comment on attachment 104417 [details]
proposed patch

The flush seems fine but I'm not really a fan of making unrelated changes that don't do anything.  We have not been trying to keep all GL state on the compositor in any particular state (nor do I think that we would want to), so restoring some bits of state in an ad-hoc fashion seems to just be adding complexity for no benefit.  If we do need a given GL state to be maintained on the compositor context then we should design, test, and be rigorous about it.

[Vangelis Kokkevis]

Comment on attachment 104417 [details]
proposed patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
>      // FIXME: These calls can be made once, when the compositor context is initialized.

I think this FIXME should be removed.  Now that the compositor's context is shared with non-compositor code, it will be impossible to assume that any given state will be stick across frames.

[Alok Priyadarshi]

(In reply to comment #6)
> (From update of attachment 104417 [details])
> The flush seems fine but I'm not really a fan of making unrelated changes that don't do anything.  We have not been trying to keep all GL state on the compositor in any particular state (nor do I think that we would want to), so restoring some bits of state in an ad-hoc fashion seems to just be adding complexity for no benefit.  If we do need a given GL state to be maintained on the compositor context then we should design, test, and be rigorous about it.

That particular change is not restoring the state in any ad-hoc manner. It is just restoring it the default state. We do that in multiple places. For example LayerRendererChromium::drawLayersInternal() restores STENCIL_TEST and BLEND state. LayerTextureUpdaterSkPicture::updateTextureRect() rebinds default frame-buffer.

Anyways I have removed it from this patch because it did not have any effect. We need to start thinking about state management as the compositor context is increasingly used by skia for content rendering.

[Alok Priyadarshi]

Comment on attachment 104417 [details]
proposed patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
>>      // FIXME: These calls can be made once, when the compositor context is initialized.
> 
> I think this FIXME should be removed.  Now that the compositor's context is shared with non-compositor code, it will be impossible to assume that any given state will be stick across frames.

DONE

[James Robinson]

I meant ad-hoc in the sense that there are other places where the compositor sets GL state and does not restore it.  I don't know if we have decided whether skia will continue to use the compositor context or not long-term.

[Alok Priyadarshi]

Created attachment 105030 [details]
proposed patch

Removed the change to restore bound texture and removed FIXME.

[James Robinson]

Comment on attachment 105030 [details]
proposed patch

Looks good.

[WebKit Review Bot]

Comment on attachment 105030 [details]
proposed patch

Clearing flags on attachment: 105030

Committed r93727: <http://trac.webkit.org/changeset/93727>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.