[Webkit-unassigned] [Bug 56514] [chromium] Properly reset VideoLayerChromium textures after lost renderer context

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56514


Victoria Kirst <vrk at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86237|review?                     |review-
               Flag|                            |




--- Comment #12 from Victoria Kirst <vrk at chromium.org>  2011-03-22 14:32:46 PST ---
(From update of attachment 86237)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.