[webkit-reviews] review denied: [Bug 56514] [chromium] Properly reset VideoLayerChromium textures after lost renderer context : [Attachment 86237] Patch

[bugzilla-daemon at webkit.org]

Victoria Kirst <vrk at chromium.org> has denied Victoria Kirst
<vrk at chromium.org>'s request for review:
Bug 56514: [chromium] Properly reset VideoLayerChromium textures after lost
renderer context
https://bugs.webkit.org/show_bug.cgi?id=56514

Attachment 86237: Patch
https://bugs.webkit.org/attachment.cgi?id=86237&action=review

------- Additional Comments from Victoria Kirst <vrk at chromium.org>
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.