Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/ --- (Updated Aug. 9, 2016, 8:37 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Plasma. Changes --- Submitted with commit 45a2f5a8286d9884fdf1161c09da5807c0528481 by David Edmundson to branch master. Repository: plasma-framework Description --- The QSGTextures are created with window()->createTextureFromId(m_texture, QSize(w,h), QuickWindow::TextureOwnsGLTexture)); this means we don't want to be deleting textures ourselves too, it will be deleted when we delete the QSGTexture, which is a scoped pointer inside our QSGNode. BUG: 355644 REVIEW: Diffs - src/declarativeimports/core/windowthumbnail.cpp 6c46789b13411fe311c4d3fb60c690abd966cc38 Diff: https://git.reviewboard.kde.org/r/126131/diff/ Testing --- Thanks, David Edmundson
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review98219 --- Ship it! Ship It! - Martin Gräßlin On July 21, 2016, 3:37 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated July 21, 2016, 3:37 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 6c46789b13411fe311c4d3fb60c690abd966cc38 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > >
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/ --- (Updated July 21, 2016, 1:37 p.m.) Review request for KDE Frameworks and Plasma. Changes --- Updated to use the manual deletion rather than QSGTexture's deletion. I didn't push this earlier because I uploaded this at a time where we had a completely unrelated crash in WindowsThumbnail and someone said this makes it worse. Which is possible, but also unrelated. I'm bumping it now because I went through a bug report (https://bugs.kde.org/show_bug.cgi?id=365946) where it seems this is the cause and it reminded me of it. Repository: plasma-framework Description --- The QSGTextures are created with window()->createTextureFromId(m_texture, QSize(w,h), QuickWindow::TextureOwnsGLTexture)); this means we don't want to be deleting textures ourselves too, it will be deleted when we delete the QSGTexture, which is a scoped pointer inside our QSGNode. BUG: 355644 REVIEW: Diffs (updated) - src/declarativeimports/core/windowthumbnail.cpp 6c46789b13411fe311c4d3fb60c690abd966cc38 Diff: https://git.reviewboard.kde.org/r/126131/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 22, 2015, 1:24 p.m., Alex Merry wrote: > > Ship It! > > Mindaugas Baranauskas wrote: > Seems that with this patch KDE Plasma crash more often: > https://bugs.kde.org/show_bug.cgi?id=357895#c22 > > David Edmundson wrote: > @Mindaugas > > Can you try this patch now that 1e196fdfb2a6eaf1664e1155c086616d55c6712b > which fixes the bug you mentioned is merged. It's an unrelated change. I would be happpy if someone else could test. - Mindaugas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88691 --- On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 1:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 22, 2015, 1:24 p.m., Alex Merry wrote: > > Ship It! > > Mindaugas Baranauskas wrote: > Seems that with this patch KDE Plasma crash more often: > https://bugs.kde.org/show_bug.cgi?id=357895#c22 @Mindaugas Can you try this patch now that 1e196fdfb2a6eaf1664e1155c086616d55c6712b which fixes the bug you mentioned is merged. It's an unrelated change. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88691 --- On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 1:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 22, 2015, 1:24 p.m., Alex Merry wrote: > > Ship It! Seems that with this patch KDE Plasma crash more often: https://bugs.kde.org/show_bug.cgi?id=357895#c22 - Mindaugas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88691 --- On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 1:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 23, 2015, 7:13 a.m., Martin Gräßlin wrote:
> > are you sure it means that? I mean I did read the documentation when
> > writing that code and did deliberately wrote the code like that. So I'm a
> > little bit surprised that I got it exactly the other way around.
>
> David Edmundson wrote:
> How do you interpret:
> "The texture object owns the texture id and will delete the GL texture
> when the texture object is deleted." ?
>
>
> Docs aside, relevant Qt code is:
>
> QSGTexture *QSGEngine::createTextureFromId(uint id, const QSize &size,
> CreateTextureOptions options) const
> {
> Q_D(const QSGEngine);
> if (d->sgRenderContext->isValid()) {
> QSGPlainTexture *texture = new QSGPlainTexture();
> texture->setTextureId(id);
> texture->setHasAlphaChannel(options & TextureHasAlphaChannel);
> texture->setOwnsTexture(options & TextureOwnsGLTexture);
> texture->setTextureSize(size);
> return texture;
> }
> return 0;
> }
>
>
> QSGPlainTexture::~QSGPlainTexture()
> if (m_texture_id && m_owns_texture &&
> QOpenGLContext::currentContext())
>
> QOpenGLContext::currentContext()->functions()->glDeleteTextures(1,
> &m_texture_id);
>
> Martin Gräßlin wrote:
> given the code in plasma-framework, I'm quite certain that I didn't want
> Qt to take over ownership of it. So I think we should turn it around and make
> our code do what was intended. I just checked and we do proper cleanup
> handling. discardPixmap is called from the dtor, so if we do cleanup, why
> should Qt?
>why should Qt?
Because you told it to :P
but sure, that works too. Will make new RR.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126131/#review88703
---
On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126131/
> ---
>
> (Updated Nov. 22, 2015, 1:57 a.m.)
>
>
> Review request for KDE Frameworks and Plasma.
>
>
> Repository: plasma-framework
>
>
> Description
> ---
>
> The QSGTextures are created with
>
> window()->createTextureFromId(m_texture, QSize(w,h),
> QuickWindow::TextureOwnsGLTexture));
>
> this means we don't want to be deleting textures ourselves too, it will
> be deleted when we delete the QSGTexture, which is a scoped pointer
> inside our QSGNode.
>
> BUG: 355644
> REVIEW:
>
>
> Diffs
> -
>
> src/declarativeimports/core/windowthumbnail.cpp
> 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2
>
> Diff: https://git.reviewboard.kde.org/r/126131/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> David Edmundson
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 23, 2015, 8:13 a.m., Martin Gräßlin wrote:
> > are you sure it means that? I mean I did read the documentation when
> > writing that code and did deliberately wrote the code like that. So I'm a
> > little bit surprised that I got it exactly the other way around.
>
> David Edmundson wrote:
> How do you interpret:
> "The texture object owns the texture id and will delete the GL texture
> when the texture object is deleted." ?
>
>
> Docs aside, relevant Qt code is:
>
> QSGTexture *QSGEngine::createTextureFromId(uint id, const QSize &size,
> CreateTextureOptions options) const
> {
> Q_D(const QSGEngine);
> if (d->sgRenderContext->isValid()) {
> QSGPlainTexture *texture = new QSGPlainTexture();
> texture->setTextureId(id);
> texture->setHasAlphaChannel(options & TextureHasAlphaChannel);
> texture->setOwnsTexture(options & TextureOwnsGLTexture);
> texture->setTextureSize(size);
> return texture;
> }
> return 0;
> }
>
>
> QSGPlainTexture::~QSGPlainTexture()
> if (m_texture_id && m_owns_texture &&
> QOpenGLContext::currentContext())
>
> QOpenGLContext::currentContext()->functions()->glDeleteTextures(1,
> &m_texture_id);
given the code in plasma-framework, I'm quite certain that I didn't want Qt to
take over ownership of it. So I think we should turn it around and make our
code do what was intended. I just checked and we do proper cleanup handling.
discardPixmap is called from the dtor, so if we do cleanup, why should Qt?
- Martin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126131/#review88703
---
On Nov. 22, 2015, 2:57 a.m., David Edmundson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126131/
> ---
>
> (Updated Nov. 22, 2015, 2:57 a.m.)
>
>
> Review request for KDE Frameworks and Plasma.
>
>
> Repository: plasma-framework
>
>
> Description
> ---
>
> The QSGTextures are created with
>
> window()->createTextureFromId(m_texture, QSize(w,h),
> QuickWindow::TextureOwnsGLTexture));
>
> this means we don't want to be deleting textures ourselves too, it will
> be deleted when we delete the QSGTexture, which is a scoped pointer
> inside our QSGNode.
>
> BUG: 355644
> REVIEW:
>
>
> Diffs
> -
>
> src/declarativeimports/core/windowthumbnail.cpp
> 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2
>
> Diff: https://git.reviewboard.kde.org/r/126131/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> David Edmundson
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
> On Nov. 23, 2015, 7:13 a.m., Martin Gräßlin wrote:
> > are you sure it means that? I mean I did read the documentation when
> > writing that code and did deliberately wrote the code like that. So I'm a
> > little bit surprised that I got it exactly the other way around.
How do you interpret:
"The texture object owns the texture id and will delete the GL texture when the
texture object is deleted." ?
Docs aside, relevant Qt code is:
QSGTexture *QSGEngine::createTextureFromId(uint id, const QSize &size,
CreateTextureOptions options) const
{
Q_D(const QSGEngine);
if (d->sgRenderContext->isValid()) {
QSGPlainTexture *texture = new QSGPlainTexture();
texture->setTextureId(id);
texture->setHasAlphaChannel(options & TextureHasAlphaChannel);
texture->setOwnsTexture(options & TextureOwnsGLTexture);
texture->setTextureSize(size);
return texture;
}
return 0;
}
QSGPlainTexture::~QSGPlainTexture()
if (m_texture_id && m_owns_texture && QOpenGLContext::currentContext())
QOpenGLContext::currentContext()->functions()->glDeleteTextures(1,
&m_texture_id);
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126131/#review88703
---
On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126131/
> ---
>
> (Updated Nov. 22, 2015, 1:57 a.m.)
>
>
> Review request for KDE Frameworks and Plasma.
>
>
> Repository: plasma-framework
>
>
> Description
> ---
>
> The QSGTextures are created with
>
> window()->createTextureFromId(m_texture, QSize(w,h),
> QuickWindow::TextureOwnsGLTexture));
>
> this means we don't want to be deleting textures ourselves too, it will
> be deleted when we delete the QSGTexture, which is a scoped pointer
> inside our QSGNode.
>
> BUG: 355644
> REVIEW:
>
>
> Diffs
> -
>
> src/declarativeimports/core/windowthumbnail.cpp
> 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2
>
> Diff: https://git.reviewboard.kde.org/r/126131/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> David Edmundson
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88705 --- Deleting a texture twice is legal in OpenGL, since unused texture ID's are silently ignored by glDeleteTextures(). That doesn't mean it's a good idea to do it, since a texture ID can be reused for a different texture after it's been passed to glDeleteTextures(). But regardless, it shouldn't crash as long as the textures pointer is valid. - Fredrik Höglund On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 1:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88703 --- are you sure it means that? I mean I did read the documentation when writing that code and did deliberately wrote the code like that. So I'm a little bit surprised that I got it exactly the other way around. - Martin Gräßlin On Nov. 22, 2015, 2:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 2:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/#review88691 --- Ship it! Ship It! - Alex Merry On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126131/ > --- > > (Updated Nov. 22, 2015, 1:57 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > The QSGTextures are created with > > window()->createTextureFromId(m_texture, QSize(w,h), > QuickWindow::TextureOwnsGLTexture)); > > this means we don't want to be deleting textures ourselves too, it will > be deleted when we delete the QSGTexture, which is a scoped pointer > inside our QSGNode. > > BUG: 355644 > REVIEW: > > > Diffs > - > > src/declarativeimports/core/windowthumbnail.cpp > 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 > > Diff: https://git.reviewboard.kde.org/r/126131/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126131: Don't delete gl texture twice in thumbnail
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126131/ --- (Updated Nov. 22, 2015, 1:57 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description (updated) --- The QSGTextures are created with window()->createTextureFromId(m_texture, QSize(w,h), QuickWindow::TextureOwnsGLTexture)); this means we don't want to be deleting textures ourselves too, it will be deleted when we delete the QSGTexture, which is a scoped pointer inside our QSGNode. BUG: 355644 REVIEW: Diffs - src/declarativeimports/core/windowthumbnail.cpp 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 Diff: https://git.reviewboard.kde.org/r/126131/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
