Bug 57530

Summary: [Qt] GraphicsContext3D internal buffers are not freed
Product: WebKit Reporter: Jarkko Sakkinen <jarkko.j.sakkinen>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
fix
benjamin: review-
QString::fromUtf8()
benjamin: review-
free internal buffers none

Description Jarkko Sakkinen 2011-03-31 01:06:40 PDT
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.
Comment 1 Jarkko Sakkinen 2011-03-31 02:41:55 PDT
Created attachment 87685 [details]
fix
Comment 2 Benjamin Poulain 2011-04-05 06:02:36 PDT
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().
Comment 3 Jarkko Sakkinen 2011-04-05 09:56:19 PDT
Created attachment 88264 [details]
QString::fromUtf8()
Comment 4 Benjamin Poulain 2011-04-06 09:06:45 PDT
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.
Comment 5 Andrew Wason 2011-04-13 14:32:54 PDT
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
Comment 6 Benjamin Poulain 2011-04-13 16:19:07 PDT
Comment on attachment 89460 [details]
free internal buffers

Looks good.
Comment 7 Benjamin Poulain 2011-04-13 16:20:16 PDT
(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.
Comment 8 WebKit Commit Bot 2011-04-14 04:01:31 PDT
Comment on attachment 89460 [details]
free internal buffers

Clearing flags on attachment: 89460

Committed r83834: <http://trac.webkit.org/changeset/83834>
Comment 9 WebKit Commit Bot 2011-04-14 04:01:34 PDT
All reviewed patches have been landed.  Closing bug.