Bug 57530 – [Qt] GraphicsContext3D internal buffers are not freed

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jarkko Sakkinen]

There's code for destroying internal FBO, color texture and depth buffer from the code. These resouces aren't freed when shader OpenGL context is created with the viewport.

[Jarkko Sakkinen]

Created attachment 87685 [details]
fix

[Benjamin Poulain]

Comment on attachment 87685 [details]
fix

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:597
> +        void* addr = m_glWidget->context()->getProcAddress(QString::fromAscii(nameWithExt.utf8().data()));

QString::fromAscii() is almost never valid inside a library. It does not actually convert from Ascii, but from the default codec, which can be set to anything by the user code.
You should use QString::fromLatin1() or QLatin1String() or in thisn case: QString::fromUtf8().

[Jarkko Sakkinen]

Created attachment 88264 [details]
QString::fromUtf8()

[Benjamin Poulain]

Comment on attachment 88264 [details]
QString::fromUtf8()

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:455
> +    m_glWidget->makeCurrent();
> +    if (m_glWidget->isValid()) {
> +        ::glDeleteTextures(1, &m_texture);
> +        deleteRenderbuffers(1, &m_depthBuffer);
> +        deleteFramebuffers(1, &m_canvasFbo);
> +    }

Please resubmit the patch with just this.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:597
> +        void* addr = m_glWidget->context()->getProcAddress(QString::fromUtf8(nameWithExt.utf8().data()));

Actually, looking at it, shouldn't it be QString(nameWithExt)?
I don't see why we should pass by UTF-8 to convert for WTF::String to QString.

You should commit this separately. It seems unrelated with the resource cleaning.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:665
> +    if ((width == m_currentWidth && height == m_currentHeight) || (!m_internal))

No problem with that but you can commit that separately, with the above change, in a code cleaning patch.

[Andrew Wason]

Created attachment 89460 [details]
free internal buffers

This is Jarkkos patch without the extra compiler warning cleanup, which is now in a patch on bug #58478

[Benjamin Poulain]

Comment on attachment 89460 [details]
free internal buffers

Looks good.

[Benjamin Poulain]

(In reply to comment #5)
> Created an attachment (id=89460) [details]
> free internal buffers
> 
> This is Jarkkos patch without the extra compiler warning cleanup, which is now in a patch on bug #58478

Good job doing the updated patch. Jarkko is on vacation IIRC.

[WebKit Commit Bot]

Comment on attachment 89460 [details]
free internal buffers

Clearing flags on attachment: 89460

Committed r83834: <http://trac.webkit.org/changeset/83834>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.