[Webkit-unassigned] [Bug 167544] [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #3)
> Comment on attachment 300011 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300011&action=review
> 
> Nice!
> 
> Now I know all this recent work to bring back on-demand accelerated
> composting is going to have a dramatic memory reduction benefit, but these
> changes seem risky and I don't think we should backport them to 2.14 now
> that we're nearly to the end of the release cycle. My recommendation is to
> let it bake in trunk at this point. Not just this commit, but the whole set.
> It's your call, of course.

All this work is to make 2.14 usable again for many people. so all these commits expect the one adding new API will be backported to 2.14, but not right now. My plan is to make a new 2.15 release as soon as possible next week, once the tests are in better shape. Then we test the new release for a while, and continue fixing issues that might show up in the bots, ro reported by Andrés. And then, with all the fixes I'll backport everything to 2.14 and make a new release.

> > Source/WebKit2/ChangeLog:24
> > +        starting a timer of 5 secons to discard it if not reused.
> 
> seconds
> 
> > Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:382
> > +    m_previousLayerTreeHost = WTFMove(m_layerTreeHost);
> 
> If something ever goes wrong, better to have a null pointer dereference than
> a use after free, so I think you should set m_layerTreeHost to nullptr here,
> even if it's not strictly necessary. And it looks like it *is* necessary,
> since you have checks in several places to only use m_previousLayerTreeHost
> if m_layerTreeHost is null. If it's really not needed, please explain why.

Not it's not needed at all. Because m_layerTreeHost is a RefPtr, so !m_layerTreeHost checks the internal pointer and the move assignment already sets the internal pointer to nullptr.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:229
> > +    m_isDiscardable = discardable;
> > +    if (m_isDiscardable) {
> > +        m_discardableSyncActions = OptionSet<DiscardableSyncActions>();
> > +        return;
> > +    }
> 
> Personally, I would split the rest of this function below this point into a
> new function named
> ThreadedCoordinatedLayerTreeHost::applyDiscardableSyncActions.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:111
> > +    enum class DiscardableSyncActions { UpdateSize = 1 << 1, UpdateViewport = 1 << 2, UpdateScale = 1 << 3, UpdateBackground = 1 << 4 };
> 
> I would also prefer not to write enums on one line like this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170129/cce2d36f/attachment-0001.html>