Bug 92182 – [chromium] Turn the debug HUD into a layer so that it can be drawn as a WebCompositorQuad

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

[Dana Jansens]

[chromium] Turn the debug HUD into a layer so that it can be drawn as a WebCompositorQuad

[Dana Jansens]

Created attachment 154193 [details]
Patch

[Dana Jansens]

Created attachment 154195 [details]
Patch

remove the changes in CCDamageTracker by having layerPropertyChanged() return true when layerIsAlwaysDamaged().
remove the changes to turn on debug rects (oops)

[Adrienne Walker]

Comment on attachment 154195 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/HeadsUpDisplayLayerChromium.h:44
> +    explicit HeadsUpDisplayLayerChromium();

No need for explicit here.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:63
> +    // FIXME: Use contentBounds() for the texture.

Don't contentBounds == bounds here?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:92
> +    printf("layerPropertyChanged %d\n", layerPropertyChanged());

Whomp.

[Antoine Labour]

Comment on attachment 154193 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:83
> +        resourceProvider->upload(m_hudTexture->id(), locker.pixels(), layerRect, layerRect, layerRect);

Note: FYI this will be a problem with the host compositor, similar to other uses of willDraw in CCTextureLayerImpl or CCVideoLayerImpl. Essentially when we will want to upload data to it, it will be in use by the host compositor and things will go bad.
We'll have to fix it similarly to the way we'll fix the other classes, e.g. double-buffering. You don't have to change it now, but it would be useful for future reference to add an ASSERT that !resourceProvider->inUseByConsumer(m_hudTexture->id()), and a FIXME.

[Dana Jansens]

Comment on attachment 154193 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:83
>> +        resourceProvider->upload(m_hudTexture->id(), locker.pixels(), layerRect, layerRect, layerRect);
> 
> Note: FYI this will be a problem with the host compositor, similar to other uses of willDraw in CCTextureLayerImpl or CCVideoLayerImpl. Essentially when we will want to upload data to it, it will be in use by the host compositor and things will go bad.
> We'll have to fix it similarly to the way we'll fix the other classes, e.g. double-buffering. You don't have to change it now, but it would be useful for future reference to add an ASSERT that !resourceProvider->inUseByConsumer(m_hudTexture->id()), and a FIXME.

ya this was intentional punting :) good idea with the assert and fixme!

[Dana Jansens]

Comment on attachment 154195 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/HeadsUpDisplayLayerChromium.h:44
>> +    explicit HeadsUpDisplayLayerChromium();
> 
> No need for explicit here.

Ah the copypaste. thanks.

>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:63
>> +    // FIXME: Use contentBounds() for the texture.
> 
> Don't contentBounds == bounds here?

Well.. yes. Hm. I guess everything is going to get very small tho in hidpi. I will reword this FIXME.

>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:92
>> +    printf("layerPropertyChanged %d\n", layerPropertyChanged());
> 
> Whomp.

removed

[Dana Jansens]

Created attachment 154203 [details]
Patch

[Adrienne Walker]

Comment on attachment 154203 [details]
Patch

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

R=me.  I love all the HUD code removal from the proxy and LRC.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:107
> +    // parent compositor. We will probably need to hold on to m_frame for

m_frame -> m_hudTexture?

[Dana Jansens]

Comment on attachment 154203 [details]
Patch

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

Thanks

>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplayLayerImpl.cpp:107
>> +    // parent compositor. We will probably need to hold on to m_frame for
> 
> m_frame -> m_hudTexture?

Ya! HUD is not video.

[Dana Jansens]

Created attachment 154206 [details]
Patch

[Dana Jansens]

Created attachment 154385 [details]
diff-to-fix-asserts

[Dana Jansens]

Created attachment 154386 [details]
Patch

[Dana Jansens]

Created attachment 154388 [details]
Patch

remove the explicit i added that wasnt needed

[Adrienne Walker]

Comment on attachment 154388 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/HeadsUpDisplayLayerChromium.cpp:70
> +    return CCHeadsUpDisplayLayerImpl::create(m_layerId, m_fontAtlas.release());

Is there ever a time where the impl-side layer goes away and m_fontAtlas needs to be reused? In other words, maybe create m_fontAtlas if needed in an update function just to be safe?

[Dana Jansens]

Comment on attachment 154388 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/HeadsUpDisplayLayerChromium.cpp:70
>> +    return CCHeadsUpDisplayLayerImpl::create(m_layerId, m_fontAtlas.release());
> 
> Is there ever a time where the impl-side layer goes away and m_fontAtlas needs to be reused? In other words, maybe create m_fontAtlas if needed in an update function just to be safe?

We never delete the impl tree, and we never turn hud off, so the impl layer will always be there. In order to know if you need to make a new atlas you need to know if the impl layer has one. But we have to make the atlas before we block the impl thread.

I would say that if we make a way to turn HUD off/on, then the turn-on mechanism should be the one to signal the atlas to be created. Right now creating a HUDLayerChromium is the mechanism we use to signal that the atlas needs to be created.

wdyt?

[Adrienne Walker]

Comment on attachment 154388 [details]
Patch

R=me(again).  I'll buy that argument.  If we make the HUD toggleable, there'll be a lot more that will need to be changed.

[Shawn Singh]

Comment on attachment 154388 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:268
> +    if (m_hudLayer)
> +        m_hudLayer->removeFromParent();

I'm a little confused about this - does this mean we will be re-creating the hudLayer on every commit?  won't that mean we are also re-creating the fontAtlas stuff that should only be done once?

[Shawn Singh]

(In reply to comment #17)
> (From update of attachment 154388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154388&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:268
> > +    if (m_hudLayer)
> > +        m_hudLayer->removeFromParent();
> 
> I'm a little confused about this - does this mean we will be re-creating the hudLayer on every commit?  won't that mean we are also re-creating the fontAtlas stuff that should only be done once?

I guess my comment was just 10 minutes out-dated

[Adrienne Walker]

(In reply to comment #17)
> (From update of attachment 154388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154388&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:268
> > +    if (m_hudLayer)
> > +        m_hudLayer->removeFromParent();
> 
> I'm a little confused about this - does this mean we will be re-creating the hudLayer on every commit?  won't that mean we are also re-creating the fontAtlas stuff that should only be done once?

Removing the hud layer from its parent doesn't clear it and cause it to be recreated.  It's still owned by the CCLTH.

[Dana Jansens]

Committed r123644: <http://trac.webkit.org/changeset/123644>

[James Robinson]

This patch appears to have inverted all the colors (specifically swizzled red<->blue), since the old HUD shader was a swizzle shader and the texture shader doesn't swizzle.  It's easy enough to "correct" for this by swizzling the colors back when we paint, since we programatically paint everything in the HUD, but it's a bit strange.