Bug 56514 – [chromium] Properly reset VideoLayerChromium textures after lost renderer context

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

[Victoria Kirst]

[chromium] Properly reset VideoLayerChromium textures after lost renderer context

[Victoria Kirst]

Created attachment 86017 [details]
Patch

[Kenneth Russell]

Comment on attachment 86017 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:96
> +    if (replacingRenderer)
> +        resetFrameParameters();

LayerChromium::setLayerRenderer() will call cleanupResources() if (layerRenderer() && layerRenderer() != renderer), which will call releaseCurrentFrame() which contains a call to resetFrameParameters(). Doesn't that make this override redundant?

[Victoria Kirst]

(In reply to comment #2)
> (From update of attachment 86017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86017&action=review
> 
> > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:96
> > +    if (replacingRenderer)
> > +        resetFrameParameters();
> 
> LayerChromium::setLayerRenderer() will call cleanupResources() if (layerRenderer() && layerRenderer() != renderer), which will call releaseCurrentFrame() which contains a call to resetFrameParameters(). Doesn't that make this override redundant?

Good question! It's not redundant because m_currentFrame can be null, and in that case releaseCurrentFrame() will not call resetFrameParameters() because it exits out early.

And actually, saveCurrentFrame/releaseCurrentFrame are hooks to be used when we have hardware decoding implemented in Chrome. Right now we don't have hardware decode implemented, so m_currentFrame is _always_ null.

[Kenneth Russell]

Comment on attachment 86017 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:96
>>> +        resetFrameParameters();
>> 
>> LayerChromium::setLayerRenderer() will call cleanupResources() if (layerRenderer() && layerRenderer() != renderer), which will call releaseCurrentFrame() which contains a call to resetFrameParameters(). Doesn't that make this override redundant?
> 
> Good question! It's not redundant because m_currentFrame can be null, and in that case releaseCurrentFrame() will not call resetFrameParameters() because it exits out early.
> 
> And actually, saveCurrentFrame/releaseCurrentFrame are hooks to be used when we have hardware decoding implemented in Chrome. Right now we don't have hardware decode implemented, so m_currentFrame is _always_ null.

Wouldn't it be better to move the call to resetFrameParameters() to the top of releaseCurrentFrame() in that case? It shouldn't be necessary to override setLayerRenderer unless something more than simple resource reallocation needs to occur in response.

[Victoria Kirst]

Hey Kenneth! Wanted your opinion on a style issue...

So essentially there are two types of VideoFrameChromiums, SystemMemory-backed frames and texture-backed frames Both frame types are saved into m_textures because we want the draw method to treat these the same way.

But for SystemMemory frames, we allocate and take of the textures, so we have to delete them after destruction. But if we're already given the textures, we do not take ownership, so we should not destroy them.

I was thinking I would create another m_textures field (i.e. m_texturesToPaint) that stores textures we're painting to screen. Then in m_textures we would keep textures we've allocated and must delete (never ones we've gotten from VideoFrameChromium) and m_texturesToPaint will be ones drawn to screen (either type of texture).

Does that sound reasonable?

(I am also planning on doing some general code clean-up and variable renaming because right now the code is pretty misleading/confusing wrt texture-backed frames.)

[Victoria Kirst]

(In reply to comment #5)
Ahhh ignore this comment, Ken! I'm doing some more serious rewriting because this is pretty fragile. I will just upload the patch in a little bit. :)

[Victoria Kirst]

Created attachment 86234 [details]
Patch

[Victoria Kirst]

Comment on attachment 86234 [details]
Patch

The logic was getting pretty gross so I did some mild refactoring. Please let me know your thoughts and/or if you have questions about my changes.

[Victoria Kirst]

Comment on attachment 86234 [details]
Patch

Ah there are a few more style mistakes I want to clean-up ...

[Victoria Kirst]

Created attachment 86237 [details]
Patch

[Kenneth Russell]

Comment on attachment 86237 [details]
Patch

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

Looks better overall, but a few questions. Also please mark the bug cq? if you would like me to cq+ it.

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:96
> +        Texture texture = m_textures[plane];
> +        if (!texture.isEmpty && texture.ownedByLayerRenderer)
> +            GLC(context, context->deleteTexture(texture.id));

The fact that this code is only in the destructor looks suspicious to me. Since cleanupResources() might be called multiple times (though perhaps only in response to the GPU process being restarted?) it seems this code should be placed elsewhere and called multiple times from code below. I might be wrong about that though.

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:365
> +        m_textures[plane].id = 0;
> +        m_textures[plane].size = IntSize();
> +        m_textures[plane].visibleSize = IntSize();
> +        m_textures[plane].ownedByLayerRenderer = false;
> +        m_textures[plane].isEmpty = true;

What if m_textures[plane] was previously non-empty and was owned by the layer renderer? The overwrite of id here will cause a resource leak. Or is it guaranteed that if this is called, m_textures only contained stale data from an earlier GPU process?

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:378
> +        m_textures[plane].id = frame->texture(plane);
> +        m_textures[plane].size = frame->requiredTextureSize(plane);
> +        m_textures[plane].visibleSize = computeVisibleSize(frame, plane);
> +        m_textures[plane].ownedByLayerRenderer = false;
> +        m_textures[plane].isEmpty = false;

Same basic question here.

[Victoria Kirst]

Comment on attachment 86237 [details]
Patch

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

I will update the patch to fix saveFrame/resetFrame! Would like amarinichev's OK before r+.

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:96
>> +            GLC(context, context->deleteTexture(texture.id));
> 
> The fact that this code is only in the destructor looks suspicious to me. Since cleanupResources() might be called multiple times (though perhaps only in response to the GPU process being restarted?) it seems this code should be placed elsewhere and called multiple times from code below. I might be wrong about that though.

OK, looks like cleanupResources() is only called in response to a lost context.

(cleanupResources() is called from setLayerRenderer when a renderer is being replaced, and that seems to be triggered only if the context is lost at the end of WebViewImpl::composite().)

@amarinichev, can you confirm this is true?

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:365
>> +        m_textures[plane].isEmpty = true;
> 
> What if m_textures[plane] was previously non-empty and was owned by the layer renderer? The overwrite of id here will cause a resource leak. Or is it guaranteed that if this is called, m_textures only contained stale data from an earlier GPU process?

Ah, yeah, I was assuming that if you start playing a video with software decoding, you will always use software decoding, and vice versa (meaning that resetFrameParameter() is always called when ownedByLayerRenderer is false). I will update the patch to be robust to this case, though.

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:378
>> +        m_textures[plane].isEmpty = false;
> 
> Same basic question here.

I will similarly update this as well.

[Alexey Marinichev]

> OK, looks like cleanupResources() is only called in response to a lost context.
> 
> (cleanupResources() is called from setLayerRenderer when a renderer is being replaced, and that seems to be triggered only if the context is lost at the end of WebViewImpl::composite().)
> 
> @amarinichev, can you confirm this is true?

Yes, lost context reallocates a renderer, and when the old renderer is destroyed, it calls its cleanupResources method.

[Victoria Kirst]

Created attachment 86535 [details]
Patch

[Kenneth Russell]

Comment on attachment 86535 [details]
Patch

amarinichev indicated offline that this looks good, and looks good to me too. Nice work.

[WebKit Commit Bot]

Comment on attachment 86535 [details]
Patch

Clearing flags on attachment: 86535

Committed r81737: <http://trac.webkit.org/changeset/81737>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.